Skip to content

fix encoding issues in router, proxy, partition rewriting#7682

Merged
alexrashed merged 2 commits intomasterfrom
fix-encoding-issues
Feb 14, 2023
Merged

fix encoding issues in router, proxy, partition rewriting#7682
alexrashed merged 2 commits intomasterfrom
fix-encoding-issues

Conversation

@alexrashed
Copy link
Member

This PR addresses several encoding issues:

  • get_raw_base_url now avoids multi-encoding by avoiding the usage of werkzeug.sansio.utils.get_current_url.
  • SimpleRequestsClient now avoids multi-encoding by avoiding the usage of werkzeug.sansio.utils.get_current_url.
  • Proxy uses the encoded path of the given request (instead of the decoded one) if no forward_path is given.
  • Router now matches on the encoded path (instead of the decoded one).
    • This is a quite fundamental change, but it is necessary, because otherwise the encoding information is lost for parsed path parameters (f.e. when using /<path:path>).
  • ArnPartitionRewriteHandler now uses the correct forward path (the modified one, instead of the original one).
  • ArnPartitionRewriteHandler does not decode string values anymore. It uses the encoding-specific regex if necessary anyways.

/cc @whummer

@alexrashed alexrashed requested a review from thrau as a code owner February 14, 2023 13:16
@alexrashed alexrashed temporarily deployed to localstack-ext-tests February 14, 2023 13:16 — with GitHub Actions Inactive
@github-actions
Copy link

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 29m 33s ⏱️ - 2m 11s
1 725 tests ±0  1 366 ✔️  - 1  359 💤 +1  0 ±0 
2 439 runs  ±0  1 740 ✔️  - 1  699 💤 +1  0 ±0 

Results for commit e23e64c. ± Comparison against base commit abe1ba0.

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! i think all we need to document somewhere is that @route and Router.add behave differently from werkzeug/flask now, in that we have to specify URL encoded paths now.
so @route("/foo bar/baz") has to become @route("/foo%20bar/baz"), if i understood correctly.

@alexrashed alexrashed temporarily deployed to localstack-ext-tests February 14, 2023 15:14 — with GitHub Actions Inactive
@alexrashed alexrashed merged commit 87557f4 into master Feb 14, 2023
@alexrashed alexrashed deleted the fix-encoding-issues branch February 14, 2023 15:31
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