From 75088ac09083f22c9a803c4ec620853c7f5ff09d Mon Sep 17 00:00:00 2001 From: Vijay Nayar Date: Sun, 24 Mar 2024 18:33:25 +0100 Subject: [PATCH 1/6] Decode path parameters and encode URLRouter paths. 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: https://github.com/vibe-d/vibe-http/issues/34 --- source/vibe/http/router.d | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/source/vibe/http/router.d b/source/vibe/http/router.d index 31b9219..d799e02 100644 --- a/source/vibe/http/router.d +++ b/source/vibe/http/router.d @@ -125,11 +125,14 @@ final class URLRouter : HTTPServerRequestHandler { URLRouter match(Handler)(HTTPMethod method, string path, Handler handler) if (isValidHandler!Handler) { + import vibe.core.path : InetPath; import std.algorithm; assert(path.length, "Cannot register null or empty path!"); assert(count(path, ':') <= maxRouteParameters, "Too many route parameters"); logDebug("add route %s %s", method, path); - m_routes.addTerminal(path, Route(method, path, handlerDelegate(handler))); + // Perform URL-encoding on the path before adding it as a route. + string iPath = InetPath.fromUnencodedString(path).toString(); + m_routes.addTerminal(iPath, Route(method, iPath, handlerDelegate(handler))); return this; } @@ -193,7 +196,7 @@ final class URLRouter : HTTPServerRequestHandler { /// Handles a HTTP request by dispatching it to the registered route handlers. void handleRequest(HTTPServerRequest req, HTTPServerResponse res) { - import vibe.core.path : PosixPath; + import vibe.core.path : PosixPath, InetPathFormat; auto method = req.method; @@ -211,7 +214,9 @@ final class URLRouter : HTTPServerRequestHandler { // 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(); + //try path = (cast(PosixPath)req.requestPath).toString(); + // ^ This causes URLs like /bob/x%2Fb to fail to match. + try path = req.requestPath.toString(); catch (Exception e) return; if (path.length < m_prefix.length || path[0 .. m_prefix.length] != m_prefix) return; @@ -223,7 +228,12 @@ final class URLRouter : HTTPServerRequestHandler { if (r.method != method) return false; logDebugV("route match: %s -> %s %s %s", req.requestPath, r.method, r.pattern, values); - foreach (i, v; values) req.params[m_routes.getTerminalVarNames(ridx)[i]] = v; + foreach (i, v; values) { + import std.uri : decodeComponent; + import vibe.core.path : InetPath; + req.params[m_routes.getTerminalVarNames(ridx)[i]] = decodeComponent(v); + //req.params[m_routes.getTerminalVarNames(ridx)[i]] = InetPathFormat.decodeSingleSegment(v); + } if (m_computeBasePath) req.params["routerRootDir"] = calcBasePath(); r.cb(req, res); return res.headerWritten; @@ -446,10 +456,12 @@ final class URLRouter : HTTPServerRequestHandler { void b(HTTPServerRequest req, HTTPServerResponse) { result ~= "B"; } void c(HTTPServerRequest req, HTTPServerResponse) { assert(req.params["test"] == "x", "Wrong variable contents: "~req.params["test"]); result ~= "C"; } void d(HTTPServerRequest req, HTTPServerResponse) { assert(req.params["test"] == "y", "Wrong variable contents: "~req.params["test"]); result ~= "D"; } + void e(HTTPServerRequest req, HTTPServerResponse) { assert(req.params["test"] == "z/z", "Wrong variable contents: "~req.params["test"]); result ~= "E"; } router.get("/test", &a); router.post("/test", &b); router.get("/a/:test", &c); router.get("/a/:test/", &d); + router.get("/e/:test", &e); auto res = createTestHTTPServerResponse(); router.handleRequest(createTestHTTPServerRequest(URL("http://localhost/")), res); @@ -467,6 +479,8 @@ final class URLRouter : HTTPServerRequestHandler { //assert(result == "ABC", "Matched empty string or slash as var. "~result); router.handleRequest(createTestHTTPServerRequest(URL("http://localhost/a/y/"), HTTPMethod.GET), res); assert(result == "ABCD", "Didn't match 1-character infix variable."); + router.handleRequest(createTestHTTPServerRequest(URL("http://localhost/e/z%2Fz"), HTTPMethod.GET), res); + assert(result == "ABCDE", "URL-escaped '/' confused router."); } @safe unittest { @@ -774,7 +788,7 @@ private struct MatchTree(T) { dst[] = null; - // folow the path throgh the match graph + // follow the path through the match graph foreach (i, char ch; text) { auto var = term.varMap.get(nidx, VarIndex.max); From 92a559d8539a2f715ae58a281082a5c96622b40b Mon Sep 17 00:00:00 2001 From: Vijay Nayar Date: Sun, 24 Mar 2024 21:37:41 +0100 Subject: [PATCH 2/6] Replace use of with . --- source/vibe/http/router.d | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/vibe/http/router.d b/source/vibe/http/router.d index d799e02..6487c2a 100644 --- a/source/vibe/http/router.d +++ b/source/vibe/http/router.d @@ -125,13 +125,13 @@ final class URLRouter : HTTPServerRequestHandler { URLRouter match(Handler)(HTTPMethod method, string path, Handler handler) if (isValidHandler!Handler) { - import vibe.core.path : InetPath; + import vibe.core.path : InetPath, PosixPath; import std.algorithm; assert(path.length, "Cannot register null or empty path!"); assert(count(path, ':') <= maxRouteParameters, "Too many route parameters"); logDebug("add route %s %s", method, path); // Perform URL-encoding on the path before adding it as a route. - string iPath = InetPath.fromUnencodedString(path).toString(); + string iPath = (cast(InetPath) PosixPath(path)).toString(); m_routes.addTerminal(iPath, Route(method, iPath, handlerDelegate(handler))); return this; } From 1d7abe235e1a4d5a03d7a8e3cb294acbf056fe58 Mon Sep 17 00:00:00 2001 From: Vijay Nayar Date: Tue, 26 Mar 2024 23:10:09 +0100 Subject: [PATCH 3/6] Add logic to decode percent-encoded but unreserved characters during path matching. --- source/vibe/http/router.d | 44 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/source/vibe/http/router.d b/source/vibe/http/router.d index 6487c2a..f11af88 100644 --- a/source/vibe/http/router.d +++ b/source/vibe/http/router.d @@ -461,7 +461,7 @@ final class URLRouter : HTTPServerRequestHandler { router.post("/test", &b); router.get("/a/:test", &c); router.get("/a/:test/", &d); - router.get("/e/:test", &e); + router.get("/e/:test", &e); auto res = createTestHTTPServerResponse(); router.handleRequest(createTestHTTPServerRequest(URL("http://localhost/")), res); @@ -532,6 +532,7 @@ final class URLRouter : HTTPServerRequestHandler { //assert(ensureMatch("/foo%2fbar/", "/foo/bar/") !is null); //assert(ensureMatch("/:foo/", "/foo%2Fbar/", ["foo": "foo/bar"]) is null); assert(ensureMatch("/:foo/", "/foo/bar/") !is null); + assert(ensureMatch("/test", "/tes%74") is null); } unittest { // issue #2561 @@ -760,6 +761,36 @@ private struct MatchTree(T) { return false; } + private static uint hexDigit(char ch) @safe nothrow @nogc { + assert(ch >= '0' && ch <= '9' || ch >= 'A' && ch <= 'F' || ch >= 'a' && ch <= 'f'); + if (ch >= '0' && ch <= '9') return ch - '0'; + else if (ch >= 'a' && ch <= 'f') return ch - 'a' + 10; + else return ch - 'A' + 10; + } + + /// Reads a single character from text, decoding any unreserved percent-encoded character, so + /// that it matches the format used for route matches. + static char nextMatchChar(string text, ref size_t i) { + import std.ascii : isHexDigit; + + char ch = text[i]; + // See if we have to decode an encoded unreserved character. + if (ch == '%' && i + 2 < text.length && isHexDigit(text[i+1]) && isHexDigit(text[i+2])) { + uint c = hexDigit(text[i+1]) * 16 + hexDigit(text[i+2]); + // Check if we have an encoded unreserved character: + // https://en.wikipedia.org/wiki/Percent-encoding + if (c >= 'A' && c <= 'Z' || c >= 'a' && c <= 'z' || c >= '0' && c <= '9' + || c == '-' || c == '_' || c == '.' || c == '~') { + // Decode the character before performing route matching. + ch = cast(char) c; + i += 3; + return ch; + } + } + i += 1; + return ch; + } + private inout(Node)* matchTerminals(string text) inout { if (!m_nodes.length) return null; @@ -767,7 +798,12 @@ private struct MatchTree(T) { auto n = &m_nodes[0]; // follow the path through the match graph - foreach (i, char ch; text) { + + // Routes match according to their percent-encoded normal form, with reserved-characters + // percent-encoded and unreserved-charcters not percent-encoded. + size_t i = 0; + while (i < text.length) { + char ch = nextMatchChar(text, i); auto nidx = n.edges[cast(size_t)ch]; if (nidx == NodeIndex.max) return null; n = &m_nodes[nidx]; @@ -789,7 +825,8 @@ private struct MatchTree(T) { dst[] = null; // follow the path through the match graph - foreach (i, char ch; text) { + size_t i = 0; + while (i < text.length) { auto var = term.varMap.get(nidx, VarIndex.max); // detect end of variable @@ -804,6 +841,7 @@ private struct MatchTree(T) { activevarstart = i; } + char ch = nextMatchChar(text, i); nidx = m_nodes[nidx].edges[cast(ubyte)ch]; assert(nidx != NodeIndex.max); } From fef4345b48711289bd63ce521b51461e9b1f884b Mon Sep 17 00:00:00 2001 From: Vijay Nayar Date: Tue, 23 Apr 2024 11:28:17 +0200 Subject: [PATCH 4/6] Change router path format from Posix to Inet to permit URL-escaped path characters. --- source/vibe/http/router.d | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/source/vibe/http/router.d b/source/vibe/http/router.d index f11af88..ad93819 100644 --- a/source/vibe/http/router.d +++ b/source/vibe/http/router.d @@ -131,7 +131,8 @@ final class URLRouter : HTTPServerRequestHandler { assert(count(path, ':') <= maxRouteParameters, "Too many route parameters"); logDebug("add route %s %s", method, path); // Perform URL-encoding on the path before adding it as a route. - string iPath = (cast(InetPath) PosixPath(path)).toString(); + //string iPath = (cast(InetPath) PosixPath(path)).toString(); + string iPath = InetPath(path).toString(); m_routes.addTerminal(iPath, Route(method, iPath, handlerDelegate(handler))); return this; } @@ -457,11 +458,13 @@ final class URLRouter : HTTPServerRequestHandler { void c(HTTPServerRequest req, HTTPServerResponse) { assert(req.params["test"] == "x", "Wrong variable contents: "~req.params["test"]); result ~= "C"; } void d(HTTPServerRequest req, HTTPServerResponse) { assert(req.params["test"] == "y", "Wrong variable contents: "~req.params["test"]); result ~= "D"; } void e(HTTPServerRequest req, HTTPServerResponse) { assert(req.params["test"] == "z/z", "Wrong variable contents: "~req.params["test"]); result ~= "E"; } + void f(HTTPServerRequest req, HTTPServerResponse) { result ~= "F"; } router.get("/test", &a); router.post("/test", &b); router.get("/a/:test", &c); router.get("/a/:test/", &d); router.get("/e/:test", &e); + router.get("/f%2F", &f); auto res = createTestHTTPServerResponse(); router.handleRequest(createTestHTTPServerRequest(URL("http://localhost/")), res); @@ -481,6 +484,8 @@ final class URLRouter : HTTPServerRequestHandler { assert(result == "ABCD", "Didn't match 1-character infix variable."); router.handleRequest(createTestHTTPServerRequest(URL("http://localhost/e/z%2Fz"), HTTPMethod.GET), res); assert(result == "ABCDE", "URL-escaped '/' confused router."); + router.handleRequest(createTestHTTPServerRequest(URL("http://localhost/f%2F"), HTTPMethod.GET), res); + assert(result == "ABCDEF", "Unable to match '%2F' in path."); } @safe unittest { @@ -522,8 +527,8 @@ final class URLRouter : HTTPServerRequestHandler { return ret; } - assert(ensureMatch("/foo bar/", "/foo%20bar/") is null); // normalized pattern: "/foo%20bar/" - //assert(ensureMatch("/foo%20bar/", "/foo%20bar/") is null); // normalized pattern: "/foo%20bar/" + //assert(ensureMatch("/foo bar/", "/foo%20bar/") is null); // normalized pattern: "/foo%20bar/" + assert(ensureMatch("/foo%20bar/", "/foo%20bar/") is null); // normalized pattern: "/foo%20bar/" assert(ensureMatch("/foo/bar/", "/foo/bar/") is null); // normalized pattern: "/foo/bar/" //assert(ensureMatch("/foo/bar/", "/foo%2fbar/") !is null); //assert(ensureMatch("/foo%2fbar/", "/foo%2fbar/") is null); // normalized pattern: "/foo%2Fbar/" From 886fffaf30612a0086d573da110a65bcd391b5f8 Mon Sep 17 00:00:00 2001 From: Vijay Nayar Date: Tue, 23 Apr 2024 22:00:14 +0200 Subject: [PATCH 5/6] Apply s-ludwig's suggested algorithm to insert normalized paths into the router. --- source/vibe/http/router.d | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/source/vibe/http/router.d b/source/vibe/http/router.d index ad93819..90fe52f 100644 --- a/source/vibe/http/router.d +++ b/source/vibe/http/router.d @@ -126,13 +126,17 @@ final class URLRouter : HTTPServerRequestHandler { if (isValidHandler!Handler) { import vibe.core.path : InetPath, PosixPath; - import std.algorithm; + import vibe.textfilter.urlencode : urlDecode; + import std.algorithm : count, map; assert(path.length, "Cannot register null or empty path!"); assert(count(path, ':') <= maxRouteParameters, "Too many route parameters"); logDebug("add route %s %s", method, path); // Perform URL-encoding on the path before adding it as a route. - //string iPath = (cast(InetPath) PosixPath(path)).toString(); - string iPath = InetPath(path).toString(); + string iPath = PosixPath(path) + .bySegment + .map!(s => InetPath.Segment(urlDecode(s.name), s.separator)) + .InetPath + .toString; m_routes.addTerminal(iPath, Route(method, iPath, handlerDelegate(handler))); return this; } @@ -215,8 +219,6 @@ final class URLRouter : HTTPServerRequestHandler { // 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(); - // ^ This causes URLs like /bob/x%2Fb to fail to match. try path = req.requestPath.toString(); catch (Exception e) return; @@ -527,7 +529,7 @@ final class URLRouter : HTTPServerRequestHandler { return ret; } - //assert(ensureMatch("/foo bar/", "/foo%20bar/") is null); // normalized pattern: "/foo%20bar/" + assert(ensureMatch("/foo bar/", "/foo%20bar/") is null); // normalized pattern: "/foo%20bar/" assert(ensureMatch("/foo%20bar/", "/foo%20bar/") is null); // normalized pattern: "/foo%20bar/" assert(ensureMatch("/foo/bar/", "/foo/bar/") is null); // normalized pattern: "/foo/bar/" //assert(ensureMatch("/foo/bar/", "/foo%2fbar/") !is null); From 1d24e181a2a5c3efdfffab8341baac4ddbb313cd Mon Sep 17 00:00:00 2001 From: Vijay Nayar Date: Wed, 24 Apr 2024 09:34:43 +0200 Subject: [PATCH 6/6] Code cleanup. --- source/vibe/http/router.d | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/source/vibe/http/router.d b/source/vibe/http/router.d index 90fe52f..ecf705b 100644 --- a/source/vibe/http/router.d +++ b/source/vibe/http/router.d @@ -201,7 +201,7 @@ final class URLRouter : HTTPServerRequestHandler { /// Handles a HTTP request by dispatching it to the registered route handlers. void handleRequest(HTTPServerRequest req, HTTPServerResponse res) { - import vibe.core.path : PosixPath, InetPathFormat; + import vibe.textfilter.urlencode : urlDecode; auto method = req.method; @@ -232,10 +232,7 @@ final class URLRouter : HTTPServerRequestHandler { logDebugV("route match: %s -> %s %s %s", req.requestPath, r.method, r.pattern, values); foreach (i, v; values) { - import std.uri : decodeComponent; - import vibe.core.path : InetPath; - req.params[m_routes.getTerminalVarNames(ridx)[i]] = decodeComponent(v); - //req.params[m_routes.getTerminalVarNames(ridx)[i]] = InetPathFormat.decodeSingleSegment(v); + req.params[m_routes.getTerminalVarNames(ridx)[i]] = urlDecode(v); } if (m_computeBasePath) req.params["routerRootDir"] = calcBasePath(); r.cb(req, res); @@ -768,8 +765,9 @@ private struct MatchTree(T) { return false; } + /// Given a hexadecimal character in [0-9a-fA-F], convert it to an integer value in [0, 15]. private static uint hexDigit(char ch) @safe nothrow @nogc { - assert(ch >= '0' && ch <= '9' || ch >= 'A' && ch <= 'F' || ch >= 'a' && ch <= 'f'); + assert((ch >= '0' && ch <= '9') || (ch >= 'A' && ch <= 'F') || (ch >= 'a' && ch <= 'f')); if (ch >= '0' && ch <= '9') return ch - '0'; else if (ch >= 'a' && ch <= 'f') return ch - 'a' + 10; else return ch - 'A' + 10;