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

Filter query string in env variables #155

Merged
merged 2 commits into from
Nov 30, 2021
Merged

Conversation

shalvah
Copy link
Contributor

@shalvah shalvah commented Nov 11, 2021

PHP includes the query string in QUERY_STRING and REQUEST_URI env vars, so we have to make sure to filter in those too. Fix for #153

@shalvah shalvah requested a review from joshuap November 11, 2021 11:38
@shalvah
Copy link
Contributor Author

shalvah commented Nov 12, 2021

On second thought, this is a CGI thing, not necessarily a PHP thing. Ruby's CGI also includes QUERY_STRING, but our Ruby library doesn't report it.

I think we'll switch to that approach instead. The query string is already reported in the URL; there's no need for the duplication and extra variables. As an added bonus, we can cut down on the noise in the error report.

@shalvah shalvah force-pushed the filter-query-string-in-env branch from ec2a085 to c1265c0 Compare November 12, 2021 15:40
@shalvah
Copy link
Contributor Author

shalvah commented Nov 12, 2021

Updated. Diff's a bit hard to see because I rearranged the keys alphabetically, but the main change is removing the QUERY_STRING and REQUEST_URI from the whitelist.

@joshuap joshuap assigned joshuap and unassigned joshuap Nov 15, 2021
Copy link
Member

@joshuap joshuap left a comment

Choose a reason for hiding this comment

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

Thanks!

@shalvah shalvah merged commit c803199 into master Nov 30, 2021
@shalvah shalvah deleted the filter-query-string-in-env branch November 30, 2021 06:30
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.

2 participants