fix(users): thread next through OIDC login + open-redirect hardening (bookshelf-bxtat) #1392

Merged

zombor merged 2 commits from bd-bookshelf-bxtat into main 2026-08-08 15:44:34 +00:00

Conversation 5 Commits 2 Files changed 9 +304 -22

zombor commented 2026-08-08 01:58:57 +00:00

Owner

Copy link

Summary

SSO login previously ignored the ?next= return-to destination — clicking a
deep link while logged out, then signing in via SSO, always landed on /
instead of the originally-requested page. Local login already worked
correctly.

Root cause

• loginPageHandler (OIDC-only auto-redirect) and loginHandler
  (credential-POST guard) redirected to /auth/oidc/login without
  forwarding next.
• The "Sign in with SSO" button in login.html linked to /auth/oidc/login
  with no next.
• oidcLoginHandler never captured next — only state/nonce/PKCE cookies.
• oidcCallbackHandler hardcoded the post-login redirect to /.

Fix

• Extracted a shared safeNextPath() helper, used by BOTH local and OIDC
  paths. This also closes a latent open-redirect gap in local login: the old
  inline check missed the /\evil.com backslash variant that some browsers
  normalize to // before following a redirect.
• next is now forwarded through all 3 handoff points into
  /auth/oidc/login?next=<validated>.
• oidcLoginHandler stores the validated next in a new short-lived
  bookshelf_oidc_next cookie (same Path=/auth/oidc, HttpOnly, Secure,
  SameSite=Lax, TTL as the existing state/nonce/verifier cookies).
• oidcCallbackHandler re-validates the cookie value (defense in depth) and
  redirects there instead of /, then clears the cookie. Falls back to /
  when the cookie is absent, empty, or hostile.

Test plan

• New safeNextPath table-driven unit tests (same-origin path preserved;
  //evil.com, /\evil.com, https://evil.com, empty → /).
• oidcLoginHandler/oidcCallbackHandler unit tests: cookie is set from a
  validated next query param, hostile values fall back to /, callback
  redirects to the stored (re-validated) next and clears the cookie.
• Existing local-login next handler tests remain green.
• Extended e2e/api/journey_9_oidc_login_test.go with a full round-trip
  step: GET /auth/oidc/login?next=/books/123 → fake IdP → callback lands on
  /books/123.
• docs/content/docs/accounts-access/signing-in.md updated — SSO section now
  correctly states you're returned to the page you were trying to view.

Closes bead bookshelf-bxtat on merge.

## Summary SSO login previously ignored the `?next=` return-to destination — clicking a deep link while logged out, then signing in via SSO, always landed on `/` instead of the originally-requested page. Local login already worked correctly. ## Root cause - `loginPageHandler` (OIDC-only auto-redirect) and `loginHandler` (credential-POST guard) redirected to `/auth/oidc/login` without forwarding `next`. - The "Sign in with SSO" button in `login.html` linked to `/auth/oidc/login` with no `next`. - `oidcLoginHandler` never captured `next` — only state/nonce/PKCE cookies. - `oidcCallbackHandler` hardcoded the post-login redirect to `/`. ## Fix - Extracted a shared `safeNextPath()` helper, used by BOTH local and OIDC paths. This also closes a latent open-redirect gap in local login: the old inline check missed the `/\evil.com` backslash variant that some browsers normalize to `//` before following a redirect. - `next` is now forwarded through all 3 handoff points into `/auth/oidc/login?next=<validated>`. - `oidcLoginHandler` stores the validated `next` in a new short-lived `bookshelf_oidc_next` cookie (same `Path=/auth/oidc`, `HttpOnly`, `Secure`, `SameSite=Lax`, TTL as the existing state/nonce/verifier cookies). - `oidcCallbackHandler` re-validates the cookie value (defense in depth) and redirects there instead of `/`, then clears the cookie. Falls back to `/` when the cookie is absent, empty, or hostile. ## Test plan - New `safeNextPath` table-driven unit tests (same-origin path preserved; `//evil.com`, `/\evil.com`, `https://evil.com`, empty → `/`). - `oidcLoginHandler`/`oidcCallbackHandler` unit tests: cookie is set from a validated `next` query param, hostile values fall back to `/`, callback redirects to the stored (re-validated) next and clears the cookie. - Existing local-login `next` handler tests remain green. - Extended `e2e/api/journey_9_oidc_login_test.go` with a full round-trip step: `GET /auth/oidc/login?next=/books/123` → fake IdP → callback lands on `/books/123`. - `docs/content/docs/accounts-access/signing-in.md` updated — SSO section now correctly states you're returned to the page you were trying to view. Closes bead bookshelf-bxtat on merge.

zombor added 1 commit 2026-08-08 01:58:58 +00:00

fix(users): thread next through OIDC login + open-redirect hardening ... 5540c2c96a

SSO login previously dropped the ?next= return-to destination, always
landing users on / after OIDC login even when they clicked a deep link
while logged out. Local login already preserved next; the OIDC path
never captured it.

- Extract a shared safeNextPath() helper used by both local and OIDC
  login paths. Closes a latent open-redirect gap in local login: the
  old inline check missed the "/\\evil.com" backslash variant that
  some browsers normalize to "//" before following a redirect.
- Forward next through all three OIDC handoff points (OIDC-only
  auto-redirect, credential-POST guard, and the login.html SSO
  button) as a validated query param.
- oidcLoginHandler stores the validated next in a short-lived
  bookshelf_oidc_next cookie (same attrs as the existing state/nonce/
  verifier cookies).
- oidcCallbackHandler re-validates the cookie value (defense in
  depth) and redirects there instead of hardcoding '/', clearing the
  cookie afterward.

Bead: bookshelf-bxtat

zombor commented 2026-08-08 02:11:28 +00:00

Author

Owner

Copy link

Code Review — PR #1392 (bookshelf-bxtat)

Reviewed the diff (internal/users/handler.go, oidc_handler.go, template, tests, e2e journey, docs) for correctness/security/convention per .claude/rules/review-standard.md. No re-run of tests — CI is green.

Findings

[MINOR] internal/users/handler.go:86-98 (safeNextPath) — no length cap on next
safeNextPath accepts any string starting with a single / (not // or /\\) with no upper bound on length. oidcLoginHandler (oidc_handler.go:135-138) stores the raw value verbatim in the bookshelf_oidc_next cookie. An attacker (or a legitimately very long deep-link path) can push a several-KB value into a Set-Cookie header; browsers will just silently drop/truncate an oversized cookie (no crash), so impact is cosmetic (deep-link falls back to / on truncation) rather than a security hole. Consider capping accepted length (e.g. 512 bytes) in safeNextPath for defense-in-depth and a cleaner failure mode.

[MINOR] internal/users/handler.go:86-98 (safeNextPath) — control characters / CRLF not explicitly rejected
safeNextPath doesn't strip control chars (e.g. a decoded \r/\n from %0d%0a) before accepting the value as a redirect target / cookie value. In this codebase's Go stdlib this is currently neutralized (net/http's cookie writer sanitizes invalid bytes out of cookie values, and the header writer replaces \r/\n in header values before writing Location), so there's no live CRLF-injection path today — but the safety is implicit/incidental rather than asserted by this function. Consider an explicit reject of control characters in safeNextPath so the guarantee doesn't silently depend on net/http internals staying that way.

What was verified clean

• Open-redirect coverage: safeNextPath (handler.go:79-98) correctly rejects //evil.com, /\\evil.com (backslash-normalize bypass), https://evil.com, bare evil.com (no leading /), and empty string — all fall back to /. Traced every consumption point: loginPageHandler (handler.go:132), loginHandler (handler.go:220), oidcLoginHandler (oidc_handler.go:293), and — critically — oidcCallbackHandler re-validates the cookie-sourced value again (oidc_handler.go:318, next := safeNextPath(cookieNext)) before redirecting, so a tampered/pre-existing hostile oidc_next cookie can't cause an off-site redirect even if it somehow bypassed the write-time check. Both local-login inline checks (previously at handler.go:108/:208 pre-diff) are now fully replaced by the shared helper — no lingering divergent inline check remains.
• Cookie attributes: oidc_next cookie is built from the same cookieAttrs base as state/nonce/verifier (oidc_handler.go:118-124, applied at :302-305) — Path=/auth/oidc, HttpOnly, Secure=d.SecureCookies, SameSite=Lax, MaxAge=oidcStateCookieTTL (10 min). It is explicitly cleared in clearOIDCCookies (oidc_handler.go:249-251) with matching Path=/auth/oidc and MaxAge=-1, and oidcCallbackHandler calls clearOIDCCookies unconditionally right after reading it (oidc_handler.go:319) — confirmed by the new It("clears the oidc_next cookie on success"...) test.
• Template escaping: templates/pages/login.html:476 — <a href="/auth/oidc/login?next={{.Next}}"> — .Next is server-validated (already passed through safeNextPath before being placed on the page-data struct) AND html/template's contextual autoescaper applies proper URL-query escaping in this position regardless, so this is not raw interpolation / not an injection vector.
• Fallback behavior: every consumption point defaults cleanly to / when next is absent/empty/invalid — verified via next_path_test.go table entries and the OIDC-only redirect tests (oidcLoginURL("") → "/auth/oidc/login" with no query string, exercised by the pre-existing "redirects to /auth/oidc/login (bypass closed)" case in handler_test.go).
• Conventions: curried DI unchanged, var-at-top style followed in loginPageHandler/loginHandler, all new/changed test files are black-box (package users_test), safeNextPath is exposed to tests only via the existing export_test.go shim pattern (consistent with ExportOIDCLoginHandler etc. already in that file) — not a white-box violation. New Its are one-assertion-per-behavior except the pre-existing findCookie-nil-guard-then-field-assert idiom, which already existed in this file before the diff (e.g. oidc_handler_test.go original nonce/pkce cookie tests) — not a new violation introduced here. No .golangci.yml or coverage-exclusion changes in the diff.
• Docs: docs/content/docs/accounts-access/signing-in.md update is end-user framed ("you are returned to the page you were trying to view"), no source-code/internal references.

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## Code Review — PR #1392 (bookshelf-bxtat) Reviewed the diff (`internal/users/handler.go`, `oidc_handler.go`, template, tests, e2e journey, docs) for correctness/security/convention per `.claude/rules/review-standard.md`. No re-run of tests — CI is green. ### Findings [MINOR] internal/users/handler.go:86-98 (safeNextPath) — no length cap on `next` `safeNextPath` accepts any string starting with a single `/` (not `//` or `/\\`) with no upper bound on length. `oidcLoginHandler` (oidc_handler.go:135-138) stores the raw value verbatim in the `bookshelf_oidc_next` cookie. An attacker (or a legitimately very long deep-link path) can push a several-KB value into a Set-Cookie header; browsers will just silently drop/truncate an oversized cookie (no crash), so impact is cosmetic (deep-link falls back to `/` on truncation) rather than a security hole. Consider capping accepted length (e.g. 512 bytes) in `safeNextPath` for defense-in-depth and a cleaner failure mode. [MINOR] internal/users/handler.go:86-98 (safeNextPath) — control characters / CRLF not explicitly rejected `safeNextPath` doesn't strip control chars (e.g. a decoded `\r`/`\n` from `%0d%0a`) before accepting the value as a redirect target / cookie value. In this codebase's Go stdlib this is currently neutralized (`net/http`'s cookie writer sanitizes invalid bytes out of cookie values, and the header writer replaces `\r`/`\n` in header values before writing `Location`), so there's no live CRLF-injection path today — but the safety is implicit/incidental rather than asserted by this function. Consider an explicit reject of control characters in `safeNextPath` so the guarantee doesn't silently depend on `net/http` internals staying that way. ### What was verified clean - **Open-redirect coverage**: `safeNextPath` (handler.go:79-98) correctly rejects `//evil.com`, `/\\evil.com` (backslash-normalize bypass), `https://evil.com`, bare `evil.com` (no leading `/`), and empty string — all fall back to `/`. Traced every consumption point: `loginPageHandler` (handler.go:132), `loginHandler` (handler.go:220), `oidcLoginHandler` (oidc_handler.go:293), and — critically — `oidcCallbackHandler` re-validates the cookie-sourced value again (oidc_handler.go:318, `next := safeNextPath(cookieNext)`) before redirecting, so a tampered/pre-existing hostile `oidc_next` cookie can't cause an off-site redirect even if it somehow bypassed the write-time check. Both local-login inline checks (previously at handler.go:108/:208 pre-diff) are now fully replaced by the shared helper — no lingering divergent inline check remains. - **Cookie attributes**: `oidc_next` cookie is built from the same `cookieAttrs` base as state/nonce/verifier (oidc_handler.go:118-124, applied at :302-305) — `Path=/auth/oidc`, `HttpOnly`, `Secure=d.SecureCookies`, `SameSite=Lax`, `MaxAge=oidcStateCookieTTL` (10 min). It is explicitly cleared in `clearOIDCCookies` (oidc_handler.go:249-251) with matching `Path=/auth/oidc` and `MaxAge=-1`, and `oidcCallbackHandler` calls `clearOIDCCookies` unconditionally right after reading it (oidc_handler.go:319) — confirmed by the new `It("clears the oidc_next cookie on success"...)` test. - **Template escaping**: `templates/pages/login.html:476` — `<a href="/auth/oidc/login?next={{.Next}}">` — `.Next` is server-validated (already passed through `safeNextPath` before being placed on the page-data struct) AND `html/template`'s contextual autoescaper applies proper URL-query escaping in this position regardless, so this is not raw interpolation / not an injection vector. - **Fallback behavior**: every consumption point defaults cleanly to `/` when `next` is absent/empty/invalid — verified via `next_path_test.go` table entries and the OIDC-only redirect tests (`oidcLoginURL("") → "/auth/oidc/login"` with no query string, exercised by the pre-existing "redirects to /auth/oidc/login (bypass closed)" case in handler_test.go). - **Conventions**: curried DI unchanged, var-at-top style followed in `loginPageHandler`/`loginHandler`, all new/changed test files are black-box (`package users_test`), `safeNextPath` is exposed to tests only via the existing `export_test.go` shim pattern (consistent with `ExportOIDCLoginHandler` etc. already in that file) — not a white-box violation. New `It`s are one-assertion-per-behavior except the pre-existing `findCookie`-nil-guard-then-field-assert idiom, which already existed in this file before the diff (e.g. oidc_handler_test.go original nonce/pkce cookie tests) — not a new violation introduced here. No `.golangci.yml` or coverage-exclusion changes in the diff. - **Docs**: `docs/content/docs/accounts-access/signing-in.md` update is end-user framed ("you are returned to the page you were trying to view"), no source-code/internal references. REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor commented 2026-08-08 02:14:46 +00:00

Author

Owner

Copy link

Security Review — PR #1392 (bookshelf-bxtat)

Adversarial review of next/open-redirect threading through local + OIDC login. Confirmed a real, PoC-verified bypass of safeNextPath — details below.

[BLOCKER] internal/users/handler.go:86-96 (safeNextPath) and internal/users/handler.go:272 — open-redirect via embedded ASCII TAB (control-char) bypasses the // / /\\ prefix checks
safeNextPath only rejects values that literally start with "//" or "/\\". It does not reject embedded ASCII control characters (TAB 0x09, and to a lesser extent CR/LF). Per the WHATWG URL spec, browsers strip all ASCII tab/newline bytes from a URL string wherever they occur (not just leading/trailing) before parsing it — so a value like "/\t/evil.com" passes safeNextPath unchanged (first char is /, second char is TAB, so neither the // nor /\\ prefix check fires), and the browser reconstitutes it as "//evil.com" — a protocol-relative URL — after stripping the tab.

This is directly exploitable on the local-login success path, which is the most dangerous of the three consumers because next goes straight from r.FormValue("next") through safeNextPath into http.Redirect(w, r, next, ...) with no intermediate cookie or url.QueryEscape round-trip to accidentally sanitize it:

POST /login  (valid credentials)
next=/%09/evil.com

• r.FormValue("next") URL-decodes %09 to a literal TAB byte → "/\t/evil.com".
• safeNextPath("/\t/evil.com") returns it unchanged (verified with a standalone repro against the actual function body).
• http.Redirect(w, r, next, http.StatusFound) at handler.go:272 writes Location: /\t/evil.com.
• Verified against net/http's actual header serialization (Header.Write): Go only replaces \n/\r with a space (headerNewlineToSpace, net/http/header.go) — it does not touch TAB, and textproto.TrimString only trims leading/trailing whitespace, not embedded whitespace. The raw wire bytes are literally Location: /\t/evil.com\r\n.
• The browser then applies the WHATWG URL parser's "remove all ASCII tab or newline from input" step (applies to the whole string, not just the ends), collapsing this to //evil.com, which it resolves as a protocol-relative URL and navigates to https://evil.com (or the current scheme) — a full off-site redirect immediately after a successful login, e.g. to a phishing/credential-harvesting page.

Repro (ran locally against the literal safeNextPath body from this diff):

input="/\t/evil.com" safeNextPath="/\t/evil.com"   // unchanged — validator does not catch it
raw header bytes: "Content-Type: text/html; charset=utf-8\r\nLocation: /\t/evil.com\r\n"  // tab survives verbatim on the wire

I also checked the two other consumers of next:

• oidcLoginURL() (oidc_handler.go) applies url.QueryEscape(next) before embedding it in the same-origin /auth/oidc/login?next=... redirect — this percent-encodes the tab (%09), so that hop isn't directly exploitable at that redirect, but the escaped value gets URL-decoded straight back to a raw tab when oidcLoginHandler re-parses r.URL.Query().Get("next"), then stored via safeNextPath into the oidc_next cookie. It only survives to be caught downstream because Go's http.SetCookie/sanitizeCookieValue strips control byte 0x09 from cookie values at set-time, which happens to collapse the string to the literal "//evil.com" before storage — a form the // prefix check then catches when oidcCallbackHandler re-validates via safeNextPath(cookieNext). That's incidental, implementation-detail luck (a change to Go's cookie sanitization, or any code path that stores/forwards next without going through http.SetCookie, reopens this), not a designed defense — the shared validator itself is unsound and must be fixed at the source, not rely on this side-effect.

Fix: reject (fall back to /) if next contains any ASCII control character (< 0x20 or 0x7F), not just the ////\\ literal prefixes, e.g.:

func safeNextPath(next string) string {
    if next == "" || !strings.HasPrefix(next, "/") {
        return "/"
    }
    for i := 0; i < len(next); i++ {
        if next[i] < 0x20 || next[i] == 0x7f {
            return "/"
        }
    }
    if strings.HasPrefix(next, "//") || strings.HasPrefix(next, "/\\") {
        return "/"
    }
    return next
}

Add a regression test: Entry("a tab-embedded protocol-relative bypass is rejected", "/\t/evil.com", "/") (and \n/\r siblings) to internal/users/next_path_test.go.

[MINOR] internal/users/next_path_test.go:241-254 — test table doesn't cover the control-character bypass class
The existing DescribeTable covers //, /\\, absolute URLs, empty, and no-leading-slash, but not embedded TAB/CR/LF. Add cases for "/\t/evil.com", "/\n/evil.com", "/\r/evil.com" once the BLOCKER fix lands, so this bypass can't silently regress.

[MINOR] internal/users/oidc_handler.go:279-284 (oidcLoginURL) — relies on next already having been validated upstream, with only a comment as the contract
oidcLoginURL documents "next is expected to already be validated by safeNextPath" but takes a raw string with no type-level enforcement. Low risk today (both call sites do validate first), but a future caller could pass an unvalidated next and nothing would catch it. Not blocking — flagging for awareness; a comment-only contract is acceptable at this size.

Everything else checked out:

• OIDC CSRF/replay protections (state/nonce/PKCE code_verifier) are untouched by this change — next is threaded independently and validated separately from cookieState/cookieNonce/cookieVerifier, which still flow unchanged into d.HandleCallback.
• oidc_next cookie flags are correct: HttpOnly, Secure (from d.SecureCookies), SameSite=Lax, Path=/auth/oidc, 10-minute TTL, and it's explicitly cleared in clearOIDCCookies on every callback outcome (success and every error branch that redirects to /login).
• login.html's href="/auth/oidc/login?next={{.Next}}" is in a html/template URL-query-value context, which auto-escapes/percent-encodes .Next — no template-injection/attribute-breakout path found.
• No secrets/PII/next redirect target logged; audit.Record calls in this diff are unrelated to next and already go through audit.SanitizeDescription.
• Absolute URLs with an embedded @ (e.g. /https://user@evil), and non-/-prefixed schemes (javascript:, http://, https:/evil) are all correctly rejected or rendered inert as same-origin relative paths by the existing prefix checks — could not defeat those.

REVIEW VERDICT: 1 blocker, 0 major, 2 minor

## Security Review — PR #1392 (bookshelf-bxtat) Adversarial review of `next`/open-redirect threading through local + OIDC login. Confirmed a real, PoC-verified bypass of `safeNextPath` — details below. [BLOCKER] internal/users/handler.go:86-96 (`safeNextPath`) and internal/users/handler.go:272 — open-redirect via embedded ASCII TAB (control-char) bypasses the `//` / `/\\` prefix checks `safeNextPath` only rejects values that literally start with `"//"` or `"/\\"`. It does not reject embedded ASCII control characters (TAB `0x09`, and to a lesser extent CR/LF). Per the WHATWG URL spec, browsers strip *all* ASCII tab/newline bytes from a URL string wherever they occur (not just leading/trailing) before parsing it — so a value like `"/\t/evil.com"` passes `safeNextPath` unchanged (first char is `/`, second char is TAB, so neither the `//` nor `/\\` prefix check fires), and the browser reconstitutes it as `"//evil.com"` — a protocol-relative URL — after stripping the tab. This is directly exploitable on the **local-login success path**, which is the most dangerous of the three consumers because `next` goes straight from `r.FormValue("next")` through `safeNextPath` into `http.Redirect(w, r, next, ...)` with **no intermediate cookie or `url.QueryEscape` round-trip** to accidentally sanitize it: ``` POST /login (valid credentials) next=/%09/evil.com ``` - `r.FormValue("next")` URL-decodes `%09` to a literal TAB byte → `"/\t/evil.com"`. - `safeNextPath("/\t/evil.com")` returns it **unchanged** (verified with a standalone repro against the actual function body). - `http.Redirect(w, r, next, http.StatusFound)` at handler.go:272 writes `Location: /\t/evil.com`. - Verified against `net/http`'s actual header serialization (`Header.Write`): Go only replaces `\n`/`\r` with a space (`headerNewlineToSpace`, net/http/header.go) — it does **not** touch TAB, and `textproto.TrimString` only trims leading/trailing whitespace, not embedded whitespace. The raw wire bytes are literally `Location: /\t/evil.com\r\n`. - The browser then applies the WHATWG URL parser's "remove all ASCII tab or newline from input" step (applies to the *whole* string, not just the ends), collapsing this to `//evil.com`, which it resolves as a protocol-relative URL and navigates to `https://evil.com` (or the current scheme) — a full off-site redirect immediately after a successful login, e.g. to a phishing/credential-harvesting page. Repro (ran locally against the literal `safeNextPath` body from this diff): ``` input="/\t/evil.com" safeNextPath="/\t/evil.com" // unchanged — validator does not catch it raw header bytes: "Content-Type: text/html; charset=utf-8\r\nLocation: /\t/evil.com\r\n" // tab survives verbatim on the wire ``` I also checked the two other consumers of `next`: - `oidcLoginURL()` (oidc_handler.go) applies `url.QueryEscape(next)` before embedding it in the same-origin `/auth/oidc/login?next=...` redirect — this percent-encodes the tab (`%09`), so that hop isn't directly exploitable *at that redirect*, but the escaped value gets URL-decoded straight back to a raw tab when `oidcLoginHandler` re-parses `r.URL.Query().Get("next")`, then stored via `safeNextPath` into the `oidc_next` cookie. It only survives to be caught downstream because Go's `http.SetCookie`/`sanitizeCookieValue` strips control byte 0x09 from cookie *values* at set-time, which happens to collapse the string to the literal `"//evil.com"` before storage — a form the `//` prefix check then catches when `oidcCallbackHandler` re-validates via `safeNextPath(cookieNext)`. That's incidental, implementation-detail luck (a change to Go's cookie sanitization, or any code path that stores/forwards `next` without going through `http.SetCookie`, reopens this), not a designed defense — the shared validator itself is unsound and must be fixed at the source, not rely on this side-effect. Fix: reject (fall back to `/`) if `next` contains any ASCII control character (`< 0x20` or `0x7F`), not just the `//`/`/\\` literal prefixes, e.g.: ```go func safeNextPath(next string) string { if next == "" || !strings.HasPrefix(next, "/") { return "/" } for i := 0; i < len(next); i++ { if next[i] < 0x20 || next[i] == 0x7f { return "/" } } if strings.HasPrefix(next, "//") || strings.HasPrefix(next, "/\\") { return "/" } return next } ``` Add a regression test: `Entry("a tab-embedded protocol-relative bypass is rejected", "/\t/evil.com", "/")` (and `\n`/`\r` siblings) to `internal/users/next_path_test.go`. [MINOR] internal/users/next_path_test.go:241-254 — test table doesn't cover the control-character bypass class The existing `DescribeTable` covers `//`, `/\\`, absolute URLs, empty, and no-leading-slash, but not embedded TAB/CR/LF. Add cases for `"/\t/evil.com"`, `"/\n/evil.com"`, `"/\r/evil.com"` once the BLOCKER fix lands, so this bypass can't silently regress. [MINOR] internal/users/oidc_handler.go:279-284 (`oidcLoginURL`) — relies on `next` already having been validated upstream, with only a comment as the contract `oidcLoginURL` documents "next is expected to already be validated by safeNextPath" but takes a raw `string` with no type-level enforcement. Low risk today (both call sites do validate first), but a future caller could pass an unvalidated `next` and nothing would catch it. Not blocking — flagging for awareness; a comment-only contract is acceptable at this size. Everything else checked out: - OIDC CSRF/replay protections (`state`/`nonce`/PKCE `code_verifier`) are untouched by this change — `next` is threaded independently and validated separately from `cookieState`/`cookieNonce`/`cookieVerifier`, which still flow unchanged into `d.HandleCallback`. - `oidc_next` cookie flags are correct: `HttpOnly`, `Secure` (from `d.SecureCookies`), `SameSite=Lax`, `Path=/auth/oidc`, 10-minute TTL, and it's explicitly cleared in `clearOIDCCookies` on every callback outcome (success and every error branch that redirects to `/login`). - `login.html`'s `href="/auth/oidc/login?next={{.Next}}"` is in a `html/template` URL-query-value context, which auto-escapes/percent-encodes `.Next` — no template-injection/attribute-breakout path found. - No secrets/PII/`next` redirect target logged; `audit.Record` calls in this diff are unrelated to `next` and already go through `audit.SanitizeDescription`. - Absolute URLs with an embedded `@` (e.g. `/https://user@evil`), and non-`/`-prefixed schemes (`javascript:`, `http://`, `https:/evil`) are all correctly rejected or rendered inert as same-origin relative paths by the existing prefix checks — could not defeat those. REVIEW VERDICT: 1 blocker, 0 major, 2 minor

zombor referenced this pull request from a commit 2026-08-08 02:21:11 +00:00

fix(users): close TAB-embedded open-redirect BLOCKER in safeNextPath

zombor added 1 commit 2026-08-08 02:21:12 +00:00

fix(users): close TAB-embedded open-redirect BLOCKER in safeNextPath ... 45d80ad976

safeNextPath only rejected values starting with "//" or "/\\". It did
not reject embedded C0 control chars (0x00-0x1F, 0x7F). Browsers strip
TAB/CR/LF from a URL per the WHATWG URL spec wherever they occur, and
net/http's header writer only strips \r/\n (not \t), so a value like
"/\t/evil.com" passed safeNextPath unchanged, was written verbatim
into the Location header, and the browser then stripped the TAB
client-side -> "//evil.com", an off-site redirect. Exploitable
directly on the local-login success path.

- safeNextPath now rejects any next containing a byte < 0x20 or 0x7F.
- Added a length cap (512 bytes) so oversized values are also rejected.
- oidcLoginURL now defensively re-validates through safeNextPath
  instead of relying on caller discipline.
- Regression tests: control-char + oversized cases in next_path_test.go,
  and a POST /login next=/%09/evil.com handler test proving the
  local-login exploit path is closed.

Security review finding on PR #1392.

zombor commented 2026-08-08 02:30:16 +00:00

Author

Owner

Copy link

Code Re-Review — PR #1392 (bookshelf-bxtat) at 45d80ad9

Focused on the delta since the prior review (open-redirect BLOCKER fix + 2 code MINORs).

Confirmed resolved from prior review:

• Length cap present: internal/users/handler.go:96-98 (maxNextPathBytes = 512, rejects len(next) > 512), asserted by next_path_test.go ("an oversized next is rejected").
• Control-char reject present and test-asserted, not relying on stdlib normalization: isControlByte (r < 0x20 || r == 0x7F) at handler.go:118-121, wired via strings.IndexFunc(next, isControlByte) at handler.go:112-114; covered by TAB/CR/LF/DEL table entries in next_path_test.go and the black-box handler_test.go:559-570 POST /login case (next=%2F%09%2Fevil.com → asserts exact Location: /).
• safeNextPath is now the single shared choke point used by every entry point: loginPageHandler (handler.go:153), loginHandler (handler.go:241), oidcLoginHandler (oidc_handler.go:106), and defensively both in oidcLoginURL (oidc_handler.go:~28, re-validates before building the /auth/oidc/login?next= redirect) and in oidcCallbackHandler (oidc_handler.go:156-159, re-validates the cookie-sourced next before using it in the final Location redirect). No entry point bypasses it — grepped for any remaining inline strings.HasPrefix(next, ...) duplication and found none on the branch.
• isControlByte predicate is correct: strictly C0 (<0x20) + DEL (0x7F); does not reject any valid path byte (letters, digits, /, -, _, ., %-encoded sequences decode before this check runs and query-string percent-encoding is already decoded by net/http before Query().Get/FormValue, so a literal raw TAB in the decoded value is exactly the byte being defended against).
• Valid next values (e.g. /books/123, /books/9) still round-trip unchanged — covered by both next_path_test.go and the OIDC-only deep-link tests in handler_test.go (?next=/books/9 → Location: /auth/oidc/login?next=%2Fbooks%2F9) and the new e2e Journey-9 step (e2e/api/journey_9_oidc_login_test.go) which proves the full cookie round-trip through a real HTTP flow.
• templates/pages/login.html:50 (href="/auth/oidc/login?next={{.Next}}") is safe: .Next is already safeNextPath-validated server-side before being placed in template data, and html/template's contextual URL-attribute autoescaping provides defense-in-depth regardless. Matches the pre-existing hidden-input pattern at line 19.
• Test files are black-box (package users_test), using the new ExportSafeNextPath re-export (export_test.go) rather than reaching into unexported internals directly. No coverage-exclusion or .golangci.yml changes in this diff.

Findings:

[MINOR] internal/users/oidc_handler_test.go — new callback Its violate one-Expect-per-It
Two of the new tests bundle two Expect calls into a single It: "redirects to the stored next path on success" (Expect(w.Code)... + Expect(w.Header().Get("Location"))...) and "redirects to / instead of the hostile value (defense in depth)" (same pair). This mirrors an already-existing pattern in this file (the pre-existing "redirects to home on success..." It has the same two-Expect shape), so it's not a new regression this delta introduced, but it does propagate the violation into new tests rather than fixing it. Suggested fix: split each into a status-code It and a Location It (or fold the status check into the value assertion per project-conventions.md's "fold no-error check into value assertion" guidance, using Expect(w.Code, w.Header().Get("Location")).To(...)-style if a single combined actual is preferred) — low priority, can ride with a future cleanup of the surrounding pre-existing pattern rather than blocking this PR.

No other issues found. The fix correctly closes the embedded-control-byte / oversized-input gaps flagged previously, keeps a single validation choke point, and the new tests (unit + e2e) demonstrate both the attack path is closed and legitimate deep-links still work end-to-end.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Re-Review — PR #1392 (bookshelf-bxtat) at 45d80ad9 Focused on the delta since the prior review (open-redirect BLOCKER fix + 2 code MINORs). **Confirmed resolved from prior review:** - Length cap present: `internal/users/handler.go:96-98` (`maxNextPathBytes = 512`, rejects `len(next) > 512`), asserted by `next_path_test.go` ("an oversized next is rejected"). - Control-char reject present and test-asserted, not relying on stdlib normalization: `isControlByte` (`r < 0x20 || r == 0x7F`) at `handler.go:118-121`, wired via `strings.IndexFunc(next, isControlByte)` at `handler.go:112-114`; covered by TAB/CR/LF/DEL table entries in `next_path_test.go` and the black-box `handler_test.go:559-570` POST `/login` case (`next=%2F%09%2Fevil.com` → asserts exact `Location: /`). - `safeNextPath` is now the single shared choke point used by every entry point: `loginPageHandler` (`handler.go:153`), `loginHandler` (`handler.go:241`), `oidcLoginHandler` (`oidc_handler.go:106`), and defensively both in `oidcLoginURL` (`oidc_handler.go:~28`, re-validates before building the `/auth/oidc/login?next=` redirect) and in `oidcCallbackHandler` (`oidc_handler.go:156-159`, re-validates the cookie-sourced `next` before using it in the final `Location` redirect). No entry point bypasses it — grepped for any remaining inline `strings.HasPrefix(next, ...)` duplication and found none on the branch. - `isControlByte` predicate is correct: strictly C0 (`<0x20`) + DEL (`0x7F`); does not reject any valid path byte (letters, digits, `/`, `-`, `_`, `.`, `%`-encoded sequences decode before this check runs and query-string percent-encoding is already decoded by `net/http` before `Query().Get`/`FormValue`, so a literal raw TAB in the decoded value is exactly the byte being defended against). - Valid `next` values (e.g. `/books/123`, `/books/9`) still round-trip unchanged — covered by both `next_path_test.go` and the OIDC-only deep-link tests in `handler_test.go` (`?next=/books/9` → `Location: /auth/oidc/login?next=%2Fbooks%2F9`) and the new e2e Journey-9 step (`e2e/api/journey_9_oidc_login_test.go`) which proves the full cookie round-trip through a real HTTP flow. - `templates/pages/login.html:50` (`href="/auth/oidc/login?next={{.Next}}"`) is safe: `.Next` is already `safeNextPath`-validated server-side before being placed in template data, and `html/template`'s contextual URL-attribute autoescaping provides defense-in-depth regardless. Matches the pre-existing hidden-input pattern at line 19. - Test files are black-box (`package users_test`), using the new `ExportSafeNextPath` re-export (`export_test.go`) rather than reaching into unexported internals directly. No coverage-exclusion or `.golangci.yml` changes in this diff. **Findings:** [MINOR] internal/users/oidc_handler_test.go — new callback `It`s violate one-Expect-per-It Two of the new tests bundle two `Expect` calls into a single `It`: `"redirects to the stored next path on success"` (`Expect(w.Code)...` + `Expect(w.Header().Get("Location"))...`) and `"redirects to / instead of the hostile value (defense in depth)"` (same pair). This mirrors an already-existing pattern in this file (the pre-existing `"redirects to home on success..."` It has the same two-Expect shape), so it's not a new regression this delta introduced, but it does propagate the violation into new tests rather than fixing it. Suggested fix: split each into a status-code It and a Location It (or fold the status check into the value assertion per `project-conventions.md`'s "fold no-error check into value assertion" guidance, using `Expect(w.Code, w.Header().Get("Location")).To(...)`-style if a single combined actual is preferred) — low priority, can ride with a future cleanup of the surrounding pre-existing pattern rather than blocking this PR. No other issues found. The fix correctly closes the embedded-control-byte / oversized-input gaps flagged previously, keeps a single validation choke point, and the new tests (unit + e2e) demonstrate both the attack path is closed and legitimate deep-links still work end-to-end. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-08-08 02:30:34 +00:00

Author

Owner

Copy link

Security re-review — PR #1392 (bookshelf-bxtat), commit 45d80ad9

Verified the fix for the embedded-control-byte open-redirect BLOCKER from the prior review.

Payload zoo re-run against new safeNextPath

• /%09/evil.com (TAB, 0x09) → strings.IndexFunc(next, isControlByte) decodes the raw 0x09 byte as its own rune (valid ASCII), 0x09 < 0x20 → caught → falls back to /. Closed. Confirmed by the new regression test asserting Location == "/" exactly (not just non-nil) in internal/users/handler_test.go.
• \n (0x0A), \r (0x0D), \x0b (VT), \x0c (FF), \x7f (DEL), NUL (0x00) — all satisfy r < 0x20 || r == 0x7F → all caught. Table-driven unit coverage in internal/users/next_path_test.go exercises TAB/LF/CR/DEL explicitly; VT/FF/NUL aren't individually enumerated in the table but are covered by the same predicate — no code path treats them differently, so this is a coverage nit, not a gap.
• UTF-8 smuggling check: strings.IndexFunc decodes runes via utf8.DecodeRuneInString. A raw C0/DEL byte is always a complete, valid 1-byte rune in UTF-8, so it can't be "hidden" inside a multi-byte sequence — continuation bytes in valid UTF-8 are always ≥0x80, and an invalid/overlong sequence decodes to utf8.RuneError (also not < 0x20) without re-synthesizing a low ASCII byte. Traced through to http.Redirect's hexEscapeNonASCII, which percent-encodes every byte >= 0x80 individually (byte-wise, not rune-wise) before writing the Location header — so even a malformed sequence that slipped past validation would arrive at the browser as literal %XX text, never as a raw control byte or a decoded slash. No smuggling path found.
• Unicode "slash lookalikes" (U+2044 fraction slash, U+FF0F fullwidth solidus, RTL override U+202E, etc.): all are non-ASCII, so hexEscapeNonASCII percent-encodes them in the Location header before it reaches the browser. Browsers do not decode percent-escapes in Location back into path-separator semantics — they follow the URL as an opaque percent-encoded path segment. Not a redirect-bypass vector. (Residual note, not a finding: if next is ever echoed back into HTML — it isn't, in this diff — html/template auto-escaping would apply.)
• 512-byte cap: len(next) > maxNextPathBytes — Go len() on a string is byte length, which is the correct measure for the header-size concern the comment describes. No off-by-one; > (not >=) means exactly 512 bytes is still accepted, which is fine (the boundary is documented as "longer is rejected").
• oidcLoginURL defensive re-validation: re-calls safeNextPath(next) before building the redirect URL; idempotent (a value that already passed safeNextPath maps to itself). Correctly special-cases "/" to omit the query param. Query value is url.QueryEscaped.
• Callback-path defense in depth: oidcCallbackHandler re-validates the cookie-sourced next via safeNextPath(cookieNext) before redirecting — closes the theoretical gap where a tampered/stale cookie value bypasses the login-time validation. Cookie attributes (Path, HttpOnly, Secure, SameSite=Lax, matching MaxAge) are consistent with the existing state/nonce/verifier cookies, and clearOIDCCookies now also expires the new oidcNextCookieName cookie.
• Regression test assertion quality: internal/users/handler_test.go new Context("with next=/%09/evil.com …") asserts Expect(resp.Header.Get("Location")).To(Equal("/")) — an exact-match assertion, not a weaker non-nil/non-empty check. Good.
• Test hygiene: next_path_test.go is package users_test and reaches the unexported safeNextPath only through the documented ExportSafeNextPath re-export in export_test.go — consistent with the project's black-box test convention.

Findings

None. All payload-zoo cases are closed by the new isControlByte check, and the multi-byte/Unicode-lookalike smuggling paths are independently closed by Go's net/http.Redirect byte-wise non-ASCII percent-escaping — no bypass reaches the browser as a literal separator or control byte.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security re-review — PR #1392 (bookshelf-bxtat), commit 45d80ad9 Verified the fix for the embedded-control-byte open-redirect BLOCKER from the prior review. ### Payload zoo re-run against new `safeNextPath` - `/%09/evil.com` (TAB, 0x09) → `strings.IndexFunc(next, isControlByte)` decodes the raw 0x09 byte as its own rune (valid ASCII), `0x09 < 0x20` → caught → falls back to `/`. **Closed.** Confirmed by the new regression test asserting `Location == "/"` exactly (not just non-nil) in `internal/users/handler_test.go`. - `\n` (0x0A), `\r` (0x0D), `\x0b` (VT), `\x0c` (FF), `\x7f` (DEL), NUL (0x00) — all satisfy `r < 0x20 || r == 0x7F` → all caught. Table-driven unit coverage in `internal/users/next_path_test.go` exercises TAB/LF/CR/DEL explicitly; VT/FF/NUL aren't individually enumerated in the table but are covered by the same predicate — no code path treats them differently, so this is a coverage nit, not a gap. - **UTF-8 smuggling check:** `strings.IndexFunc` decodes runes via `utf8.DecodeRuneInString`. A raw C0/DEL byte is always a complete, valid 1-byte rune in UTF-8, so it can't be "hidden" inside a multi-byte sequence — continuation bytes in valid UTF-8 are always ≥0x80, and an invalid/overlong sequence decodes to `utf8.RuneError` (also not `< 0x20`) without re-synthesizing a low ASCII byte. Traced through to `http.Redirect`'s `hexEscapeNonASCII`, which percent-encodes every byte `>= 0x80` individually (byte-wise, not rune-wise) before writing the `Location` header — so even a malformed sequence that slipped past validation would arrive at the browser as literal `%XX` text, never as a raw control byte or a decoded slash. No smuggling path found. - **Unicode "slash lookalikes"** (U+2044 fraction slash, U+FF0F fullwidth solidus, RTL override U+202E, etc.): all are non-ASCII, so `hexEscapeNonASCII` percent-encodes them in the `Location` header before it reaches the browser. Browsers do not decode percent-escapes in `Location` back into path-separator semantics — they follow the URL as an opaque percent-encoded path segment. Not a redirect-bypass vector. (Residual note, not a finding: if `next` is ever echoed back into HTML — it isn't, in this diff — `html/template` auto-escaping would apply.) - **512-byte cap:** `len(next) > maxNextPathBytes` — Go `len()` on a string is byte length, which is the correct measure for the header-size concern the comment describes. No off-by-one; `>` (not `>=`) means exactly 512 bytes is still accepted, which is fine (the boundary is documented as "longer is rejected"). - **`oidcLoginURL` defensive re-validation:** re-calls `safeNextPath(next)` before building the redirect URL; idempotent (a value that already passed `safeNextPath` maps to itself). Correctly special-cases `"/"` to omit the query param. Query value is `url.QueryEscape`d. - **Callback-path defense in depth:** `oidcCallbackHandler` re-validates the *cookie-sourced* `next` via `safeNextPath(cookieNext)` before redirecting — closes the theoretical gap where a tampered/stale cookie value bypasses the login-time validation. Cookie attributes (`Path`, `HttpOnly`, `Secure`, `SameSite=Lax`, matching `MaxAge`) are consistent with the existing state/nonce/verifier cookies, and `clearOIDCCookies` now also expires the new `oidcNextCookieName` cookie. - **Regression test assertion quality:** `internal/users/handler_test.go` new `Context("with next=/%09/evil.com …")` asserts `Expect(resp.Header.Get("Location")).To(Equal("/"))` — an exact-match assertion, not a weaker non-nil/non-empty check. Good. - **Test hygiene:** `next_path_test.go` is `package users_test` and reaches the unexported `safeNextPath` only through the documented `ExportSafeNextPath` re-export in `export_test.go` — consistent with the project's black-box test convention. ### Findings None. All payload-zoo cases are closed by the new `isControlByte` check, and the multi-byte/Unicode-lookalike smuggling paths are independently closed by Go's `net/http.Redirect` byte-wise non-ASCII percent-escaping — no bypass reaches the browser as a literal separator or control byte. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-08 02:30:58 +00:00

Author

Owner

Copy link

Code Review — PR #1390 (bookshelf-bz643.6)

Reviewed the diff (17→3 browser e2e journey consolidation) against .claude/rules/review-standard.md and the CLAUDE.md E2E Testing Policy. Spot-checked every deleted file against its claimed destination in the 3 new journey files.

Summary of what I verified as correctly preserved:

• journey_bookdrop_test.go: all 7 original files' distinctive assertions present (bottom-bar .closest() checks, extract-pattern preview+apply+screenshot, field-parity series_total/page_count/categories/age_rating/content_rating, comic editor fetched-chip-pill rendering, auto-save-without-Save-click + DB persistence + Accept-flush, DragEvent merge-order reorder+persist, settings poll-interval CSS-hidden regression guard). Screenshots preserved at the same points.
• journey_metadata_fetch_test.go: scan-button + LLM-provider-modal correctly promoted to standalone top-level Ordered, Serial Describes (fixes the real Ginkgo "Invalid Serial Node in Non-Serial Ordered Container" CI failure from the first push — verified Serial never appears nested inside another Ordered parent in the diff). Variant-cover, save-validation, cover-apply (fallback-only bookshelf-4xxe semantics), and Author Search (Audnexus, all 5 steps incl. bio-expand screenshot) all faithfully retained.
• journey_magic_shelf_test.go: create-modal, filter-stays-on-shelf (localStorage+navigation), infinite-scroll-stays-on-rule (IntersectionObserver regression guard for bookshelf-di1h) all faithfully retained with matching seed data and assertions.
• Top-level Describe count drops 52->40 (this PR's contribution), all top-level Describes are Ordered (make e2e-policy-check enforces this), each Ordered journey reusing a page across Its consistently calls refreshPageTimeout/page.Timeout at the start of each step via BeforeEach or explicit re-set.
• CI green, PR mergeable.

Finding:

[MAJOR] e2e/browser/journey_magic_shelf_test.go:8-15 — dropped DOM regression-guard coverage for the library-field <select> render (bookshelf-lkra), justification comment is factually inaccurate

The consolidation comment claims: "only the full-stack 'submit' It is kept; the field-picker render/select-population Its asserted DOM structure with no additional Chromium-unique behavior beyond what the submit It already proves end-to-end." This is not accurate. The original journey_magic_shelf_library_dropdown_test.go had 4 Its:

1. clicking + Add Rule inserts a rule with a field picker
2. selecting Library renders a <select> (not freeform input) and posts screenshot — the actual regression guard for bookshelf-lkra (a real historical bug: the library field rendering as a freeform text input instead of a populated <select>)
3. the library select contains the seeded library as an option
4. submitting a library rule saves the shelf and returns matching books — the full-stack round trip

Only It #4 survived, as "Library field rule saves and filters books" (journey_magic_shelf_test.go:520-621). But that surviving It explicitly bypasses the field-picker DOM — see its own comment around line 585: "Inject the rules JSON directly into the hidden input (bypasses the multi-step UI builder for robustness...)". It never opens the field picker, never clicks the "Library" field item, and never asserts a <select class="rule-value-enum"> renders. Nor does the "Create Magic Shelf Modal" journey earlier in the same file (lines 44-218) touch field selection — it only tests the modal opening and shelf-name submission.

Net effect: the bookshelf-lkra regression guard (library rule field renders a <select> populated with the user's real libraries, not a freeform text input) is now completely unverified by any browser e2e spec, while the PR's own inline comment claims it's redundantly covered. If this DOM path regresses (e.g. a future refactor of the rule-builder's field-picker JS reverts to a plain text input for enum fields), no test will catch it.

Fix: restore at minimum It #2 (selecting Library renders a <select>...) as a nested It in the "Create Magic Shelf Modal" Describe (or a small dedicated Describe) in journey_magic_shelf_test.go, driving the real field-picker UI (open picker -> click library field item -> assert select.rule-value-enum exists) before falling back to the JSON-injection shortcut for the full-stack round trip. It #1 and #3 can stay dropped (weaker DOM-presence checks with lower unique signal), but #2 is the one genuine regression guard.

REVIEW VERDICT: 0 blocker, 1 major, 0 minor

## Code Review — PR #1390 (bookshelf-bz643.6) Reviewed the diff (17→3 browser e2e journey consolidation) against `.claude/rules/review-standard.md` and the CLAUDE.md E2E Testing Policy. Spot-checked every deleted file against its claimed destination in the 3 new journey files. **Summary of what I verified as correctly preserved:** - `journey_bookdrop_test.go`: all 7 original files' distinctive assertions present (bottom-bar `.closest()` checks, extract-pattern preview+apply+screenshot, field-parity series_total/page_count/categories/age_rating/content_rating, comic editor fetched-chip-pill rendering, auto-save-without-Save-click + DB persistence + Accept-flush, DragEvent merge-order reorder+persist, settings poll-interval CSS-hidden regression guard). Screenshots preserved at the same points. - `journey_metadata_fetch_test.go`: scan-button + LLM-provider-modal correctly promoted to standalone top-level `Ordered, Serial` Describes (fixes the real Ginkgo "Invalid Serial Node in Non-Serial Ordered Container" CI failure from the first push — verified `Serial` never appears nested inside another `Ordered` parent in the diff). Variant-cover, save-validation, cover-apply (fallback-only bookshelf-4xxe semantics), and Author Search (Audnexus, all 5 steps incl. bio-expand screenshot) all faithfully retained. - `journey_magic_shelf_test.go`: create-modal, filter-stays-on-shelf (localStorage+navigation), infinite-scroll-stays-on-rule (IntersectionObserver regression guard for bookshelf-di1h) all faithfully retained with matching seed data and assertions. - Top-level `Describe` count drops 52->40 (this PR's contribution), all top-level Describes are `Ordered` (make e2e-policy-check enforces this), each Ordered journey reusing a page across `It`s consistently calls `refreshPageTimeout`/`page.Timeout` at the start of each step via `BeforeEach` or explicit re-set. - CI green, PR mergeable. **Finding:** [MAJOR] e2e/browser/journey_magic_shelf_test.go:8-15 — dropped DOM regression-guard coverage for the library-field `<select>` render (bookshelf-lkra), justification comment is factually inaccurate The consolidation comment claims: "only the full-stack 'submit' It is kept; the field-picker render/select-population Its asserted DOM structure with no additional Chromium-unique behavior beyond what the submit It already proves end-to-end." This is not accurate. The original `journey_magic_shelf_library_dropdown_test.go` had 4 Its: 1. `clicking + Add Rule inserts a rule with a field picker` 2. `selecting Library renders a <select> (not freeform input) and posts screenshot` — the actual regression guard for bookshelf-lkra (a real historical bug: the library field rendering as a freeform text input instead of a populated `<select>`) 3. `the library select contains the seeded library as an option` 4. `submitting a library rule saves the shelf and returns matching books` — the full-stack round trip Only It #4 survived, as "Library field rule saves and filters books" (journey_magic_shelf_test.go:520-621). But that surviving It explicitly **bypasses** the field-picker DOM — see its own comment around line 585: "Inject the rules JSON directly into the hidden input (bypasses the multi-step UI builder for robustness...)". It never opens the field picker, never clicks the "Library" field item, and never asserts a `<select class="rule-value-enum">` renders. Nor does the "Create Magic Shelf Modal" journey earlier in the same file (lines 44-218) touch field selection — it only tests the modal opening and shelf-name submission. Net effect: the bookshelf-lkra regression guard (library rule field renders a `<select>` populated with the user's real libraries, not a freeform text input) is now completely unverified by any browser e2e spec, while the PR's own inline comment claims it's redundantly covered. If this DOM path regresses (e.g. a future refactor of the rule-builder's field-picker JS reverts to a plain text input for enum fields), no test will catch it. Fix: restore at minimum It #2 (`selecting Library renders a <select>...`) as a nested `It` in the "Create Magic Shelf Modal" Describe (or a small dedicated Describe) in `journey_magic_shelf_test.go`, driving the real field-picker UI (open picker -> click library field item -> assert `select.rule-value-enum` exists) before falling back to the JSON-injection shortcut for the full-stack round trip. It #1 and #3 can stay dropped (weaker DOM-presence checks with lower unique signal), but #2 is the one genuine regression guard. REVIEW VERDICT: 0 blocker, 1 major, 0 minor

zombor referenced this pull request 2026-08-08 02:39:36 +00:00

OIDC Test Connection: structured per-check diagnostic [shot:oidc-test-connection-structured-diagnostic] (bookshelf-tm38.15) #1396

zombor referenced this pull request 2026-08-08 02:42:23 +00:00

fix(review-minors): batch of 8 review-minor follow-ups (bookshelf-ifwpa) #1397

zombor referenced this pull request 2026-08-08 02:55:53 +00:00

feat(oidc): configurable per-login session duration override (bookshelf-tm38.11) #1395

zombor referenced this pull request 2026-08-08 02:56:24 +00:00

feat(oidc): configurable per-login session duration override (bookshelf-tm38.11) #1395

zombor referenced this pull request 2026-08-08 03:05:05 +00:00

OIDC Test Connection: structured per-check diagnostic [shot:oidc-test-connection-structured-diagnostic] (bookshelf-tm38.15) #1396

zombor referenced this pull request 2026-08-08 03:05:58 +00:00

OIDC Test Connection: structured per-check diagnostic [shot:oidc-test-connection-structured-diagnostic] (bookshelf-tm38.15) #1396

zombor referenced this pull request 2026-08-08 03:06:14 +00:00

OIDC Test Connection: structured per-check diagnostic [shot:oidc-test-connection-structured-diagnostic] (bookshelf-tm38.15) #1396

zombor referenced this pull request 2026-08-08 03:06:26 +00:00

OIDC Test Connection: structured per-check diagnostic [shot:oidc-test-connection-structured-diagnostic] (bookshelf-tm38.15) #1396

zombor referenced this pull request 2026-08-08 03:06:58 +00:00

OIDC Test Connection: structured per-check diagnostic [shot:oidc-test-connection-structured-diagnostic] (bookshelf-tm38.15) #1396

zombor referenced this pull request 2026-08-08 03:07:23 +00:00

OIDC Test Connection: structured per-check diagnostic [shot:oidc-test-connection-structured-diagnostic] (bookshelf-tm38.15) #1396

zombor referenced this pull request from a commit 2026-08-08 15:36:52 +00:00

fix(users): close TAB-embedded open-redirect BLOCKER in safeNextPath

zombor force-pushed bd-bookshelf-bxtat from 45d80ad976 to 916f9e74ee 2026-08-08 15:36:53 +00:00 Compare

zombor merged commit a78d499a03 into main 2026-08-08 15:44:34 +00:00

zombor referenced this pull request from a commit 2026-08-08 15:44:37 +00:00

Merge pull request 'fix(users): thread next through OIDC login + open-redirect hardening (bookshelf-bxtat)' (#1392) from bd-bookshelf-bxtat into main

zombor referenced this pull request 2026-08-12 20:00:32 +00:00

fix(review): address batch MINOR/def-in-depth follow-ups (bookshelf-syyzy) #1437

zombor referenced this pull request 2026-08-12 20:12:48 +00:00

fix(review): address batch MINOR/def-in-depth follow-ups (bookshelf-syyzy) #1437

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1392

No description provided.