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

[enh] Support multiple '/' in URL #410

Closed
Geod24 opened this Issue Nov 27, 2013 · 2 comments

Comments

Projects
None yet
2 participants
@Geod24
Contributor

Geod24 commented Nov 27, 2013

Hi,
Using the test application, this works:

http://42.42.42.42:8080/
http://42.42.42.42:8080///////////

=> Same result, as expected.

But given the following code:

auto routes = new URLRouter;
routes.any("/test", &hello);
listenHTTP(settings, routes);

Those behaves differently:

http://42.42.42.42:8080/test/
http://42.42.42.42:8080/////test//////

Of course that's a silly way of writting URL. In addition, URI's RFC (http://www.ietf.org/rfc/rfc2396.txt) describe the identifier as single slash. So current behaviour is correct. But that's a common practice in web server to just ignore strings of slash.

I think vibe should exhibit the same behavior for different reasons:

  • Better compatibility with existing software;
  • Users will expect this;
  • Expecting server/path/to/file and server//path/to/file to serve different content is plain wrong;
  • Current behavior makes URL rewritting slightly more painful (that's how I ran into it).

And as a bonus, a little bug I found along the way:

auto routes = new URLRouter;
routes.any("*", &debug);

[...]
void debug(HTTPServerRequest req, HTTPServerResponse res)
{
        logInfo("[REQUEST] => "  ~  to!string(req.method)
                   ~ " " ~ req.fullURL.toString());
}

Throws:

Internal error information:
object.Exception@../.dub/packages/vibe-d-0.7.18/source/vibe/inet/path.d(370): Empty path entries not allowed.
----------------
./testfail(pure @safe bool std.exception.enforce!(bool).enforce(bool, lazy const(char)[], immutable(char)[], ulong)+0x6b) [0x669847]
./testfail(vibe.inet.path.PathEntry[] vibe.inet.path.splitPath(immutable(char)[])+0x414) [0x710768]
./testfail(ref vibe.inet.path.Path vibe.inet.path.Path.__ctor(immutable(char)[])+0x28) [0x70ead4]
./testfail(const(@property vibe.inet.url.URL function()) vibe.http.server.HTTPServerRequest.fullURL+0x261) [0x645e3d]
./testfail(void app.debug(vibe.http.server.HTTPServerRequest, vibe.http.server.HTTPServerResponse)+0x23) [0x5f9ef7]
[...]
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig May 14, 2014

Member

A few remarks:

  • Treating "///test///", or even "/././test//" the same as "/test/" in the URLRouter would considerably increase the complexity of the match algorithm and all browsers that I've tested perform path normalization before sending the final request, so I'd treat this as a low priority issue
  • In case of serving files, though, the behavior should be as expected and ""/"."/".." are correctly resolved
  • The "Empty path entries not..." error is fixed now by the above commit
Member

s-ludwig commented May 14, 2014

A few remarks:

  • Treating "///test///", or even "/././test//" the same as "/test/" in the URLRouter would considerably increase the complexity of the match algorithm and all browsers that I've tested perform path normalization before sending the final request, so I'd treat this as a low priority issue
  • In case of serving files, though, the behavior should be as expected and ""/"."/".." are correctly resolved
  • The "Empty path entries not..." error is fixed now by the above commit
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig May 16, 2014

Member

Closing in favor of #667

Member

s-ludwig commented May 16, 2014

Closing in favor of #667

@s-ludwig s-ludwig closed this May 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment