mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-09 13:01:00 +00:00
Stop redirecting to the address the process bound to
Reported from the iOS side, iOS #96. `GET /api/auth/native/login` answers location: https://0.0.0.0:3000/auth/login?flow=native behind a proxy that does not pass `Host` through. `NextResponse.redirect` needs an absolute URL, the route built one with `new URL(path, req.url)`, and `req.url` there is the address the Node process bound to. That is the primary onboarding path. ASWebAuthenticationSession follows the redirect, iOS refuses the restricted port, and the person lands on a browser error page with no way forward. Distributed builds carry no associated domains on purpose, so the browser handoff is also how passkeys are meant to work; without it a new account is down to a password and a confirmation code. Two things hid it. Every error branch of that route builds the fixed healthlog:// scheme with no host at all, so the route behaved correctly on every failure and broke only when it was supposed to succeed. And its own test built a request whose host matched, so the absolute URL it produced looked right. The assertion there used toContain, which is satisfied by the broken value just as happily as the correct one. The same line was in six more places: the error redirects of the Withings, WHOOP and Fitbit connect routes, which send a browser to /settings/integrations. Behind the same proxy those land on the same unreachable address, so all seven move to a relative Location. A relative Location is valid per RFC 7231 and the client resolves it against the address it actually called, which is the public one by definition. The alternative is reading X-Forwarded-Host, and that would put one more proxy setting between a self-hoster and a working sign-in. Three checks, as the report asked for. The route test now drives a request whose URL names the bind address, which is what a proxy hop looks like from inside the handler. A structural test freezes the pattern out of the four files and, separately, requires each of them to still reach for the helper, because a file that simply deleted its redirect would pass the first check and fail its users. The helper refuses a protocol-relative value, which is a host in disguise. Verified by breaking it: putting the old call back fails four checks, the last of them reporting the reported value verbatim. Refs iOS #96.
This commit is contained in:
1 parent
4d2fc06fe7
commit
cc8c69aa07
7 files changed
+206
-16
No files matched your search
@@ -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");
|
||||
});
|
||||
});
|
||||
@@ -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,
|
||||
|
||||
@@ -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({
|
||||
|
||||
@@ -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",
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -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",
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -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",
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -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 },
|
||||
});
|
||||
}
|
||||
Reference in new issue
Block a user