feat(webserver): add OIDC authentication - #263
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded configurable OpenID Connect authentication for the Web UI, including configuration, provider discovery, PKCE, callback validation, account provisioning, session creation, password-login restrictions, UI support, documentation, and integration tests. ChangesOIDC authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant WebServer
participant oidcService
participant LocalAuth
Browser->>WebServer: Start OIDC login
WebServer->>oidcService: Generate state and authorization URL
oidcService-->>Browser: Redirect to provider
Browser->>WebServer: Return with authorization code
WebServer->>oidcService: Exchange code and verify claims
oidcService-->>WebServer: Verified identity
WebServer->>LocalAuth: Resolve or provision account
WebServer-->>Browser: Set session and redirect
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/webserver/routes_oidc.go (1)
131-163: 🔒 Security & Privacy | 🔵 TrivialNote the trust model: any valid provider identity gets full local access.
resolveOIDCAccountdeliberately maps every successfully-verified OIDC identity onto upbrr's single local account, regardless of whether the username claim matches — by design, authorization is delegated entirely to the identity provider. Worth calling out for operators: the IdP application/client used here must be scoped to only trusted users, since any account able to complete the flow against the configured client gains full access to the single local account.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/webserver/routes_oidc.go` around lines 131 - 163, Update the OIDC authentication documentation or operator-facing configuration guidance associated with resolveOIDCAccount to explicitly state that every successfully verified provider identity receives full access to the single local account, regardless of username matching. Warn operators to restrict the configured IdP application/client to trusted users, while preserving the existing identity-to-local-account mapping behavior.internal/webserver/oidc.go (1)
20-29: 🩺 Stability & Availability | 🔵 TrivialRace-enabled test coverage for the new mutex-protected state.
This file introduces two mutexes (
mu,flowMu) guarding shared state accessed from concurrent HTTP handlers. Worth running the package's tests with-raceto confirm no data races in the discovery-cache and flow-store paths.As per coding guidelines, "Run focused race-enabled Go tests for touched packages; run broader tests when shared behavior or regressions are plausible."
Also applies to: 58-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/webserver/oidc.go` around lines 20 - 29, Run the focused Go tests for the OIDC webserver package with the race detector enabled, covering the mutex-protected discovery-cache and flow-store paths guarded by mu and flowMu. Address any races reported by the tests before completing the change.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/webserver/oidc.go`:
- Around line 85-123: Add an explicit, bounded timeout for the discovery
operation in ensureProvider by deriving a child context with the service’s
configured discovery timeout before calling oidc.NewProvider. Ensure the timeout
context is canceled after discovery and preserve the existing request context
propagation and error handling.
---
Nitpick comments:
In `@internal/webserver/oidc.go`:
- Around line 20-29: Run the focused Go tests for the OIDC webserver package
with the race detector enabled, covering the mutex-protected discovery-cache and
flow-store paths guarded by mu and flowMu. Address any races reported by the
tests before completing the change.
In `@internal/webserver/routes_oidc.go`:
- Around line 131-163: Update the OIDC authentication documentation or
operator-facing configuration guidance associated with resolveOIDCAccount to
explicitly state that every successfully verified provider identity receives
full access to the single local account, regardless of username matching. Warn
operators to restrict the configured IdP application/client to trusted users,
while preserving the existing identity-to-local-account mapping behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f923231b-af6b-4e94-977e-89471f5a4cd3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
README.mdcmd/upbrr/main.gogo.modgui/frontend/src/styles.cssgui/frontend/src/webRoot.tsxinternal/webserver/cli_config.gointernal/webserver/oidc.gointernal/webserver/oidc_test.gointernal/webserver/routes.gointernal/webserver/routes_oidc.gointernal/webserver/runtime_snapshot.gointernal/webserver/server.go
adf1177 to
2c63472
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/upbrr/main.go (1)
402-404: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact queued-item errors before logging.
Line 403 logs arbitrary workflow errors unchanged; provider/API payloads can contain secrets. Sanitize the error before
logger.Errorf.Proposed fix
- logger.Errorf("queue: %q failed, continuing with remaining items: %v", sourcePath, err) + logger.Errorf("queue: %q failed, continuing with remaining items: %s", sourcePath, logging.SanitizeMessage(err.Error()))As per coding guidelines, “Redact every free-form status or error field before logging.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/upbrr/main.go` around lines 402 - 404, Sanitize the error used by the logger.Errorf call in the queue-processing failure path before logging it, ensuring arbitrary provider or API payloads are redacted while preserving the existing sourcePath context and continuation behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/upbrr/main.go`:
- Around line 770-772: Update the OIDC client-secret handling around
oidcCfg.ClientSecret and normalizeCLIConfigLoaded so secrets remain verbatim
across environment, flag, and persisted-config loading paths. Remove trimming or
normalization for this opaque credential while preserving existing handling for
other configuration values.
---
Outside diff comments:
In `@cmd/upbrr/main.go`:
- Around line 402-404: Sanitize the error used by the logger.Errorf call in the
queue-processing failure path before logging it, ensuring arbitrary provider or
API payloads are redacted while preserving the existing sourcePath context and
continuation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8700ccbc-2c5e-448f-9b69-da3e87a9ccd5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
README.mdcmd/upbrr/main.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
| if visited["oidc-client-secret"] { | ||
| oidcCfg.ClientSecret = strings.TrimSpace(env.OIDCClientSecret) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline cmd/upbrr/main.go --view expanded | sed -n '1,220p'
else
echo "ast-grep unavailable"
fi
echo "== relevant lines =="
sed -n '730,800p' cmd/upbrr/main.go | cat -n | sed 's/^/cmd/upbrr/main.go:/'
echo "== oidc env/config parsing references =="
rg -n "OIDCClientSecret|oidc-client-secret|ClientSecret|InteractionMode|unattended" cmd/upbrr/main.go internal cmd pkg 2>/dev/null | sed -n '1,240p'Repository: autobrr/upbrr
Length of output: 5829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant main.go lines =="
NL="$(printf '\n')"
sed -n '745,785p' cmd/upbrr/main.go > /tmp/oidc_section.txt
n=0
while IFS= read -r line; do
n=$((n+1))
echo "$((744+n)):$line"
done < /tmp/oidc_section.txt
echo "== serve env parsing =="
sed -n '650,715p' cmd/upbrr/main.go > /tmp/srvparse.txt
n=0
while IFS= read -r line; do
n=$((n+1))
echo "$((649+n)):$line"
done < /tmp/srvparse.txt
echo "== command setup flags for oidc-client-secret =="
sed -n '528,630p' cmd/upbrr/main.go > /tmp/cmdsetup.txt
n=0
while IFS= read -r line; do
n=$((n+1))
echo "$((527+n)):$line"
done < /tmp/cmdsetup.txt
echo "== OIDC env/config references =="
rg -n "OIDCClientSecret|oidc-client-secret|ClientSecret" cmd/upbrr/main.go internal webserver config 2>/dev/null | sed -n '1,240p'
echo "== behavior probe for trimming whitespace variants =="
python3 - <<'PY'
import string
samples = [
"", "abc", " abc", "abc ", " \t abc \n\r", "abc\tdef", " abc\tdef ",
]
for s in samples:
print(repr(s), "trim=", repr(s.strip()), "changed=", s != s.strip())
PYRepository: autobrr/upbrr
Length of output: 9443
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== webserver/cli_config.go =="
cat -n internal/webserver/cli_config.go | sed -n '30,175p'
echo "== parseServeOptions and option handling =="
rg -n "func parseServeOptions|oidc-client-secret|CLIOIDCClientSecret|Flag|Value" cmd/upbrr/main.go | sed -n '1,220p'
echo "== search TrimSpaces on ClientSecret across repo =="
rg -n "ClientSecret|client_secret|ClientSecret =" --glob '*.go' | sed -n '1,260p'Repository: autobrr/upbrr
Length of output: 8679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== parseServeOptions implementation =="
sed -n '335,425p' cmd/upbrr/main.go > /tmp/parseopts.txt
n=0
while IFS= read -r line; do
n=$((n+1))
echo "$((334+n)):$line"
done < /tmp/parseopts.txt
echo "== servePersistConfig implementation =="
sed -n '805,830p' cmd/upbrr/main.go > /tmp/persist.txt
n=0
while IFS= read -r line; do
n=$((n+1))
echo "$((804+n)):$line"
done < /tmp/persist.txt
echo "== all OIDC ClientSecret trimming assignments =="
python3 - <<'PY'
import pathlib, re
for path in pathlib.Path('.').rglob('*.go'):
try:
txt = path.read_text(errors='ignore')
except Exception:
continue
if 'ClientSecret' not in txt or 'TrimSpace' not in txt and 'TrimSpace' not in ''.join(['']):
continue
for i,line in enumerate(txt.splitlines(),1):
if 'ClientSecret' in line and ('TrimSpace' in line or '= value' in line or '.OIDC.ClientSecret' in line):
print(f"{path}:{i}:{line.strip()}")
PY
echo "== behavior probe for whitespace-sensitive secret changes =="
python3 - <<'PY'
samples = ["abc", " abc", "abc ", " \t abc \n\r", "abc\tdef", "secret\t", "\tsecret"]
for s in samples:
print(repr(s), "trim=", repr(s.strip()), "changed=", s != s.strip(), "empty_after_trim=", s.strip() == "")
PYRepository: autobrr/upbrr
Length of output: 5361
Preserve the OIDC client secret across loading paths.
ClientSecret is currently trimmed from environment variables and during normalizeCLIConfigLoaded, so a registered secret such as " secret " will change and can break token exchange. Keep the secret verbatim for the opaque credential fields from env, flag, and persisted config.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/upbrr/main.go` around lines 770 - 772, Update the OIDC client-secret
handling around oidcCfg.ClientSecret and normalizeCLIConfigLoaded so secrets
remain verbatim across environment, flag, and persisted-config loading paths.
Remove trimming or normalization for this opaque credential while preserving
existing handling for other configuration values.
2c63472 to
4b0ee57
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
webui/src/styles.css (1)
1973-1983: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a visible focus state for the SSO link.
In SSO-only mode this anchor is the only interactive control, and the default focus ring against the gradient on a dark card is easy to miss.
♻️ Suggested addition
text-decoration: none; cursor: pointer; } + +.web-auth-shell .web-auth-card__sso:hover { + filter: brightness(1.08); +} + +.web-auth-shell .web-auth-card__sso:focus-visible { + outline: 2px solid `#fdba74`; + outline-offset: 2px; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webui/src/styles.css` around lines 1973 - 1983, Add a clearly visible :focus or :focus-visible state for .web-auth-shell .web-auth-card__sso, using an outline or equivalent high-contrast focus indicator that remains distinguishable against the gradient and dark card background. Preserve the existing link styling and interaction behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/webserver/oidc_test.go`:
- Around line 77-88: Update the OIDC test setup so signIDToken no longer calls
t.Fatalf from the /token handler goroutine; sign the token in the test goroutine
via an error-returning helper and have the handler return an HTTP error if
signing fails. Protect shared nonce and lastCodeVerifier accesses with one
mutex, including their reads and writes in the relevant handlers and test
assertions, so the test passes cleanly under -race.
In `@internal/webserver/routes_oidc.go`:
- Around line 145-152: Update resolveOIDCAccount so that when provisioning after
!exists fails because another concurrent login created the account, it falls
back to loading the existing account and returns that result instead of
propagating the bootstrap error. Preserve the current provisioning path for
successful creation and propagate unrelated errors.
In `@README.md`:
- Around line 191-200: Update the OIDC configuration example near the
environment variables to document the required `/upbrr/` path prefix for
reverse-proxy deployments. Add a prefixed callback URI example or explicitly
state that UPBRR_WEB_OIDC_REDIRECT_URL must include the proxy prefix, while
preserving the existing root-path example.
---
Nitpick comments:
In `@webui/src/styles.css`:
- Around line 1973-1983: Add a clearly visible :focus or :focus-visible state
for .web-auth-shell .web-auth-card__sso, using an outline or equivalent
high-contrast focus indicator that remains distinguishable against the gradient
and dark card background. Preserve the existing link styling and interaction
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 790d9b51-6222-4a38-b260-5da18aae00fa
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
README.mdcmd/upbrr/main.gogo.modinternal/webserver/cli_config.gointernal/webserver/oidc.gointernal/webserver/oidc_test.gointernal/webserver/routes.gointernal/webserver/routes_oidc.gointernal/webserver/runtime_snapshot.gointernal/webserver/server.gowebui/src/styles.csswebui/src/webRoot.tsx
Add OpenID Connect sign-in to the embedded web UI, so upbrr can sit behind an identity provider (Authentik, Keycloak, Authelia, ...) instead of relying only on its local username and password. The implementation mirrors autobrr's own OIDC support (coreos/go-oidc/v3 + x/oauth2, matching config vocabulary) so operators running both meet the same settings. Design notes: - OIDC is additive by default. `disable_built_in_login` opts in to SSO-only, and is enforced on the server: both /api/auth/login and /api/auth/bootstrap refuse, rather than the form merely being hidden in the UI. Configuring it without OIDC enabled is rejected, since that would leave no way to sign in. - A successful login mints exactly the session a password login does, so nothing downstream changes. - upbrr is single-user: an OIDC login resolves to the existing local account rather than a second identity, because that account's username and encryption key seed derive the key protecting stored tracker credentials. On a fresh SSO-only install the first login provisions that account, which keeps the first-run setup form closed to unauthenticated callers. - The authorization flow uses state (bound to the browser by cookie), a nonce, and PKCE S256 when the provider advertises it. States are single-use. - Provider discovery is lazy and retried, so an identity provider that is down or starts after upbrr cannot prevent upbrr from starting. Settings are available via `web-config.json` and `UPBRR_WEB_OIDC_*` env vars.
Discovery ran under the service mutex with no deadline. A provider that
accepted the connection but never answered would therefore hold a login open
indefinitely, and — because discovery is serialized — park every concurrent
login behind the lock.
Bound discovery, token exchange, and ID token verification. The timeout lives
on the service and is carried by its HTTP client rather than by a context
alone: go-oidc refreshes the key set on its own background context, where a
deadline of ours would never apply, so only a client timeout covers that path.
Verified against go-oidc v3.19.0, whose Verifier() documents the background
context and propagates the provider's client to the key set.
Add a regression test that parks a provider mid-request and asserts the call
fails rather than hangs. It passes a context with no deadline on purpose: a
test context with one would pass against the unbounded implementation too, and
prove nothing. Confirmed the test fails ("discovery is not bounded") when the
bound is removed.
Also document four functions flagged by review coverage.
4b0ee57 to
fa47e46
Compare
What
Adds OpenID Connect sign-in to the embedded web UI, so upbrr can sit behind an identity provider (Authentik, Keycloak, Authelia, Pocket ID, Zitadel, …) rather than relying only on its local username and password.
Why
Today the only way to put upbrr behind an SSO stack is a forward-auth proxy in front of it, which leaves you logging in twice: once at the proxy, once at upbrr.
--dev-no-authis the only way to skip the second login and it (correctly) refuses to bind to anything but loopback, so it isn't an answer for a containerised deployment.Approach
Mirrors autobrr's own OIDC support as closely as makes sense: same library (
coreos/go-oidc/v3+x/oauth2) and the same config vocabulary (oidcEnabled,oidcIssuer,oidcClientId,oidcClientSecret,oidcRedirectUrl,oidcScopes,oidcDisableBuiltInLogin), so anyone running both projects meets the same settings twice. Settings are available inweb-config.jsonand asUPBRR_WEB_OIDC_*env vars, following the existingUPBRR_WEB_*precedence rules.Two routes:
GET /api/auth/oidc/loginstarts the flow,GET /api/auth/oidc/callbackcompletes it and mints exactly the session a password login mints, so nothing downstream changes.Design decisions worth your review
Single-user account mapping. upbrr stores one account, and that account's username +
encryption_key_seedderive the key protecting stored tracker credentials. So a successful OIDC login resolves to the existing local record rather than creating a parallel identity — otherwise stored tracker secrets would stop decrypting. Authorization is delegated to the provider: restrict who may use the application there. On a fresh SSO-only install the first login provisions the record (with a random, never-disclosed password) purely so it carries the seed.disableBuiltInLoginis enforced server-side, not just in the UI. Both/api/auth/loginand/api/auth/bootstrapreturn 403. Closing bootstrap matters: leaving it open on an SSO-only deployment would let an unauthenticated caller claim the account through the trust-on-first-use window. SettingdisableBuiltInLoginwithoutenabledis rejected at config load, since it would leave no way to sign in at all.Lazy provider discovery. Discovery happens on first use and is retried, not performed once at startup. An IdP that is briefly unreachable, or that boots after upbrr, shouldn't stop upbrr from starting. (This is the one place I deliberately diverge from autobrr.)
Flow hardening. State is bound to the browser with an
HttpOnlycookie and compared in constant time; nonce is verified against the ID token; PKCE S256 is used automatically when the provider advertises it; states are single-use, so an authorization code can't be replayed.Testing
12 new tests in
internal/webserverrun against a fake OpenID provider (real RSA-signed ID tokens + JWKS, so signature/nonce verification is genuinely exercised rather than stubbed):disableBuiltInLoginblocks both password routes and leaves no account behind/api/auth/statusadvertises the flags the login page keys offFull local run against the current tree:
go test -race ./...(109 packages, all green),make lint(architecture / path / literal policies +workflow-contracts-check+ golangci-lint v2.12.2, 0 issues),make logpolicy,golangci-lint fmt --diffempty, and the frontendlint/lint:dead/lint:style/typecheck/test:unit/format:check.Notes
disableBuiltInLoginunset the login page shows a "Sign in with SSO" button next to the password form, so operators can verify SSO works before they depend on it.internal/redaction.--persist-web-configwill write the client secret toweb-config.json(0600). Documented, with the env-only alternative called out.Rebased onto current
main(post-#273)Rebased past
refactor!: replace Wails with embedded WebUI and durable release workflow API(#273). Nothing about the feature changed; the branch was adapted to the new tree:webui/src/webRoot.tsx(wasgui/frontend/src/webRoot.tsx) now usesapi/client—authClient,initializeWebClient,updateWebCSRFToken,withBasePath— in place of the removedutils/runtimebrowser bridge. The oldisBrowserMode()guard around reading the?oidc_error=code is gone, since with Wails removed this root only ever renders in a browser.handleAuthStatusno longer computesnativeBrowseEnabled, so the OIDC change there is just theneedsSetupline (the setup form must stay closed on an SSO-only deployment).*Serverlog wrappers moved alongsidereplaceRuntimeGenerationinruntime_snapshot.go.go.mod: addedcoreos/go-oidc/v3andx/oauth2on top of the new module set.oidc_test.gotable literals expanded one element per line for the newcmd/literalpolicychecker.Two review notes, addressed and not:
resolveOIDCAccountdoc comment now say explicitly that any identity completing the flow against the configured client signs in to the single local account whether or not its username matches, that upbrr authenticates while the provider authorizes, and that there is no narrower permission tier beneath it — so the provider application must be scoped to trusted users.cmd/upbrr/main.go's queue-failurelogger.Errorfis about pre-existingmaincode introduced by refactor!: replace Wails with embedded WebUI and durable release workflow API #273, outside this diff — the bot flagged it as outside the diff range itself. Happy to send it as a separate PR rather than widen this one.Summary by CodeRabbit
web-config.json/UPBRR_WEB_OIDC_*environment variables (including scopes and built-in login disabling), and provider discovery behavior.