Skip to content

Commit 313ea52

Browse files
committed
fix(app): decode percent-encoded path segments to prevent auth bypass
Middleware using `event.path.startsWith()` for access control could be bypassed by percent-encoding characters (e.g. `/api/%61dmin` instead of `/api/admin`). With wildcard/catch-all routes, the encoded path would skip the middleware check but still match the route handler. Path segments are now decoded via `decodePath()` from ufo early in the request lifecycle. Query strings are left untouched to avoid double-decoding.
1 parent c049dc0 commit 313ea52

File tree

2 files changed

+338
-3
lines changed

2 files changed

+338
-3
lines changed

src/app.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { joinURL, parseURL, withoutTrailingSlash } from "ufo";
1+
import { joinURL, parseURL, withoutTrailingSlash, decodePath } from "ufo";
22
import type { AdapterOptions as WSOptions, Peer } from "crossws";
33
import {
44
lazyEventHandler,
@@ -140,8 +140,10 @@ export function createAppEventHandler(stack: Stack, options: AppOptions) {
140140
event.node.req.originalUrl =
141141
event.node.req.originalUrl || event.node.req.url || "/";
142142

143-
// Keep a copy of incoming url
144-
const _reqPath = event._path || event.node.req.url || "/";
143+
// Decode percent-encoded path segments to prevent auth bypass via encoding tricks.
144+
// Only decode the path portion, not the query string, to avoid double-decoding.
145+
const _reqPath = _decodePath(event._path || event.node.req.url || "/");
146+
event._path = _reqPath;
145147

146148
// Layer path is the path without the prefix
147149
let _layerPath: string;
@@ -340,6 +342,14 @@ function cachedFn<T>(fn: () => T): () => T {
340342
};
341343
}
342344

345+
function _decodePath(url: string): string {
346+
const qIndex = url.indexOf("?");
347+
if (qIndex === -1) {
348+
return decodePath(url);
349+
}
350+
return decodePath(url.slice(0, qIndex)) + url.slice(qIndex);
351+
}
352+
343353
function websocketOptions(
344354
evResolver: EventHandlerResolver,
345355
appOptions: AppOptions,

test/security.test.ts

Lines changed: 325 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,325 @@
1+
import supertest, { SuperTest, Test } from "supertest";
2+
import { describe, it, expect, beforeEach } from "vitest";
3+
import {
4+
createApp,
5+
createRouter,
6+
App,
7+
Router,
8+
toNodeListener,
9+
eventHandler,
10+
getHeader,
11+
createError,
12+
getRouterParams,
13+
getQuery,
14+
useBase,
15+
} from "../src";
16+
17+
describe("security: path encoding bypass", () => {
18+
let app: App;
19+
let router: Router;
20+
let request: SuperTest<Test>;
21+
22+
beforeEach(() => {
23+
app = createApp({ debug: false });
24+
25+
// Middleware that protects /api/admin routes
26+
app.use(
27+
eventHandler((event) => {
28+
if (event.path.startsWith("/api/admin")) {
29+
const token = getHeader(event, "authorization");
30+
if (token !== "Bearer admin-secret-token") {
31+
throw createError({ statusCode: 403, statusMessage: "Forbidden" });
32+
}
33+
}
34+
}),
35+
);
36+
37+
router = createRouter();
38+
39+
// Protected admin endpoint with dynamic param
40+
router.get(
41+
"/api/admin/:action",
42+
eventHandler((event) => {
43+
const params = getRouterParams(event, { decode: true });
44+
return { admin: true, action: params.action };
45+
}),
46+
);
47+
48+
// Public endpoint
49+
router.get(
50+
"/api/public",
51+
eventHandler(() => {
52+
return { public: true };
53+
}),
54+
);
55+
56+
app.use(router);
57+
request = supertest(toNodeListener(app)) as any;
58+
});
59+
60+
it("blocks unauthenticated access to /api/admin/users", async () => {
61+
const res = await request.get("/api/admin/users");
62+
expect(res.status).toBe(403);
63+
});
64+
65+
it("allows authenticated access to /api/admin/users", async () => {
66+
const res = await request
67+
.get("/api/admin/users")
68+
.set("Authorization", "Bearer admin-secret-token");
69+
expect(res.status).toBe(200);
70+
expect(res.body).toEqual({ admin: true, action: "users" });
71+
});
72+
73+
it("allows access to public endpoint", async () => {
74+
const res = await request.get("/api/public");
75+
expect(res.status).toBe(200);
76+
});
77+
78+
// Percent-encoding a single character in the protected prefix
79+
it("should NOT bypass auth via /api/%61dmin/users (%61 = a)", async () => {
80+
const res = await request.get("/api/%61dmin/users");
81+
expect(res.status).not.toBe(200);
82+
});
83+
84+
it("should NOT bypass auth via /api/admi%6e/users (%6e = n)", async () => {
85+
const res = await request.get("/api/admi%6e/users");
86+
expect(res.status).not.toBe(200);
87+
});
88+
89+
// Encoding in a different path segment
90+
it("should NOT bypass auth via /%61pi/admin/users (%61 = a)", async () => {
91+
const res = await request.get("/%61pi/admin/users");
92+
expect(res.status).not.toBe(200);
93+
});
94+
95+
// Double-encoded percent (%25 = literal %) should not resolve to the protected path
96+
it("should NOT bypass auth via double encoding /api/%2561dmin/users", async () => {
97+
const res = await request.get("/api/%2561dmin/users");
98+
expect(res.status).not.toBe(200);
99+
});
100+
101+
// Uppercase hex variants
102+
it("should NOT bypass auth via /api/%41dmin/users (uppercase %41 = A)", async () => {
103+
const res = await request.get("/api/%41dmin/users");
104+
expect(res.status).not.toBe(200);
105+
});
106+
107+
// Multiple encoded characters
108+
it("should NOT bypass auth via fully encoded /api/admin path", async () => {
109+
// /api/%61%64%6d%69%6e/users = /api/admin/users
110+
const res = await request.get("/api/%61%64%6d%69%6e/users");
111+
expect(res.status).not.toBe(200);
112+
});
113+
});
114+
115+
describe("security: path encoding bypass with wildcard routes", () => {
116+
let app: App;
117+
let router: Router;
118+
let request: SuperTest<Test>;
119+
120+
beforeEach(() => {
121+
app = createApp({ debug: false });
122+
123+
// Middleware protecting /api/admin
124+
app.use(
125+
eventHandler((event) => {
126+
if (event.path.startsWith("/api/admin")) {
127+
const token = getHeader(event, "authorization");
128+
if (token !== "Bearer admin-secret-token") {
129+
throw createError({ statusCode: 403, statusMessage: "Forbidden" });
130+
}
131+
}
132+
}),
133+
);
134+
135+
router = createRouter();
136+
137+
// Catch-all route (simulates Nuxt/Nitro file-based routing)
138+
router.get(
139+
"/api/**",
140+
eventHandler((event) => {
141+
return { path: event.path, params: getRouterParams(event) };
142+
}),
143+
);
144+
145+
app.use(router);
146+
request = supertest(toNodeListener(app)) as any;
147+
});
148+
149+
it("blocks /api/admin/users without auth via wildcard", async () => {
150+
const res = await request.get("/api/admin/users");
151+
expect(res.status).toBe(403);
152+
});
153+
154+
it("should NOT bypass auth with wildcard via /api/%61dmin/users", async () => {
155+
const res = await request.get("/api/%61dmin/users");
156+
expect(res.status).not.toBe(200);
157+
});
158+
159+
it("should NOT bypass auth with wildcard via /api/admi%6e/users", async () => {
160+
const res = await request.get("/api/admi%6e/users");
161+
expect(res.status).not.toBe(200);
162+
});
163+
});
164+
165+
describe("path decoding: no regressions", () => {
166+
let app: App;
167+
let router: Router;
168+
let request: SuperTest<Test>;
169+
170+
beforeEach(() => {
171+
app = createApp({ debug: false });
172+
router = createRouter();
173+
174+
// Echo handler returning path and query info
175+
router.get(
176+
"/echo/**",
177+
eventHandler((event) => {
178+
return {
179+
path: event.path,
180+
query: getQuery(event),
181+
params: getRouterParams(event),
182+
};
183+
}),
184+
);
185+
186+
router.get(
187+
"/item/:id",
188+
eventHandler((event) => {
189+
return {
190+
path: event.path,
191+
params: getRouterParams(event),
192+
};
193+
}),
194+
);
195+
196+
app.use(router);
197+
request = supertest(toNodeListener(app)) as any;
198+
});
199+
200+
it("preserves query strings without double-decoding", async () => {
201+
const res = await request.get("/echo/test?val=%2561");
202+
expect(res.status).toBe(200);
203+
// %2561 should stay raw in the query portion of event.path
204+
expect(res.body.path).toBe("/echo/test?val=%2561");
205+
// getQuery decodes once: %2561 -> %61 (literal percent-sixty-one)
206+
expect(res.body.query.val).toBe("%61");
207+
});
208+
209+
it("preserves encoded values in query strings", async () => {
210+
const res = await request.get(
211+
"/echo/test?name=hello%20world&redirect=%2Fhome",
212+
);
213+
expect(res.status).toBe(200);
214+
expect(res.body.query.name).toBe("hello world");
215+
expect(res.body.query.redirect).toBe("/home");
216+
});
217+
218+
it("preserves query string with multiple params", async () => {
219+
const res = await request.get("/echo/test?a=1&b=2&c=%263");
220+
expect(res.status).toBe(200);
221+
expect(res.body.query).toMatchObject({ a: "1", b: "2", c: "&3" });
222+
});
223+
224+
it("decodes path but not query in the same request", async () => {
225+
// Path has %74 (t), query has %2561 (should stay as-is)
226+
const res = await request.get("/echo/%74est?key=%2561");
227+
expect(res.status).toBe(200);
228+
expect(res.body.path).toBe("/echo/test?key=%2561");
229+
expect(res.body.query.key).toBe("%61");
230+
});
231+
232+
it("preserves encoded slash %2F in path (not decoded to /)", async () => {
233+
const res = await request.get("/echo/a%2Fb");
234+
expect(res.status).toBe(200);
235+
// decodePath from ufo preserves %2F
236+
expect(res.body.path).toBe("/echo/a%2Fb");
237+
});
238+
239+
it("decodes space %20 in path segments", async () => {
240+
const res = await request.get("/echo/hello%20world");
241+
expect(res.status).toBe(200);
242+
expect(res.body.path).toBe("/echo/hello world");
243+
});
244+
245+
it("handles already-decoded paths unchanged", async () => {
246+
const res = await request.get("/echo/normal/path");
247+
expect(res.status).toBe(200);
248+
expect(res.body.path).toBe("/echo/normal/path");
249+
});
250+
251+
it("handles path with no query string", async () => {
252+
const res = await request.get("/item/42");
253+
expect(res.status).toBe(200);
254+
expect(res.body.path).toBe("/item/42");
255+
expect(res.body.params).toEqual({ id: "42" });
256+
});
257+
258+
it("handles empty query string", async () => {
259+
const res = await request.get("/echo/test?");
260+
expect(res.status).toBe(200);
261+
// Node.js/supertest strips trailing empty "?", so path has no query
262+
expect(res.body.path).toBe("/echo/test");
263+
});
264+
265+
it("originalUrl preserves the raw request URL", async () => {
266+
const router2 = createRouter();
267+
const app2 = createApp({ debug: false });
268+
let capturedOriginalUrl: string | undefined;
269+
app2.use(
270+
eventHandler((event) => {
271+
capturedOriginalUrl = event.node.req.originalUrl;
272+
}),
273+
);
274+
router2.get(
275+
"/test/**",
276+
eventHandler(() => "ok"),
277+
);
278+
app2.use(router2);
279+
const req2 = supertest(toNodeListener(app2)) as any;
280+
await req2.get("/test/%61bc");
281+
expect(capturedOriginalUrl).toBe("/test/%61bc");
282+
});
283+
});
284+
285+
describe("path decoding with useBase", () => {
286+
let app: App;
287+
let request: SuperTest<Test>;
288+
289+
beforeEach(() => {
290+
app = createApp({ debug: false });
291+
const baseHandler = eventHandler((event) => {
292+
return { path: event.path };
293+
});
294+
app.use("/api", useBase("/api", baseHandler));
295+
request = supertest(toNodeListener(app)) as any;
296+
});
297+
298+
it("decodes path with useBase prefix", async () => {
299+
const res = await request.get("/api/t%65st");
300+
expect(res.status).toBe(200);
301+
// useBase strips the /api prefix, so handler sees /test
302+
expect(res.body.path).toBe("/test");
303+
});
304+
});
305+
306+
describe("path decoding with onRequest hook", () => {
307+
it("event.path is decoded in onRequest", async () => {
308+
let hookPath: string | undefined;
309+
const app = createApp({
310+
debug: false,
311+
onRequest(event) {
312+
hookPath = event.path;
313+
},
314+
});
315+
const router = createRouter();
316+
router.get(
317+
"/api/**",
318+
eventHandler(() => "ok"),
319+
);
320+
app.use(router);
321+
const request = supertest(toNodeListener(app)) as any;
322+
await request.get("/api/%61dmin");
323+
expect(hookPath).toBe("/api/admin");
324+
});
325+
});

0 commit comments

Comments
 (0)