Redirect an anonymous hit on /settings to sign-in, not a 500 #191
@@ -105,3 +105,20 @@ Read most (opens and searches):
|
|||||||
- `src/ui/chrome.ts` 1
|
- `src/ui/chrome.ts` 1
|
||||||
- `views/index.ejs` 1
|
- `views/index.ejs` 1
|
||||||
- `views/partials/shell.ejs` 1
|
- `views/partials/shell.ejs` 1
|
||||||
|
|
||||||
|
## Reading cost, chunk e4d3bb0ac092, finished 2026-10-31T08:02:27Z
|
||||||
|
|
||||||
|
154733 new input tokens before the first commit.
|
||||||
|
|
||||||
|
Read most (opens and searches):
|
||||||
|
|
||||||
|
- `todo.md` 10
|
||||||
|
- `README.md` 6
|
||||||
|
- `package.json` 3
|
||||||
|
- `.gitignore` 1
|
||||||
|
- `.npmrc` 1
|
||||||
|
- `AGENTS.md` 1
|
||||||
|
- `ci.sh` 1
|
||||||
|
- `public/css/styles.css` 1
|
||||||
|
- `renovate.json` 1
|
||||||
|
- `src/e2e-console-guard.test.ts` 1
|
||||||
|
|||||||
@@ -240,6 +240,17 @@ test("return_to: a deep link while logged out returns to that page after login",
|
|||||||
await expect(page.locator("h1")).toHaveText("Users");
|
await expect(page.locator("h1")).toHaveText("Users");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("settings: an anonymous visit signs in first, then lands on the settings page", async ({ page }) => {
|
||||||
|
test.setTimeout(90_000);
|
||||||
|
await page.goto("/settings");
|
||||||
|
await expect(page).toHaveURL(/\/login(\?|$)/);
|
||||||
|
await page.fill('input[name="identifier"]', ADMIN_EMAIL);
|
||||||
|
await page.fill('input[name="password"]', ADMIN_PASSWORD);
|
||||||
|
await page.locator('.auth-form button[type="submit"]').click();
|
||||||
|
await expect(page).toHaveURL(/\/settings(\?|$)/);
|
||||||
|
await expect(page.locator("h1")).toHaveText("Account settings");
|
||||||
|
});
|
||||||
|
|
||||||
test("mocked SSO login: the provider button signs a user in via OIDC", async ({ page }) => {
|
test("mocked SSO login: the provider button signs a user in via OIDC", async ({ page }) => {
|
||||||
test.setTimeout(90_000);
|
test.setTimeout(90_000);
|
||||||
await page.goto("/login");
|
await page.goto("/login");
|
||||||
|
|||||||
+43
-35
@@ -38,52 +38,35 @@ const FORBIDDEN: RouteResult = { status: 403, view: "403" };
|
|||||||
const selfOrigin = (ctx: RequestContext, secure: boolean): string =>
|
const selfOrigin = (ctx: RequestContext, secure: boolean): string =>
|
||||||
`${secure ? "https" : "http"}://${ctx.req.headers.host ?? "127.0.0.1:3000"}`;
|
`${secure ? "https" : "http"}://${ctx.req.headers.host ?? "127.0.0.1:3000"}`;
|
||||||
|
|
||||||
// Themed Kratos self-service page (one route per AUTH_FLOWS entry: login/registration/recovery/
|
// Where Kratos lands the browser after sign-in, or undefined for its default. A host-relative
|
||||||
// verification/settings).
|
// return_to (the gate's deep link) or a chosen language goes through /auth/complete, which mints the
|
||||||
function flowPage(kratos: KratosPublic, flowType: FlowType, secureCookies: boolean): BuiltinRoute["handler"] {
|
// session JWT first and carries the language across Kratos; any other target (the OAuth2 login
|
||||||
return async (ctx: RequestContext, csrf: RequestCsrf): Promise<RouteResult> => {
|
// challenge) is passed as-is, since Kratos allow-lists it.
|
||||||
const pathname = ctx.url.pathname;
|
function flowReturnTo(ctx: RequestContext, secureCookies: boolean): string | undefined {
|
||||||
// Already signed in? Re-authenticating / re-registering is pointless — send them to the app
|
|
||||||
// dashboard. (/settings, /recovery, /verification stay reachable — a signed-in user can use those.)
|
|
||||||
if (ctx.user && (flowType === "login" || flowType === "registration")) return { redirect: "/dashboard" };
|
|
||||||
const cookie = ctx.req.headers.cookie;
|
|
||||||
const flowId = ctx.url.searchParams.get("flow");
|
|
||||||
// Only the Kratos calls are in the try, so a render/buildFlowView bug below falls through to
|
|
||||||
// the catch-all 500 (with a stack), not the "Ory unreachable" 503.
|
|
||||||
let flow: Flow;
|
|
||||||
try {
|
|
||||||
if (!flowId) {
|
|
||||||
// No flow yet: init one server-side, relay Kratos' CSRF cookie, bounce to ?flow=<id>.
|
|
||||||
// A `return_to` is baked into the flow so Kratos lands there after login instead of the
|
|
||||||
// default completion route. A first-party deep link (host-relative, from the gate's
|
|
||||||
// return_to) is wrapped through /auth/complete so the session JWT is minted before the
|
|
||||||
// user reaches the page; an absolute target (the OAuth2 login challenge) is passed
|
|
||||||
// as-is — Kratos allow-lists it. localPath rejects an off-origin "//evil.com".
|
|
||||||
const raw = ctx.url.searchParams.get("return_to");
|
const raw = ctx.url.searchParams.get("return_to");
|
||||||
const local = localPath(raw);
|
const local = localPath(raw);
|
||||||
const chosen = chosenLocale(ctx);
|
const chosen = chosenLocale(ctx);
|
||||||
let returnTo: string | undefined;
|
if (!local && !chosen) return raw ?? undefined;
|
||||||
if (local || chosen) {
|
|
||||||
// The flow's return target is the host's, not Kratos' — so the language the visitor picked
|
|
||||||
// on the sign-in page survives the round-trip through Kratos and lands on the page after
|
|
||||||
// it. Without this the most-travelled path in the product (pick Swedish → sign in) drops
|
|
||||||
// straight back to Accept-Language.
|
|
||||||
const complete = new URL(`${selfOrigin(ctx, secureCookies)}/auth/complete`);
|
const complete = new URL(`${selfOrigin(ctx, secureCookies)}/auth/complete`);
|
||||||
if (local) complete.searchParams.set("return_to", ctx.localeHref(local));
|
if (local) complete.searchParams.set("return_to", ctx.localeHref(local));
|
||||||
if (chosen) complete.searchParams.set("locale", chosen);
|
if (chosen) complete.searchParams.set("locale", chosen);
|
||||||
returnTo = complete.toString();
|
return complete.toString();
|
||||||
} else if (raw) returnTo = raw;
|
|
||||||
const { flow: initiated, setCookie } = await kratos.initBrowserFlow(flowType, { ...(cookie ? { cookie } : {}), ...(returnTo ? { returnTo } : {}) });
|
|
||||||
if (setCookie.length) ctx.res.appendHeader("set-cookie", setCookie);
|
|
||||||
return { redirect: `${pathname}?flow=${initiated.id}` };
|
|
||||||
}
|
}
|
||||||
flow = await kratos.getFlow(flowType, flowId, cookie ? { cookie } : {});
|
|
||||||
} catch (err) {
|
// What a failed Kratos call returns instead of the flow; rethrows what is genuinely unexpected.
|
||||||
|
function flowFailure(err: unknown, ctx: RequestContext, flowType: FlowType): RouteResult {
|
||||||
|
const pathname = ctx.url.pathname;
|
||||||
// Expired/unknown flow → restart by re-initialising (drop the stale ?flow=).
|
// Expired/unknown flow → restart by re-initialising (drop the stale ?flow=).
|
||||||
if (err instanceof KratosError && [403, 404, 410].includes(err.status)) return { redirect: pathname };
|
if (err instanceof KratosError && [403, 404, 410].includes(err.status)) return { redirect: pathname };
|
||||||
|
// No Kratos session on a page that needs one. /settings is public, since recovery lands there with
|
||||||
|
// a Kratos session but no app JWT, so the gate never sends an anonymous visitor to sign in.
|
||||||
|
// Not for /login itself, which would redirect to itself.
|
||||||
|
if (err instanceof KratosError && err.status === 401 && flowType !== "login") {
|
||||||
|
return { redirect: `/login?return_to=${encodeURIComponent(pathname)}` };
|
||||||
|
}
|
||||||
// Already authenticated at Kratos but no app JWT yet (e.g. straight after registration, whose
|
// Already authenticated at Kratos but no app JWT yet (e.g. straight after registration, whose
|
||||||
// `session` hook signs the user in but routes to verification, not /auth/complete — so ctx.user
|
// `session` hook signs the user in but routes to verification, not /auth/complete — so ctx.user
|
||||||
// is null and the "already signed in" short-circuit above can't fire). Initialising a login/
|
// is null and the "already signed in" short-circuit can't fire). Initialising a login/
|
||||||
// registration flow then returns Kratos 400 `session_already_available`. Recover by completing
|
// registration flow then returns Kratos 400 `session_already_available`. Recover by completing
|
||||||
// login (mint the JWT from the live session), honouring return_to — never a 500.
|
// login (mint the JWT from the live session), honouring return_to — never a 500.
|
||||||
if (err instanceof KratosError && err.status === 400 && err.body.includes("session_already_available")) {
|
if (err instanceof KratosError && err.status === 400 && err.body.includes("session_already_available")) {
|
||||||
@@ -98,6 +81,31 @@ function flowPage(kratos: KratosPublic, flowType: FlowType, secureCookies: boole
|
|||||||
}
|
}
|
||||||
throw err; // any other Kratos 4xx → the catch-all (genuinely unexpected)
|
throw err; // any other Kratos 4xx → the catch-all (genuinely unexpected)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Themed Kratos self-service page (one route per AUTH_FLOWS entry: login/registration/recovery/
|
||||||
|
// verification/settings).
|
||||||
|
function flowPage(kratos: KratosPublic, flowType: FlowType, secureCookies: boolean): BuiltinRoute["handler"] {
|
||||||
|
return async (ctx: RequestContext, csrf: RequestCsrf): Promise<RouteResult> => {
|
||||||
|
// Already signed in? Re-authenticating / re-registering is pointless — send them to the app
|
||||||
|
// dashboard. (/settings, /recovery, /verification stay reachable — a signed-in user can use those.)
|
||||||
|
if (ctx.user && (flowType === "login" || flowType === "registration")) return { redirect: "/dashboard" };
|
||||||
|
const cookie = ctx.req.headers.cookie;
|
||||||
|
const flowId = ctx.url.searchParams.get("flow");
|
||||||
|
// Only the Kratos calls are in the try, so a render/buildFlowView bug below falls through to
|
||||||
|
// the catch-all 500 (with a stack), not the "Ory unreachable" 503.
|
||||||
|
let flow: Flow;
|
||||||
|
try {
|
||||||
|
if (!flowId) {
|
||||||
|
// No flow yet: init one server-side, relay Kratos' CSRF cookie, bounce to ?flow=<id>.
|
||||||
|
const returnTo = flowReturnTo(ctx, secureCookies);
|
||||||
|
const { flow: initiated, setCookie } = await kratos.initBrowserFlow(flowType, { ...(cookie ? { cookie } : {}), ...(returnTo ? { returnTo } : {}) });
|
||||||
|
if (setCookie.length) ctx.res.appendHeader("set-cookie", setCookie);
|
||||||
|
return { redirect: `${ctx.url.pathname}?flow=${initiated.id}` };
|
||||||
|
}
|
||||||
|
flow = await kratos.getFlow(flowType, flowId, cookie ? { cookie } : {});
|
||||||
|
} catch (err) {
|
||||||
|
return flowFailure(err, ctx, flowType);
|
||||||
|
}
|
||||||
// Rendered inside the unified app shell, so set a fresh CSRF cookie when minted — the
|
// Rendered inside the unified app shell, so set a fresh CSRF cookie when minted — the
|
||||||
// shell's Sign-out form (shown on /settings, where the user is signed in) needs the token.
|
// shell's Sign-out form (shown on /settings, where the user is signed in) needs the token.
|
||||||
csrf.setCookie();
|
csrf.setCookie();
|
||||||
|
|||||||
@@ -15,8 +15,6 @@ const LIMITS = { lines: 50, complexity: 20 };
|
|||||||
const grandfathered: Record<string, { lines?: number; complexity?: number }> = {
|
const grandfathered: Record<string, { lines?: number; complexity?: number }> = {
|
||||||
"src/http/app.ts createApp": { lines: 318, complexity: 28 },
|
"src/http/app.ts createApp": { lines: 318, complexity: 28 },
|
||||||
"src/http/app.ts createApp > handleRequest": { lines: 168, complexity: 43 },
|
"src/http/app.ts createApp > handleRequest": { lines: 168, complexity: 43 },
|
||||||
"src/auth/routes.ts flowPage": { lines: 64 },
|
|
||||||
"src/auth/routes.ts flowPage > anonymous": { lines: 62, complexity: 23 },
|
|
||||||
"src/auth/flow-view.ts buildFlowView": { complexity: 24 },
|
"src/auth/flow-view.ts buildFlowView": { complexity: 24 },
|
||||||
"src/auth/hydra-admin.ts createHydraAdmin": { lines: 87 },
|
"src/auth/hydra-admin.ts createHydraAdmin": { lines: 87 },
|
||||||
"src/auth/kratos-admin.ts createKratosAdmin": { lines: 68 },
|
"src/auth/kratos-admin.ts createKratosAdmin": { lines: 68 },
|
||||||
|
|||||||
@@ -792,6 +792,26 @@ test("themed auth GET: an existing Kratos session (no app JWT yet) recovers via
|
|||||||
assert.equal((await fetch(url2 + "/login", { redirect: "manual" })).status, 500);
|
assert.equal((await fetch(url2 + "/login", { redirect: "manual" })).status, 500);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("themed auth GET: an anonymous hit on /settings (Kratos 401) bounces to sign-in with the page as return_to, never 500", async (t) => {
|
||||||
|
let failure = new KratosError("Kratos init settings flow failed (401)", 401, "");
|
||||||
|
const throwing = async (): Promise<never> => { throw failure; };
|
||||||
|
const app = createApp({ jwks: staticJwks([ecJwk]), kratos: { ...mockKratos(throwing), initBrowserFlow: throwing } });
|
||||||
|
await new Promise<void>((r) => app.listen(0, r));
|
||||||
|
t.after(() => app.close());
|
||||||
|
const url = `http://localhost:${(app.address() as AddressInfo).port}`;
|
||||||
|
|
||||||
|
const init = await fetch(url + "/settings", { redirect: "manual" });
|
||||||
|
assert.equal(init.status, 303);
|
||||||
|
assert.equal(init.headers.get("location"), "/login?return_to=%2Fsettings");
|
||||||
|
const existing = await fetch(url + "/settings?flow=f1", { redirect: "manual" });
|
||||||
|
assert.equal(existing.headers.get("location"), "/login?return_to=%2Fsettings");
|
||||||
|
// /login redirecting to itself would loop, so a 401 there stays the catch-all 500.
|
||||||
|
assert.equal((await fetch(url + "/login", { redirect: "manual" })).status, 500);
|
||||||
|
// Only a 401 means "sign in": an Ory outage on /settings is still the honest 503.
|
||||||
|
failure = new KratosError("Kratos init settings flow failed (500)", 500, "");
|
||||||
|
assert.equal((await fetch(url + "/settings", { redirect: "manual" })).status, 503);
|
||||||
|
});
|
||||||
|
|
||||||
// return_to: a deep-link login lands back on the requested page. The gate redirects to
|
// return_to: a deep-link login lands back on the requested page. The gate redirects to
|
||||||
// /login?return_to=<host-relative path>; /login bakes that into the Kratos flow so completion
|
// /login?return_to=<host-relative path>; /login bakes that into the Kratos flow so completion
|
||||||
// returns there — but a first-party path must route via /auth/complete first (to mint the JWT).
|
// returns there — but a first-party path must route via /auth/complete first (to mint the JWT).
|
||||||
@@ -820,6 +840,23 @@ test("login return_to: a first-party deep link is wrapped through /auth/complete
|
|||||||
assert.equal(lastReturnTo, "//evil.com");
|
assert.equal(lastReturnTo, "//evil.com");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("login return_to: a chosen language alone still routes through /auth/complete, so it survives the sign-in", async (t) => {
|
||||||
|
let lastReturnTo: string | undefined;
|
||||||
|
const kratos: KratosPublic = {
|
||||||
|
...mockKratos(async (_t, id) => loginFlow(id)),
|
||||||
|
initBrowserFlow: async (_t: FlowType, opts = {}) => { lastReturnTo = opts.returnTo; return { flow: { id: "new1", ui: { action: "", method: "post", nodes: [] } }, setCookie: [] }; },
|
||||||
|
};
|
||||||
|
const app = createApp({ i18n: createI18n(await loadI18n()), kratos });
|
||||||
|
await new Promise<void>((r) => app.listen(0, r));
|
||||||
|
t.after(() => app.close());
|
||||||
|
const url = `http://localhost:${(app.address() as AddressInfo).port}`;
|
||||||
|
|
||||||
|
await fetch(url + "/login?locale=sv-SE", { redirect: "manual" });
|
||||||
|
assert.match(lastReturnTo ?? "", /^http:\/\/[^/]+\/auth\/complete\?locale=sv-SE$/);
|
||||||
|
await fetch(url + "/login?locale=sv-SE&return_to=" + encodeURIComponent("/admin/users"), { redirect: "manual" });
|
||||||
|
assert.match(lastReturnTo ?? "", /^http:\/\/[^/]+\/auth\/complete\?return_to=%2Fadmin%2Fusers%3Flocale%3Dsv-SE&locale=sv-SE$/);
|
||||||
|
});
|
||||||
|
|
||||||
// "Ory down ⇒ no logins" is documented; the auth path should say so honestly (503), not the
|
// "Ory down ⇒ no logins" is documented; the auth path should say so honestly (503), not the
|
||||||
// generic "error on our end" 500 the catch-all renders.
|
// generic "error on our end" 500 the catch-all renders.
|
||||||
test("auth flow when Ory is unreachable → an honest 503, not the catch-all 500", async (t) => {
|
test("auth flow when Ory is unreachable → an honest 503, not the catch-all 500", async (t) => {
|
||||||
|
|||||||
@@ -6,15 +6,15 @@
|
|||||||
|
|
||||||
`Bar = 1`
|
`Bar = 1`
|
||||||
|
|
||||||
`Next ID = 39`
|
`Next ID = 40`
|
||||||
|
|
||||||
## Items
|
## Items
|
||||||
|
|
||||||
| ID | Release | Blocked by | Exempt | Item | L | E | I | R | S | Moves | Score |
|
| ID | Release | Blocked by | Exempt | Item | L | E | I | R | S | Moves | Score |
|
||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
|
|---|---|---|---|---|---|---|---|---|---|---|---|
|
||||||
| 20 | 0.4.21 | | | **Redirect an anonymous hit on a session-needing flow page to sign-in, instead of a 500.** | 2 | 2 | 2 | 4 | 3 | 2 by 4 | 4.2 |
|
|
||||||
| 4 | 0.4.21 | | | **Refuse a permission save whose baseline no longer matches Keto, so concurrent edits are not silently lost.** | 2 | 2 | 2 | 4 | 4 | 2 by 4 | 2.8 |
|
| 4 | 0.4.21 | | | **Refuse a permission save whose baseline no longer matches Keto, so concurrent edits are not silently lost.** | 2 | 2 | 2 | 4 | 4 | 2 by 4 | 2.8 |
|
||||||
| 7 | 0.4.21 | | | **Confirm a successful permission save, and report a partial failure instead of hiding it.** | 2 | 2 | 2 | 4 | 4 | 2 by 4 | 2.8 |
|
| 7 | 0.4.21 | | | **Confirm a successful permission save, and report a partial failure instead of hiding it.** | 2 | 2 | 2 | 4 | 4 | 2 by 4 | 2.8 |
|
||||||
|
| 39 | 0.4.21 | | | **Pass an absolute `return_to` on to Kratos even when the URL also carries a chosen `locale`.** | 1 | 2 | 2 | 3 | 2 | 2 by 2 | 2.8 |
|
||||||
| 22 | 0.4.21 | | | **Offer a `users:read` holder a View row action on the users list, not Edit.** | 2 | 2 | 1 | 3 | 3 | 2 by 2 | 2.1 |
|
| 22 | 0.4.21 | | | **Offer a `users:read` holder a View row action on the users list, not Edit.** | 2 | 2 | 1 | 3 | 3 | 2 by 2 | 2.1 |
|
||||||
| 34 | 0.4.21 | | | **Serve every stylesheet and script under a content-versioned URL with a year-long immutable `Cache-Control`, so a page view downloads none of them again.** | 4 | 2 | 3 | 7 | 6 | 8 by 6, 2 by 2 | 1.8 |
|
| 34 | 0.4.21 | | | **Serve every stylesheet and script under a content-versioned URL with a year-long immutable `Cache-Control`, so a page view downloads none of them again.** | 4 | 2 | 3 | 7 | 6 | 8 by 6, 2 by 2 | 1.8 |
|
||||||
| 3 | 0.4.21 | | | **Guard the group paths against self-lockout, or accept the gap explicitly.** | 3 | 2 | 2 | 5 | 6 | 2 by 6 | 1.4 |
|
| 3 | 0.4.21 | | | **Guard the group paths against self-lockout, or accept the gap explicitly.** | 3 | 2 | 2 | 5 | 6 | 2 by 6 | 1.4 |
|
||||||
@@ -32,12 +32,6 @@
|
|||||||
|
|
||||||
## Details
|
## Details
|
||||||
|
|
||||||
### 20. Redirect an anonymous hit on a session-needing flow page to sign-in, instead of a 500.
|
|
||||||
|
|
||||||
`flowPage` (`src/auth/routes.ts`) rethrows a 401, which Kratos answers to a settings-flow init with no
|
|
||||||
session. `/settings` stays `public`, since recovery lands there with a Kratos session but no app JWT;
|
|
||||||
a 401 should redirect to `/login` with the page as `return_to`. Add the missing E2E.
|
|
||||||
|
|
||||||
### 4. Refuse a permission save whose baseline no longer matches Keto, so concurrent edits are not silently lost.
|
### 4. Refuse a permission save whose baseline no longer matches Keto, so concurrent edits are not silently lost.
|
||||||
|
|
||||||
Sketch: post the rendered set as a hidden baseline; when it no longer matches Keto, re-render with
|
Sketch: post the rendered set as a hidden baseline; when it no longer matches Keto, re-render with
|
||||||
@@ -49,6 +43,17 @@ Sketch: post the rendered set as a hidden baseline; when it no longer matches Ke
|
|||||||
half-applied set behind the generic error page. The `alert alert-pos` pattern of the recovery-code
|
half-applied set behind the generic error page. The `alert alert-pos` pattern of the recovery-code
|
||||||
banner is available.
|
banner is available.
|
||||||
|
|
||||||
|
### 39. Pass an absolute `return_to` on to Kratos even when the URL also carries a chosen `locale`.
|
||||||
|
|
||||||
|
`flowReturnTo` (`src/auth/routes.ts`) returns the absolute target as-is only when no locale is chosen.
|
||||||
|
With `/login?return_to=<absolute OAuth2 login URL>&locale=sv-SE` and no `?flow=`, it builds
|
||||||
|
`<origin>/auth/complete?locale=sv-SE` instead and drops the absolute target, so after sign-in the
|
||||||
|
visitor lands on the dashboard and the OAuth2 login challenge never continues. Normal paths don't hit
|
||||||
|
it, because Hydra's redirect carries no `locale` and the language picker acts on a page that already
|
||||||
|
holds a `?flow=`; a hand-built URL does. Fix by keeping the absolute target when `localPath` rejects
|
||||||
|
it, losing the language across that round-trip instead, and add the case to the `login return_to`
|
||||||
|
test in `src/http/app.test.ts`.
|
||||||
|
|
||||||
### 22. Offer a `users:read` holder a View row action on the users list, not Edit.
|
### 22. Offer a `users:read` holder a View row action on the users list, not Edit.
|
||||||
|
|
||||||
The pencil opens the read-only detail page, so it is harmless, but the label contradicts what the
|
The pencil opens the read-only detail page, so it is harmless, but the label contradicts what the
|
||||||
|
|||||||
Reference in New Issue
Block a user