Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disable option display_errors by default #146

Merged
merged 7 commits into from
Nov 9, 2021
Merged

Disable option display_errors by default #146

merged 7 commits into from
Nov 9, 2021

Conversation

ligurio
Copy link
Member

@ligurio ligurio commented Oct 29, 2021 •
edited
Loading

Fixes https://github.com/tarantool/security/issues/8
Fixes #129
Fixes #135
Fixes a CMake warnings for passed package names

PR depends on #141

NOTE: Issue https://github.com/tarantool/security/issues/8 must be closed manually after merging.

@ligurio
cmake: fix warnings for passed package names
442bb5f 
cmake produces warnings like below:

  The package name passed to `find_package_handle_standard_args` (TARANTOOL)
  does not match the name of the calling package (Tarantool).  This can lead
  to problems in calling code that expects `find_package` result variables
  (e.g., `_FOUND`) to follow a certain pattern.
Call Stack (most recent call first):
  cmake/FindTarantool.cmake:27 (find_package_handle_standard_args)
  CMakeLists.txt:10 (find_package)

This patch fixes package name, warnings are gone and output
becomes clear and readable.
@ligurio ligurio force-pushed the ligurio/gh-8-disable-display-errors branch from e1e506a to 24c942c Compare October 29, 2021 13:22
DifferentialOrange
DifferentialOrange approved these changes Nov 2, 2021
View reviewed changes
Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is worth to duplicate commit messages (like the one about Django) additional info to PR description since public users do not have access to tarantool/security ticket.

@ligurio
Copy link
Member Author

ligurio commented Nov 2, 2021 •
edited
Loading

I think it is worth to duplicate commit messages (like the one about Django) additional info to PR description since public users do not have access to tarantool/security ticket.

It was already there, except link to site about django. Well, added it too.

Screenshot from 2021-11-02 15-47-39

@ligurio ligurio force-pushed the ligurio/gh-8-disable-display-errors branch from 24c942c to d91f072 Compare November 2, 2021 12:47
@DifferentialOrange
Copy link
Member

I think it is worth to duplicate commit messages (like the one about Django) additional info to PR description since public users do not have access to tarantool/security ticket.

It was already there, except link to site about django. Well, added it too.

I mean this description
image

@ligurio
Copy link
Member Author

ligurio commented Nov 9, 2021

I think it is worth to duplicate commit messages (like the one about Django) additional info to PR description since public users do not have access to tarantool/security ticket.

It was already there, except link to site about django. Well, added it too.

I mean this description

What's a point to publish a description of security issue before fixing it?

@ligurio
Copy link
Member Author

ligurio commented Nov 9, 2021

@DifferentialOrange I added to PR description all issues that fixed by this PR. Also removed commits related to #137, so we can merge these commits without waiting of fix for #137.

@ligurio ligurio force-pushed the ligurio/gh-8-disable-display-errors branch from d91f072 to 3401460 Compare November 9, 2021 18:43
Totktonada
Totktonada reviewed Nov 9, 2021
View reviewed changes
http/server.lua Outdated Show resolved Hide resolved
Totktonada
Totktonada approved these changes Nov 9, 2021
View reviewed changes
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except several minor nits. Feel free to push after fixes.

http/server.lua Show resolved Hide resolved
@ligurio
test: allow to set custom http server options
ce63aea 
Part of tarantool/security#8
@ligurio
http: disable display_errors option by default
ed2e2a4 
When option 'display_errors' is enabled httpd return application errors
and backtraces to the client (like PHP). It is unexpected that the
debugging information is sent to a client by default. It is usual to
have such option, but have it disabled by default: see Django's DEBUG
setting [1] for example.

1. https://docs.djangoproject.com/en/3.2/ref/settings/#debug

Part of tarantool/security#8
@ligurio
test: test default content-type for render option
170665d 
Part of tarantool/security#8
@ligurio
http: set default content-type to 'text/plain; charset=utf-8'
87e027e 
Default content-type 'text/plain; charset=utf-8' is used when
content-type is not set. Because without it web browser may interpret
the response in an arbitrary way.

Fixes tarantool/security#8
@ligurio ligurio force-pushed the ligurio/gh-8-disable-display-errors branch from 3401460 to 87e027e Compare November 9, 2021 19:08
@ligurio
Copy link
Member Author

ligurio commented Nov 9, 2021

I would also leave several words why default content type may be important from security point of view.

Updated commit message.

@ligurio ligurio merged commit 35449df into master Nov 9, 2021
@ligurio ligurio deleted the ligurio/gh-8-disable-display-errors branch November 9, 2021 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Totktonada Totktonada Totktonada approved these changes

@DifferentialOrange DifferentialOrange DifferentialOrange approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

URL in README.md is broken Readme is wrong
3 participants
@ligurio @DifferentialOrange @Totktonada