-
Notifications
You must be signed in to change notification settings - Fork 41
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
Parameter and wildcard matches are decodeURIed twice #171
Comments
I think it's safe to remove the extra calls to decodeURI ... and Medium's graph-tests still pass without it. @majelbstoat @nicks do you have context on this to advise? (sorry, I know it's a hot path through old code...bad combination) |
Oh man, I don't remember the context on this. It was something to do with media resources though, I'm pretty sure. If all the tests in medium2 pass on it, including functional tests, then let's go for it, and if it breaks something, we can write more tests for that. Hard to know the full impact, but if there's a bug that this fixes, this seems reasonable. |
sgtm. if you change something, and no tests break, then it must be a safe On Wed, Jun 22, 2016 at 11:21 PM, Jamie Talbot notifications@github.com
|
@majelbstoat @nicks thanks for taking a look! would you mind releasing a new version on npm when you have a chance? |
published! On Fri, Jun 24, 2016 at 12:56 PM, Kyle Mahan notifications@github.com
|
Thanks! 🔥 |
Tracking down a bug in medium2, I noticed matador is unescaping parameters one too many times. For example, if
/dir/*
http://hi%5Ethere
/dir/http%3A%2F%2Fhi%255Ethere
*
ishttp://hi^there
(^
should be%5E
)I added a test demonstrating: https://github.com/Medium/matador/compare/kylewm/over-decoding with the result:
The text was updated successfully, but these errors were encountered: