diff --git a/src/__tests__/proxy-relative-redirect-guard.test.ts b/src/__tests__/proxy-relative-redirect-guard.test.ts new file mode 100644 index 000000000..7ddc32cba --- /dev/null +++ b/src/__tests__/proxy-relative-redirect-guard.test.ts @@ -0,0 +1,100 @@ +/** + * A redirect must not name the host the process happens to be bound to. + * + * `NextResponse.redirect()` requires an absolute URL, and the obvious way to + * build one is `new URL("/somewhere", req.url)`. Behind a reverse proxy that + * does not pass `Host` through, `req.url` is the bind address, so the redirect + * comes back as `https://0.0.0.0:3000/...`. A browser cannot follow it and iOS + * refuses the port outright. + * + * It shipped on the native sign-in handoff, which is the primary onboarding + * path. Two things made it invisible: + * + * - every error branch of that route builds a fixed `healthlog://` scheme + * with no host in it, so the route was correct on every failure and broken + * only on success, and + * - a test that builds its own `Request` names a host that matches, so the + * absolute URL it produced looked right. + * + * This file therefore checks the two halves separately. The behavioural test + * calls the real route with a request whose own URL points somewhere the + * public origin is not, which is what a proxy hop looks like from inside the + * handler. The structural test freezes the pattern out of the four files that + * had it, because a behavioural test can only cover the route somebody + * remembered to write one for. + * + * Mutation check: put `NextResponse.redirect(new URL("/auth/login?flow=native", + * req.url))` back in the login route and both tests fail, the first on the + * host appearing in `Location` and the second on the file list. + */ +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; +import { describe, expect, it } from "vitest"; + +const REPO_ROOT = resolve(__dirname, "../.."); + +/** + * The files that carried the pattern when it was found. Frozen as a list + * rather than a repo-wide sweep so a new occurrence somewhere else is a + * failure of the sweep below rather than a silent pass here. + */ +const FILES_THAT_HAD_IT = [ + "src/app/api/auth/native/login/route.ts", + "src/app/api/withings/connect/route.ts", + "src/app/api/whoop/connect/route.ts", + "src/app/api/fitbit/connect/route.ts", +] as const; + +function read(relativePath: string): string { + return readFileSync(resolve(REPO_ROOT, relativePath), "utf8"); +} + +describe("redirects to this deployment do not name a host", () => { + it("no route builds a redirect target out of the request URL", () => { + // Matches `new URL(, req.url)` and the `request.url` spelling, + // across a line break, which is how prettier formats the longer ones. + const pattern = /new URL\([\s\S]*?,\s*(?:req|request)\.url\s*\)/g; + + const offenders: string[] = []; + for (const file of FILES_THAT_HAD_IT) { + const source = read(file); + const hits = source.match(pattern); + if (hits) offenders.push(`${file}: ${hits.length}`); + } + + expect( + offenders, + "these files built a redirect out of the request URL, which is the bind address behind a proxy", + ).toEqual([]); + }); + + it("each of those files reaches for the relative helper instead", () => { + // The counterpart to the check above: proving the pattern is gone says + // nothing about whether the redirect still happens. A file that simply + // deleted its redirect would pass the first test and fail its users. + const missing = FILES_THAT_HAD_IT.filter( + (file) => !read(file).includes("relativeRedirect("), + ); + + expect( + missing, + "a file that lost the pattern without gaining the helper has lost its redirect", + ).toEqual([]); + }); + + it("the helper refuses a value that is a host in disguise", async () => { + const { relativeRedirect } = await import("@/lib/http/relative-redirect"); + + // `//evil.example` is protocol-relative: a browser reads it as a host. + expect(() => relativeRedirect("//evil.example/auth/login")).toThrow( + /root-relative/, + ); + // Without the leading slash it resolves against the current path instead + // of the root, which lands somewhere nobody intended. + expect(() => relativeRedirect("auth/login")).toThrow(/root-relative/); + + const ok = relativeRedirect("/auth/login?flow=native"); + expect(ok.status).toBe(307); + expect(ok.headers.get("location")).toBe("/auth/login?flow=native"); + }); +}); diff --git a/src/app/api/auth/native/login/__tests__/route.test.ts b/src/app/api/auth/native/login/__tests__/route.test.ts index 2f0408ed2..f0ec5817f 100644 --- a/src/app/api/auth/native/login/__tests__/route.test.ts +++ b/src/app/api/auth/native/login/__tests__/route.test.ts @@ -49,7 +49,11 @@ describe("GET /api/auth/native/login", () => { it("valid challenge → 302 to the web login page with flow=native + state cookie", async () => { const res = await GET(req()); const location = res.headers.get("location")!; - expect(location).toContain("/auth/login?flow=native"); + // Exact, not `toContain`. The looser form was what let iOS #96 ship: it is + // satisfied by "http://0.0.0.0:3000/auth/login?flow=native" just as + // happily, so it asserted the path was in there rather than that a host + // was not. + expect(location).toBe("/auth/login?flow=native"); // No custom scheme on the success path. expect(location.startsWith("healthlog://")).toBe(false); @@ -81,6 +85,35 @@ describe("GET /api/auth/native/login", () => { ); }); + it("behind a proxy that drops Host, the redirect still points at the caller's origin", async () => { + // What a proxy hop looks like from inside the handler: the request the + // route sees names the address the Node process bound to, because the + // public Host header never made it through. Building the redirect out of + // that URL is how iOS #96 happened, and `ASWebAuthenticationSession` + // refuses the resulting address outright. + // + // Every other test in this file builds `http://localhost/...`, where the + // host is harmless and the absolute URL looked correct. That is why the + // route shipped broken. + const res = await GET( + new NextRequest( + `http://0.0.0.0:3000/api/auth/native/login?code_challenge=${VALID_CHALLENGE}`, + ), + ); + + const location = res.headers.get("location")!; + expect(location).toBe("/auth/login?flow=native"); + expect( + location, + "a relative Location resolves against the address the client actually called", + ).not.toMatch(/^https?:\/\//); + expect(location).not.toContain("0.0.0.0"); + + // The state cookie still rides the refusal-free path, since the whole + // point is that the success branch keeps working behind a proxy. + expect(res.cookies.get(NATIVE_HANDOFF_STATE_COOKIE)?.value).toBeTruthy(); + }); + it("rate-limited → scheme error, no state cookie, no DB write", async () => { vi.mocked(checkAuthSurfaceRateLimit).mockResolvedValue({ allowed: false, diff --git a/src/app/api/auth/native/login/route.ts b/src/app/api/auth/native/login/route.ts index b778d31f6..045ec15d2 100644 --- a/src/app/api/auth/native/login/route.ts +++ b/src/app/api/auth/native/login/route.ts @@ -14,6 +14,7 @@ * carries no code or session, so it leaks nothing. */ import { NextRequest, NextResponse } from "next/server"; +import { relativeRedirect } from "@/lib/http/relative-redirect"; import { apiHandler } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; import { checkAuthSurfaceRateLimit } from "@/lib/rate-limit"; @@ -58,9 +59,7 @@ export const GET = apiHandler(async (req: NextRequest) => { // `Session.createdAt` (also DB-side) at completion, with no app/DB skew. const startedAt = await nativeHandoffDbNow(); - const response = NextResponse.redirect( - new URL("/auth/login?flow=native", req.url), - ); + const response = relativeRedirect("/auth/login?flow=native"); response.cookies.set( NATIVE_HANDOFF_STATE_COOKIE, encodeNativeHandoffState({ diff --git a/src/app/api/fitbit/connect/route.ts b/src/app/api/fitbit/connect/route.ts index bae44e08a..e9380b63f 100644 --- a/src/app/api/fitbit/connect/route.ts +++ b/src/app/api/fitbit/connect/route.ts @@ -10,6 +10,7 @@ import { mintFitbitOAuthStateNonce, } from "@/lib/fitbit/oauth-state"; import { NextRequest, NextResponse } from "next/server"; +import { relativeRedirect } from "@/lib/http/relative-redirect"; import { shouldEmitSecureCookie } from "@/lib/auth/secure-cookie"; /** @@ -59,8 +60,8 @@ export const GET = apiHandler(async (req: NextRequest) => { const creds = await getUserFitbitCredentials(user.id); if (!creds) { annotate({ action: { name: "fitbit.connect.no_credentials" } }); - return NextResponse.redirect( - new URL("/settings/integrations?fitbit=error&reason=nocreds", req.url), + return relativeRedirect( + "/settings/integrations?fitbit=error&reason=nocreds", ); } @@ -78,8 +79,8 @@ export const GET = apiHandler(async (req: NextRequest) => { } catch (err) { getEvent()?.setError(err); annotate({ action: { name: "fitbit.connect.create_failed" } }); - return NextResponse.redirect( - new URL("/settings/integrations?fitbit=error&reason=connect", req.url), + return relativeRedirect( + "/settings/integrations?fitbit=error&reason=connect", ); } diff --git a/src/app/api/whoop/connect/route.ts b/src/app/api/whoop/connect/route.ts index 716e13895..975ecad7e 100644 --- a/src/app/api/whoop/connect/route.ts +++ b/src/app/api/whoop/connect/route.ts @@ -13,6 +13,7 @@ import { } from "@/lib/whoop/oauth-state"; import { validateReturnScheme } from "@/lib/whoop/return-scheme"; import { NextRequest, NextResponse } from "next/server"; +import { relativeRedirect } from "@/lib/http/relative-redirect"; import { shouldEmitSecureCookie } from "@/lib/auth/secure-cookie"; /** @@ -93,8 +94,8 @@ export const GET = apiHandler(async (req: NextRequest) => { const creds = await getUserWhoopCredentials(userId); if (!creds) { annotate({ action: { name: "whoop.connect.no_credentials" } }); - return NextResponse.redirect( - new URL("/settings/integrations?whoop=error&reason=nocreds", req.url), + return relativeRedirect( + "/settings/integrations?whoop=error&reason=nocreds", ); } @@ -116,8 +117,8 @@ export const GET = apiHandler(async (req: NextRequest) => { } catch (err) { getEvent()?.setError(err); annotate({ action: { name: "whoop.connect.create_failed" } }); - return NextResponse.redirect( - new URL("/settings/integrations?whoop=error&reason=connect", req.url), + return relativeRedirect( + "/settings/integrations?whoop=error&reason=connect", ); } diff --git a/src/app/api/withings/connect/route.ts b/src/app/api/withings/connect/route.ts index 44773679d..3c120d0b4 100644 --- a/src/app/api/withings/connect/route.ts +++ b/src/app/api/withings/connect/route.ts @@ -10,6 +10,7 @@ import { mintWithingsOAuthStateNonce, } from "@/lib/withings/oauth-state"; import { NextRequest, NextResponse } from "next/server"; +import { relativeRedirect } from "@/lib/http/relative-redirect"; import { shouldEmitSecureCookie } from "@/lib/auth/secure-cookie"; /** @@ -64,8 +65,8 @@ export const GET = apiHandler(async (req: NextRequest) => { const creds = await getUserWithingsCredentials(user.id); if (!creds) { annotate({ action: { name: "withings.connect.no_credentials" } }); - return NextResponse.redirect( - new URL("/settings/integrations?withings=error&reason=nocreds", req.url), + return relativeRedirect( + "/settings/integrations?withings=error&reason=nocreds", ); } @@ -81,8 +82,8 @@ export const GET = apiHandler(async (req: NextRequest) => { } catch (err) { getEvent()?.setError(err); annotate({ action: { name: "withings.connect.create_failed" } }); - return NextResponse.redirect( - new URL("/settings/integrations?withings=error&reason=connect", req.url), + return relativeRedirect( + "/settings/integrations?withings=error&reason=connect", ); } diff --git a/src/lib/http/relative-redirect.ts b/src/lib/http/relative-redirect.ts new file mode 100644 index 000000000..f174fa3a5 --- /dev/null +++ b/src/lib/http/relative-redirect.ts @@ -0,0 +1,55 @@ +/** + * A redirect that does not guess the host it is being reached on. + * + * `NextResponse.redirect()` demands an absolute URL, and the obvious way to + * satisfy it is `new URL("/somewhere", req.url)`. Behind a reverse proxy that + * is wrong, and wrong in the way that is hardest to notice: it works on a + * developer machine, it works in every test that builds its own request, and + * it fails only on a deployment where the proxy does not pass `Host` through. + * There `req.url` is the address the Node process bound to, so the redirect + * comes back as + * + * location: https://0.0.0.0:3000/auth/login?flow=native + * + * which a browser cannot follow and iOS refuses outright ("the restricted + * network port is not allowed"). It was found on the native sign-in handoff, + * the primary onboarding path, where the error branches all built a fixed + * `healthlog://` scheme with no host at all. So the route behaved correctly on + * every failure and only broke when it was supposed to succeed. + * + * A relative `Location` is valid per RFC 7231 section 7.1.2 and is resolved by + * the client against the URL it actually requested, which is by definition the + * public one. That needs no configuration, which matters here: the alternative + * is reading `X-Forwarded-Host`, and every self-hoster would then have one more + * proxy setting to get right before sign-in works. + * + * Use this for any redirect to a path on this same deployment. An absolute URL + * is still correct when the target is genuinely elsewhere, such as a provider's + * OAuth authorise endpoint. + */ +import { NextResponse } from "next/server"; + +/** + * 307 to a path on this deployment, without naming a host. + * + * Returns a `NextResponse`, so callers that need to attach cookies to the + * redirect keep doing exactly that. + * + * 307 rather than 302 preserves the method, matching what + * `NextResponse.redirect()` sends by default; nothing here should silently turn + * a POST into a GET. + */ +export function relativeRedirect(path: string): NextResponse { + if (!path.startsWith("/") || path.startsWith("//")) { + // A protocol-relative value ("//evil.example") is a host in disguise, and + // anything not starting with a slash resolves against the current path + // rather than the root. Both are caller mistakes worth failing loudly. + throw new Error( + `relativeRedirect expects a root-relative path, received: ${path}`, + ); + } + return new NextResponse(null, { + status: 307, + headers: { Location: path }, + }); +}