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

Paths with URL-encoded characters fail to match #124

Closed
jamesarosen opened this issue Oct 29, 2015 · 8 comments
Closed

Paths with URL-encoded characters fail to match #124

jamesarosen opened this issue Oct 29, 2015 · 8 comments

Comments

@jamesarosen
Copy link

Given application code that makes GET /foos/my%20foo,

// This matches and request.params.name is "my foo"
this.get('/foos/:name', function(request){
  const foo =  JSON.stringify({ name: request.params.name });
  return [ 200, {"Content-Type": "application/json"}, foo ];
});

// But this never matches:
this.get('/foos/my foo', function(request) {
  ...
});

// Nor does this:
this.get('/foos/my%20foo', function(request) {
  ...
});

Note: I've only tested this with Mirage, so its possible the fault lies there. I don't see any tests for URL-encoded matches in this project, though.

@jamesarosen
Copy link
Author

Possibly related to #111 as this behaves differently on Chrome (46) and PhantomJS (1.9.8).

tundal45 added a commit to tundal45/route-recognizer that referenced this issue Oct 29, 2015
Since the route recognizer match supports decoding before the match,
it makes sense for the parser to also decode URI components before
it starts parsing the route.

This should solve pretenderjs/pretender#124.

While the test I added failed both in chrome & phantomjs, this might
also be relevant to pretenderjs/pretender#111.
@jamesarosen
Copy link
Author

I changed _handlerFor to

_handlerFor: function(verb, url, request){
    console.log(verb + '  ' + url);
    var registry = this.hosts.forURL(url)[verb];
    var matches = registry.recognize(parseURL(url).fullpath);

    var match = matches ? matches[0] : null;
    if (match) {
      console.log('params: ' + JSON.stringify(match.params));
      request.params = match.params;
      request.queryParams = matches.queryParams;
    }

    return match;
  },

and got the following output:

PUT /api/service/service1234/version/1/request_settings/my%20request%20setting%20%3F%20%235
params: {"serviceId":"service1234","versionNumber":"1","name":"my request setting "}

but it should be

PUT /api/service/service1234/version/1/request_settings/my%20request%20setting%20%3F%20%235
params: {"serviceId":"service1234","versionNumber":"1","name":"my request setting ? #5"}

or at least

PUT /api/service/service1234/version/1/request_settings/my%20request%20setting%20%3F%20%235
params: {"serviceId":"service1234","versionNumber":"1","name":"my request setting %3f %235"}

if this library doesn't want to decode params. Regardless, it shouldn't strip part of a param.

@jamesarosen
Copy link
Author

Failing test:

test('URL-encoded params are decoded', function() {
  var params;
  pretender.get('/some/:x/:y/:z', function(request) {
    params = request.params;
  })

  $.ajax({url: '/some/a%20b/c%23d/e%3ff'});
  equal(params.x, 'a b');
  equal(params.y, 'c#d');
  equal(params.z, 'e?f');
});

with

Chrome 46.0.2490 (Mac OS X 10.10.5) pretender invoking URL-encoded params are decoded FAILED
Expected: c#d
Actual: c%23d

Expected: e?f
Actual: e%3ff

PhantomJS 1.9.8 (Mac OS X 0.0.0) pretender invoking URL-encoded params are decoded FAILED
Expected: e?f
Actual: e

@jamesarosen
Copy link
Author

Possibly related: tildeio/route-recognizer#71

@jamesarosen
Copy link
Author

If I apply tildeio/route-recognizer#72, and re-run my tests in Pretender, the Chrome failures go away, but the PhantomJS one doesn't.

@rwjblue
Copy link
Contributor

rwjblue commented Jul 4, 2016

I believe that this may be fixed by [email protected] (which landed tildeio/route-recognizer#91). Can someone test?

@bantic
Copy link
Member

bantic commented Aug 15, 2016

I checked this with [email protected] and the failing test above passes in Chrome v53 out-of-the-box. It does fail for me in PhantomJS 1.9.8, but if I upgrade to PhantomJS 2.1.1 the failing test case passes there also.

(I updated these npm dependencies locally to make my system happily run PhantomJS 2.1.1: karma -> 1.2.0, karma-phantomjs-launcher -> 1.0.1, karma-chrome-launcher: 1.0.1, phantomjs -> "^2.0", karma-qunit -> 1.2.0).

@trek
Copy link
Member

trek commented Aug 25, 2016

Handled by #163

@trek trek closed this as completed Aug 25, 2016
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

No branches or pull requests

4 participants