Parameter and wildcard matches are decodeURIed twice

[kylewm]

Tracking down a bug in medium2, I noticed matador is unescaping parameters one too many times. For example, if

1. there's a route /dir/*
2. client wants to send the parameter http://hi%5Ethere
3. client hits the path /dir/http%3A%2F%2Fhi%255Ethere
4. matador tells me * is http://hi^there (^ should be %5E)

I added a test demonstrating: https://github.com/Medium/matador/compare/kylewm/over-decoding with the result:

✖ testNamedParameterEscaping

AssertionError: 'hello%20world' == 'hello world'
    at Object.equal (/Users/kylemahan/src/matador/node_modules/nodeunit/lib/types.js:83:39)
    at /Users/kylemahan/src/matador/tests/functional/routing_test.js:17:10
    ...

✖ testWildcardEscaping

AssertionError: 'http://example.com/abc%5E123' == 'http://example.com/abc^123'
    at Object.equal (/Users/kylemahan/src/matador/node_modules/nodeunit/lib/types.js:83:39)
    at /Users/kylemahan/src/matador/tests/functional/routing_test.js:31:10
    ...

[kylewm]

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)

[majelbstoat]

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.

[nicks]

sgtm. if you change something, and no tests break, then it must be a safe change!

On Wed, Jun 22, 2016 at 11:21 PM, Jamie Talbot notifications@github.com wrote:

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, asked 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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Medium/matador/issues/171#issuecomment-227939845, or mute the thread https://github.com/notifications/unsubscribe/AARAcXHeNxKCfNSfHZYEi_R2CKFXiATsks5qOfvVgaJpZM4I8YHq .

[kylewm]

@majelbstoat @nicks thanks for taking a look! would you mind releasing a new version on npm when you have a chance?

[nicks]

published!

On Fri, Jun 24, 2016 at 12:56 PM, Kyle Mahan notifications@github.com wrote:

@majelbstoat https://github.com/majelbstoat @nicks https://github.com/nicks thanks for taking a look! would you mind releasing a new version on npm when you have a chance?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Medium/matador/issues/171#issuecomment-228400417, or mute the thread https://github.com/notifications/unsubscribe/AARAcQE6cmrFlZyOTO5WWPQBoxWGWmigks5qPAxKgaJpZM4I8YHq .

[kylewm]

Thanks! :fire: