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

hotfix(router) allow url escaped uris #2377

Closed
wants to merge 1 commit into from

Conversation

bungle
Copy link
Member

@bungle bungle commented Apr 12, 2017

Summary

Change the router to use ngx.var.request_uri instead of the normalized ngx.var.uri. Makes proxying more transparent.

Issues resolved

Fix #2366

else
local s = find(uri, "?", 2, true)
if s then
uri = sub(uri, 1, s - 1)
Copy link
Member

@thibaultcha thibaultcha Apr 13, 2017

Choose a reason for hiding this comment

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

We need to strip the uri prior to the find_api call. The reason is that as this stands now, we would greatly increase the risk of cache miss because the same URL path may have different querystring parameters, hence resulting in a different cache key.

Copy link
Member

Choose a reason for hiding this comment

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

As a side note, it may be worth it to introduce unit tests that ensure the cache is hit when appropriate in the future. Part of a future improvement we can do to ensure the stability of such a critical component :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh, good catch indeed. Sorry for this screw up :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

The fix is done and pushed, I need to add the cache hit / miss tests still.

@thibaultcha thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Apr 13, 2017
bungle added a commit that referenced this pull request Apr 14, 2017
@thibaultcha thibaultcha added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Apr 15, 2017
Change the router to use `ngx.var.request_uri` instead of
the normalized `ngx.var.uri`. Makes proxying more transparent.

Fixes #2366
@bungle bungle force-pushed the feat/allow-url-encoded-uris-2 branch from 7292b55 to 29f81cb Compare April 21, 2017 11:42
@thibaultcha
Copy link
Member

Merged with minor modifications, thanks!

@thibaultcha thibaultcha deleted the feat/allow-url-encoded-uris-2 branch April 25, 2017 02:00
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.

Router doesn't match URL encoded URIs
2 participants