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

Fix bug that prevented to filter sensitive data when there is an exception with multiple 'values' #483

Merged
merged 1 commit into from
Aug 2, 2017

Conversation

danydev
Copy link
Contributor

@danydev danydev commented Aug 2, 2017

While I was reviewing the code to make sure that sensitive data is correctly filtered, I noticed the following bug: if the exception has 2 arrays in 'values' key, only the 1st one is correctly filtered.

The reason is because there is a not necessary return that I removed to fix the bug.
Obviously I also verified that all the callers of this function didn't use its return value. It's likely a bug introduced when porting the implementation from another language where a callback or similar has been used.

Also added test to make sure a regression can not be introduced.

@stayallive
Copy link
Collaborator

Good catch!

@stayallive stayallive requested a review from dcramer August 2, 2017 13:23
Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Great catch, thanks for the fix!

@Jean85 Jean85 merged commit e2f3c47 into getsentry:master Aug 2, 2017
@Jean85
Copy link
Collaborator

Jean85 commented Aug 2, 2017

@stayallive do we backport this to the 1.7.x branch to release a 1.7.1 patch immediately?

@danydev danydev deleted the sensitive-data-filtering-bugfix branch August 2, 2017 14:58
@stayallive
Copy link
Collaborator

@Jean85 I would say yes. This has a low impact since it will be catched by the server correctly IIRC, it does still travel over an (https) connection which can be prevented.

Jean85 pushed a commit that referenced this pull request Aug 2, 2017
(cherry picked from commit e2f3c47)
@Jean85 Jean85 mentioned this pull request Aug 2, 2017
@Jean85
Copy link
Collaborator

Jean85 commented Aug 2, 2017

Done: https://github.com/getsentry/sentry-php/releases/tag/1.7.1
As you can see, I've also opened #486 to re-merge into master the changes (changelog mostly).

Jean85 added a commit that referenced this pull request Aug 2, 2017
* 1.7.0

* Backport #483 to be released as 1.7.1

(cherry picked from commit e2f3c47)

* Release patch 1.7.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants