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.

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

Make sure to escape error messages #8197

Merged
merged 1 commit into from Jun 26, 2015
Merged

Make sure to escape error messages #8197

merged 1 commit into from Jun 26, 2015

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Jun 24, 2015

Just in case an exception that is displayed there contains any user input

@tsteur tsteur added c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. Needs Review PRs that need a code review labels Jun 24, 2015
mattab
mattab reviewed Jun 24, 2015
View reviewed changes
core/testMinimumPhpVersion.php
}

if (!$isCli) {
if (version_compare('5.2.3', PHP_VERSION) > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

version_compare should be < 0

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@tsteur tsteur added this to the 2.14.0 milestone Jun 24, 2015
@mattab
Copy link
Member

mattab commented Jun 25, 2015

Looks good to me

@mattab
Copy link
Member

mattab commented Jun 25, 2015

Actually there is a regression where we lose the bold message in: http://builds-artifacts.piwik.org/ui-tests.master/13711.7/UIIntegrationTest_db_connect_error

  • we need to strip_tags and remove <br> only when in CLI

@tsteur
Copy link
Member Author

tsteur commented Jun 25, 2015

we need to strip_tags and remove
only when in CLI

Then htmlentites would escape them and the html would be visible. Looking deeper at the code this method Piwik_GetErrorMessagePage is only used in 2 or 3 parts and the one that contains user input is kinda escaped before already (https://github.com/piwik/piwik/blob/2.14.0-b10/core/ExceptionHandler.php#L70-L72) and doc block says @param string $message Main message, must be html encoded before calling

mattab pushed a commit that referenced this pull request Jun 26, 2015
Merge pull request #8197 from piwik/escape_error_message
f9b9925 
Make sure to escape error messages
@mattab mattab merged commit f9b9925 into master Jun 26, 2015
@mattab mattab deleted the escape_error_message branch June 26, 2015 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. Needs Review PRs that need a code review
Projects
None yet
Milestone
2.14.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tsteur @mattab