Merge pull request #821 from MBombeck/fix/proxy-relative-redirects

Stop redirecting to the address the process bound to

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(<anything>, 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");
+  });
+});

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,

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({

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",
     );
   }
 

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",
     );
   }
 

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",
     );
   }
 

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 },
+  });
+}