Loopback login page: explain admission refusals instead of 'Login failed'

[platypii]

An unadmitted user saw a bare "Login failed / You can close this tab and return to the terminal" in the browser tab. That hides the actual outcome: authentication worked, admission did not, and only an admin can fix it.

The loopback receiver now maps the server's D7 refusal codes to their own pages:

• no_membership: the account is not associated with an authorized organization, with a link to https://hyperparam.app/contact to request access.
• org_not_permitted: not a member of the requested org, check --org or contact us.
• org_selection_required: re-run with --org <name>.

Any other error code keeps the existing generic page. Titles and details stay our own literals, so the unescaped interpolation in respond() is still safe; a couple of them now carry a link or <code> on purpose. The terminal already explained these codes via explainLoginError; this is the browser half of the same taxonomy.

Two tests added: the refusal page names the reason and the contact URL, and an unknown code still gets the generic page.

[philcunliffe]

Review: findings (one real bug, fixed; two handed back)

Your safety claim holds. I traced it rather than taking it on trust.

respond() (src/core/remote/loopback.js:303) has exactly four call sites, all in that file. Three pass string literals. The only non-literal path is :201, where page resolves to a REFUSAL_PAGES entry or GENERIC_FAILURE_PAGE, both module-scope object literals built from string literals and CONTACT_URL. No template hole in either is fed by anything dynamic.

Specifically confirmed:

• The refusal code selects, never interpolates.safeError is a lookup key at :200 and otherwise appears only in the Error message and the ERROR_KIND log at :202. It never reaches LANDING_PAGE, including on the fallback path.
• The org name never reaches the page.--org is a /login/start selector; the two org pages say a generic <code>--org</code> and do not echo the user's value. That also matches LLP 0058 D7's "never enumerate the user's orgs".
• Nothing else request-derived reaches HTML.state, code, error_description, headers and path are unused in the page or handled before respond(). No error .message is interpolated.
• No token or auth-code leakage into page or logs. fail() (:142) logs only the bounded safeError; the code is never logged.
• State validation is unchanged. The new branch sits at :194, strictly after the returnedState !== state guard at :190.

One note for the future: respond() uses String.prototype.replace with a string replacement, which honors $&, $`, $' and $1in the replacement. Inert today because every replacement is a $-free literal, but it means the "literals only" invariant is carrying more weight than XSS prevention alone. Worth a line in the respond() JSDoc if anyone loosens it.

Fixed and pushed (99e26d7)

1. major - prototype-chain lookup defeated the generic fallback.src/core/remote/loopback.js:200:

constpage=REFUSAL_PAGES[safeError]??GENERIC_FAILURE_PAGE

REFUSAL_PAGES is a plain object literal, so it inherits from Object.prototype, and sanitizeErrorCode permits printable ASCII. So an attacker-chosen code can be any inherited key: REFUSAL_PAGES['constructor'] returns the Object function, which is non-nullish, so ?? accepts it, and page.title / page.detail are undefined. Reproduced against the real listener:

{"error":"constructor", "h1":"undefined","p":"undefined"}
{"error":"toString", "h1":"undefined","p":"undefined"}
{"error":"__proto__", "h1":"undefined","p":"undefined"}
{"error":"valueOf", "h1":"undefined","p":"undefined"}
{"error":"hasOwnProperty","h1":"undefined","p":"undefined"}

Not an injection (only the literal string undefined renders), but a regression against master, where every error= value produced the generic page. It contradicts exactly what your second test asserts. Typecheck cannot catch it: tsconfig.json does not set noUncheckedIndexedAccess, so TS types the index access as non-optional and treats the ?? branch as dead.

Fixed with Object.hasOwn, already this repo's idiom for untrusted keys (client_detach_disk.js:200,233,245).

2. minor - the refusal map cited LLP 0058 D7 in prose but carried no @ref, so /ref-check could not validate it. Its terminal twin explainLoginError already carries @ref LLP 0058#d7 [implements]. Added the matching annotation; the <a id="d7"></a> anchor exists at llp/0058-oidc-login-client.decision.md:165.

3. minor - two of three mapped codes were untested. Only no_membership had one, so a typo in either other detail string would have shipped green. Added a test covering both.

On your second test, 'an unknown error code still gets the generic page': it passes unmodified on master, so it does not fail if the mapping is removed. Fine for a fall-through guard, but it means neither added test was load-bearing against the fallback path, which is how finding 1 got through.

Handed back (design calls, not pushed)

4. minor - the browser and terminal give different remedies for the same code.explainLoginError (src/core/cli/remote_commands.js:751) tells the user to ask an admin to invite you on no_membership; your page sends them to https://hyperparam.app/contact. On a self-hosted hypaware-server, which LLP 0058's Consequences explicitly supports ("provider-agnostic and works against any hypaware-server with login configured"), the org admin is the customer's own admin and hyperparam.app is the wrong destination. Hardcoding a vendor URL into a page served for any deployment is your call to make, not mine. Options: drop the link and mirror the terminal's "ask an admin", or derive the contact target from the identity origin. Either way the two halves should agree.

5. minor - access_denied is in the taxonomy but not in your map.explainLoginError handles four codes; you map three. access_denied (llp/0059-oidc-login-client.design.md:47) still gets the bare "Login failed" page, which is the exact stranding this PR sets out to fix: the user denied consent at the provider and the tab does not say so. Probably deliberate since it is a provider error rather than an admission refusal, but the two halves now cover different sets, so worth stating either way.

Checked and clean

Status, Content-Type with charset, Connection: close and res.end() are uniform across all branches (respond() is shared and unmodified). The refusal branch calls fail(), which clears the timer and calls server.close() as before, so no refusal path leaves the CLI hanging on a live listener. Empty and missing error both route to 'unknown_error' then the generic page; searchParams.get cannot return a non-string. No em dashes, no semicolons, no @typedef, no inline import('...') types.

Verification

• npm test: 3289 tests, 3288 pass, 0 fail, 1 skipped (baseline 3287/3286/0/1)
• npm run typecheck: clean
• The added prototype-key test was confirmed to fail on the pre-fix code (not ok 7) and pass after, so it is load-bearing.

Pushed: 99e26d7.

[philcunliffe]

Review round 2: findings

The only delta since the round 1 record (bfa7f06) is 99e26d7, which neutral pushed under neutral:adopt: the Object.hasOwn guard, the two missing refusal-code tests, and the @ref LLP 0058#d7 annotation. Reviewed at this head: the guard is correct and the comment explains why the plain index was unsafe; the added test was verified to fail on the pre-fix code, so it is load-bearing. CI is green and the PR is mergeable at 99e26d7.

Findings 4 and 5 from round 1 still stand, and both are yours to decide.

Blocking ask - finding 4.no_membership sends the browser to https://hyperparam.app/contact while explainLoginError (src/core/cli/remote_commands.js:751) tells the same user to ask their admin for an invite. On a self-hosted hypaware-server, which LLP 0058's Consequences explicitly supports, the admin is the customer's own and the vendor URL is the wrong destination. Either drop the link and mirror the terminal's wording, or derive the contact target from the identity origin. The two halves should agree.

Finding 5 is optional.access_denied is in the taxonomy but not in your map, so it still gets the bare 'Login failed' page. Plausibly deliberate (it is a provider error, not an admission refusal), but the browser and terminal halves now cover different sets. A sentence either way settles it.

Reply on this thread or push to the branch, and neutral will re-engage on its next tick.

[philcunliffe]

Requesting changes on one point; the full review records are in the thread (rounds 1 and 2).

Your safety claim about the unescaped interpolation holds - I traced every value reaching the HTML and none is request- or server-derived. But the lookup behind it had a real bug: REFUSAL_PAGES[safeError] ?? GENERIC_FAILURE_PAGE reads through the prototype chain, so ?error=constructor (or toString, __proto__, valueOf, hasOwnProperty) returned a non-nullish value that ?? accepted and rendered a page titled "undefined" - a regression against master, where every code produced the generic page. Fixed and pushed as 99e26d7 with Object.hasOwn, plus tests for the two mapped codes that had none.

The blocking ask:no_membership links to https://hyperparam.app/contact, while explainLoginError tells the same user to ask their admin for an invite. On a self-hosted hypaware-server (explicitly supported per LLP 0058's Consequences) the admin is the customer's own and the vendor URL is wrong. Drop the link and mirror the terminal's wording, or derive the contact target from the identity origin - your call, but the two halves should agree on the remedy.

Optional: access_denied is in the taxonomy but not in your map, so it still gets the bare 'Login failed' page.

[platypii]

Took the derive-from-origin option. managedContactUrl(identityBase) in builtin_remotes.js returns the contact URL only when the identity base's origin matches a shipped BUILTIN_REMOTES entry, and oidc_login threads it into the receiver. On a Hyperparam-run target the page links https://hyperparam.app/contact; anywhere else it says "ask your admin for access", which is the terminal's remedy.

Matched against the shipped built-ins rather than effectiveRemotes on purpose: a user who repoints the hyperparam name at their own server is self-hosting and should not inherit the vendor link with the name.

The lookup is now a switch in refusalPage(), so the prototype-key hazard your 99e26d7 fixed cannot come back through a table index; your tests still cover it. contactUrl is a parameter now, so it is checked against a plain-https pattern before it reaches the unescaped interpolation, with a test that a javascript: or markup-bearing value falls back to the admin wording.

Also took the optional one: access_denied gets its own page.

Thanks for the Object.hasOwn catch and the added tests.

[platypii]

Findings 4 and 5 are addressed at e3de451; CI is green. Review 4849490929 was submitted against 99e26d7 and predates the fix — requesting dismissal so this can merge.

[philcunliffe]

Review round 3: clean

e3de451 resolves the blocking ask. Every property the earlier rounds established was re-verified against a live listener rather than by reading, and each new test was revert-tested. One test gap was found and pushed as d13ee90 (test-only). No findings against the implementation.

Does the ask land? Yes

You took the derive-from-origin option. Rendered pages, probed live:

code	Hyperparam-run target	self-hosted target	explainLoginError
no_membership	"contact us at hyperparam.app/contact"	"ask your admin for access"	"ask an admin to invite you"
org_not_permitted	"Check --org, or contact us"	"Check --org, or ask your admin"	"check the --org name"
org_selection_required	"re-run with --org <name>"	same	"re-run with --org <name>"
access_denied	"The identity provider did not complete the sign-in"	same	"login was denied at the provider"

On a self-hosted server the browser now gives exactly the terminal's remedy, with no vendor URL anywhere in the body (asserted with doesNotMatch(/hyperparam\.app/)). That was the actual harm and it is gone. Matching against shipped BUILTIN_REMOTES rather than effectiveRemotes is the right call, and the docblock explains why: repointing the hyperparam name at your own box is self-hosting, and the name should not carry the vendor link with it. access_denied is closed too.

One residual, non-blocking: on a Hyperparam-run target the halves still differ (browser "contact us", terminal "ask an admin to invite you"). Defensible, since nobody has an org to be invited into there and the terminal is the stale half. If you want them symmetric, explainLoginError's no_membership string is where the same managedContactUrl treatment would go. Your call.

The origin check: exact, not spoofable, not over-broad

managedContactUrl (builtin_remotes.js:46) compares new URL(identityBase).origin === new URL(target.url).origin. Exact compare of a WHATWG origin, so scheme, host and port are all in scope and normalized by the parser. Not a suffix or substring match.

Provenance is static: identityBase is deriveIdentityBase(entry.url) (credentials.js:73), where entry.url comes from readConfiguredRemotes (config plus shipped built-ins), computed in runBrowserLogin before the browser opens. No redirect, Location header or response body can influence it. The only startLoopbackReceiver call site is oidc_login.js:55.

28 probes. Matching (all genuinely the built-in origin): the plain origin, an uppercased host, an explicit :443, and a userinfo-carrying URL. Failing closed to the admin wording:

https://evil-hypaware.hyperparam.app/... -> undefined (kills endsWith)
https://hypaware.hyperparam.app.evil.com/... -> undefined
https://hypaware.hyperparam.app@evil.com/... -> undefined (origin is evil.com)
https://hypaware.hyperparam.app。evil.com/ -> undefined (U+3002 becomes a real dot)
https://hypaware.hyperparaѕ.app/ -> undefined (Cyrillic homoglyph, punycoded)
https://hypaware.hyperparam.app./... -> undefined (trailing dot)
http://hypaware.hyperparam.app/... -> undefined (scheme is in the origin)
https://hypaware.hyperparam.app:8443/... -> undefined
https://hyperparam.app/v1/identity -> undefined (apex is not the built-in)
'', undefined, 'not a url', '//host/...' -> undefined

IDN, case, default-port, userinfo and punycode are all handled by URL before the compare, which is why the homoglyph and full-width-dot probes fail closed. The blast radius of even a false match is bounded: the returned value is the module constant MANAGED_CONTACT_URL, never any part of the input, so nothing host-derived can reach the page regardless.

Re-verification after the loopback.js rework

No unescaped injection: holds.respond() still has four in-file call sites. title/detail come from refusalPage(), every returned string a local literal except the interpolated link, which is gated by SAFE_CONTACT_URL (loopback.js:80). That regex is fully anchored with a [\w.-] / [\w./-] charset: no ", ', <, >, &, %, : or space survives it, and no $ either, which incidentally also seals the String.prototype.replace$& hazard raised in round 1. No nested quantifier, so no ReDoS. Probed the fallback with javascript:, data:, https://x.example/"><script>, "onmouseover=", '><img src=x onerror=>, http:// and $&$` : every one falls through to "ask your admin for access", zero <script> in the body.

Nothing new reaches the page. A callback carrying code, access_token and error_description=DESC<script> puts none of them in the body. No hostname appears either, neither the target's nor 127.0.0.1. That would have been a blocker.

The Object.hasOwn guard is superseded by something stronger.refusalPage() uses a switch with strict string equality, so there is no property lookup to walk a prototype chain at all. Re-ran the round-1 probe plus four more (constructor, toString, __proto__, valueOf, hasOwnProperty, isPrototypeOf, propertyIsEnumerable, toLocaleString, __defineGetter__): all render the generic page. The hazard cannot return through a table index because there is no table.

State validation unchanged and still first.returnedState !== state (:229) precedes params.has('error') (:233). A refusal with a wrong state renders "Unexpected login callback" and correctly does not settle the flow, so the login kept waiting and ended only at the timeout.

Leakage and shutdown clean.fail() logs only the bounded safeError; the code is never logged; sanitizeErrorCode untouched. The refusal branch still reaches clearTimeout plus server.close(), and no path leaves the CLI on a live listener.

oidc_login.js

Threads the target through and nothing more: one import, plus contactUrl: managedContactUrl(identityBase) on the existing startReceiver call. PKCE pair, state generation, start-URL construction, code exchange and finally { receiver.close() } are byte-identical.

Revert tests

test	mutation	result
self-hosted target sends the reader to their own admin	link hardcoded to the vendor URL	fails
admission refusal links contact on a managed target	same	passes (correctly, the positive case)
a contact URL that is not a plain https link is dropped	drop the SAFE_CONTACT_URL check	fails
a provider denial says so	delete the access_denied case	fails
receiver gets a contact URL only for a Hyperparam-run target	managedContactUrl returns unconditionally	fails

One gap, fixed and pushed (d13ee90, test-only)

minor - nothing pinned the origin compare to being exact.test/core/remote-oidc-login.test.js:68 tested only hypaware.hyperparam.app (match) and hyp.internal (no match). Both are far from the built-in, so loosening the compare kept the suite green: rewriting the compare as endsWith, as includes, and as a hostname-only match each left all 20 tests passing, while endsWith handed https://evil-hypaware.hyperparam.app the vendor link.

Added four near-miss identity bases (lookalike label, two suffix domains, plain http). All three loosenings now fail. Your implementation was already correct; this only stops it drifting.

Conventions

No em dashes, no semicolons outside the CSS string, no @typedef, no inline import('...') types, @import specifiers root-anchored. The @ref LLP 0058#d7 moved into refusalPage's JSDoc with no blank line breaking attachment, and the <a id="d7"> anchor is at llp/0058-oidc-login-client.decision.md:165.

Verification

• npm test: 3293 tests, 3292 pass, 0 fail, 1 skipped
• npm run typecheck: clean
• Pushed: d13ee90 (test-only)

You nailed this one. The parameterization is the right shape, the fail-closed default is the right default, the switch is a better answer to the prototype hazard than the guard it replaces, and gating the interpolated URL behind a charset check rather than trusting the caller is exactly the instinct that keeps respond()'s unescaped contract safe as it grows a parameter.

[philcunliffe]

Approving. The blocking ask is resolved: on a self-hosted target the browser now gives the same remedy the terminal does, with no vendor URL in the body, and access_denied is covered too.

I probed the origin check rather than reading it. new URL(a).origin === new URL(b).origin is exact, and 28 probes confirm it fails closed on every near-miss I could construct: lookalike labels, suffix domains, userinfo tricks, a full-width dot, a Cyrillic homoglyph, a trailing dot, a scheme downgrade, and a non-default port. Even a false match could only return the module constant, never any part of the input.

Also re-verified after the rework that nothing request-, server-, or origin-derived reaches the unescaped HTML (no hostname in the page), that state validation still precedes the refusal branch, and that the prototype-key hazard is structurally gone now that the lookup is a switch rather than a table index.

One test-only commit pushed (d13ee90): nothing in the suite held the compare to being exact, so endsWith/includes/hostname-only loosenings all stayed green. Four near-miss identity bases now fail them.

Merging is the maintainer's call; neutral holds here.