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

cherrypy: fix handling of raw application/x-www-form-urlencoded traffic #54901

Merged

Conversation

nicholasmhughes
Copy link
Collaborator

@nicholasmhughes nicholasmhughes commented Oct 5, 2019

What does this PR do?

handle the preservation of raw application/x-www-form-urlencoded traffic in cherrypy

What issues does this PR fix or reference?

N/A

Previous Behavior

cherrypy natively parsed the incoming x-www-form-urlencoded traffic into a dictionary, which was then put into the post key when the event fired. the raw_body was manually set to an empty string, because cherrypy will leave raw_body as None when parsing x-www-form-urlencoded traffic. therefore, the body key of the event was always an empty string in the case of x-www-form-urlencoded traffic, where that key would normally contain the raw request when processing other types of traffic.

I consider this "broken" because it doesn't provide a way to handle the raw data at all when parsing x-www-form-urlencoded traffic. This functionality is required when attempting to validate Slack webhooks since the raw x-www-form-urlencoded string is hashed for a signature and cannot be deserialized because it will destroy the order of the parameters and provide no mechanism to validate the traffic.

New Behavior

instead of using the native cherrypy parsing, which throws away the raw request when creating the params dictionary, we read the raw data into a string (which ends up in the body key in the event) and then perform manual parsing of the tokens into a dictionary using parse_qsl to set the contents of the post key in the event. since the post key output functionality isn't changed and the body key was always empty and therefore wasn't relied on, this is a low impact change.

Tests written?

Yes

Commits signed with GPG?

Yes

salt/netapi/rest_cherrypy/app.py Outdated Show resolved Hide resolved
salt/netapi/rest_cherrypy/app.py Outdated Show resolved Hide resolved
@nicholasmhughes nicholasmhughes requested a review from a team as a code owner December 6, 2019 15:12
@ghost ghost requested a review from dwoz December 6, 2019 15:12
@sagetherage sagetherage added this to Unassigned in Core Open PRs via automation Mar 19, 2020
@sagetherage sagetherage moved this from Unassigned to Assigned in Core Open PRs Mar 19, 2020
@Ch3LL Ch3LL removed the request for review from a team April 15, 2020 14:39
@dwoz dwoz added has-failing-test Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases labels May 13, 2020
@nicholasmhughes
Copy link
Collaborator Author

@dwoz added test and all tests passing

@dwoz dwoz removed Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases has-failing-test labels Jul 27, 2020
@nicholasmhughes
Copy link
Collaborator Author

@dwoz @sagetherage assuming these tests pass (I'm going to bed), can this PR get some love? I think we're coming up on 11 months from the original PR date and I keep having to track to new formats and tests.

dwoz
dwoz approved these changes Aug 6, 2020
@dwoz dwoz merged commit 9848ace into saltstack:master Aug 6, 2020
26 checks passed
@nicholasmhughes nicholasmhughes deleted the fix_cherrypy_urlencoded_rawbody2 branch August 6, 2020 22:59
@sagetherage sagetherage added the Magnesium Mg release after Na prior to Al label Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Magnesium Mg release after Na prior to Al
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants