Better error handling and disabled display of exception data by default #2509
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nitriques
changed the title
|
What happens if there is a fatal error before someone can log in.. and you are the developer? |
You ssh the box and check the logs. Everything is still there. Security has a cost. It's worth to mention that the installer still leaks errors tho. I can live with that. Finally, for the core dev, there's only one line to comment out. I'd rather to that then having full paths disclosed. I am sure @brendo wants feedback as bad as I do guys (cc @michael-e @jensscherbl in particular) |
| * @param Exception $e | ||
| * The Exception object | ||
| * @return string | ||
| * The result of the Exception's render function | ||
| */ | ||
| public static function handler(Exception $e) | ||
| public final static function handler(Exception $e) |
nitriques
force-pushed
the
errorhandler
branch
2 times, most recently
from
e852549
to
94df814
Compare
|
@brendo Rebased and PSR-2 things fixed. |
|
Not super comfortable about putting this in |
|
Let's do 2.7.0 then. |
nitriques
force-pushed
the
errorhandler
branch
from
1dd60ed
to
a489fb5
Compare
4 tasks
They are not needed anymore since we use a auto loader
This commit adds a grunt task (named `php`) that will run phpcs with the PSR-2 standard. Note that you have to install php_codesniffer which has been added as a require-dev lib. Doing `composer install` will mess up Symphony's auto loader, so please `git checkout vendor` after the composer call
This commit introduce a new jQuery plugin called Default Value, which gives a input a source element to value its value from, when a specified event occurs on this source element. This behaviour is switched to off when the users focuses on the input field and then blurs out, leaving a value in the input. This commit also hooks this behaviours on the Data Source name input and on the Event name input. Closes symphonycms#2511
This function should be checking new value for emptiness, but it was checking the database value instead. Additionally, at this point, the database value would always be `NULL`. Changed to use the correct variable.
When there is no data written to the database, return `true` instead of `false`. Logically, the operation is successful. See: http://php.net/manual/en/sessionhandlerinterface.write.php When no data has been read, the read method should return an emtpy string, but it was returning NULL instead. See: http://php.net/manual/en/sessionhandlerinterface.read.php
This way, developer can change the sorting order, which is Symphony's order by default
Url seems to be case sensitive now. Closes symphonycms#2580
…onycms#2530) * Set "Sections Index" as default area of default (first) author Because not having a default area isn’t considered in the UI. * Set "/blueprints/sections/" as default area fallback for developers symphonycms#2529 * Fix Whitespace
A very small formatting fix.
PHP 7 cares.
In MySQL 5.7 a datetime field cannot have a default value of "0000-00-00" for the date, the minimum value is "1000-01-01".
This thing really complicates things and should have been removed earlier
This change is to protect our users against a poorly setup server. PHP can allow pretty scary things security-wise, so it's best to make sure things that can only have one valid setting should be enforced. Thanks to @hyp3rlinx for this.
This change makes it possible to have non-url safe characters in the base path
This is a precaution calls what makes sure the SAPI module used by the hosting company chdir's the user at the right place
This is what constant are for
This commits reuse the code from the edit action in order to validate if the current user can delete the author. Even if the Delete button is not present on the page, a request can be crafted in order for an author to delete another. This change also validates that the password of the current author is checked before doing any un-undoable things. Rebase of 8b20b8f
This changes uses the array returned by $section->fetchVisibleColumns() in order to create the element names array to pass to the entry manager. This yeilds a nice performance update when a section contains many (hundreads) of fields but only a few of them are needed for the publish table view. Rebase of 34bd49f
That's for uniformity only. Cherry picked version of 5476e2f
Rebased version of 05693e0 Closes symphonycms#2520
EntryPreCheckPostFieldData
* Publish Filtering : Increase flexibility of comparison modes 6e764d1 The current markup of the „comparison mode“-selectboxes in the publish filtering interface doesn’t allow for multiple modes that don’t have a filter prefix (like „regexp:“, „greater than:“, etc.) as it uses the prefix as unique identifier (option value). The default comparison mode „is“ is expected as the only mode without a filter prefix. But filter modes like „between“ („$x to $y“) as included in Number Field (and possibly date fields too) don’t have a prefix either and therefore won’t work with the current code. This PR changes that behavior by shifting to the filter title as unique identifier and adding the filter prefix as optional data-attribute that’s only used for building the actual filtering-url-parameters. Old Behavior: - Filter title is used as option title - Filter prefix is used as value-attribute New Behavior: - Filter title is used as option title - Filter title is used as value-attribute - Filter prefix is included as data-attribute * Fix code indenting 4a7db7a * Improve construction of comparison array ccc5e9e * Fix code indenting e56bdde * Use „data-comparison“ as unique identifier of the chosen comparison mode d37e0b8 Use the „data-comparison“-attribute as unique identifier for the chosen comparison mode of a filtering instance - both in the selectbox and the list of context help texts. The „value“-attribute of the comparison mode selectbox now again is used for serving the actual filter prefix used to construct the filtering-parameters in the url. * Remove leading whitespace from filter value input field ac61c11
The doc stated that E_NOTICE were ignored while the code only ignores E_STRICT (see line 423)
This method should never get called directly. Nothing ever calls it in the current base code.
Those changes makes sure we do not leak exception information before doing the authentication. It also makes sure that exception raised in the exception handler won't leak info if the GenericExceptionHandler is disabled. The same logic is also applied in the shutdown function. Instead of masking the error like we did, the logic is applied only in the render part of things, which should make it easier for us to debug. It also removes the wrong HTTP status that we were returning (500 instead of 404)
This implementation copies what was already present in the GenericErrorHandler, i.e. skip logging if disabled = true. This also prevents errors caught in GenericErrorHandler to be also loggued a second time in the GenericExceptionHandler::handler method.
Both SymphonyErrorPageHandler and DatabaseExceptionHandler needed care: SymphonyErrorPageHandler: - throwCustomError must not touch GenericExceptionHandler::$enabled since it allows data leakage - defaults the diabled and not template cases to GenericExceptionHandler::render DatabaseExceptionHandler: - Filter out data if the Handler is not enabled. This handler was already getting the right template since it uses the same as the parent. GenericExceptionHandler: - refactored the headers part to share code in SymphonyErrorPageHandler
Both the GenericErrorHandler and the GenericExceptionHandler's handler function should not be overriden.
FrontendPageNotFoundExceptionHandler can bypass the new anti-leak system since it's a legitimate message.
It order to be PSR-2 compatible. Ref https://github.com/symphonycms/symphony-2/pull/2509/files#r41956816
|
Replaced by #2597 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ok this one is a biggie.
I've been wrapping my head around this for quite some time now and with #2504 I though that it was to time to do it.
I won't repeat what I've already described in each commit, it should be easy to follow.
The biggest problem was that the
GenericExceptionHandlerwas only disabled after a failed auth. Now, it's activated after a successful auth.Basically, when the
GenericExceptionHandleris disabled (i.e$enabled === false), we still let it through but display a generic message that does not leak anything. This is not a breaking change since we shortcut'ed the thing with a 404 (which was better then nothing)I did not changed the default value in the class per se, but made it switch to false before we register the functions. I am still not sure about that.