auth-providers: OIDC callback hardening (3 commits)#10931
Open
MichaelUray wants to merge 8 commits into
Open
Conversation
…500) Production-blocking: every OIDC callback (bogus or real code) returned 500 Internal Server Error, even with valid sessions. No log was emitted between `try auth via openid` and the 500 — passport strategy threw silently before `handleProviderAuth` could log. Per Codex plan-review amendments: - Switch to explicit-callback passport.authenticate variant so strategy errors surface as err/info/status args instead of being swallowed. - Privacy-safe diagnostic shape (no raw code/state/tokens/user). Fields: stage, errMessage/Name/Stack, hasUser, infoSummary, statusCode, hasSession + sessionKeys (names only), cookie + state + code PRESENCE (lengths only), host, forwardedProto. - Permanent invalid-callback guard: ALL failures → 302 to /login with ?error=oidc_callback_failed, never 500. Same wrap covers handleProviderAuth throws. Image-tag: hardcoreeng/account:integration-wac-2026-06-21-codex-r7. Next step (P0-T2): user attempts login, we capture the actual error from the diagnostic, then ship a surgical fix in r8. Signed-off-by: Michael Uray <michael.uray@gmail.com> Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
Codex plan-review Critical: server/account/src/admin.ts:15-18 built the ADMIN_EMAILS set without normalizing, AND isAdminEmail() didn't normalize the input. Today value is clean so isAdmin works, but a future env-value like ADMIN_EMAILS=" Michael@Uray.io " would silently break admin detection (Set lookup would miss the lowercased input). Mongo backend already has this hardening (collections/mongo.ts:1141-1147). Postgres + this helper didn't. Now BOTH sides normalize: env split -> trim -> toLowerCase -> filter non-empty + warn on no-@ entries (Codex Optional: warn not reject -- too sharp for a latent fix). Lookup also trim().toLowerCase(). Standalone upstream-PR-able per Codex Q-I3. Tests cover whitespace, case-insensitive env, case-insensitive lookup, invalid-shape warn, empty/unset env, multi-entry. Signed-off-by: Michael Uray <michael.uray@gmail.com> Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
…amendment) Codex E4 amendment (optional, low-priority): hasCookieHeader is sufficient to diagnose missing session-cookie issues; the cookieHeaderLen byte count adds no operational value and is one more PII-adjacent surface to keep an eye on. Drop it from both diagnostic paths in pods/authProviders/src/openid.ts. Signed-off-by: Michael Uray <michael.uray@gmail.com> Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
|
Connected to Huly®: UBERF-16534 |
Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
…TH-2) Bei jedem fehlgeschlagenen OIDC-Callback wurde ein umfangreicher Diag-Block (errStack/sessionKeys/host/…) auf error-Level geloggt -> anonymes Log- Flooding via wiederholter invalider Callbacks. Normalbetrieb loggt jetzt nur eine knappe Fehlerzeile; der volle Diag-Block nur mit OIDC_DEBUG=true. Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
… allowed (L-AUTH-3) Ein Tippfehler wie ADMIN_EMAILS=admin,michel@... legte bislang einen unbeabsichtigten Admin-Identifier an (Eintrag ohne @ wurde still behalten). Jetzt werden @-lose Eintraege per Default verworfen (fail-closed) und nur bei ADMIN_EMAILS_ALLOW_LOGIN_ID=true als Login-ID akzeptiert; Warnung stets. Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
…L-AUTH-4) handleProviderAuth liefert '' bei einem nicht aufloesbaren Login (z.B. kein Account + Signup disabled). Bisher erfolgte dann kein Redirect und next() lief ins Leere -> haengende/leere Antwort. Jetzt fail-closed: explizites 302 auf /login?error=oidc_no_account. Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three small OIDC robustness fixes that surfaced while integrating Huly with Authentik for a production WAC v1 deployment. All three are standalone and have no dependency on the forthcoming WAC v1 stack.
Commits
1.
fix(authProviders): instrument OIDC callback + redirect on fail (not 500)Problem: Every OIDC callback (bogus or real code) returned
500 Internal Server Error, even with valid sessions. No log line was emitted betweentry auth via openidand the 500 — passport-strategy errors threw silently beforehandleProviderAuthcould log them.Fix:
passport.authenticatevariant so strategy errors surface aserr/info/statusargs instead of being swallowed.302 /login?error=oidc_callback_failed, never 500. Same wrap covershandleProviderAuththrows.2.
fix(account): admin.ts trim+lowercase on both env-set AND lookupProblem:
server/account/src/admin.tsbuilt theADMIN_EMAILSset without normalizing, ANDisAdminEmail()didn't normalize the input. Today's value happens to be clean, but a future env value likeADMIN_EMAILS=" Michael@Uray.io "would silently break admin detection (Set lookup would miss the lowercased input).The Mongo backend already has this hardening (
server/account/src/collections/mongo.ts:1141-1147). Postgres + this helper didn't.Fix: Both sides normalize. Env split → trim → toLowerCase. Entries without
@are kept for backwards compatibility with deployments that use login-id style ADMIN_EMAILS values (e.g.ADMIN_EMAILS=admin,...). A warn is emitted listing the kept-but-malformed entries. Tests cover whitespace, case-insensitive env, case-insensitive lookup, kept-with-warn for no-@ entries, empty/unset env, multi-entry.3.
chore(authProviders): drop cookieHeaderLen from OIDC diagnosticsRationale:
hasCookieHeader(boolean) is sufficient to diagnose missing session-cookie issues; thecookieHeaderLenbyte count adds no operational value and is one more PII-adjacent surface. Removed from both diagnostic paths inpods/authProviders/src/openid.ts.Out of scope
The OIDC name-split heuristic for IdPs that violate OIDC §5.1 (
given_name = full_name) ships separately in #10919 — Authentik will fix this upstream in v2026.8 (PR #21544), so the Huly-side patch remains defense-in-depth.DCO
All three commits Signed-off-by Michael Uray.
maintainer_can_modify=true.