fix(auth,db): avoid OIDC login panic and Close on uninitialized store

[cursor]

Summary

Fixes two server-crash bugs found during daily critical-bug inspection.

Bug 1: OIDC UserInfo nil dereference

Impact: Server process panic during OIDC login when the provider returns no id_token and the UserInfo endpoint errors.

Root cause: In oidcRedirect, claim-building code accessed userInfo.Profile outside the if err == nil block, so a failed UserInfo fetch left userInfo nil and triggered a nil pointer dereference.

Fix: Extract oidcClaimsFromUserInfo() that returns early on UserInfo error before touching userInfo.

Trigger scenario: Configure an OIDC provider that does not include id_token in the token response and whose UserInfo endpoint is unreachable or returns an error. Any login attempt crashes the Semaphore server.

Bug 2: SqlDb Close on uninitialized connection

Impact: Panic on shutdown when database connection was never fully initialized.

Root cause: SqlDbConnection.Close() guarded d.sql.Db == nil but not d.sql == nil. If Connect() panics before gorp is initialized, defer store.Close() (e.g. in semaphore setup) still dereferences a nil d.sql.

Fix: Guard with d.sql == nil || d.sql.Db == nil.

Trigger scenario: Run semaphore setup with an invalid DB config; connection fails before gorp init, deferred Close() panics.

Validation

• go test ./api -run TestOidcClaimsFromUserInfo -count=1 — pass
• go test ./db/sql -run 'TestSqlDb.*CloseWithoutConnect' -count=1 — pass

Notes

Recent commits since last inspection (cd314c1d, 742e8761) are CI/GPG-only with no behavioral risk. Workflow migration data-loss issue (2.19.11 per-node inventory/env overrides) remains open on develop.

  

---

Note

Medium Risk
Touches authentication claim handling and DB lifecycle on failure paths; changes are narrow guards/refactors with tests, but login and shutdown are sensitive.

Overview
Fixes two server panic paths: OIDC login when there is no id_token and UserInfo fails, and database shutdown when the store was never connected.

For OIDC, claim building when relying on UserInfo is moved into oidcClaimsFromUserInfo, which returns the UserInfo error before reading userInfo (the old flow still touched userInfo.Profile after a failed fetch). oidcRedirect now calls that helper instead of inlined logic.

For the DB layer, SqlDbConnection.Close now no-ops when d.sql is nil as well as when Db is nil, so deferred Close() after a failed Connect() (e.g. setup with bad config) does not crash.

Tests cover UserInfo error/success claim paths and closing without connect.

^{Reviewed by Cursor Bugbot for commit 73186e3. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Security review

Outcome: No medium, high, or critical vulnerabilities identified in this PR.

Scope reviewed:

• api/login.go — oidcClaimsFromUserInfo refactor that returns early on UserInfo fetch errors instead of dereferencing a nil userInfo pointer.
• db/sql/SqlDb.go — nil guard in SqlDbConnection.Close() when the store was never connected.

Assessment:

• The OIDC change is a behavioral-preserving panic fix on the UserInfo error path. Successful-login claim extraction (email, sub, emailVerified, username/name assignment) matches the prior logic; failed UserInfo responses now surface as HTTP 502 instead of crashing the handler.
• Attacker-controlled OIDC inputs still pass through existing controls: OAuth state/CSRF validation, authorization-code exchange, ID-token verification (when present), and resolveExternalUser identity/email-matching gates.
• The database Close() guard only prevents a nil-pointer panic on shutdown/cleanup; it does not weaken authz or expose data.

No prior automation review threads required resolution.

  

_{Sent by Cursor Automation: Find vulnerabilities}

[Copilot]

Pull request overview

Fixes two crash-on-error-path bugs in the Semaphore server: one in the OIDC login flow when UserInfo retrieval fails, and one in the SQL DB store shutdown path when a connection was never initialized.

Changes:

• Refactors OIDC claim construction into oidcClaimsFromUserInfo() to avoid nil dereference after a UserInfo error, and wires oidcRedirect to use it.
• Hardens SqlDbConnection.Close() to no-op safely when the gorp DbMap was never initialized.
• Adds targeted unit tests covering both failure-path regressions.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
db/sql/SqlDb.go	Adds a nil guard in Close() to prevent panics on partially initialized connections.
db/sql/SqlDb_test.go	Adds tests ensuring Close() does not panic when never connected.
api/login.go	Introduces oidcClaimsFromUserInfo() and updates oidcRedirect to use it, preventing OIDC login panics.
api/login_test.go	Adds tests validating claim-building behavior when UserInfo fails and when it contains email/profile fields.