-
Notifications
You must be signed in to change notification settings - Fork 754
feat: Make URL rewriting possible #3812
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
f382217
7f552d5
a882df7
3bfeda1
b23615e
01b5067
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,47 @@ export const DEFAULT_CONN_INFO: any = { | |
| remoteAddr: { transport: "tcp", hostname: "localhost", port: 1234 }, | ||
| }; | ||
|
|
||
| const MAX_REWRITE_COUNT = 16; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 16 feels generous for an internal-rewrite cap — even legitimate rewrite chains rarely go past 2–3 hops. Not a blocker, but
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're absolutely right. Lets go with 8. |
||
|
|
||
| function normalizeRequestUrl(req: Request, trustProxy: boolean): URL { | ||
| const url = new URL(req.url); | ||
| // Prevent open redirect attacks. | ||
| url.pathname = url.pathname.replace(/\/+/g, "/"); | ||
|
|
||
| // Apply X-Forwarded-* headers when behind a reverse proxy. | ||
| if (trustProxy) { | ||
| const proto = req.headers.get("x-forwarded-proto"); | ||
| if (proto) { | ||
| url.protocol = proto + ":"; | ||
| } | ||
| const host = req.headers.get("x-forwarded-host"); | ||
| if (host) { | ||
| url.host = host; | ||
| } | ||
| } | ||
|
|
||
| return url; | ||
| } | ||
|
|
||
| function getRewriteUrl(currentUrl: URL, pathOrUrl: string | URL): URL { | ||
| const rewritten = pathOrUrl instanceof URL | ||
| ? new URL(pathOrUrl) | ||
| : new URL(pathOrUrl, currentUrl); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. basePath footgun: this resolves the string target against Two reasonable options:
Either is fine, but the current behavior is going to surprise users with a non-empty basePath.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's go with the auto-prefixing to keep it consistent with the route registration |
||
|
|
||
| if (rewritten.origin !== currentUrl.origin) { | ||
| throw new Error( | ||
| `ctx.rewrite() only supports same-origin URLs. Expected "${currentUrl.origin}", got "${rewritten.origin}"`, | ||
| ); | ||
| } | ||
|
|
||
| // Keep existing query params unless the target explicitly sets a query. | ||
| if (typeof pathOrUrl === "string" && !pathOrUrl.includes("?")) { | ||
| rewritten.search = currentUrl.search; | ||
| } | ||
|
|
||
| return rewritten; | ||
| } | ||
|
|
||
| const defaultOptionsHandler = (methods: string[]): () => Promise<Response> => { | ||
| return () => | ||
| Promise.resolve( | ||
|
|
@@ -417,26 +458,13 @@ export class App<State> { | |
|
|
||
| const trustProxy = this.config.trustProxy; | ||
|
|
||
| return async ( | ||
| const dispatch = async ( | ||
| req: Request, | ||
| conn: Deno.ServeHandlerInfo = DEFAULT_CONN_INFO, | ||
| ) => { | ||
| const url = new URL(req.url); | ||
| // Prevent open redirect attacks | ||
| url.pathname = url.pathname.replace(/\/+/g, "/"); | ||
|
|
||
| // Apply X-Forwarded-* headers when behind a reverse proxy | ||
| if (trustProxy) { | ||
| const proto = req.headers.get("x-forwarded-proto"); | ||
| if (proto) { | ||
| url.protocol = proto + ":"; | ||
| } | ||
| const host = req.headers.get("x-forwarded-host"); | ||
| if (host) { | ||
| url.host = host; | ||
| } | ||
| } | ||
|
|
||
| conn: Deno.ServeHandlerInfo, | ||
| state: State, | ||
| rewriteCount: number, | ||
| ): Promise<Response> => { | ||
| const url = normalizeRequestUrl(req, trustProxy); | ||
| const method = req.method.toUpperCase() as Method; | ||
| const matched = router.match(method, url); | ||
| let { params, pattern, item: handler, methodMatch } = matched; | ||
|
|
@@ -473,6 +501,24 @@ export class App<State> { | |
| this.config, | ||
| next, | ||
| buildCache!, | ||
| state, | ||
| (pathOrUrl) => { | ||
| if (rewriteCount >= MAX_REWRITE_COUNT) { | ||
| throw new Error( | ||
| `Too many internal rewrites while handling "${req.method} ${url.pathname}"`, | ||
| ); | ||
| } | ||
|
|
||
| if (req.bodyUsed) { | ||
| throw new Error( | ||
| "Cannot rewrite request after its body has already been consumed", | ||
| ); | ||
| } | ||
|
|
||
| const rewrittenUrl = getRewriteUrl(url, pathOrUrl); | ||
| const rewrittenReq = new Request(rewrittenUrl, req); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two small notes on
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| return dispatch(rewrittenReq, conn, state, rewriteCount + 1); | ||
| }, | ||
| ); | ||
|
|
||
| try { | ||
|
|
@@ -493,6 +539,11 @@ export class App<State> { | |
| return await DEFAULT_ERROR_HANDLER(ctx); | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| req: Request, | ||
| conn: Deno.ServeHandlerInfo = DEFAULT_CONN_INFO, | ||
| ) => dispatch(req, conn, {} as State, 0); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -235,6 +235,202 @@ Deno.test("App - methods with middleware", async () => { | |
| expect(await res.text()).toEqual("A"); | ||
| }); | ||
|
|
||
| Deno.test("App - ctx.rewrite() rematches and preserves state", async () => { | ||
| const LOCALES = new Set(["de", "ru"]); | ||
|
|
||
| const app = new App<{ locale?: string }>() | ||
| .use((ctx) => { | ||
| const [, first, ...rest] = ctx.url.pathname.split("/"); | ||
|
|
||
| if (ctx.state.locale === undefined && LOCALES.has(first)) { | ||
| ctx.state.locale = first; | ||
| } | ||
|
|
||
| if (LOCALES.has(first)) { | ||
| const rewritten = `/${rest.join("/")}`; | ||
| return ctx.rewrite(rewritten === "/" ? "/" : rewritten); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My bad, it was a hot afternoon |
||
| } | ||
|
|
||
| if (ctx.state.locale === undefined) { | ||
| ctx.state.locale = "en"; | ||
| } | ||
|
|
||
| return ctx.next(); | ||
| }) | ||
| .get("/hello", (ctx) => { | ||
| const q = ctx.url.searchParams.get("q") ?? ""; | ||
| return new Response( | ||
| `${ctx.state.locale}:${ctx.route}:${ctx.url.pathname}:${q}`, | ||
| ); | ||
| }); | ||
|
|
||
| const server = new FakeServer(app.handler()); | ||
|
|
||
| let res = await server.get("/de/hello?q=1"); | ||
| expect(await res.text()).toEqual("de:/hello:/hello:1"); | ||
|
|
||
| res = await server.get("/hello?q=2"); | ||
| expect(await res.text()).toEqual("en:/hello:/hello:2"); | ||
| }); | ||
|
|
||
| Deno.test("App - ctx.rewrite() from route middleware", async () => { | ||
| const app = new App() | ||
| .get("/legacy", (ctx) => ctx.rewrite("/modern")) | ||
| .get("/modern", () => new Response("ok")); | ||
|
|
||
| const server = new FakeServer(app.handler()); | ||
| const res = await server.get("/legacy"); | ||
| expect(await res.text()).toEqual("ok"); | ||
| }); | ||
|
|
||
| Deno.test("App - ctx.rewrite() allows post-processing in middleware", async () => { | ||
| const app = new App() | ||
| .use(async (ctx) => { | ||
| if (ctx.url.pathname === "/legacy") { | ||
| const res = await ctx.rewrite("/modern"); | ||
| res.headers.set("x-rewritten", "1"); | ||
| return res; | ||
| } | ||
|
|
||
| return ctx.next(); | ||
| }) | ||
| .get("/modern", () => new Response("ok")); | ||
|
|
||
| const server = new FakeServer(app.handler()); | ||
| const res = await server.get("/legacy"); | ||
| expect(await res.text()).toEqual("ok"); | ||
| expect(res.headers.get("x-rewritten")).toEqual("1"); | ||
| }); | ||
|
|
||
| Deno.test("App - ctx.rewrite() preserves method and body", async () => { | ||
| const app = new App() | ||
| .use((ctx) => { | ||
| if (ctx.url.pathname === "/old") { | ||
| return ctx.rewrite("/new"); | ||
| } | ||
| return ctx.next(); | ||
| }) | ||
| .post("/new", async (ctx) => { | ||
| return new Response(`${ctx.req.method}:${await ctx.req.text()}`); | ||
| }); | ||
|
|
||
| const server = new FakeServer(app.handler()); | ||
| const res = await server.post("/old", "payload"); | ||
| expect(await res.text()).toEqual("POST:payload"); | ||
| }); | ||
|
|
||
| Deno.test("App - ctx.rewrite() preserves query by default", async () => { | ||
| const app = new App() | ||
| .get("/from", (ctx) => ctx.rewrite("/to")) | ||
| .get("/to", (ctx) => new Response(ctx.url.searchParams.get("q") ?? "")); | ||
|
|
||
| const server = new FakeServer(app.handler()); | ||
| const res = await server.get("/from?q=123"); | ||
| expect(await res.text()).toEqual("123"); | ||
| }); | ||
|
|
||
| Deno.test( | ||
| "App - ctx.rewrite() query in target overrides current query", | ||
| async () => { | ||
| const app = new App() | ||
| .get("/from", (ctx) => ctx.rewrite("/to?q=override")) | ||
| .get( | ||
| "/to", | ||
| (ctx) => new Response(ctx.url.searchParams.get("q") ?? ""), | ||
| ); | ||
|
|
||
| const server = new FakeServer(app.handler()); | ||
| const res = await server.get("/from?q=123"); | ||
| expect(await res.text()).toEqual("override"); | ||
| }, | ||
| ); | ||
|
|
||
| Deno.test("App - ctx.rewrite() supports URL targets with basePath", async () => { | ||
| const app = new App({ basePath: "/base" }) | ||
| .get("/old", (ctx) => ctx.rewrite(new URL("/base/new?q=1", ctx.url))) | ||
| .get( | ||
| "/new", | ||
| (ctx) => | ||
| new Response( | ||
| `${ctx.url.pathname}:${ctx.url.searchParams.get("q") ?? ""}`, | ||
| ), | ||
| ); | ||
|
|
||
| const server = new FakeServer(app.handler()); | ||
| const res = await server.get("/base/old"); | ||
| expect(await res.text()).toEqual("/base/new:1"); | ||
| }); | ||
|
|
||
| Deno.test("App - ctx.rewrite() throws on rewrite loops", async () => { | ||
| const app = new App() | ||
| .use(async (ctx) => { | ||
| try { | ||
| return await ctx.next(); | ||
| } catch (err) { | ||
| return new Response(String(err), { status: 500 }); | ||
| } | ||
| }) | ||
| .use((ctx) => { | ||
| if (ctx.url.pathname === "/a") { | ||
| return ctx.rewrite("/b"); | ||
| } | ||
| if (ctx.url.pathname === "/b") { | ||
| return ctx.rewrite("/a"); | ||
| } | ||
| return ctx.next(); | ||
| }) | ||
| .get("/a", () => new Response("a")) | ||
| .get("/b", () => new Response("b")); | ||
|
|
||
| const server = new FakeServer(app.handler()); | ||
| const res = await server.get("/a"); | ||
|
|
||
| expect(res.status).toEqual(500); | ||
| expect(await res.text()).toContain("Too many internal rewrites"); | ||
| }); | ||
|
|
||
| Deno.test("App - ctx.rewrite() rejects cross-origin targets", async () => { | ||
| const app = new App() | ||
| .use(async (ctx) => { | ||
| try { | ||
| return await ctx.next(); | ||
| } catch (err) { | ||
| return new Response(String(err), { status: 500 }); | ||
| } | ||
| }) | ||
| .get("/", (ctx) => ctx.rewrite("https://deno.land/")); | ||
|
|
||
| const server = new FakeServer(app.handler()); | ||
| const res = await server.get("/"); | ||
|
|
||
| expect(res.status).toEqual(500); | ||
| expect(await res.text()).toContain("only supports same-origin URLs"); | ||
| }); | ||
|
|
||
| Deno.test("App - ctx.rewrite() rejects rewrites after body consumption", async () => { | ||
| const app = new App() | ||
| .use(async (ctx) => { | ||
| try { | ||
| return await ctx.next(); | ||
| } catch (err) { | ||
| return new Response(String(err), { status: 500 }); | ||
| } | ||
| }) | ||
| .post("/", async (ctx) => { | ||
| await ctx.req.text(); | ||
| return ctx.rewrite("/next"); | ||
| }) | ||
| .post("/next", () => new Response("ok")); | ||
|
|
||
| const server = new FakeServer(app.handler()); | ||
| const res = await server.post("/", "payload"); | ||
|
|
||
| expect(res.status).toEqual(500); | ||
| expect(await res.text()).toContain( | ||
| "request after its body has already been consumed", | ||
| ); | ||
| }); | ||
|
|
||
| Deno.test("App - .mountApp() compose apps", async () => { | ||
| const innerApp = new App<{ text: string }>() | ||
| .use((ctx) => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/latest/is the Fresh 1.x docs surface. Fresh 2's docs live underdocs/canary/the-canary-version/, which (at the moment) doesn't havecontext.md/middleware.mdequivalents. Worth confirming with maintainers whether this documentation should be mirrored/moved there — otherwise users on Fresh 2 won't find these docs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allright, I'll create a file there and one might decide to move it to the right place.
But.. Fresh 2 isn't canary anymore right?