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 request encoding in generic proxy listener chain and forwarding #6070

Merged
merged 2 commits into from May 16, 2022

Conversation

alexrashed
Copy link
Member

@alexrashed alexrashed commented May 16, 2022

This PR fixes an issue initially addressed with #6059 (which was later closed in favor of #6065).
Unfortunately, #6065 does not fix the issue described in #6059.
This PR contains the following changes:

  • It changes the path and query parameter handling for the handler chain such that the generic proxy uses the url-encoded path and query parameters within its proxy handler chain.
  • It partly reverts Minor fixes in edge forwarder and zip utils for dev/host mode #6065, since the proxy listener gets the encoded path and query parts now. /cc @whummer
  • Fixes a small issue with the pytest plugin config (moves pytest plugins to the root conftest, since it's not allowed anywhere else anymore).

The special handling with get_full_raw_path was necessary due to a specific peculiarity in Werkzeug's url property handling.
Here's a short scratch to illustrate the issue:

from werkzeug.sansio.utils import get_current_url

# Werkzeug (and with that the Quart request wrapper) internally uses get_current_url on request.url:
# > werkzeug.sansio.Request#L212:
# @cached_property
# def url(self) -> str:
#     """The full request URL with the scheme, host, root path, path,
#     and query string."""
#     return get_current_url(
#         self.scheme, self.host, self.root_path, self.path, self.query_string
#     )

# get_current_url is weird, because it _encodes_ the path segment, while it _decodes_ (some) characters in the query segment
url = get_current_url(scheme="https", root_path="root", path="pa#th", host="host", query_string=b"query-string%2Basdf")
print(url)
assert url == "https://hostroot/pa%23th?query-string+asdf" # <- mind the encoded # (%23) and the decoded %2B (+)!

@alexrashed alexrashed requested a review from thrau as a code owner May 16, 2022 15:31
@alexrashed alexrashed temporarily deployed to localstack-ext-tests May 16, 2022 15:31 Inactive
Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for digging into this.

just a minor comment: maybe we could add a query string to the test case?

@alexrashed alexrashed temporarily deployed to localstack-ext-tests May 16, 2022 16:10 Inactive
@github-actions
Copy link

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   59m 18s ⏱️ - 5m 37s
1 011 tests +1     972 ✔️ +1  39 💤 ±0  0 ±0 
1 307 runs  +1  1 241 ✔️ +1  66 💤 ±0  0 ±0 

Results for commit 2de2dfa. ± Comparison against base commit 02e7c5d.

@alexrashed alexrashed merged commit d79878b into master May 16, 2022
@alexrashed alexrashed deleted the fix-request-encoding branch May 16, 2022 17:46
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants