OpenConceptLab/ocl_issues#2856 | Sign-in from openconceptlab.org links works first time; menu no longer blanks the app

[paynejd]

closes OpenConceptLab/ocl_issues#2856

Two independent bugs, one commit each, then three small commits with fixes from the Codex review.

Sign-in fails with "Incorrect redirect_uri"

Links from openconceptlab.org land on #/?referrer=…. Sign in used the whole page URL, query string included, as the OIDC redirect_uri, but the callback rebuilt it for the code exchange from the path alone, so Keycloak refused the exchange. Retrying from the callback page then carried that page's stale state and code into the next sign-in (the redirect_uri isn't URL-encoded, so its query spills into the outer one). That's why it took a few clicks, and why the eventual success landed on the empty /oidc/login route.

Sign-in now always uses LOGIN_REDIRECT_URL, and the page to return to (its hash route, query included) waits in this tab's sessionStorage, next to the PKCE verifier, until the callback has exchanged the code. The community site already works this way. That page wins over next, which the callback still uses to rebuild the redirect_uri for sign-ins that started before this deploy. Sign-up and password reset clear a page left by an abandoned sign-in, and /oidc/login, /signin and /signup are never return pages, in any letter case or percent-encoding, since the router ignores both (from Codex passes 1–3).

Opening the menu blanks the app

UserIcon spread its sx into the <img> style. The left menu passes sx as an array for followed items, so a followed user with a profile picture got style={{0: {…}}}, React threw on style[0], and the app unmounted. Only people who follow a user with a photo hit it, which is why most accounts can't reproduce it. Only a plain-object sx is used as the image style now, and the MUI icons get sx unchanged.

Verified

• Before, on production (TB-3.0.6-alpha-65159668, test account): signing in from #/?referrer=https://openconceptlab.org/ failed with "Incorrect redirect_uri", then "Sign in to continue.", and only the third attempt succeeded. Opening the menu while following a user with a photo threw Failed to set an indexed property [0] on 'CSSStyleDeclaration' and emptied #root. With the same data minus the photo, it didn't.
• After, on a local build of this branch against the production API and Keycloak, with a real sign-in as a test account from five start pages: the referrer landing, a search with a query, a repo page, the dashboard, and the callback page. Each got a token and landed on the page it started from (the callback page lands on the dashboard), with the same redirect_uri at both steps and nothing left in sessionStorage.
• The menu, with a followed user who has a photo, without one, and with a real account's orgs and follows: no errors, and the followed user's photo renders as before.
• After the review fixes: the five sign-ins pass again, and an abandoned sign-in's saved page is cleared when Sign up is clicked (it survived before).
• The return-page guard, run from the file's own code: 22 cases (ordinary, query and encoded routes are kept; the callback, sign-in and sign-up routes in any case or encoding, and malformed encodings, are not).
• eslint --ext .jsx,.js src/ is clean.

Not in this PR

The Mapper shares this sign-in code, and its Sign in and start mapping fails the same way for visitors from openconceptlab.org: OpenConceptLab/ocl_issues#2857, fixed the same way in OpenConceptLab/oclmap#87.

🤖 Generated with Claude Code

[paynejd]

Codex adversarial review, pass 1 (codex-cli 0.159.2, commit 8035c1e)

Found three P2 issues. I’d fix the first two before merging. No critical security issue found. Reviewed both commits with git diff origin/main...HEAD; no files edited.

1. P2 — Non-root callback URLs override the saved destination.
   OIDLoginCallback.jsx:43 and OIDLoginCallback.jsx:92
   With LOGIN_REDIRECT_URL=https://host/oidc/login/, index.html supplies next=/oidc/login/. The exchange succeeds, but next takes precedence over the stored /search/?q=abc, leaving the user on the blank callback route. Other non-root callback paths likewise become the destination. The comment claiming next identifies old sign-ins is incorrect: the rewrite supplies it for new sign-ins too.
   Fix: Track the exact redirect URI and destination as separate transaction fields. Prefer the stored destination for new transactions; retain next as the legacy fallback.

2. P2 — Abandoned login destinations leak into registration/password reset.
   utils.js:919 and OIDLoginCallback.jsx:36
   Start login from /orgs/Abandoned/, return using Back, then start registration or password reset in the same tab. Those helpers replace authentication material without clearing oauth_return_to. A successful callback consumes the abandoned login’s destination and navigates there. Confirmed by executing the actual helpers.
   Fix: Initialize or clear the destination for every authentication flow, ideally storing it together with that flow’s state, verifier and redirect URI.

3. P2 — Return-route validation still permits authentication entry routes.
   utils.js:901
   /#/signin?returnTo=https%3A%2F%2Fhost%2F%23%2Fsignup stores /signup. After successful login, mounting that route immediately launches registration. Returning to /signin launches another login. This is a remaining validation gap, rather than a newly introduced loop; an infinite loop is not established.
   Fix: Reject /signin, /signup, and /oidc/login, including their routed suffixes and query variants, and fall back to /.

Other requested checks:

• Root callback: UserMenu, Error401 and forced logout preserve hash-route queries. GuestDashboard defaults to /. The callback toast reads the current destination when clicked.
• Automatic login: App.jsx:138 still omits search, so forceLoginUser loses query parameters. This predates the PR.
• Deploy compatibility: Legacy query-free next=/path exchanges retain their previous behavior. Already-in-flight redirects containing page queries still cannot be reconstructed exactly and remain subject to Incorrect redirect_uri.
• Other tabs: Missing verifier/state still takes the existing “sign in to continue” path. Storage is tab-scoped; rejected callbacks and failed exchanges consume the saved destination, so they cannot recover it on retry.
• Security: No external redirect or script injection found through the stored route: navigation assigns window.location.hash. Even accepted //evil.test/ stays in the fragment.
• Rewrite rules: Root query-code and #state= callbacks remain compatible. The non-root next ambiguity is finding 1.
• UserIcon: No blocking issue found. Plain-object styles remain intact; arrays/functions now reach MUI icons unchanged and are omitted from image styles. Existing LeftMenu arrays contain only icon color, so image dimensions remain unchanged.

Validation used read-only execution of extracted helpers/components. Live Keycloak exchanges and browser visual rendering were not tested.

[paynejd]

Codex adversarial review, pass 2 (codex-cli 0.159.2, commit c6924a6)

One P2 remains; I would fix it before merging. No P0/P1 findings.

• P2 — Auth-route guard disagrees with router matching: src/common/utils.js:901. The regex is case-sensitive, but React Router matches /signin and /signup case-insensitively. A sign-in with returnTo=https://…/#/SIGNUP stores /SIGNUP; after successful login, the callback navigates there and starts registration. /SIGNIN similarly starts another sign-in. /signin#section also bypasses the guard because # is missing from its delimiters. Suggested fix: extract the route pathname before ? or #, then reject auth paths case-insensitively, including their subpaths. Verified these bypasses against the installed router.

The other two P2 fixes are resolved: the stored page wins for navigation while next still controls the exchange redirect_uri; register/reset clear stale destinations without storing a replacement. The third is resolved only for lowercase routes without a secondary fragment.

Reviewed every login caller, storage lifecycle, other-tab handling, rewrite rules, and direct/wrapped UserIcon callers. No additional introduced issue found. Stored routes only change the hash, with no external redirect or script execution. Old in-flight path-only sign-ins remain compatible; old query-bearing sign-ins retain their pre-existing exchange failure.

Read-only checks passed for callback precedence, stale clearing, and UserIcon rendering across image/icon and tooltip variants. git diff --check passed. No files edited.

[paynejd]

Codex adversarial review, pass 3 (codex-cli 0.159.2, commit c707d33)

One blocking finding remains:

• P2 — Encoded auth paths bypass the guard, src/common/utils.js:901. A sign-in started with returnTo=https://app.example/#/%73ignin saves /%73ignin. After authentication, the callback restores it; router history applies decodeURI, matches /signin, and starts another sign-in. Likewise, /%53IGNUP starts registration. Suggested fix: apply decodeURI to the extracted path before checking the regex, reject malformed encoding, and preserve the original route for restoration.

The newest commit resolves the reported case-sensitive and /signin#section bypasses. Twelve in-memory route checks passed, including queries, nested routes, and referrer values containing literal or encoded #.

Re-reviewed all four commits and git show HEAD; no other blocking findings. No files edited.

[paynejd]

Codex adversarial review, pass 4 (codex-cli 0.159.2, commit 9a1f83f)

Nothing blocking remains. No nonblocking findings identified.

Reviewed all five commits with git diff origin/main...HEAD and the newest fix with git show HEAD.

Verified that the guard rejects /%73ignin, /%53IGNUP, and malformed path encodings while preserving the original return route—including /orgs/My%20Org/, queries, and referrers containing encoded or literal #.

Executable checks of the guard, callback, and avatar rendering passed. ESLint for all three changed files and git diff --check passed.

No files edited. Verification used mocked token exchange, not a live Keycloak login.