trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns15g rEventsFlows

An API path called with the wrong method answers 405 with an Allow header naming the methods it takes, not 404 no_route #8512

Merged torquecrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

torque/forge:torque/8493-an-api-path-called-with-the-wrong-method → main 1 commit fork

  • Merged. torque/8493-an-api-path-called-with-the-wrong-method was merged into main as 1aea79f.

Review

Sign in to review this change.
5 files changed +89 −27
src/http/respond.ts +13
@@ -74,6 +74,19 @@ export const forbidden = () => apiError("forbidden", 403, FORBIDDEN_MESSAGE);
7474 export const noRoute = (method: string, path: string) =>
7575 apiError("no_route", 404, `no ${method} route for ${path}`);
7676
77 // A path some route takes under other methods: the path is right, the method
78 // is not. `allowed` is sorted and carries HEAD with GET; the message leaves
79 // HEAD out, as it is implied.
80 export const methodNotAllowed = (method: string, allowed: string[]) =>
81 json(
82 {
83 error: "method_not_allowed",
84 message: `${method} is not taken here; use ${allowed.filter((m) => m !== "HEAD").join(", ")}`,
85 },
86 405,
87 { allow: allowed.join(", ") },
88 );
89
7790 /**
7891 * One media range beats another only when it names it (or a wildcard that
7992 * reaches it) at a strictly higher `q`; a tie — including the everything
src/http/router.ts +45 −27
@@ -85,37 +85,55 @@ export class Router<H> {
8585
8686 match(method: string, pathname: string): Match<H> | undefined {
8787 const parts = pathname === "/" ? [""] : pathname.slice(1).split("/");
88 outer: for (const route of this.routes_) {
88 for (const route of this.routes_) {
8989 if (route.method !== method) continue;
90 const params: Params = {};
91 for (const [i, segment] of route.segments.entries()) {
92 if (segment.kind === "rest") {
93 const decoded = parts.slice(i).map(decodeSegment);
94 if (decoded.some((d) => d === undefined)) continue outer;
95 params[segment.name] = decoded.join("/");
96 return { handler: route.handler, params, pattern: route.pattern };
97 }
98 const part = parts[i];
99 if (part === undefined) continue outer;
100 if (segment.kind === "literal") {
101 if (part !== segment.value) continue outer;
102 } else {
103 if (segment.suffix !== "" && !part.endsWith(segment.suffix))
104 continue outer;
105 const raw =
106 segment.suffix === ""
107 ? part
108 : part.slice(0, -segment.suffix.length);
109 const decoded = decodeSegment(raw);
110 if (decoded === undefined || decoded === "") continue outer;
111 params[segment.name] = decoded;
112 }
113 }
114 if (parts.length !== route.segments.length) continue;
115 return { handler: route.handler, params, pattern: route.pattern };
90 const params = matchSegments(route, parts);
91 if (params) return { handler: route.handler, params, pattern: route.pattern };
11692 }
11793 return undefined;
11894 }
95
96 /**
97 * The methods whose routes match the path, method ignored, sorted, with HEAD
98 * wherever GET is. Asked only after `match` has failed, to tell a wrong
99 * method from a wrong path.
100 */
101 allowed(pathname: string): string[] {
102 const parts = pathname === "/" ? [""] : pathname.slice(1).split("/");
103 const methods = new Set<string>();
104 for (const route of this.routes_) {
105 if (methods.has(route.method)) continue;
106 if (matchSegments(route, parts)) methods.add(route.method);
107 }
108 if (methods.has("GET")) methods.add("HEAD");
109 return [...methods].sort();
110 }
111 }
112
113 /** The params a route's segments take from the path, or undefined when they do not fit. */
114 function matchSegments<H>(route: CompiledRoute<H>, parts: string[]): Params | undefined {
115 const params: Params = {};
116 for (const [i, segment] of route.segments.entries()) {
117 if (segment.kind === "rest") {
118 const decoded = parts.slice(i).map(decodeSegment);
119 if (decoded.some((d) => d === undefined)) return undefined;
120 params[segment.name] = decoded.join("/");
121 return params;
122 }
123 const part = parts[i];
124 if (part === undefined) return undefined;
125 if (segment.kind === "literal") {
126 if (part !== segment.value) return undefined;
127 } else {
128 if (segment.suffix !== "" && !part.endsWith(segment.suffix)) return undefined;
129 const raw =
130 segment.suffix === "" ? part : part.slice(0, -segment.suffix.length);
131 const decoded = decodeSegment(raw);
132 if (decoded === undefined || decoded === "") return undefined;
133 params[segment.name] = decoded;
134 }
135 }
136 return parts.length === route.segments.length ? params : undefined;
119137 }
120138
121139 function decodeSegment(raw: string): string | undefined {
src/http/server.ts +5
@@ -456,6 +456,7 @@ import {
456456 import {
457457 apiError,
458458 json,
459 methodNotAllowed,
459460 noRoute,
460461 notFound,
461462 pageNotFoundAsJson,
@@ -2414,6 +2415,10 @@ async function answerGet(
24142415 // it offers no sign-in rather than one to someone who has signed in.
24152416 return notFoundPage(anonCtx, { plain: true });
24162417 }
2418 if (isApiPath && request.method !== "OPTIONS") {
2419 const allowed = apiRouter.allowed(url.pathname);
2420 if (allowed.length > 0) return methodNotAllowed(request.method, allowed);
2421 }
24172422 return noRoute(request.method, url.pathname);
24182423 }
24192424 const authStart = performance.now();
test/api-errors.test.ts +14
@@ -121,4 +121,18 @@ describe("API errors are distinguishable and self-describing", () => {
121121 expect(res.headers.get("referrer-policy")).toBe("same-origin");
122122 expect(res.headers.get("content-security-policy")).toBeTruthy();
123123 });
124
125 test("a known API path under the wrong method is 405 with Allow, an unknown one stays no_route", async () => {
126 const res = await fetch(`${base}/api/v1/repos`, { method: "PUT" });
127 expect(res.status).toBe(405);
128 expect(res.headers.get("allow")).toBe("GET, HEAD, POST");
129 const body = (await res.json()) as { error: string; message: string };
130 expect(body.error).toBe("method_not_allowed");
131 expect(body.message).toBe("PUT is not taken here; use GET, POST");
132 expect(res.headers.get("x-content-type-options")).toBe("nosniff");
133 expect(res.headers.get("content-security-policy")).toBeTruthy();
134 const nope = await err("/api/v1/nope", { method: "PUT" });
135 expect(nope.status).toBe(404);
136 expect(nope.error).toBe("no_route");
137 });
124138 });
test/router.test.ts +12
@@ -26,3 +26,15 @@ test("routes() lists method and pattern in the order added", () => {
2626 { method: "POST", pattern: "/a/:b" },
2727 ]);
2828 });
29
30 test("allowed names the methods a path takes, HEAD wherever GET is", () => {
31 const router = new Router<string>()
32 .add("POST", "/r", "make")
33 .add("GET", "/r", "list")
34 .add("DELETE", "/r/:id", "drop")
35 .add("GET", "/f/*rest", "files");
36 expect(router.allowed("/r")).toEqual(["GET", "HEAD", "POST"]);
37 expect(router.allowed("/r/7")).toEqual(["DELETE"]);
38 expect(router.allowed("/f/a/b")).toEqual(["GET", "HEAD"]);
39 expect(router.allowed("/nope")).toEqual([]);
40 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs