From 1ef0c6fc8a33386fbd87b6c09f7aac9785603d35 Mon Sep 17 00:00:00 2001 From: Aravind Date: Tue, 29 Sep 2026 16:59:13 +0530 Subject: [PATCH] layout change ui fix --- scripts/authreturnto-check.mjs | 134 ++++++++++++++++++++++ scripts/skill-check.mjs | 112 +++++++++++++++++- src/components/ui-editor/UiEditor.tsx | 114 ++++++------------ src/components/ui-tree/UiNodeBoundary.tsx | 2 +- src/lib/authReturnTo.ts | 133 +++++++++++++++++++++ src/lib/ui/intent.ts | 39 ++++++- src/pages/admin/Login.tsx | 27 ++++- 7 files changed, 475 insertions(+), 86 deletions(-) create mode 100644 scripts/authreturnto-check.mjs create mode 100644 src/lib/authReturnTo.ts diff --git a/scripts/authreturnto-check.mjs b/scripts/authreturnto-check.mjs new file mode 100644 index 0000000..f71fddb --- /dev/null +++ b/scripts/authreturnto-check.mjs @@ -0,0 +1,134 @@ +/** + * returnTo validation check. + * + * Runs the REAL module through Vite's SSR loader, the same way + * skill-check.mjs does, so the `@/` alias and the TypeScript compile are the + * app's own rather than a reimplementation of them. No test framework is added + * for a 130-line module; this follows the convention already in this directory. + * + * node scripts/authreturnto-check.mjs + * + * Exits non-zero on failure, so it can gate a build. + * + * WHAT THIS IS DEFENDING + * + * safeReturnTo decides whether a URL somebody else supplied may be navigated + * to. The cases below are therefore mostly hostile input, and each asserts the + * result is null rather than merely "not the attacker's value" — a wrong answer + * that is still a navigation is not a pass. + */ +import { createServer } from 'vite'; + +const results = []; +const record = (name, pass, detail = '') => { + results.push({ name, pass, detail }); + console.log(`[${pass ? ' ok ' : ' FAIL '}] ${name}${detail ? ` — ${detail}` : ''}`); +}; + +const ORIGIN = 'https://platform.krowforce.com'; + +// A real authorization URL, with every parameter the flow depends on, built the +// way the Go server builds it: path + RawQuery, percent-escaped into ?returnTo=. +const AUTHORIZE = + '/oauth/authorize?client_id=989c3ec1-4afa-4d76-93fa-7f45f1d45e22' + + '&redirect_uri=https%3A%2F%2Fclaude.ai%2Fapi%2Fmcp%2Fauth_callback' + + '&response_type=code' + + '&code_challenge=E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM' + + '&code_challenge_method=S256' + + '&resource=https%3A%2F%2Fplatform.krowforce.com%2Fmcp' + + '&scope=krow.read' + + '&state=vT7nQ2xK_Lp9'; + +const q = (v) => '?returnTo=' + encodeURIComponent(v); + +const server = await createServer({ server: { middlewareMode: true }, appType: 'custom', logLevel: 'error' }); +try { + globalThis.window = { location: { origin: ORIGIN, search: '' } }; + const { safeReturnTo } = await server.ssrLoadModule('/src/lib/authReturnTo.ts'); + + /* ── 1–2. The OAuth authorize URL, and its query byte for byte ─────────── */ + + const oauth = safeReturnTo(q(AUTHORIZE)); + record('1. /oauth/authorize is accepted', oauth !== null && oauth.path === AUTHORIZE, + oauth ? `via=${oauth.via}` : 'returned null'); + record('12. and is marked for full browser navigation', oauth?.via === 'browser', + `via=${oauth?.via} — React Router has no such route`); + + for (const [name, literal] of [ + ['client_id', 'client_id=989c3ec1-4afa-4d76-93fa-7f45f1d45e22'], + ['redirect_uri', 'redirect_uri=https%3A%2F%2Fclaude.ai%2Fapi%2Fmcp%2Fauth_callback'], + ['response_type', 'response_type=code'], + ['code_challenge', 'code_challenge=E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM'], + ['code_challenge_method', 'code_challenge_method=S256'], + ['resource', 'resource=https%3A%2F%2Fplatform.krowforce.com%2Fmcp'], + ['scope', 'scope=krow.read'], + ['state', 'state=vT7nQ2xK_Lp9'], + ]) { + record(`2. ${name} preserved exactly`, Boolean(oauth?.path.includes(literal))); + } + record('2. percent-encoding is not rewritten', Boolean(oauth?.path.includes('%2F')), + '%2F must not become /'); + record('2. an encoded space survives', + safeReturnTo(q('/oauth/authorize?scope=krow.read%20krow.write'))?.path.includes('%20') === true, + '%20 must not become +'); + + /* ── 3, 13. Internal admin routes keep router navigation ───────────────── */ + + const admin = safeReturnTo(q('/admin/candidates?stage=applied')); + record('3. /admin/... is accepted', admin?.path === '/admin/candidates?stage=applied'); + record('13. and is marked for router navigation', admin?.via === 'router', + `via=${admin?.via} — must not reload the app`); + record('3. bare /admin is accepted', safeReturnTo(q('/admin'))?.via === 'router'); + + /* ── 4–10. Hostile and malformed values are refused ────────────────────── */ + + const refuse = [ + ['4. external URL', 'https://evil.example'], + ['4. external URL with our path', 'https://evil.example/oauth/authorize'], + ['4. userinfo trick', 'https://platform.krowforce.com@evil.example/'], + ['4. another port on our host', 'https://platform.krowforce.com:8443/admin'], + ['5. protocol-relative', '//evil.example'], + ['5. protocol-relative with path', '//evil.example/steal'], + ['5. triple slash', '///evil.example'], + ['6. javascript:', 'javascript:alert(document.cookie)'], + ['6. javascript: mixed case', 'JaVaScRiPt:alert(1)'], + ['6. tab-obfuscated scheme', 'java\tscript:alert(1)'], + ['7. data:', 'data:text/html,'], + ['8. backslash', '/\\evil.example'], + // These two reach the slash/backslash guard specifically: the path is on + // the allowlist, so only the guard can refuse them. Without them the guard + // is unfalsifiable — removing it leaves every other case still passing, + // which a mutation run showed. + ['8. backslash in the query of an allowed path', '/admin/candidates?a=\\evil'], + ['8. backslash escape smuggled past the allowlist', '/admin/x?next=/\\evil.example'], + ['8. dot-slash-slash', '/.//evil.example'], + ['9. /admin/login itself', '/admin/login'], + ['9. /admin/login with a query', '/admin/login?returnTo=%2Fadmin'], + ['9. bare /login', '/login'], + ['10. malformed', 'http://[::1'], + ['10. file scheme', 'file:///etc/passwd'], + ['10. unrelated backend route', '/oauth/token'], + ['10. unrelated app route', '/apply'], + ['10. the MCP endpoint', '/mcp'], + ]; + for (const [name, value] of refuse) { + const got = safeReturnTo(q(value)); + record(`${name} is refused`, got === null, got ? `returned ${JSON.stringify(got)}` : ''); + } + + /* ── 11. Absent or empty falls back safely ─────────────────────────────── */ + + for (const [name, search] of [ + ['no query at all', ''], + ['other parameters only', '?foo=bar'], + ['empty returnTo', '?returnTo='], + ]) { + record(`11. ${name} returns null`, safeReturnTo(search) === null); + } +} finally { + await server.close(); +} + +const failed = results.filter((r) => !r.pass).length; +console.log(`\n${results.length - failed} passed, ${failed} failed\n`); +process.exit(failed === 0 ? 0 : 1); diff --git a/scripts/skill-check.mjs b/scripts/skill-check.mjs index 1890304..d7c2327 100644 --- a/scripts/skill-check.mjs +++ b/scripts/skill-check.mjs @@ -7976,8 +7976,30 @@ console.log('\n── Candidates vs Talent Pool ──'); return html.slice(0, at) + html.slice(i); }; - record('the layout controls are present and separable', - now.includes('
{ + const withBar = `

kept

Apply

also kept

`; + const stripped = stripControls(withBar); + return !stripped.includes('data-ui-controls') + && stripped.includes('

kept

') + && stripped.includes('

also kept

'); + })()); now = stripControls(now); @@ -8308,6 +8330,92 @@ console.log('\n── Candidates vs Talent Pool ──'); return r.ok && ids.indexOf('timeline') < ids.indexOf('activity-privileged-notice'); })()); + /* ── Where "to the top" is allowed to be said ──────────────────────────── */ + + /** + * The destination phrasings, because the missing ones read as a broken feature. + * + * Reported from production: "show the pipeline move to top" was answered "I + * could not find that on this page." The target resolved perfectly well — the + * refusal came from `planMove`, which knew `to the top` and did not know + * `to top`, and a move with no destination falls through to `unknown`. The + * user cannot tell that apart from the section not existing. + */ + const movesFirst = (q) => { + const m = ask(q); + if (m?.kind !== 'plan' || m.op.op !== 'reorder') return false; + return m.op.order[0] === 'timeline'; + }; + const movesLast = (q) => { + const m = ask(q); + if (m?.kind !== 'plan' || m.op.op !== 'reorder') return false; + return m.op.order[m.op.order.length - 1] === 'timeline'; + }; + + for (const q of [ + 'move timeline to top', + 'move timeline to the top', + 'move the timeline to the very top', + 'move timeline up', + 'move the timeline first', + 'put the timeline at the top', + 'show the timeline move to top', + ]) { + record(`"${q}" → timeline first`, movesFirst(q)); + } + + for (const q of [ + 'move timeline to bottom', + 'move timeline to the bottom', + 'move the timeline to the end', + 'move timeline down', + 'move the timeline last', + ]) { + record(`"${q}" → timeline last`, movesLast(q)); + } + + /** + * A destination word inside a section's NAME is not a destination. + * + * `over` lives inside `coverage`, `end` inside `trends`. Matched as + * substrings, "move coverage trends to the bottom" satisfied both the top + * reading and the bottom one — and the top is tested first, so it moved the + * opposite way from the one asked for. Whole-word matching is what fixed it; + * this is the case that proves it. + */ + const coverageCase = (() => { + const placed = opsMod4.applyOperation(tree4, { + op: 'add', parent: null, + node: { + id: 'cov-1', type: 'flow', data: { source: 'candidates.activity' }, + props: { title: 'Coverage trends' }, + }, + }, { registry: reg4 }); + if (!placed.ok) return { pass: false, detail: 'fixture could not be placed' }; + + /* Put it at the TOP first, so "to the bottom" is a real change. Added at + the end, it is already there and the answer is a refusal rather than a + plan — which would pass this check for the wrong reason. */ + const atTop = opsMod4.applyOperation(placed.tree, { + op: 'reorder', + parent: null, + order: ['cov-1', ...placed.tree.map((n) => n.id).filter((id) => id !== 'cov-1')], + }, { registry: reg4 }); + if (!atTop.ok) return { pass: false, detail: 'fixture could not be placed first' }; + + const m = intentMod.matchUiEdit('move coverage trends to the bottom', + { tree: atTop.tree, registry: reg4 }); + if (m?.kind !== 'plan' || m.op.op !== 'reorder') { + return { pass: false, detail: JSON.stringify(m) }; + } + return { + pass: m.op.order[m.op.order.length - 1] === 'cov-1', + detail: m.op.order.join(', '), + }; + })(); + record('a section named "Coverage trends" still moves to the bottom', + coverageCase.pass, coverageCase.detail); + record('"Change the hiring activity to a table." → replace with table', (() => { const m = ask('Change the hiring activity to a table.'); return m?.kind === 'plan' && m.op.op === 'replace' && m.op.target === 'flow-1' && m.op.type === 'table'; diff --git a/src/components/ui-editor/UiEditor.tsx b/src/components/ui-editor/UiEditor.tsx index ebfde17..9948000 100644 --- a/src/components/ui-editor/UiEditor.tsx +++ b/src/components/ui-editor/UiEditor.tsx @@ -1,88 +1,61 @@ import React from 'react'; -import { RotateCcw, SlidersHorizontal, Undo2 } from 'lucide-react'; +import { RotateCcw, Undo2 } from 'lucide-react'; import { Button } from '@/components/ds'; import { useUiEditing } from '@/components/ui-tree/UiEditingProvider'; -import { inspectTree } from '@/lib/ui/inspect'; -import { nodeRegistry } from '@/lib/ui/registry'; -import { TreePanel } from './TreePanel'; -import { NodeInspector } from './NodeInspector'; -import { NodePicker } from './NodePicker'; /** - * The visual editor. + * The preview bar for a layout change. * * It is a *client* of the UI system, not a second implementation of it. Every - * control it draws ends in one call — `propose(op)` on the editing session — - * with an operation object of exactly the shape Owliver produces for the same - * change. From there the two are indistinguishable: same validation, same - * preview merge, same Apply, same `preferences.uiLayouts`. + * control it draws ends in one call on the editing session, with an operation + * of exactly the shape Owliver produces for the same change. From there the two + * are indistinguishable: same validation, same preview merge, same Apply, same + * `preferences.uiLayouts`. * - * That is the whole architecture: + * Owliver → operation → propose → validate → preview + * ↓ Apply + * preferences.uiLayouts * - * editor / Owliver → operation → propose → validate → preview - * ↓ Apply - * preferences.uiLayouts + * WHAT THIS USED TO BE, AND WHY IT IS NOT * - * There is no page in this file, no component name, and no branch on what a - * node is. What can be done to the selected node comes from its registration; - * what can be added comes from the registry and the closed data vocabulary; - * what it is showing comes from the tree. A page that migrates tomorrow is - * editable tomorrow with nothing here changed. + * There was a `Customise layout` button here, and behind it a tree panel, a + * node inspector and a picker — a visual editor sitting beside Owliver, both + * producing the same operations. It was removed on request: two ways to + * rearrange a page is two things to explain, and the panel was on every admin + * screen whether or not anybody was arranging anything. + * + * Owliver is now the only way to propose a layout change. This component keeps + * the other half of that conversation — a person still has to SEE the change + * and decide. `TreePanel`, `NodeInspector` and `NodePicker` are left in the + * directory, unimported: they are the editor, intact, if it is ever wanted + * back. + * + * Nothing about applying moved. `useAssistant` calls `apply` and `discard` on + * the session directly, so "Apply" typed into the panel works exactly as the + * button does — see the `ui-apply` branch there. */ -export function UiEditor({ registry = nodeRegistry }) { +export function UiEditor() { const editing = useUiEditing(); - const [open, setOpen] = React.useState(false); - const [selectedId, setSelectedId] = React.useState(null); /* Rendered only where a page has opted into composition. A page that has not is not broken; it simply has nothing to arrange. */ if (!editing) return null; const { - tree, propose, discard, apply, undo, reset, + discard, apply, undo, reset, previewing, customised, saving, problems, skipped, - /* The page this session belongs to. Handed to the picker so what can be - added here is decided by the registry rather than by the picker being - shown everything that exists. */ - page, } = editing; - const nodes = inspectTree(tree, { registry }); - const selected = nodes.find((node) => node.id === selectedId) || null; - /* A node's own container, for the reorder buttons and for the picker. */ - const siblings = selected ? nodes.filter((node) => node.parent === selected.parent) : []; - const parent = selected?.container ? selected : nodes.find((n) => n.id === selected?.parent) || null; - - /** - * The one door out of this component. - * - * Everything the panels do arrives here as an operation and goes straight to - * the session. Nothing is applied, nothing is stored, and nothing is - * validated locally — `propose` refuses what cannot be kept and the refusal - * is shown below. - */ - const operate = (op) => { - const result = propose(op); - /* A removed node cannot stay selected; a replaced one keeps its id. */ - if (result.ok && op.op === 'remove') setSelectedId(null); - }; + /* Nothing proposed and nothing saved means nothing to say. Without this the + component drew an empty bar on every page, which is most of what the button + was being blamed for. */ + if (!previewing && !customised && !problems.length && !skipped.length) return null; return (
-
- - - {/* The preview bar. Unsaved and saved have to be told apart at a - glance, because the whole promise is that nothing is kept until - somebody says so. */} +
+ {/* Unsaved and saved have to be told apart at a glance, because the + whole promise is that nothing is kept until somebody says so. */} {(previewing || customised) && (
{previewing && ( @@ -122,25 +95,6 @@ export function UiEditor({ registry = nodeRegistry }) { {skipped.length} saved change{skipped.length === 1 ? '' : 's'} no longer apply to this page.

)} - - {open && ( -
-
- - -
- -
- {selected - ? - : ( -

- Choose a section on the left to see what can be changed about it. -

- )} -
-
- )}
); } diff --git a/src/components/ui-tree/UiNodeBoundary.tsx b/src/components/ui-tree/UiNodeBoundary.tsx index 681554b..c6f9889 100644 --- a/src/components/ui-tree/UiNodeBoundary.tsx +++ b/src/components/ui-tree/UiNodeBoundary.tsx @@ -88,7 +88,7 @@ export class UiNodeBoundary extends React.Component

- It is still on the page and can be hidden or removed from Customise layout. + It is still on the page. Ask Owliver to hide it.

); diff --git a/src/lib/authReturnTo.ts b/src/lib/authReturnTo.ts new file mode 100644 index 0000000..7edfbf1 --- /dev/null +++ b/src/lib/authReturnTo.ts @@ -0,0 +1,133 @@ +/** + * Where an interrupted flow resumes after sign-in. + * + * Kept in one module because it is security-sensitive and easy to drift: this is + * the only place that decides whether a URL somebody else supplied may be + * navigated to. + * + * WHO SETS ?returnTo=, AND WHY THIS EXISTS + * + * The API does. `GET /oauth/authorize` is the browser leg of the MCP OAuth + * flow, and it needs a signed-in person to show a consent screen to. When + * nobody is signed in it redirects to the configured login path, carrying its + * own path and query so the authorization request survives the round trip: + * + * /admin/login?returnTo=%2Foauth%2Fauthorize%3Fclient_id%3D...%26state%3D... + * + * Without this module the person signs in, lands on the dashboard, and the + * authorization request is gone — the connector can never finish. + * + * WHY AN ALLOWLIST RATHER THAN "ANY SAME-ORIGIN PATH" + * + * Only two kinds of destination are reachable this way, and they are reached + * differently, so naming them is both safer and necessary: + * + * /oauth/authorize a BACKEND route. React Router has no such path, so + * routing to it client-side renders the not-found page and + * the request never reaches the server. It needs a real + * browser navigation. + * /admin/... an in-app route, which is what the existing + * `location.state.from` mechanism already carries. Router + * navigation, exactly as before. + * + * Anything else has no business arriving in this parameter, and the narrower + * rule means a future backend route cannot be reached through here by accident. + * + * THE SAME-ORIGIN CHECK IS NOT ENOUGH ON ITS OWN + * + * Values like `/.//evil.example` and `/\evil.example` resolve same-origin and + * then normalise to a protocol-relative `//evil.example` when assigned to + * location — an open redirect through a check that appeared to pass. So the + * resolved path must also begin with exactly one slash and contain no + * backslash. The allowlist below would catch these anyway; both are kept + * because each is load-bearing on its own, and a later edit that loosens the + * allowlist must not silently lose the other. + * + * WHY THE QUERY STRING IS PASSED THROUGH UNTOUCHED + * + * `url.pathname + url.search` is the original text, byte for byte. Nothing here + * reads, rewrites or re-serialises the parameters, and that is deliberate: + * touching `url.searchParams` at all makes the browser re-encode the whole + * query, which can turn `%20` into `+` inside `scope`, or re-spell the + * percent-encoding of `redirect_uri` and `resource`. Every one of those is a + * value the authorization server compares EXACTLY — `code_challenge` against + * the verifier, `redirect_uri` against the registered URI, `state` against what + * the client sent. A re-encoded query is a different query, and the failure + * would surface much later as a mismatched PKCE challenge. + */ + +/** How the destination has to be reached. */ +export type ReturnVia = 'browser' | 'router'; + +export interface ReturnTarget { + /** A path on this origin, with its query preserved exactly. */ + path: string; + /** + * 'browser' for a backend route, which must bypass React Router. + * 'router' for an in-app route, which must not reload the page. + */ + via: ReturnVia; +} + +/** The backend route the OAuth browser leg returns to. */ +const OAUTH_AUTHORIZE_PATH = '/oauth/authorize'; + +/** The login itself, which would sign a person in and show them the login. */ +const LOGIN_PATH = '/admin/login'; + +/** The in-app console. Everything under it is a React Router destination. */ +const ADMIN_PREFIX = '/admin'; + +/** + * Resolve a `returnTo` query parameter to a safe destination. + * + * @param search The query string to read, defaulting to the document's. The + * login page passes the router's value explicitly so this never reaches for a + * global it does not need. + * @returns The destination and how to reach it, or null when there is nothing + * safe to return to — in which case the caller keeps its existing behaviour. + */ +export function safeReturnTo(search?: string): ReturnTarget | null { + if (typeof window === 'undefined') return null; + + const raw = new URLSearchParams(search ?? window.location.search).get('returnTo'); + if (!raw) return null; + + let url: URL; + try { + url = new URL(raw, window.location.origin); + } catch { + // Malformed. Nothing safe to do with it. + return null; + } + + // Cross-origin, and anything carrying a scheme of its own — javascript:, + // data:, https://evil.example — fails here: the resolved origin is not ours. + if (url.origin !== window.location.origin) return null; + + const path = url.pathname + url.search; + + // Exactly one leading slash, no backslash. See the note above on why the + // origin check does not cover this. The backslash test also reaches values + // the allowlist below would have accepted — a backslash survives unencoded + // into `search`, so `/admin/x?next=/\evil.example` is on the allowlist and + // refused only here. The cost is a legitimate `state` containing a raw + // backslash being refused too; that fails closed, landing the person on the + // dashboard rather than anywhere an attacker chose. + if (!path.startsWith('/') || path.startsWith('//') || path.includes('\\')) return null; + + if (url.pathname === OAUTH_AUTHORIZE_PATH) { + return { path, via: 'browser' }; + } + + // The login is refused before the admin prefix is considered, because it sits + // underneath it. + if (url.pathname === LOGIN_PATH) return null; + + if (url.pathname === ADMIN_PREFIX || url.pathname.startsWith(ADMIN_PREFIX + '/')) { + return { path, via: 'router' }; + } + + // Everything else: not a destination this parameter is for. + return null; +} diff --git a/src/lib/ui/intent.ts b/src/lib/ui/intent.ts index 4c723de..4efc5a3 100644 --- a/src/lib/ui/intent.ts +++ b/src/lib/ui/intent.ts @@ -37,6 +37,22 @@ const canon = (value) => String(value ?? '') const has = (text, ...words) => words.some((w) => text.includes(canon(w))); +/** + * Whole-word match, for words that live inside longer ones. + * + * `has` is a substring test. That is right for phrases and wrong for short + * words: `over` is inside `coverage` and `overtime`, `end` is inside `trends` + * and `calendar`, `up` is inside `group`. Read as substrings, "move coverage to + * the bottom" matched both a top reading and a bottom one — and the top is + * tested first, so it moved the opposite way from the one asked for. + * + * Single words only. A phrase is unambiguous as a substring and stays on `has`. + */ +const hasWord = (text, ...words) => { + const tokens = new Set(String(text ?? '').split(' ')); + return words.some((w) => tokens.has(canon(w))); +}; + /** * The verbs, as data. * @@ -443,8 +459,27 @@ function planUnhide(text, tree, registry) { function planMove(text, tree, registry) { const roots = tree.map((node) => describeNode(node, { registry })); - const before = has(text, 'above', 'before', 'over', 'on top of', 'to the top', 'first'); - const after = has(text, 'below', 'under', 'beneath', 'after', 'to the bottom', 'last', 'end'); + /** + * Where it should end up. + * + * The phrase list is long because people say this a dozen ways and the ones + * that were missing were the short ones: "move it to top" was refused while + * "move it to the top" worked, which reads as the feature being broken rather + * than as a phrasing it does not know. A destination is the whole of what + * this function needs, so failing to recognise one costs the entire request. + * + * Short words go through `hasWord` — see its note. `top` and `bottom` alone + * are enough on their own here: by the time a sentence has reached planMove + * it is already a move, and a bare "top" in a move is a destination. + */ + const before = has(text, 'on top of', 'to the top', 'to top', 'to the very top', + 'at the top', 'to the start', 'to the front', 'right to the top') + || hasWord(text, 'above', 'before', 'over', 'top', 'topmost', 'first', 'up', 'upward', 'upwards'); + + const after = has(text, 'to the bottom', 'to bottom', 'to the very bottom', + 'at the bottom', 'to the end', 'to the back', 'right to the bottom') + || hasWord(text, 'below', 'under', 'underneath', 'beneath', 'after', 'bottom', + 'last', 'end', 'down', 'downward', 'downwards'); /* Split on the positional word so the two halves name two different nodes: "move timeline above the notice" is a subject and a reference. */ diff --git a/src/pages/admin/Login.tsx b/src/pages/admin/Login.tsx index acc527d..9d21519 100644 --- a/src/pages/admin/Login.tsx +++ b/src/pages/admin/Login.tsx @@ -5,6 +5,7 @@ import { cn } from '@/lib/utils'; import { KROW_LOGO_URL } from '@/assets/brand'; import { Checkbox } from '@/components/ds'; import { useAuth } from '@/lib/AuthContext'; +import { safeReturnTo } from '@/lib/authReturnTo'; const DEMO_EMAIL = 'demo@krow.app'; @@ -26,6 +27,24 @@ export default function AdminLogin() { const location = useLocation(); const { login } = useAuth(); + /* Two kinds of "where was I going", and they are not interchangeable. + + A ?returnTo= in the QUERY is put there by the API, not by this app. The + OAuth authorization endpoint redirects here when nobody is signed in, + carrying its own path and query so the authorization request survives the + round trip. It names a route on the BACKEND (/oauth/authorize), which React + Router does not have and must not be given — routing to it client-side + renders the not-found page and the connector never finishes. So it is + followed with a real navigation, which safeReturnTo reports as `browser`. + + `location.state.from` is the in-app case: a guard bounced someone off a + page in this bundle (ProtectedRoute and AdminRoute both set it). That is a + router destination and stays one, unchanged. + + The query wins when both exist. It is the more specific instruction, and it + is the one the person is actually in the middle of. */ + const fromQuery = safeReturnTo(location.search); + const from = location.state?.from; const returnTo = typeof from === 'string' && from.startsWith('/admin') && from !== '/admin/login' ? from @@ -79,7 +98,13 @@ export default function AdminLogin() { } setStatus('success'); - setTimeout(() => navigate(returnTo, { replace: true }), 320); + setTimeout(() => { + // `replace`, not `assign`: the login should not sit in history between + // the authorization request and the consent screen, or Back from consent + // returns to a login the person has already completed. + if (fromQuery?.via === 'browser') window.location.replace(fromQuery.path); + else navigate(fromQuery?.path ?? returnTo, { replace: true }); + }, 320); }; const busy = status !== 'idle';