crueber/walhub

Watch

Fork 0

OIDC browser login is dead: root 401s with no login path, /_auth/login 501s, and incomplete OIDC config boots silently #344

New issue

Closed

opened 2026-09-11 18:25:18 +00:00 by crueber · 3 comments

crueber commented

2026-09-11 18:25:18 +00:00

Owner

What's wrong

With OIDC configured on hub.packden.us, browser login is completely dead and there is no way in:

The root URL / returns 401 plain text authentication required (with WWW-Authenticate: Bearer) for a normal browser GET — no redirect to the authenticator, no login page, no hint about what to do.
There is no login affordance anywhere: GET /_auth/login answers 501 browser login is not enabled.
Reproduce: curl -i https://hub.packden.us/ with a browser Accept: text/html → 401; curl -i https://hub.packden.us/_auth/login → 501.

Root cause (code evidence)

The redirect branch exists but is gated on config the instance doesn't satisfy. authFailure (internal/server/middleware.go:529-546) sends browser-ish GETs without an Authorization header to /_auth/login?next=… (307) only when s.authSvc.BrowserLoginEnabled() (middleware.go:537-541) — otherwise it falls through to the bare 401 at :543-544.
BrowserLoginEnabled() requires all four of: mode == "oidc" and non-empty session_secret and non-empty oauth_client_id and non-empty oauth_client_secret (internal/server/auth.go:306-310). The live / _auth/login 501 ("browser login is not enabled", auth_oidc.go) proves one or more of those is empty on the running instance — the same state makes every browser GET a dead-end 401, because anonymous read is also off in OIDC mode.
Config validation does not catch it. internal/config/validate.go:111-115 only checks that oauth_client_id/oauth_client_secret are paired, and that a non-empty session_secret is ≥ 32 bytes. Nothing requires the trio when auth.mode == "oidc", so a partially-configured OIDC instance boots happily into an unusable state — no startup error, no health signal, no setup-page warning. That is the systemic defect: the misconfiguration is silent.
Nothing advertises browser login to the SPA. The discovery auth block is {"bearer": true, "setup": …, "browser": …, "authenticate": …} (live, GET /api/v1) — no login/browser_login member — and no page renders a "Log in with OIDC" button. Even when enabled, an unauthenticated browser that somehow reaches the SPA has no rendered affordance (the spec's answer is the 307 into the provider, which never fires when disabled).
Spec deviation to note: docs/go/06_server_http.md:62 specifies the 307-to-/_auth/login behavior for browser-ish GETs. The code implements it, but the spec's precondition (browser login enabled) is unverifiable at boot — the doc should also state what a browser sees when the precondition is unmet (today: an unexplained 401).

Note on the reported trigger

"Restarted with credentials and now everything 401s" is consistent with #3: a plausible key-name or partial-set mistake (oauth_client_id / oauth_client_secret / session_secret under server.auth, or their WALHUB__SERVER__AUTH__* env spellings) leaves the trio incomplete while the process starts clean. The audit should confirm the exact running config (or at least enumerate the three keys and the env spellings in the issue thread) and then decide whether the primary fix is validation, a login page, or both.

What's needed

Fail loudly at startup: when auth.mode == "oidc" and the browser-login trio is incomplete, config validation must refuse to start (or boot into a clearly-signposted degraded state that still serves a login/diagnostic page) — never the silent current state. Name the missing key(s) in the error.
A real login entry point: an unauthenticated browser hitting any gated page must get something usable — either the 307 to the provider (when enabled) or a rendered "Log in with OIDC" page/button that starts the flow (/_auth/login), plus a clear explanation when browser login is disabled. The /_auth/login 501 body ("browser login is not enabled") is a developer string; it must not be what an end user at / effectively experiences.
Advertise it: expose browser-login availability in the discovery auth block (and/or /services/setup.json) so the SPA can render the login button only when it works, and so /setup can warn when OIDC is selected without the trio.
Tests: a config-validation test for mode=oidc with each of the three keys missing; a middleware test asserting a browser-ish GET with Accept: text/html redirects to /_auth/login?next=… when enabled and returns the diagnostic page (not a bare 401) when not.

Acceptance criteria

A browser GET to / on an OIDC-configured instance either 307s to the provider or renders a page with a working "Log in with OIDC" button; never a bare authentication required with no path forward.
auth.mode = "oidc" with any of session_secret / oauth_client_id / oauth_client_secret missing fails config validation with an error naming the missing key(s) — no silent boot.
Discovery (GET /api/v1 auth block) and/or setup JSON report browser-login availability; /setup warns when OIDC is selected but the trio is incomplete.
/_auth/login with browser login enabled starts the provider flow and returns next correctly after callback; the 501 path is unreachable in a validly-configured instance.
Existing bearer/token auth paths unaffected (static tokens still work in oidc mode).
Tests per §"Tests" above; the middleware decision table is headless-testable and asserted for both enabled and disabled states.

Ties to #345 (public/private visibility)

This issue and #345 (public repos viewable without an account) must land coherently, because public becomes the default repo visibility there:

#345 makes visibility the read authority for repo-scoped surfaces — a public repo becomes readable by signed-out visitors even with server.auth.anonymous_read = false, which is exactly the gate currently 401-ing this instance's root URL.
After #345, an unauthenticated browser GET to / on a healthy instance should render the public app (explore, public repos) with a "Log in with OIDC" affordance from this issue — the 401-with-no-path-forward disappears for repos the visitor can read, and this issue's login flow covers everything else.
The two tickets share the gate chain (Env.gate, smart.go, lfs.go, the SPA shell's gated()): whichever lands first must not assume the other's semantics. Sequence note for the implementer: #345 changes what anonymous users can read; this issue changes how an unauthenticated browser gets an identity. The broken state being fixed here (bare 401, no login path, silent misconfig boot) is independent of visibility and must be fixed regardless.
Config-validation hardening from this issue (refuse mode=oidc without the session/client trio) stays required under #345's semantics — anonymous browsing of public repos does not remove the need for a working login flow.

## What's wrong With OIDC configured on hub.packden.us, **browser login is completely dead and there is no way in**: - The root URL `/` returns **401 plain text `authentication required`** (with `WWW-Authenticate: Bearer`) for a normal browser GET — no redirect to the authenticator, no login page, no hint about what to do. - **There is no login affordance anywhere**: `GET /_auth/login` answers **501 `browser login is not enabled`**. - Reproduce: `curl -i https://hub.packden.us/` with a browser `Accept: text/html` → 401; `curl -i https://hub.packden.us/_auth/login` → 501. ## Root cause (code evidence) 1. **The redirect branch exists but is gated on config the instance doesn't satisfy.** `authFailure` (`internal/server/middleware.go:529-546`) sends browser-ish GETs without an `Authorization` header to `/_auth/login?next=…` (307) **only when `s.authSvc.BrowserLoginEnabled()`** (`middleware.go:537-541`) — otherwise it falls through to the bare 401 at :543-544. 2. **`BrowserLoginEnabled()` requires all four of**: `mode == "oidc"` **and** non-empty `session_secret` **and** non-empty `oauth_client_id` **and** non-empty `oauth_client_secret` (`internal/server/auth.go:306-310`). The live `/ _auth/login` 501 ("browser login is not enabled", `auth_oidc.go`) proves one or more of those is empty on the running instance — the same state makes every browser GET a dead-end 401, because anonymous read is also off in OIDC mode. 3. **Config validation does not catch it.** `internal/config/validate.go:111-115` only checks that `oauth_client_id`/`oauth_client_secret` are **paired**, and that a non-empty `session_secret` is ≥ 32 bytes. Nothing requires the trio when `auth.mode == "oidc"`, so a partially-configured OIDC instance **boots happily into an unusable state** — no startup error, no health signal, no setup-page warning. That is the systemic defect: the misconfiguration is silent. 4. **Nothing advertises browser login to the SPA.** The discovery auth block is `{"bearer": true, "setup": …, "browser": …, "authenticate": …}` (live, `GET /api/v1`) — no `login`/`browser_login` member — and no page renders a "Log in with OIDC" button. Even when enabled, an unauthenticated browser that somehow reaches the SPA has no rendered affordance (the spec's answer is the 307 into the provider, which never fires when disabled). 5. **Spec deviation to note**: `docs/go/06_server_http.md:62` specifies the 307-to-`/_auth/login` behavior for browser-ish GETs. The code implements it, but the spec's precondition (browser login enabled) is unverifiable at boot — the doc should also state what a browser sees when the precondition is unmet (today: an unexplained 401). ## Note on the reported trigger "Restarted with credentials and now everything 401s" is consistent with #3: a plausible key-name or partial-set mistake (`oauth_client_id` / `oauth_client_secret` / `session_secret` under `server.auth`, or their `WALHUB__SERVER__AUTH__*` env spellings) leaves the trio incomplete while the process starts clean. The audit should confirm the exact running config (or at least enumerate the three keys and the env spellings in the issue thread) and then decide whether the primary fix is validation, a login page, or both. ## What's needed 1. **Fail loudly at startup**: when `auth.mode == "oidc"` and the browser-login trio is incomplete, config validation must **refuse to start** (or boot into a clearly-signposted degraded state that still serves a login/diagnostic page) — never the silent current state. Name the missing key(s) in the error. 2. **A real login entry point**: an unauthenticated browser hitting any gated page must get something usable — either the 307 to the provider (when enabled) or a rendered **"Log in with OIDC"** page/button that starts the flow (`/_auth/login`), plus a clear explanation when browser login is disabled. The `/_auth/login` 501 body ("browser login is not enabled") is a developer string; it must not be what an end user at `/` effectively experiences. 3. **Advertise it**: expose browser-login availability in the discovery auth block (and/or `/services/setup.json`) so the SPA can render the login button only when it works, and so `/setup` can warn when OIDC is selected without the trio. 4. **Tests**: a config-validation test for `mode=oidc` with each of the three keys missing; a middleware test asserting a browser-ish GET with `Accept: text/html` redirects to `/_auth/login?next=…` when enabled and returns the diagnostic page (not a bare 401) when not. ## Acceptance criteria - [ ] A browser GET to `/` on an OIDC-configured instance either 307s to the provider or renders a page with a working "Log in with OIDC" button; never a bare `authentication required` with no path forward. - [ ] `auth.mode = "oidc"` with any of `session_secret` / `oauth_client_id` / `oauth_client_secret` missing fails config validation with an error naming the missing key(s) — no silent boot. - [ ] Discovery (`GET /api/v1` auth block) and/or setup JSON report browser-login availability; `/setup` warns when OIDC is selected but the trio is incomplete. - [ ] `/_auth/login` with browser login enabled starts the provider flow and returns `next` correctly after callback; the 501 path is unreachable in a validly-configured instance. - [ ] Existing bearer/token auth paths unaffected (static tokens still work in oidc mode). - [ ] Tests per §"Tests" above; the middleware decision table is headless-testable and asserted for both enabled and disabled states. ## Ties to #345 (public/private visibility) This issue and **#345 (public repos viewable without an account)** must land coherently, because **public becomes the default repo visibility** there: - **#345 makes visibility the read authority for repo-scoped surfaces** — a public repo becomes readable by signed-out visitors even with `server.auth.anonymous_read = false`, which is exactly the gate currently 401-ing this instance's root URL. - After #345, an unauthenticated browser GET to `/` on a healthy instance should render the public app (explore, public repos) with a **"Log in with OIDC"** affordance from this issue — the 401-with-no-path-forward disappears for repos the visitor can read, and this issue's login flow covers everything else. - The two tickets share the gate chain (`Env.gate`, `smart.go`, `lfs.go`, the SPA shell's `gated()`): whichever lands first must not assume the other's semantics. Sequence note for the implementer: #345 changes *what* anonymous users can read; this issue changes *how an unauthenticated browser gets an identity*. The broken state being fixed here (bare 401, no login path, silent misconfig boot) is independent of visibility and must be fixed regardless. - Config-validation hardening from this issue (refuse `mode=oidc` without the session/client trio) stays required under #345's semantics — anonymous browsing of public repos does not remove the need for a working login flow.

crueber added the

oidc

label

2026-09-11 18:25:19 +00:00

crueber added this to the v1 milestone

2026-09-11 18:25:19 +00:00

crueber referenced this issue from a commit

2026-09-11 22:53:59 +00:00

Fix #344: OIDC browser login fail-closed with a real login entry

crueber referenced this issue from a pull request that will close it,

2026-09-11 22:54:05 +00:00

Fix #344: OIDC browser login #352

crueber commented

2026-09-11 22:54:08 +00:00

Author

Owner

Fix landed as PR #352 (branch fix/issue-344): #352 — validation refuses mode=oidc without the trio (naming keys), browsers get 307-or-login-page instead of bare 401/501-string, browser_login/login_url advertised in discovery + setup.json, /setup warns per-key. Bearer/token paths untouched; #345 coherence noted in the PR. Not merging per instructions — review requested.

Fix landed as PR #352 (branch fix/issue-344): https://git.packden.us/crueber/walhub/pulls/352 — validation refuses mode=oidc without the trio (naming keys), browsers get 307-or-login-page instead of bare 401/501-string, browser_login/login_url advertised in discovery + setup.json, /setup warns per-key. Bearer/token paths untouched; #345 coherence noted in the PR. Not merging per instructions — review requested.

crueber commented

2026-09-11 22:57:54 +00:00

Author

Owner

VERDICT: ready to merge (one comment-only fix pushed as 8f88de6; details below).

Trio validation refuses, names keys, non-oidc exempt — PASS. internal/config/validate.go checkOIDC early-returns nil for non-oidc modes, and the new trio block names each missing key (server.auth.session_secret/oauth_client_id/oauth_client_secret). Pinned by TestValidateOIDCTrioMissing (each-missing x3, all-missing, non-oidc exempt).
Refusal composes with setup-only mode correctly: Load runs Validate, errors -> resolveConfig stateInvalid -> serve-only-/setup+health with 503 elsewhere (cmd/walhub/serve.go:43-58, config.go:32-58). So "refuse to start" (PR body wording) is mechanistically "invalid file -> setup-only" per law 9+10 — the standard fail-closed path, same as the pre-existing oidc-allowlist refusal. Doc amendments say "refuses", matching law 9's existing language. No code issue; suggest softening "refuse-to-start" to "refuse-to-serve (setup-only)" in the PR body if edited further.
Browser entry: 307 enabled / 401+page disabled — PASS. authFailure (internal/server/middleware.go): GET + browserLooks + no Authorization + enabled -> 307 /_auth/login?next=; same minus enabled -> 401 with WWW-Authenticate: Bearer realm="walgit" retained and text/html login page. Status preserved, challenge retained. Button href /_auth/login?next=<QueryEscape(RequestURI)> correct. /_auth/login renders the same page (501) for browsers, plain string otherwise (auth_oidc.go:78-90). Hostile next (//evil, scheme URL) sanitizes to / before state-signing, tested.
Git-client safety (CRITICAL) holds: browserLooks = Accept contains text/html OR Sec-Fetch-Dest: document OR UA contains Mozilla. Git sends User-Agent: git/* + Accept: */* — never matches; smart-HTTP/LFS routes authenticate inside handlers, untouched. Plain/JSON-Accept clients keep text/plain bodies (tested both directions). No HTML can leak to credential helpers.
Nits (non-blocking): (a) FIXED by me (pushed 8f88de6): loginUnavailableHTML comment claimed sanitizeNext confined next at all call sites — true only for the /_auth/login site; the middleware site passes raw RequestURI and safety comes from QueryEscape. Comment now states the actual mechanism. Re-tested -race, gofmt clean. (b) Suggestion only: the page text is OIDC-specific but also serves token/none-mode instances (anonymous_read=false) where OIDC was never intended — "Log in with OIDC" button is dead there. Acceptable per criterion 2 (generic disabled explanation present); a mode-aware variant could be a follow-up.
Bearer/token paths unaffected — PASS. TestStaticTokensWorkInOIDCMode pins static-token auth in oidc mode.
Discovery + setup.json + /setup warnings — PASS. browser_login+login_url in GET /api/v1 (always-present keys, URL empty when disabled) and /services/setup.json (browser_login bool, login_url absolute only when enabled); web/src/lib/setup.js fails each missing trio key on its own row pre-save. Minor asymmetry (discovery "" vs setup.json absent key) is tested on both sides — harmless.
#345 coherence — PASS. Diff touches none of Env.gate/smart.go/lfs.go/shell gating; only the login path in authFailure. No visibility semantics assumed; anonymous_read=false still gates as before.
Fixtures — intended-rule updates, not weakenings. validate_test base + setup_merge_test file fixture + setup-form oidc bases add the now-required trio so the previously-valid cases stay valid under the new rule. No budget assertion touched.
Verification results (scratch worktree @8f88de6): config 95.7% / api 95.4% / server 96.6% (all >= 95%); -race pass on config+api full packages and all six new server tests + trio/discovery tests; gofmt/vet clean; node --test on setup-oidc-trio + setup-form 53/53; go build clean. Full internal/server package shows failures ONLY of the form "ui shell missing — run make build" (unbuilt web/dist in scratch worktrees); confirmed identical failures on a pristine main worktree, i.e. pre-existing environmental, unrelated to this PR. (One transient TestVerifyTokenWireNegatives/tampered_mac-adjacent line in an earlier run did not recur and the test passes in isolation and in the re-run — flake, not PR-related.)
Docs — accurate. 06_server_http.md §2.2#8 row now states the disabled-browser 401+page (closes the spec gap the issue flagged) + §14 entry with the refuse-vs-degraded rationale and #345 layering note; 07_api.md shape; 11_config_cli.md rule 2 + §14 entry. Code matches all three.

MERGE RECOMMENDATION: ready to merge.

Review of PR #352 (fix/issue-344) against #344's six acceptance criteria + #345 coherence. Verified in scratch worktrees (main worktree untouched, still clean on main). No browser used — tests + reasoning only, as instructed; no docker, no system packages, live instance untouched. VERDICT: ready to merge (one comment-only fix pushed as 8f88de6; details below). 1. Trio validation refuses, names keys, non-oidc exempt — PASS. `internal/config/validate.go` `checkOIDC` early-returns nil for non-oidc modes, and the new trio block names each missing key (`server.auth.session_secret/oauth_client_id/oauth_client_secret`). Pinned by `TestValidateOIDCTrioMissing` (each-missing x3, all-missing, non-oidc exempt). Refusal composes with setup-only mode correctly: `Load` runs `Validate`, errors -> `resolveConfig` stateInvalid -> serve-only-/setup+health with 503 elsewhere (`cmd/walhub/serve.go:43-58`, `config.go:32-58`). So "refuse to start" (PR body wording) is mechanistically "invalid file -> setup-only" per law 9+10 — the standard fail-closed path, same as the pre-existing oidc-allowlist refusal. Doc amendments say "refuses", matching law 9's existing language. No code issue; suggest softening "refuse-to-start" to "refuse-to-serve (setup-only)" in the PR body if edited further. 2. Browser entry: 307 enabled / 401+page disabled — PASS. `authFailure` (`internal/server/middleware.go`): GET + browserLooks + no Authorization + enabled -> 307 `/_auth/login?next=`; same minus enabled -> 401 with `WWW-Authenticate: Bearer realm="walgit"` retained and `text/html` login page. Status preserved, challenge retained. Button href `/_auth/login?next=<QueryEscape(RequestURI)>` correct. `/_auth/login` renders the same page (501) for browsers, plain string otherwise (`auth_oidc.go:78-90`). Hostile next (`//evil`, scheme URL) sanitizes to `/` before state-signing, tested. Git-client safety (CRITICAL) holds: `browserLooks` = Accept contains text/html OR Sec-Fetch-Dest: document OR UA contains Mozilla. Git sends `User-Agent: git/*` + `Accept: */*` — never matches; smart-HTTP/LFS routes authenticate inside handlers, untouched. Plain/JSON-Accept clients keep `text/plain` bodies (tested both directions). No HTML can leak to credential helpers. Nits (non-blocking): (a) FIXED by me (pushed 8f88de6): `loginUnavailableHTML` comment claimed sanitizeNext confined next at all call sites — true only for the `/_auth/login` site; the middleware site passes raw RequestURI and safety comes from QueryEscape. Comment now states the actual mechanism. Re-tested -race, gofmt clean. (b) Suggestion only: the page text is OIDC-specific but also serves token/none-mode instances (anonymous_read=false) where OIDC was never intended — "Log in with OIDC" button is dead there. Acceptable per criterion 2 (generic disabled explanation present); a mode-aware variant could be a follow-up. 3. Bearer/token paths unaffected — PASS. `TestStaticTokensWorkInOIDCMode` pins static-token auth in oidc mode. 4. Discovery + setup.json + /setup warnings — PASS. `browser_login`+`login_url` in `GET /api/v1` (always-present keys, URL empty when disabled) and `/services/setup.json` (`browser_login` bool, `login_url` absolute only when enabled); `web/src/lib/setup.js` fails each missing trio key on its own row pre-save. Minor asymmetry (discovery `""` vs setup.json absent key) is tested on both sides — harmless. 5. #345 coherence — PASS. Diff touches none of Env.gate/smart.go/lfs.go/shell gating; only the login path in authFailure. No visibility semantics assumed; anonymous_read=false still gates as before. 6. Fixtures — intended-rule updates, not weakenings. `validate_test` base + `setup_merge_test` file fixture + `setup-form` oidc bases add the now-required trio so the previously-valid cases stay valid under the new rule. No budget assertion touched. 7. Verification results (scratch worktree @8f88de6): config 95.7% / api 95.4% / server 96.6% (all >= 95%); -race pass on config+api full packages and all six new server tests + trio/discovery tests; gofmt/vet clean; node --test on setup-oidc-trio + setup-form 53/53; go build clean. Full `internal/server` package shows failures ONLY of the form "ui shell missing — run make build" (unbuilt web/dist in scratch worktrees); confirmed identical failures on a pristine main worktree, i.e. pre-existing environmental, unrelated to this PR. (One transient `TestVerifyTokenWireNegatives/tampered_mac`-adjacent line in an earlier run did not recur and the test passes in isolation and in the re-run — flake, not PR-related.) 8. Docs — accurate. 06_server_http.md §2.2#8 row now states the disabled-browser 401+page (closes the spec gap the issue flagged) + §14 entry with the refuse-vs-degraded rationale and #345 layering note; 07_api.md shape; 11_config_cli.md rule 2 + §14 entry. Code matches all three. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-11 22:58:09 +00:00

crueber referenced this issue from a commit

2026-09-11 22:58:09 +00:00

Merge pull request 'Fix #344: OIDC browser login' (#352) from fix/issue-344 into main

crueber commented

2026-09-11 22:58:09 +00:00

Author

Owner

Fixed by PR #352 (review clean + one comment-accuracy fix by reviewer; trio refusal, browser entry, git-client safety, #345 coherence verified), merged. Closing.

crueber referenced this issue from a commit

2026-09-11 23:19:41 +00:00

Fix #345: public/private visibility enforced across listings, git, and UI

crueber referenced this issue

2026-09-11 23:19:48 +00:00

Fix #345: public/private visibility #353

crueber referenced this issue

2026-09-11 23:19:52 +00:00

Public repositories: viewable without an account; repos get public (default) / private visibility enforced across listings, git, and the UI #345

crueber referenced this issue

2026-09-11 23:26:46 +00:00

Public repositories: viewable without an account; repos get public (default) / private visibility enforced across listings, git, and the UI #345