Conversation
…ncluding provider settings and routes. Enhance user authentication flow by integrating OIDC login functionality and updating UI components for better user experience.
1 task
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds OpenID Connect (OIDC) SSO support: database schema and migrations, server-side models and admin/public controllers for provider management, OIDC login/callback flows with user provisioning, admin UI and frontend login integration, config entries and i18n strings. Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser as "Browser"
participant Frontend as "Frontend UI"
participant Backend as "App Backend"
participant Cache as "Cache (state/nonce)"
participant OIDC as "OIDC Provider"
participant DB as "Database"
User->>Frontend: Click "Login with [Provider]"
Frontend->>Backend: GET /api/user/auth/oidc/login?provider_uuid
Backend->>DB: SELECT provider config by uuid
Backend->>Cache: store state + nonce (TTL 10m)
Backend->>Frontend: 302 Redirect to OIDC auth endpoint
Frontend->>OIDC: Auth request (client_id, redirect_uri, scope, state, nonce)
OIDC->>User: Prompt for credentials
OIDC->>Frontend: Redirect to callback with code & state
Frontend->>Backend: GET /api/user/auth/oidc/callback?code&state
Backend->>Cache: validate state, retrieve nonce
Backend->>OIDC: POST token endpoint (code, client_secret)
OIDC->>Backend: Return id_token (+ access_token)
Backend->>Backend: Decode/validate id_token (iss, aud, claims)
Backend->>DB: Find user by (oidc_provider, oidc_subject)
alt user exists
Backend->>DB: Update user oidc_email (if present)
else user missing and auto-provision allowed
Backend->>DB: Insert new user from claims
end
Backend->>Backend: completeLogin (session, activity)
Backend->>Frontend: Redirect to dashboard
Backend->>Cache: clear state
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
1 task
1 task
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
backend/app/Controllers/User/Auth/LoginController.php (1)
259-264: Public reuse is fine, but Line 263 carries dead weight.
$requestis unused incompleteLogin(...). Since this method is now public, trim the signature so callers don’t cargo-cult an argument that does nothing.♻️ Suggested cleanup
- public function completeLogin(array $userInfo, Request $request): Response + public function completeLogin(array $userInfo): ResponseAnd update call sites in this file:
- return $this->completeLogin($userInfo, $request); + return $this->completeLogin($userInfo);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/User/Auth/LoginController.php` around lines 259 - 264, The completeLogin method signature includes an unused Request $request parameter; remove that parameter from the public function declaration (public function completeLogin(array $userInfo): Response) and update all call sites within this file that pass a Request to completeLogin to stop passing that argument; also remove any now-unused import/usage of the Request type in this class and run a quick test to ensure no other files rely on the old signature.backend/app/Controllers/User/Auth/OidcController.php (1)
54-55: Tiny cleanup:$configis summoned twice and used zero times.Drop the dead locals to keep the controller signal clean.
Also applies to: 117-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/User/Auth/OidcController.php` around lines 54 - 55, Remove the unused local variable $config in OidcController.php: delete the assignments that set $config = $app->getConfig() (both occurrences around the earlier block and the later one at lines referenced) since $config is never used; keep the existing $providerUuid = $request->query->get('provider') lines intact so behavior is unchanged.backend/app/Chat/OidcProvider.php (1)
46-49: Dynamic SQL column lists are one refactor away from chaos.
createProvider()andupdateProvider()trust raw$datakeys as SQL identifiers. Today’s callers are disciplined; future ones might not be. Add a model-level allowlist for writable columns.🛡️ Proposed hardening
class OidcProvider { private static string $table = 'featherpanel_oidc_providers'; + private const WRITABLE_FIELDS = [ + 'uuid', + 'name', + 'issuer_url', + 'client_id', + 'client_secret', + 'scopes', + 'email_claim', + 'subject_claim', + 'group_claim', + 'group_value', + 'auto_provision', + 'require_email_verified', + 'enabled', + ]; @@ - $fields = array_keys($data); + $fields = array_values(array_intersect(array_keys($data), self::WRITABLE_FIELDS)); + if (empty($fields)) { + return false; + } - $placeholders = array_map(fn ($f) => ':' . $f, $fields); + $placeholders = array_map(fn ($f) => ':' . $f, $fields); + $params = array_intersect_key($data, array_flip($fields)); @@ - if ($stmt->execute($data)) { + if ($stmt->execute($params)) { @@ - $fields = array_keys($data); + $fields = array_values(array_intersect(array_keys($data), self::WRITABLE_FIELDS)); + if (empty($fields)) { + return false; + }Also applies to: 69-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Chat/OidcProvider.php` around lines 46 - 49, The INSERT/UPDATE builders in OidcProvider::createProvider and OidcProvider::updateProvider currently trust raw $data keys as SQL identifiers; add a model-level allowlist (e.g., a private static array like $writable = [...]) and filter incoming $data keys against that list before building $fields and $placeholders/SET clauses, rejecting or ignoring any keys not in the allowlist and throwing or returning an error if no valid writable fields remain; apply the same whitelist filtering logic to both createProvider and updateProvider so only permitted columns are used to prepare the SQL.frontendv2/src/app/(app)/auth/login/LoginForm.tsx (1)
216-216:showLocalLoginis currently ceremonial — wire it or drop it.Line 216 hardcodes
showLocalLogin = true, so the branch never really branches. Either connect it to a real setting/feature flag or inline the form rendering and keep the codepath less theatrical.Also applies to: 248-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontendv2/src/app/`(app)/auth/login/LoginForm.tsx at line 216, The localLogin toggle is hardcoded—either wire showLocalLogin to a real feature flag/prop or remove it and inline the local login UI; update the LoginForm component to read a real source (e.g., a feature flag hook like useFeatureFlag('localLogin'), an auth config prop, or an env-based setting) instead of const showLocalLogin = true, then adjust the conditional rendering around the local-login branch (the JSX currently guarded by showLocalLogin between the local login and alternative flows) so it actually toggles, or delete the variable and merge the chosen UI path directly into the component.frontendv2/src/app/(app)/admin/oidc-providers/page.tsx (1)
119-139: Add client-side required-field guards before firing save requests.Right now the form can submit empty required fields (and empty
client_secreton create), then just eats a backend rejection. Cheap fix, better UX, fewer doomed requests.🛠️ Suggested guard
const handleSave = async () => { if (!editing) return; + const missingRequired = + editing.name.trim() === '' || + editing.issuer_url.trim() === '' || + editing.client_id.trim() === '' || + (!editing.uuid && clientSecret.trim() === ''); + if (missingRequired) { + toast.error(t('validation.fill_all_fields')); + return; + } setSaving(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontendv2/src/app/`(app)/admin/oidc-providers/page.tsx around lines 119 - 139, handleSave currently fires without validating required fields so empty name/issuer_url/client_id (and the client_secret on create) can be submitted; add client-side guards at the start of handleSave to validate editing.name, editing.issuer_url, editing.client_id are non-empty and, if creating (e.g., no editing.id or a boolean/new flag), require clientSecret too; on validation failure return early, call setSaving(false) and surface an error to the user (via existing form error state or toast) instead of sending the request. Ensure these checks reference handleSave, payload, clientSecret and editing so reviewers can locate and update the existing logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/Config/PublicConfig.php`:
- Around line 123-130: The LoginController::put() currently allows
username/password auth without checking
ConfigInterface::OIDC_DISABLE_LOCAL_LOGIN; add an early guard in the put()
method that reads ConfigInterface::OIDC_DISABLE_LOCAL_LOGIN and if true rejects
non-admin local logins (throw HTTP 403 or the controller's auth exception)
before validating credentials or 2FA. Use the existing config access mechanism
in the controller (the same service/var used to read public config) and the
controller's user/admin check (e.g., User::isAdmin or the project’s isAdmin role
check) so admins can still login; keep the error response consistent with other
auth errors.
In `@backend/app/Controllers/Admin/OidcProvidersController.php`:
- Around line 62-67: Responses from index, create, and update are returning
client_secret in cleartext; update the controller to never include client_secret
in API responses by filtering it out before calling ApiResponse::success (e.g.,
map the array/collection returned by OidcProvider::getAllProviders() and remove
or nullify the client_secret field) and ensure any create/update logic that
currently returns the newly persisted provider object also strips client_secret
(apply the same filter to the create and update handlers). Use the existing
symbols index, create, update, OidcProvider::getAllProviders(), and
ApiResponse::success to locate and modify the return payloads.
In `@backend/app/Controllers/User/Auth/OidcController.php`:
- Around line 220-225: The current OidcController email check allows
linking/logins when email isn't verified; only proceed when the claim is
explicitly truthy. Update the checks around $requireEmailVerified and
$claims['email_verified'] so that any path which links or authenticates an
existing local account (the block using $emailVerified at the shown check and
the similar logic at lines 243-251) only continues if $emailVerified is strictly
truthy (e.g., === true or === 'true'); otherwise abort/redirect (e.g., return
new RedirectResponse('/auth/login?error=oidc_email_not_verified')). Ensure the
same strict-truthy check is used in both the initial gating block and the later
linking logic to prevent unverified-email linking.
- Around line 130-136: The OidcController currently reads claims via
decodeJwtWithoutVerification() and trusts nonce/exp without signature
validation; replace that flow by fetching the provider's JWKS (from provider
metadata) and performing full JWT signature verification and standard OIDC claim
checks (iss, aud, exp, nbf, iat) before using any claims; specifically, stop
using decodeJwtWithoutVerification(), verify the ID token signature against the
provider keys, compare the token nonce to the cached['nonce'] retrieved earlier
(and only then call Cache::forget('oidc_state_' . $state') on success or ensure
cleanup on failure), and reject the token if expiry/issuer/audience/nonce
validation fails. Ensure this same verification is applied in the other
callback-handling locations flagged (the other OIDC sections around where
decodeJwtWithoutVerification() is used).
- Around line 172-177: The outbound curl call in OidcController (the
$ch/curl_init($tokenEndpoint) block that sets CURLOPT_POSTFIELDS and reads
$tokenResponse) lacks connect/read timeouts and proper transport/error checks;
update the request to set CURLOPT_CONNECTTIMEOUT (e.g. 5s) and CURLOPT_TIMEOUT
(e.g. 10s) or the millisecond variants, enable CURLOPT_FAILONERROR, and after
curl_exec() check curl_errno()/curl_error() and the HTTP status via
curl_getinfo($ch, CURLINFO_HTTP_CODE); handle non-2xx responses by
logging/throwing a clear error and ensure curl_close($ch) always runs (use a
finally-style flow), and apply the same changes to the other similar curl block
(lines around 358-370) so both discovery/token requests are resilient to flaky
IdPs.
In `@backend/app/routes/user/auth.php`:
- Around line 193-201: The new unauthenticated OIDC login route ('oidc-login'
registered via registerApiRoute) needs server-side rate limiting to prevent
brute-force/stress; add a throttle (e.g., IP-based rate limiter middleware or a
RateLimiter::allow check) around the route handler that calls
OidcController->login(Request) so unauthenticated callers are limited (e.g., few
requests/minute per IP) and return a 429 when exceeded; implement this by either
passing the limiter/middleware into the registerApiRoute call for 'oidc-login'
or by wrapping the inline function to perform the RateLimiter check before
invoking (new OidcController())->login($request).
In `@backend/storage/migrations/2026-02-24.00.00-add-oidc-to-users.sql`:
- Around line 1-4: Add a uniqueness constraint on the external identity pair to
prevent duplicate bindings: alter the featherpanel_users schema to create a
UNIQUE index/constraint on (oidc_provider, oidc_subject). In the migration that
adds oidc_provider and oidc_subject ensure you either deduplicate or fail-fast
on existing duplicate (oidc_provider, oidc_subject) values before creating the
unique index, and make the index creation idempotent for repeated runs;
reference the columns oidc_provider and oidc_subject on table featherpanel_users
when making these changes.
In `@backend/storage/migrations/2026-02-24.00.01-create-oidc-providers-table.sql`:
- Line 7: The migration storing client_secret as plaintext must be replaced with
an encrypted binary column and the app must encrypt/decrypt secrets in the
controllers: change the `client_secret` TEXT column to a binary type (e.g.,
BYTEA) and update DB schema accordingly; implement an encryption helper (e.g.,
Encryptor::encryptClientSecret and Encryptor::decryptClientSecret) that uses a
configured master key (libsodium/openssl with AES-GCM or libsodium secretbox)
and call Encryptor::encryptClientSecret in OidcProvidersController (where input
is currently inserted around the code referenced at OidcProvidersController.php
line ~100) before saving, and call Encryptor::decryptClientSecret in
OidcController where the secret is read (around OidcController.php line ~149);
ensure keys come from secure config, add error handling for decryption failures,
and update any DB read/write code to treat the column as binary.
In `@frontendv2/public/locales/en.json`:
- Line 2665: The translation key admin.oidcProviders.deleteConfirm uses the
wrong interpolation syntax; replace the double-brace placeholder "{{name}}" with
the i18next-compatible single-brace "{name}" in the JSON value for
"deleteConfirm" so that t('admin.oidcProviders.deleteConfirm', { name:
provider.name }) correctly interpolates the provider name.
In `@frontendv2/src/app/`(app)/admin/oidc-providers/page.tsx:
- Around line 55-59: The API is returning full OIDC records (including
client_secret) from getAllProviders(), which OidcProvidersController::index()
forwards to the frontend; update the backend so client_secret is never sent to
the browser by either modifying OidcProvider::getAllProviders() to select
explicit columns (e.g., uuid, name, issuer, client_id, disabled, etc.) omitting
client_secret or by filtering/mapping the result in
OidcProvidersController::index() to delete/omit client_secret from each provider
object before building the JSON response (mirror the explicit-column pattern
used in OidcProvider::getEnabledProviders()).
---
Nitpick comments:
In `@backend/app/Chat/OidcProvider.php`:
- Around line 46-49: The INSERT/UPDATE builders in OidcProvider::createProvider
and OidcProvider::updateProvider currently trust raw $data keys as SQL
identifiers; add a model-level allowlist (e.g., a private static array like
$writable = [...]) and filter incoming $data keys against that list before
building $fields and $placeholders/SET clauses, rejecting or ignoring any keys
not in the allowlist and throwing or returning an error if no valid writable
fields remain; apply the same whitelist filtering logic to both createProvider
and updateProvider so only permitted columns are used to prepare the SQL.
In `@backend/app/Controllers/User/Auth/LoginController.php`:
- Around line 259-264: The completeLogin method signature includes an unused
Request $request parameter; remove that parameter from the public function
declaration (public function completeLogin(array $userInfo): Response) and
update all call sites within this file that pass a Request to completeLogin to
stop passing that argument; also remove any now-unused import/usage of the
Request type in this class and run a quick test to ensure no other files rely on
the old signature.
In `@backend/app/Controllers/User/Auth/OidcController.php`:
- Around line 54-55: Remove the unused local variable $config in
OidcController.php: delete the assignments that set $config = $app->getConfig()
(both occurrences around the earlier block and the later one at lines
referenced) since $config is never used; keep the existing $providerUuid =
$request->query->get('provider') lines intact so behavior is unchanged.
In `@frontendv2/src/app/`(app)/admin/oidc-providers/page.tsx:
- Around line 119-139: handleSave currently fires without validating required
fields so empty name/issuer_url/client_id (and the client_secret on create) can
be submitted; add client-side guards at the start of handleSave to validate
editing.name, editing.issuer_url, editing.client_id are non-empty and, if
creating (e.g., no editing.id or a boolean/new flag), require clientSecret too;
on validation failure return early, call setSaving(false) and surface an error
to the user (via existing form error state or toast) instead of sending the
request. Ensure these checks reference handleSave, payload, clientSecret and
editing so reviewers can locate and update the existing logic.
In `@frontendv2/src/app/`(app)/auth/login/LoginForm.tsx:
- Line 216: The localLogin toggle is hardcoded—either wire showLocalLogin to a
real feature flag/prop or remove it and inline the local login UI; update the
LoginForm component to read a real source (e.g., a feature flag hook like
useFeatureFlag('localLogin'), an auth config prop, or an env-based setting)
instead of const showLocalLogin = true, then adjust the conditional rendering
around the local-login branch (the JSX currently guarded by showLocalLogin
between the local login and alternative flows) so it actually toggles, or delete
the variable and merge the chosen UI path directly into the component.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backend/app/Chat/OidcProvider.phpbackend/app/Chat/User.phpbackend/app/Config/ConfigInterface.phpbackend/app/Config/PublicConfig.phpbackend/app/Controllers/Admin/OidcProvidersController.phpbackend/app/Controllers/Admin/SettingsController.phpbackend/app/Controllers/System/OidcPublicController.phpbackend/app/Controllers/User/Auth/LoginController.phpbackend/app/Controllers/User/Auth/OidcController.phpbackend/app/routes/admin/oidc.phpbackend/app/routes/system/oidc.phpbackend/app/routes/user/auth.phpbackend/storage/migrations/2026-02-24.00.00-add-oidc-to-users.sqlbackend/storage/migrations/2026-02-24.00.01-create-oidc-providers-table.sqlfrontendv2/public/locales/en.jsonfrontendv2/src/app/(app)/admin/oidc-providers/page.tsxfrontendv2/src/app/(app)/auth/login/LoginForm.tsxfrontendv2/src/config/navigation.tsxfrontendv2/src/types/settings.ts
Comment on lines
130
to
136
| $cached = Cache::get('oidc_state_' . $state); | ||
| if (!is_array($cached) || !isset($cached['nonce'], $cached['provider_uuid'])) { | ||
| return new RedirectResponse('/auth/login?error=oidc_invalid_state'); | ||
| } | ||
|
|
||
| Cache::forget('oidc_state_' . $state); | ||
|
|
Contributor
There was a problem hiding this comment.
Stop trusting unsigned ID tokens like they’re gospel.
The callback path consumes claims from decodeJwtWithoutVerification() and never validates nonce or token expiry. That guts OIDC’s core security guarantees.
🧨 Minimum hardening sketch
- $claims = $this->decodeJwtWithoutVerification($idToken);
+ // Verify ID token signature (JWKS), then parse claims.
+ $claims = $this->verifyAndDecodeIdToken(
+ $idToken,
+ (string) ($discovery['jwks_uri'] ?? ''),
+ $issuerUrl,
+ $clientId
+ );
if (!is_array($claims)) {
return new RedirectResponse('/auth/login?error=oidc_invalid_id_token');
}
+
+ if (!isset($claims['exp']) || !is_numeric($claims['exp']) || time() >= (int) $claims['exp']) {
+ return new RedirectResponse('/auth/login?error=oidc_token_expired');
+ }
+
+ if (!isset($claims['nonce']) || !is_string($claims['nonce']) || !hash_equals((string) $cached['nonce'], $claims['nonce'])) {
+ return new RedirectResponse('/auth/login?error=oidc_invalid_nonce');
+ }Also applies to: 185-203, 379-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/Controllers/User/Auth/OidcController.php` around lines 130 - 136,
The OidcController currently reads claims via decodeJwtWithoutVerification() and
trusts nonce/exp without signature validation; replace that flow by fetching the
provider's JWKS (from provider metadata) and performing full JWT signature
verification and standard OIDC claim checks (iss, aud, exp, nbf, iat) before
using any claims; specifically, stop using decodeJwtWithoutVerification(),
verify the ID token signature against the provider keys, compare the token nonce
to the cached['nonce'] retrieved earlier (and only then call
Cache::forget('oidc_state_' . $state') on success or ensure cleanup on failure),
and reject the token if expiry/issuer/audience/nonce validation fails. Ensure
this same verification is applied in the other callback-handling locations
flagged (the other OIDC sections around where decodeJwtWithoutVerification() is
used).
Comment on lines
+1
to
+4
| ALTER TABLE `featherpanel_users` | ||
| ADD COLUMN `oidc_provider` VARCHAR(255) NULL DEFAULT NULL AFTER `discord_oauth2_access_token`, | ||
| ADD COLUMN `oidc_subject` TEXT NULL DEFAULT NULL AFTER `oidc_provider`, | ||
| ADD COLUMN `oidc_email` TEXT NULL DEFAULT NULL AFTER `oidc_subject`; |
Contributor
There was a problem hiding this comment.
Missing uniqueness on external identity mapping (auth integrity risk).
Line 2–4 add OIDC identity fields but no unique constraint on (oidc_provider, oidc_subject). That can allow duplicate bindings for the same external account and produce ambiguous logins.
🛡️ Suggested migration hardening
ALTER TABLE `featherpanel_users`
ADD COLUMN `oidc_provider` VARCHAR(255) NULL DEFAULT NULL AFTER `discord_oauth2_access_token`,
- ADD COLUMN `oidc_subject` TEXT NULL DEFAULT NULL AFTER `oidc_provider`,
- ADD COLUMN `oidc_email` TEXT NULL DEFAULT NULL AFTER `oidc_subject`;
+ ADD COLUMN `oidc_subject` VARCHAR(255) NULL DEFAULT NULL AFTER `oidc_provider`,
+ ADD COLUMN `oidc_email` VARCHAR(320) NULL DEFAULT NULL AFTER `oidc_subject`,
+ ADD UNIQUE KEY `featherpanel_users_oidc_identity_unique` (`oidc_provider`, `oidc_subject`),
+ ADD KEY `featherpanel_users_oidc_email_idx` (`oidc_email`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ALTER TABLE `featherpanel_users` | |
| ADD COLUMN `oidc_provider` VARCHAR(255) NULL DEFAULT NULL AFTER `discord_oauth2_access_token`, | |
| ADD COLUMN `oidc_subject` TEXT NULL DEFAULT NULL AFTER `oidc_provider`, | |
| ADD COLUMN `oidc_email` TEXT NULL DEFAULT NULL AFTER `oidc_subject`; | |
| ALTER TABLE `featherpanel_users` | |
| ADD COLUMN `oidc_provider` VARCHAR(255) NULL DEFAULT NULL AFTER `discord_oauth2_access_token`, | |
| ADD COLUMN `oidc_subject` VARCHAR(255) NULL DEFAULT NULL AFTER `oidc_provider`, | |
| ADD COLUMN `oidc_email` VARCHAR(320) NULL DEFAULT NULL AFTER `oidc_subject`, | |
| ADD UNIQUE KEY `featherpanel_users_oidc_identity_unique` (`oidc_provider`, `oidc_subject`), | |
| ADD KEY `featherpanel_users_oidc_email_idx` (`oidc_email`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/storage/migrations/2026-02-24.00.00-add-oidc-to-users.sql` around
lines 1 - 4, Add a uniqueness constraint on the external identity pair to
prevent duplicate bindings: alter the featherpanel_users schema to create a
UNIQUE index/constraint on (oidc_provider, oidc_subject). In the migration that
adds oidc_provider and oidc_subject ensure you either deduplicate or fail-fast
on existing duplicate (oidc_provider, oidc_subject) values before creating the
unique index, and make the index creation idempotent for repeated runs;
reference the columns oidc_provider and oidc_subject on table featherpanel_users
when making these changes.
| `name` VARCHAR(255) NOT NULL, | ||
| `issuer_url` VARCHAR(255) NOT NULL, | ||
| `client_id` VARCHAR(255) NOT NULL, | ||
| `client_secret` TEXT NOT NULL, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for client_secret references in OIDC-related files
echo "=== Searching for client_secret usage ==="
rg -n "client_secret" backend/app/Chat/OidcProvider.php backend/app/Controllers/Admin/OidcProvidersController.php backend/app/Controllers/User/Auth/OidcController.php 2>/dev/null || echo "Files not found or no matches"
echo -e "\n=== Searching for encryption/decryption patterns ==="
rg -n "encrypt|decrypt|openssl|sodium|secretbox|cipher|crypto" backend/app/Chat/OidcProvider.php backend/app/Controllers/Admin/OidcProvidersController.php backend/app/Controllers/User/Auth/OidcController.php 2>/dev/null || echo "No encryption patterns found"
# Also check if files exist and their structure
echo -e "\n=== Checking if OIDC-related files exist ==="
find backend -name "*OidcProvider*" -o -name "*OidcController*" 2>/dev/null
# Search more broadly for any repository/model handling client_secret
echo -e "\n=== Searching entire backend for client_secret handling ==="
rg -n "client_secret" backend --type php 2>/dev/null | head -30Repository: MythicalLTD/FeatherPanel
Length of output: 2139
Line 7 stores crown-jewel secrets that shouldn't exist in plaintext — and the app doesn't encrypt them at all.
client_secret in TEXT is a dumpster fire when combined with what the codebase actually does: OidcController.php (line 149) reads it straight from the database as a raw string, and OidcProvidersController.php (line 100) shoves the user input directly into the table with zero encryption. No encryption layer exists anywhere—rg found zilch for encrypt, decrypt, openssl, sodium, or any crypto magic.
OAuth client secrets should be treated like nuclear launch codes: encrypted at rest, decrypted only when the OAuth flow demands it, then forgotten. Right now? They're sitting in the database like a Post-it note on a monitor in the middle of the office. This needs encryption-at-rest before any other OIDC work lands.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/storage/migrations/2026-02-24.00.01-create-oidc-providers-table.sql`
at line 7, The migration storing client_secret as plaintext must be replaced
with an encrypted binary column and the app must encrypt/decrypt secrets in the
controllers: change the `client_secret` TEXT column to a binary type (e.g.,
BYTEA) and update DB schema accordingly; implement an encryption helper (e.g.,
Encryptor::encryptClientSecret and Encryptor::decryptClientSecret) that uses a
configured master key (libsodium/openssl with AES-GCM or libsodium secretbox)
and call Encryptor::encryptClientSecret in OidcProvidersController (where input
is currently inserted around the code referenced at OidcProvidersController.php
line ~100) before saving, and call Encryptor::decryptClientSecret in
OidcController where the secret is read (around OidcController.php line ~149);
ensure keys come from secure config, add error handling for decryption failures,
and update any DB read/write code to treat the column as binary.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 6 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on March 6
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| $claims = $this->decodeJwtWithoutVerification($idToken); | ||
| if (!is_array($claims)) { | ||
| return new RedirectResponse('/auth/login?error=oidc_invalid_id_token'); | ||
| } |
There was a problem hiding this comment.
OIDC nonce claim never validated against cached value
High Severity
The OIDC nonce is generated and stored in the cache during login(), and retrieved in the callback() via $cached['nonce'], but it is never actually compared to the nonce claim inside the decoded ID token $claims. Without this check, the nonce serves no purpose, leaving the flow vulnerable to token replay attacks. The nonce from $cached needs to be validated against $claims['nonce'] after decoding the JWT.
Additional Locations (1)
| 'oidc_provider' => $providerId, | ||
| 'oidc_subject' => $subject, | ||
| 'oidc_email' => $email, | ||
| ]); |
There was a problem hiding this comment.
Email-based account linking enables account takeover
High Severity
When no user is found by OIDC subject, the code falls back to matching by email via User::getUserByEmail. If a match is found, the existing account is silently linked to the OIDC identity (overwriting oidc_provider and oidc_subject) and logged in — without any user consent or verification. An OIDC provider administrator (or attacker) who controls the email claim can impersonate any existing user whose email matches.
| $providers = OidcProvider::getAllProviders(); | ||
|
|
||
| return ApiResponse::success(['providers' => $providers], 'Providers fetched successfully', 200); | ||
| } |
There was a problem hiding this comment.
Admin API exposes OIDC client secrets in response
Medium Severity
getAllProviders() uses SELECT *, which returns client_secret in the API response for the admin list endpoint. The codebase follows a pattern of masking sensitive values (e.g., maskSensitiveSetting for DISCORD_OAUTH_CLIENT_SECRET, SMTP passwords, and API keys in SettingsController). The OIDC client_secret is not masked or excluded, exposing it to any admin user and in network traffic.
| const discordEnabled = settings?.discord_oauth_enabled === 'true'; | ||
| const oidcEnabled = oidcProviders.length > 0; | ||
| const showTurnstile = turnstileEnabled && turnstileSiteKey; | ||
| const showLocalLogin = true; |
There was a problem hiding this comment.
showLocalLogin hardcoded, ignoring disable-local-login setting
Medium Severity
showLocalLogin is hardcoded to true, meaning the OIDC_DISABLE_LOCAL_LOGIN config setting (defined in ConfigInterface, exposed in PublicConfig) has no effect. The login form always shows the local username/password fields, defeating the purpose of the setting. The variable needs to read from settings?.oidc_disable_local_login and oidc_disable_local_login needs to be added to the Settings type.
| substr($hex, 16, 4), | ||
| substr($hex, 20, 12) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Duplicate generateUuid across multiple model classes
Low Severity
OidcProvider::generateUuid() is an exact copy of User::generateUuid(), which itself is identical to existing implementations in Node::generateUuid(), Server::generateUuid(), and Spell::generateUuid(). This PR adds two more copies of the same UUID v4 generation logic instead of extracting it to a shared utility.
Additional Locations (1)
| } | ||
|
|
||
| $baseUsername = preg_replace('/[^a-zA-Z0-9_]/', '_', strtolower($usernameSource)) ?: 'user'; | ||
| $username = substr($baseUsername, 0, 32); |
There was a problem hiding this comment.
Auto-provisioned username not checked for uniqueness
Medium Severity
The autoProvisionUser method derives a username from OIDC claims but never checks whether it already exists before calling User::createUser. Every other user-creation path in the codebase (Saas.php, Users.php, PterodactylImporterController.php) calls User::getUserByUsername() to verify uniqueness first. If two OIDC users produce the same normalized username (common for similar email prefixes), the second user's auto-provisioning silently fails with a generic error.
…ing functionality and improving error handling in OIDC authentication. Update routes to include rate limiting for OIDC login requests and fix localization for provider deletion confirmation.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
backend/app/Controllers/User/Auth/OidcController.php (2)
255-263:⚠️ Potential issue | 🟠 MajorEmail-based account linking still ignores
email_verified— this is the account-takeover magnet that was already flagged.Lines 261–262 look up an existing user by email regardless of whether
$emailVerifiedStrictis true. A malicious IdP (or misconfigured provider withoutrequire_email_verified) can present an unverified email matching a local account and take it over. The fix was already proposed in the prior review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/User/Auth/OidcController.php` around lines 255 - 263, The code currently looks up an existing local user by email regardless of whether the OIDC assertion's email is verified; restrict account linking to verified emails by only calling User::getUserByEmail($email) when $email is present and $emailVerifiedStrict is true (i.e., change the condition around the lookup that follows $this->findUserByOidcSubject($providerId, $subject) so it requires $emailVerifiedStrict before assigning $user from User::getUserByEmail).
200-217:⚠️ Potential issue | 🔴 CriticalTrusting unverified JWT claims for auth is the architectural equivalent of accepting a handwritten note as a passport — this is a known critical outstanding issue.
decodeJwtWithoutVerification()skips signature validation entirely. The issuer/audience checks on Lines 205–217 are good hygiene but meaningless against a forged token — any attacker who can intercept or inject a JWT can craft arbitrary claims. The nonce (cached['nonce']) is never compared againstclaims['nonce'], andexp/nbfare never checked. This was already flagged and the root cause remains unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/User/Auth/OidcController.php` around lines 200 - 217, The code currently decodes tokens with decodeJwtWithoutVerification and then only checks iss/aud, which is insecure; replace this flow by performing full JWT verification using the provider's JWKS (verify signature, alg, and token integrity) and then validate the standard claims (iss equals issuerUrl, aud contains clientId, verify exp and nbf timestamps, and compare claims['nonce'] against cached['nonce']); update the flow in OidcController.php (functions/methods referencing decodeJwtWithoutVerification, $claims, $aud, $issuerUrl, $clientId, and cached['nonce']) to call a verified JWT validation routine and return the same RedirectResponse errors when any verification or claim check fails.
🧹 Nitpick comments (2)
backend/app/routes/user/auth.php (1)
192-215: Rate limiting on both OIDC routes — the past feedback was heard. Certified improvement.Both
oidc-loginandoidc-callbackare now capped at 10 req/min. One trivial nit: Line 201 is missing the// Default: Admin can override in ratelimit.jsoncomment that every other rate-limited route in the file has (see lines 41, 54, 67…). The callback on Line 213 has it but the login on Line 201 doesn't. Whoever wrote this clearly lost interest halfway.✏️ Proposed fix
- Rate::perMinute(10), + Rate::perMinute(10), // Default: Admin can override in ratelimit.json 'user-auth-oidc' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/routes/user/auth.php` around lines 192 - 215, Add the missing inline comment to the oidc-login route's rate limit declaration: locate the registerApiRoute call that registers 'oidc-login' and append the same comment used elsewhere ("// Default: Admin can override in ratelimit.json") to the Rate::perMinute(10) argument so it matches the 'oidc-callback' route and other rate-limited routes.backend/app/Controllers/User/Auth/OidcController.php (1)
343-348: Hardcoding"OIDC"as a last-name fallback is the kind of thing that ends up in a support ticket titled "my name is John OIDC."Line 347 sets
$lastName = 'OIDC'when no family name is available. This leaks implementation detail into the user's profile and makes no sense to the end-user or admins reading it. An empty string ornull(if the column allows it) would be far less confusing.✏️ Suggested improvement
- if ($lastName === '') { - $lastName = 'OIDC'; - } + // Leave lastName empty if not provided; the application UI should handle display.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/User/Auth/OidcController.php` around lines 343 - 348, The code assigns a hardcoded last name 'OIDC' when no family name is present (see the $lastName fallback in OidcController), which leaks implementation detail into user profiles; change the fallback to an empty string or null (prefer null if the users table allows nullable family_name) instead of 'OIDC', and update any validation/DB save logic that assumes non-null values (e.g., user creation/update flow that uses $firstName/$lastName) to handle empty/null safely so no literal "OIDC" is stored as the user's family name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/Controllers/Admin/OidcProvidersController.php`:
- Around line 115-145: The client_secret is being stored plaintext in the insert
array before calling OidcProvider::createProvider; change the flow to trim the
client_secret and then encrypt it with a server-side symmetric key (derived from
an environment/stored key material not kept in the DB) before putting it into
the $insert array, and ensure OidcProvider::createProvider stores the encrypted
value; also update any retrieval/decrypt path (e.g.,
OidcProvider::getProviderByUuid and any code that needs the raw secret) to
decrypt server-side only when needed, while keeping ApiResponse::success using
self::stripClientSecret($provider) to avoid returning secrets, and ensure key
management does not persist the encryption key in the database.
- Around line 117-131: The 'enabled' value in the $insert array of
OidcProvidersController is using inverted/unsafe logic; update the 'enabled'
element to use the same boolean normalization as 'auto_provision' and
'require_email_verified' so it only becomes 'true' when the incoming
$data['enabled'] is the literal string 'true' (otherwise 'false'), i.e. change
the expression for the 'enabled' key in the $insert array to mirror the ===
'true' check used by auto_provision/require_email_verified.
- Line 120: The OidcProvidersController currently blindly stores 'issuer_url'
(seen in the assignment 'issuer_url' => rtrim(trim($data['issuer_url']), '/')),
which allows non-HTTPS or internal host values; validate and normalize the input
before saving: in the method that processes $data in OidcProvidersController
(where 'issuer_url' is set), parse the URL (e.g., with parse_url or a URL
validator), enforce the scheme is 'https', ensure host is present and not an IP
in a private/reserved range (reject 127.*, 10.*, 192.168.*, 169.254.*,
172.16-31.*, and IPv6 equivalents), and only then trim/rtrim and persist; return
a clear validation error to the caller when the check fails.
In `@backend/app/Controllers/User/Auth/LoginController.php`:
- Line 270: The completeLogin method currently accepts an unused Request
$request parameter; remove the unused parameter from the completeLogin signature
and update every caller to stop passing a Request instance (call sites: the
three callers in LoginController and the one in OidcController that pass
$request). Specifically, edit the method declaration public function
completeLogin(array $userInfo): Response and remove any argument corresponding
to $request in the invocations inside LoginController (the two places around
lines 105 and 124 and the one at 261) and OidcController (around line 289); if
you actually intend to use request data later, instead document the parameter
inside completeLogin and add a TODO comment and a single usage (e.g., a log) to
avoid PHPMD warnings—otherwise fully remove the parameter and run tests to
ensure call signatures match.
- Around line 245-250: Move the OIDC_DISABLE_LOCAL_LOGIN guard so it runs
immediately after the user lookup and before any password verification: check
ConfigInterface::OIDC_DISABLE_LOCAL_LOGIN and call
\App\Helpers\PermissionHelper::hasPermission($userInfo['uuid'],
\App\Permissions::ADMIN_ROOT) right after fetching $userInfo, and if local login
is disabled return ApiResponse::error('Local login is disabled',
'LOCAL_LOGIN_DISABLED', 403) before calling password_verify() to avoid leaking a
valid-credential signal.
In `@backend/app/Controllers/User/Auth/OidcController.php`:
- Around line 310-318: In autoProvisionUser, stop treating
claims['preferred_username'] as an email fallback; change the logic that sets
$emailValue (currently "$email ?? ($claims['preferred_username'] ?? null)") so
the method only accepts a real email claim (e.g., mail, email, or
verified_email) and returns null when none is present or valid; add a simple
email-format validation (e.g., using filter_var) before calling User::createUser
to avoid storing non-email strings in the email column and to fail provisioning
cleanly when no valid email is provided.
- Line 54: Remove the unused local variable $config in the OidcController
methods: delete the unnecessary assignments of $config = $app->getConfig()
inside login() and callback() since the code already calls
$app->getConfig()->getSetting(...) directly; ensure no other references to
$config remain in those methods and run static analysis to confirm PHPMD
warnings are resolved.
- Around line 319-326: The code currently builds $baseUsername and $username
once and fails if User::createUser hits a DB unique constraint; change this to
attempt creation in a loop: try creating with $username, and on failure due to
collision retry up to a fixed number (e.g., 5) by appending a short random
alphanumeric suffix (e.g., 4 chars) to $baseUsername, re-sanitizing/truncating
so the final username stays <=32 chars, and retry User::createUser each loop;
stop and return null only after exhausting attempts. Update the logic around
$baseUsername/$username creation in OidcController (where User::createUser is
called) to implement the retry/suffix strategy and ensure the suffix is added
before truncation to avoid duplicates.
---
Duplicate comments:
In `@backend/app/Controllers/User/Auth/OidcController.php`:
- Around line 255-263: The code currently looks up an existing local user by
email regardless of whether the OIDC assertion's email is verified; restrict
account linking to verified emails by only calling User::getUserByEmail($email)
when $email is present and $emailVerifiedStrict is true (i.e., change the
condition around the lookup that follows
$this->findUserByOidcSubject($providerId, $subject) so it requires
$emailVerifiedStrict before assigning $user from User::getUserByEmail).
- Around line 200-217: The code currently decodes tokens with
decodeJwtWithoutVerification and then only checks iss/aud, which is insecure;
replace this flow by performing full JWT verification using the provider's JWKS
(verify signature, alg, and token integrity) and then validate the standard
claims (iss equals issuerUrl, aud contains clientId, verify exp and nbf
timestamps, and compare claims['nonce'] against cached['nonce']); update the
flow in OidcController.php (functions/methods referencing
decodeJwtWithoutVerification, $claims, $aud, $issuerUrl, $clientId, and
cached['nonce']) to call a verified JWT validation routine and return the same
RedirectResponse errors when any verification or claim check fails.
---
Nitpick comments:
In `@backend/app/Controllers/User/Auth/OidcController.php`:
- Around line 343-348: The code assigns a hardcoded last name 'OIDC' when no
family name is present (see the $lastName fallback in OidcController), which
leaks implementation detail into user profiles; change the fallback to an empty
string or null (prefer null if the users table allows nullable family_name)
instead of 'OIDC', and update any validation/DB save logic that assumes non-null
values (e.g., user creation/update flow that uses $firstName/$lastName) to
handle empty/null safely so no literal "OIDC" is stored as the user's family
name.
In `@backend/app/routes/user/auth.php`:
- Around line 192-215: Add the missing inline comment to the oidc-login route's
rate limit declaration: locate the registerApiRoute call that registers
'oidc-login' and append the same comment used elsewhere ("// Default: Admin can
override in ratelimit.json") to the Rate::perMinute(10) argument so it matches
the 'oidc-callback' route and other rate-limited routes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/Controllers/Admin/OidcProvidersController.phpbackend/app/Controllers/User/Auth/LoginController.phpbackend/app/Controllers/User/Auth/OidcController.phpbackend/app/routes/user/auth.phpbackend/storage/migrations/2026-02-24.00.02-add-oidc-users-unique-index.sqlfrontendv2/public/locales/en.json
…tion, encrypting client secrets, and improving user auto-provisioning logic. Update login flow to restrict local login based on OIDC settings and streamline user creation process with unique username generation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
backend/app/Controllers/User/Auth/LoginController.php (2)
270-270:$requestis a ghost hauntingcompleteLogin— method body never touches it.PHPMD and the previous reviewer already clocked this. Four callers dutifully pass
$requestinto a void. Either use it, document the intent with a TODO, or amputate it and update all callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/User/Auth/LoginController.php` at line 270, The parameter $request on completeLogin(array $userInfo, Request $request): Response is unused and should be fixed; either remove the Request $request parameter from the completeLogin signature and update all callers that pass $request to stop supplying it, or if $request is intended for future use, explicitly reference it (e.g., add a TODO comment and a minimal usage like /* `@var` Request $request */ or log/validate something) to suppress PHPMD warnings; update the method declaration in LoginController::completeLogin and ensure all call sites that pass $request (the four callers noted) are modified accordingly so types/signatures remain consistent.
245-250:LOCAL_LOGIN_DISABLEDstill fires afterpassword_verify— credential oracle lives on.Covered by a prior review comment. Moving the OIDC local-login guard to before
password_verifyis still unaddressed; a valid-password non-admin still gets a distinguishable 403 instead of a uniform reject.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/User/Auth/LoginController.php` around lines 245 - 250, The OIDC local-login guard block using $config->getSetting(ConfigInterface::OIDC_DISABLE_LOCAL_LOGIN, 'false') and PermissionHelper::hasPermission(...) must be executed before any password verification to avoid a credential oracle; move the entire conditional that returns ApiResponse::error('Local login is disabled', 'LOCAL_LOGIN_DISABLED', 403) so it runs prior to calling password_verify (and any early-return on credentials) inside the LoginController authentication flow (the method containing password_verify) so non-admins cannot learn validity of passwords.backend/app/Controllers/User/Auth/OidcController.php (4)
319-326: Username collision on auto-provision is still a silent, permanent account lockout.Previously flagged and still unresolved. No retry or suffix strategy exists; the second OIDC user named
john_doesimply cannot provision. Ever.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/User/Auth/OidcController.php` around lines 319 - 326, The current auto-provisioning logic in OidcController uses $usernameSource -> $baseUsername -> $username but does not handle collisions, causing permanent lockouts; update the provisioning path that computes $baseUsername/$username to check for existing usernames and, if a collision exists, append an incremental numeric suffix (e.g., _1, _2, ...) trimming the base so the final username stays <= 32 chars and only contains allowed chars; iterate until a unique username is found (with a reasonable max attempts), and if all attempts fail fall back to adding a short random suffix to guarantee uniqueness before creating the account.
54-54:$configassigned, then abandoned — ghost variable haunts bothlogin()andcallback().Previously flagged and still unresolved. Lines 54 and 117 assign
$configbut the code calls$app->getConfig()->getSetting(...)directly on lines 90 and 162. Delete both assignments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/User/Auth/OidcController.php` at line 54, Remove the unused $config assignments in the OidcController class: delete the lines that set "$config = $app->getConfig();" inside the login() and callback() methods (they're unused because code calls $app->getConfig()->getSetting(...) directly); verify no other references to $config remain in those methods and leave the existing $app->getConfig()->getSetting(...) calls intact.
310-318:preferred_usernameas email fallback is still a category error waiting to corrupt your user table.Previously flagged and still unresolved.
preferred_usernameis a display identifier, not an email address. Storing it in the email column will silently corrupt any downstream flow that assumes a valid address.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/User/Auth/OidcController.php` around lines 310 - 318, The autoProvisionUser function is incorrectly using claims['preferred_username'] as an email fallback which can corrupt the email column; change autoProvisionUser so that emailValue is derived only from the provided $email or from a validated claims['email'] (not preferred_username), and ensure the chosen value passes a strict email validation (e.g., filter_var(..., FILTER_VALIDATE_EMAIL)) before proceeding—if validation fails, treat as null and return early; update references to $emailValue and any related logic in autoProvisionUser to reflect this stricter validation and remove the preferred_username fallback.
200-203: Trusting unsigned tokens in an auth flow is the spiritual successor tomd5passwords.Previously flagged and still unresolved.
decodeJwtWithoutVerificationskips signature validation entirely;iss/audchecks don't protect against a compromised or intercepted token. JWKS-based signature verification is not optional in a production OIDC flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/User/Auth/OidcController.php` around lines 200 - 203, The code currently uses decodeJwtWithoutVerification in OidcController to parse idToken, which skips signature checks; replace this with a proper JWT signature verification step that fetches the provider JWKS, selects the key by the token's kid, and verifies the token signature and claims (iss, aud, exp, iat) before trusting the payload. Update the idToken handling path (where decodeJwtWithoutVerification and the idToken variable are used) to call a verified-verify function (or library verifier) against the provider's JWKS endpoint, validate issuer/audience/expiry, and only proceed if verification succeeds; remove any code paths that accept unsigned tokens.backend/app/Controllers/Admin/OidcProvidersController.php (3)
130-130:enabledinverted-logic riddle is still here, surviving code review like a cockroach.Duplicate of a previous comment.
'false'is the only value that disables; everything else — including booleans,null,0, missing key — silently enables the provider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/Admin/OidcProvidersController.php` at line 130, The current assignment for the 'enabled' field uses inverted logic (it treats everything except the literal string 'false' as enabled), so change the check on $data['enabled'] to only enable when the input explicitly indicates true; update the 'enabled' assignment that references $data['enabled'] to return 'true' only if $data['enabled'] is the explicit true value (e.g. string 'true' or boolean true) and return 'false' for any other case so missing/null/0/other values do not silently enable the provider.
120-120:issuer_urlstill acceptshttp://169.254.169.254/— SSRF is a feature now, apparently.Prior unresolved comment. No scheme or host validation; an admin can point the discovery URL at any internal endpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/Admin/OidcProvidersController.php` at line 120, The current assignment for 'issuer_url' in OidcProvidersController accepts arbitrary URLs (including internal IPs like 169.254.169.254); update the validation logic before assigning to ensure the URL is well-formed, uses https, and its host is not a loopback, link-local, or private/reserved IP. Specifically, replace the simple rtrim(trim(...)) step with validation that (1) uses FILTER_VALIDATE_URL or parse_url to confirm scheme is 'https', (2) extracts the host and checks whether it is an IP via FILTER_VALIDATE_IP and if so rejects link-local (169.254.0.0/16), loopback (127.0.0.0/8), and RFC1918 private ranges, and (3) if the host is a hostname, resolve/validate it or restrict to allowed domains as appropriate; only assign the sanitized rtrim(trim($data['issuer_url']), '/') to 'issuer_url' after these checks pass.
115-145: Client secret still lounging in plaintext — your DB breach is also an IdP breach.Covered by a prior unresolved review.
client_secretis stored and retrieved as a raw string. A single DB exfiltration hands an attacker every OIDC integration on the platform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/Controllers/Admin/OidcProvidersController.php` around lines 115 - 145, The client_secret is being stored and returned as plaintext; before inserting into the DB in the OidcProvidersController create flow (where OidcProvider::generateUuid(), $insert and OidcProvider::createProvider are used), encrypt or securely hash the client_secret using the app's secret/key (do not use a simple hash if the secret must be retrievable for OIDC client authentication — use a reversible, authenticated encryption like OpenSSL or your framework's Crypt service) and store the ciphertext in the 'client_secret' field; ensure OidcProvider::getProviderByUuid and any other readers never return the decrypted secret (update stripClientSecret to always remove the client_secret) and only decrypt the stored secret inside a single well-audited method used exclusively for performing outbound OIDC auth (e.g., a new OidcProvider::getDecryptedClientSecret or OidcClient::getCredentials) so the plaintext is not exposed in API responses or general getters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/Controllers/Admin/OidcProvidersController.php`:
- Around line 193-207: The loop in OidcProvidersController that builds $update
from $fields currently allows an empty string for 'client_secret', which will
overwrite the stored secret and break logins; inside the foreach (the block that
reads $fields, $data, and sets $update) add a guard for when $field ===
'client_secret' and the incoming value is a string that trims to empty: either
skip adding it to $update or return a 400 validation error (preferably reject
with a clear message) so blank client_secret values are not written to the DB.
Ensure you reference the same variables ($fields, $data, $update) and keep
behavior consistent with other validation flows in OidcProvidersController.
In `@backend/app/Controllers/User/Auth/OidcController.php`:
- Around line 135-139: The state cache is being cleared unconditionally via
Cache::forget('oidc_state_' . $state) before validating the provider response,
which can drop the stored state if $code is missing; move the Cache::forget call
to after you verify $code is present and a string (i.e., after the existing if
(!$code || !is_string($code)) check) or only call it when you are about to
continue the flow, so keep the
RedirectResponse('/auth/login?error=oidc_missing_code') path early and defer
Cache::forget until after $code validation (or after successful code exchange)
to avoid losing the state prematurely.
- Around line 255-263: The code currently links by email regardless of
verification; change the lookup so User::getUserByEmail is only called when the
OIDC assertion shows a verified email: in the block around
findUserByOidcSubject($providerId, $subject) ensure the condition becomes
something like if (!$user && $email && $emailVerifiedStrict) so that
getUserByEmail($email) is only invoked when $emailVerifiedStrict is true (keep
references to $emailVerifiedStrict, findUserByOidcSubject, User::getUserByEmail,
$email, $providerUuid, $subject).
---
Duplicate comments:
In `@backend/app/Controllers/Admin/OidcProvidersController.php`:
- Line 130: The current assignment for the 'enabled' field uses inverted logic
(it treats everything except the literal string 'false' as enabled), so change
the check on $data['enabled'] to only enable when the input explicitly indicates
true; update the 'enabled' assignment that references $data['enabled'] to return
'true' only if $data['enabled'] is the explicit true value (e.g. string 'true'
or boolean true) and return 'false' for any other case so missing/null/0/other
values do not silently enable the provider.
- Line 120: The current assignment for 'issuer_url' in OidcProvidersController
accepts arbitrary URLs (including internal IPs like 169.254.169.254); update the
validation logic before assigning to ensure the URL is well-formed, uses https,
and its host is not a loopback, link-local, or private/reserved IP.
Specifically, replace the simple rtrim(trim(...)) step with validation that (1)
uses FILTER_VALIDATE_URL or parse_url to confirm scheme is 'https', (2) extracts
the host and checks whether it is an IP via FILTER_VALIDATE_IP and if so rejects
link-local (169.254.0.0/16), loopback (127.0.0.0/8), and RFC1918 private ranges,
and (3) if the host is a hostname, resolve/validate it or restrict to allowed
domains as appropriate; only assign the sanitized
rtrim(trim($data['issuer_url']), '/') to 'issuer_url' after these checks pass.
- Around line 115-145: The client_secret is being stored and returned as
plaintext; before inserting into the DB in the OidcProvidersController create
flow (where OidcProvider::generateUuid(), $insert and
OidcProvider::createProvider are used), encrypt or securely hash the
client_secret using the app's secret/key (do not use a simple hash if the secret
must be retrievable for OIDC client authentication — use a reversible,
authenticated encryption like OpenSSL or your framework's Crypt service) and
store the ciphertext in the 'client_secret' field; ensure
OidcProvider::getProviderByUuid and any other readers never return the decrypted
secret (update stripClientSecret to always remove the client_secret) and only
decrypt the stored secret inside a single well-audited method used exclusively
for performing outbound OIDC auth (e.g., a new
OidcProvider::getDecryptedClientSecret or OidcClient::getCredentials) so the
plaintext is not exposed in API responses or general getters.
In `@backend/app/Controllers/User/Auth/LoginController.php`:
- Line 270: The parameter $request on completeLogin(array $userInfo, Request
$request): Response is unused and should be fixed; either remove the Request
$request parameter from the completeLogin signature and update all callers that
pass $request to stop supplying it, or if $request is intended for future use,
explicitly reference it (e.g., add a TODO comment and a minimal usage like /*
`@var` Request $request */ or log/validate something) to suppress PHPMD warnings;
update the method declaration in LoginController::completeLogin and ensure all
call sites that pass $request (the four callers noted) are modified accordingly
so types/signatures remain consistent.
- Around line 245-250: The OIDC local-login guard block using
$config->getSetting(ConfigInterface::OIDC_DISABLE_LOCAL_LOGIN, 'false') and
PermissionHelper::hasPermission(...) must be executed before any password
verification to avoid a credential oracle; move the entire conditional that
returns ApiResponse::error('Local login is disabled', 'LOCAL_LOGIN_DISABLED',
403) so it runs prior to calling password_verify (and any early-return on
credentials) inside the LoginController authentication flow (the method
containing password_verify) so non-admins cannot learn validity of passwords.
In `@backend/app/Controllers/User/Auth/OidcController.php`:
- Around line 319-326: The current auto-provisioning logic in OidcController
uses $usernameSource -> $baseUsername -> $username but does not handle
collisions, causing permanent lockouts; update the provisioning path that
computes $baseUsername/$username to check for existing usernames and, if a
collision exists, append an incremental numeric suffix (e.g., _1, _2, ...)
trimming the base so the final username stays <= 32 chars and only contains
allowed chars; iterate until a unique username is found (with a reasonable max
attempts), and if all attempts fail fall back to adding a short random suffix to
guarantee uniqueness before creating the account.
- Line 54: Remove the unused $config assignments in the OidcController class:
delete the lines that set "$config = $app->getConfig();" inside the login() and
callback() methods (they're unused because code calls
$app->getConfig()->getSetting(...) directly); verify no other references to
$config remain in those methods and leave the existing
$app->getConfig()->getSetting(...) calls intact.
- Around line 310-318: The autoProvisionUser function is incorrectly using
claims['preferred_username'] as an email fallback which can corrupt the email
column; change autoProvisionUser so that emailValue is derived only from the
provided $email or from a validated claims['email'] (not preferred_username),
and ensure the chosen value passes a strict email validation (e.g.,
filter_var(..., FILTER_VALIDATE_EMAIL)) before proceeding—if validation fails,
treat as null and return early; update references to $emailValue and any related
logic in autoProvisionUser to reflect this stricter validation and remove the
preferred_username fallback.
- Around line 200-203: The code currently uses decodeJwtWithoutVerification in
OidcController to parse idToken, which skips signature checks; replace this with
a proper JWT signature verification step that fetches the provider JWKS, selects
the key by the token's kid, and verifies the token signature and claims (iss,
aud, exp, iat) before trusting the payload. Update the idToken handling path
(where decodeJwtWithoutVerification and the idToken variable are used) to call a
verified-verify function (or library verifier) against the provider's JWKS
endpoint, validate issuer/audience/expiry, and only proceed if verification
succeeds; remove any code paths that accept unsigned tokens.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/Controllers/Admin/OidcProvidersController.phpbackend/app/Controllers/User/Auth/LoginController.phpbackend/app/Controllers/User/Auth/OidcController.phpbackend/app/routes/user/auth.phpbackend/storage/migrations/2026-02-24.00.02-add-oidc-users-unique-index.sqlfrontendv2/public/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/routes/user/auth.php
- frontendv2/public/locales/en.json
…cryption, and refine user lookup logic to require email verification. Update OIDC state cache handling for improved authentication flow.
github-project-automation
bot
moved this from In progress
to Done
in FeatherPanel - Kanban
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.
Including provider settings and routes. Enhance user authentication flow by integrating OIDC login functionality and updating UI components for better user experience.
Note
High Risk
Changes core authentication flows and adds auto-provisioned account creation, plus ID token claims are parsed without JWT signature verification, making misconfiguration or token forgery a security risk.
Overview
Adds generic OpenID Connect (OIDC) SSO support end-to-end: a new
OidcControllerimplements the authorization-code flow (state/nonce caching, discovery lookup, token exchange, claim checks, optional group + email_verified gating) and reusesLoginController::completeLogin(now public) to finalize sessions.Introduces admin-managed OIDC provider configuration backed by a new
featherpanel_oidc_providerstable andOidcProvidermodel, with CRUD endpoints under/api/admin/oidc/providersplus a public/api/system/oidc/providersendpoint that only exposes enabled providers (uuid/name) for login button rendering.Updates persistence and UI: users gain
oidc_provider/oidc_subject/oidc_emailcolumns and auto-provisioning can create accounts with generated UUIDs; the frontend adds an Admin “OIDC / SSO Providers” page, navigation + i18n, and shows OIDC provider buttons on the login screen (alongside Discord OAuth).Written by Cursor Bugbot for commit a20042c. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements