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

Escape Character %2F in Path Parameter of RestInterface Leads to 404 #34

Closed
vnayar opened this issue Mar 21, 2024 · 3 comments
Closed

Comments

@vnayar
Copy link
Contributor

vnayar commented Mar 21, 2024

Summary:

It seems as if URL decoding of URL path parameters happens before path segments are separated, leading the URLRouter to be unable to find matching routes if a path parameter value contains a URL encoded / character as %2F.

Reproduction Steps:

Consider the following example program:

import vibe.vibe;

void main()
{
  auto settings = new HTTPServerSettings;
  settings.port = 8090;
  settings.bindAddresses = ["::1", "127.0.0.1"];
  auto router = new URLRouter();
  router.registerRestInterface(new Api());
  auto listener = listenHTTP(settings, router);
  scope (exit) {
    listener.stopListening();
  }
  logInfo("Please open http://127.0.0.1:8090/ in your browser.");
  runApplication();
}

@path("/api")
interface ApiI {
  @path("/:name/age")
  string getAge(string _name) @safe;
}

class Api : ApiI {
  override
  string getAge(string _name) @safe {
    return _name ~ " is 3 years old.";
  }
}

Called with the :name parameter set to bob works as expected:

l$ curl http://localhost:8090/api/bob/age
"bob is 3 years old."

However, escaping a / character as %2F causes a 404 error. For example, URL encoding the value bob/ham results in bob%2Fham and the following interaction:

$ curl http://localhost:8090/api/bob%2Fham/age
404 - Not Found

Not Found

Internal error information:
No routes match path '/api/bob%2Fham/age'

Expected Result:

For the URL http://localhost:8090/api/bob%2Fham/age, the value bob%2Fham should have been recognized as being the path parameter :name, URL decoded, and then given as the function parameter _name with value bob/ham.

@s-ludwig s-ludwig transferred this issue from vibe-d/vibe.d Mar 24, 2024
@s-ludwig
Copy link
Member

Relevant place in the implementation:

// NOTE: Instead of failing, we just ignore requests with invalid path
// segments (i.e. containing path separators) here. Any request
// handlers later in the queue may still choose to process them
// appropriately.
try path = (cast(PosixPath)req.requestPath).toString();
catch (Exception e) return;

In order to allow matching such internal slashes, we'd have to do some deeper changes here, but as long as they are just part of a placeholder, we may get away with replacing slashes in path segments with some Unicode character as a replacement for the matching process. When filling the placeholders later, this would then have to be reverted appropriately.

It does have the disadvantage of being ambiguous for the case where a path segment actually already contains this replacement character in the input and we'd have to think well about possible security implications (and in doubt just ignore request that contain them).

vnayar added a commit to vnayar/vibe-core that referenced this issue Mar 24, 2024
This utility function will be used to url-encode paths added to a URLRouter, which
avoids the problem of having to resolve encoding problems when resolving routes.

See: vibe-d/vibe-http#34
vnayar added a commit to vnayar/vibe-http that referenced this issue Mar 24, 2024
There are two changes in this MR that work together:
* Remove logic to abort routing if a path segment contains a path separator, e.g. '%2F'.
* URL path parameters and decoded before being passed to route handlers.
* To still permit matching URL routes that are not URL encoded, e.g. '/foo bar', encode
  these paths before adding them to route matching.

See: vibe-d#34
@vnayar
Copy link
Contributor Author

vnayar commented Mar 24, 2024

I added two small changes that should make this possible:

@vnayar
Copy link
Contributor Author

vnayar commented Apr 24, 2024

Issue is resolved with the merging of #35.

@vnayar vnayar closed this as completed Apr 24, 2024
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

2 participants