Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 17 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds optional OIDC ID-token decoding to GenericSocialiteProvider: provider captures Changes
Sequence DiagramsequenceDiagram
participant Client
participant Provider as GenericSocialiteProvider
participant OAuth as OAuthServer
participant Decoder as JWTDecoder
Client->>OAuth: OAuth callback (code)
OAuth->>Provider: token response (access_token, id_token)
Provider->>Provider: store access_token + id_token
alt OIDC_USE_ID_TOKEN = true
Provider->>Decoder: decode id_token (alg, key)
Decoder-->>Provider: decoded claims
Provider-->>Client: return user from claims
else OIDC_USE_ID_TOKEN = false
Provider->>OAuth: request /userinfo with access_token
OAuth-->>Provider: userinfo payload
Provider-->>Client: return user from userinfo
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
|
Oups! J'ai envoyé dans main! @dbarzin préfères-tu que je recommence en pointant sur dev ? |
|
C'est OK, rien en DEV pour l'instant. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Providers/Socialite/GenericSocialiteProvider.php`:
- Around line 108-112: The provider currently caches the ID token in
getAccessTokenResponse() via $this->idToken and then getUserByToken($token)
ignores its $token argument when services.oidc.use_id_token is true; change the
logic so getUserByToken uses the token parameter passed in instead of a cached
$this->idToken, and only read an ID token from getAccessTokenResponse() to
populate a local/flow-scoped value (not shared state) or pass it explicitly into
getUserByToken when calling from the authorization-code flow; if an ID token is
not available, have getUserByToken fall back to calling the /userinfo endpoint
rather than throwing, ensuring code paths referencing getAccessTokenResponse(),
getUserByToken($token) and $this->idToken no longer rely on instance-level
cached state.
- Around line 169-185: The decodeIdToken method currently only verifies the JWT
signature; after JWT::decode($idToken, ...) you must validate standard OIDC
claims: compare the token's 'iss' against a configured expected issuer (use
config('services.oidc.issuer') or construct it if absent), ensure the 'aud'
contains the configured client id (config('services.oidc.client_id')), and if an
'azp' claim exists verify it equals the configured client id; if any check fails
throw an Exception describing which claim failed. Update decodeIdToken to read
these config values and perform the checks on the decoded token array (from
json_decode(json_encode($decoded), true)), failing fast on mismatch to prevent
authentication bypass.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c6f29c13-bd71-415f-9673-d33eed2575c8
📒 Files selected for processing (4)
.env.exampleapp/Providers/Socialite/GenericSocialiteProvider.phpcomposer.jsonconfig/services.php
|
@wouldsmina Tu peux vérifier les commentaires de CodeRabbit ? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/Providers/Socialite/GenericSocialiteProvider.php (1)
177-193:⚠️ Potential issue | 🔴 CriticalValidate
iss,aud, andazpbefore accepting the ID token.Lines 186-192 only verify the JWT signature. That still accepts tokens signed by the same issuer key for a different client, which is a relying-party auth bypass.
Suggested fix
protected function decodeIdToken($idToken) { $alg = config('services.oidc.jwt_alg', 'RS256'); $key = config('services.oidc.jwt_secret_or_key'); if (!$key) { throw new \Exception('JWT secret or public key not configured'); } try { $decoded = JWT::decode($idToken, new Key($key, $alg)); } catch (\Exception $e) { throw new \Exception('Failed to decode ID token: '.$e->getMessage(), 0, $e); } - return json_decode(json_encode($decoded), true); + $claims = json_decode(json_encode($decoded), true); + $clientId = config('services.oidc.client_id'); + $expectedIssuer = rtrim(config('services.oidc.issuer', $this->getOIDCUrl()), '/'); + $aud = $claims['aud'] ?? null; + $audiences = is_array($aud) ? $aud : ($aud !== null ? [$aud] : []); + + if (($claims['iss'] ?? null) !== $expectedIssuer) { + throw new \Exception('Invalid ID token issuer'); + } + + if (! in_array($clientId, $audiences, true)) { + throw new \Exception('Invalid ID token audience'); + } + + if (isset($claims['azp']) && $claims['azp'] !== $clientId) { + throw new \Exception('Invalid ID token authorized party'); + } + + if (count($audiences) > 1 && ! isset($claims['azp'])) { + throw new \Exception('Missing ID token authorized party'); + } + + return $claims; }#!/bin/bash # Verify decodeIdToken() has no relying-party claim checks today. sed -n '177,215p' app/Providers/Socialite/GenericSocialiteProvider.php rg -n "JWT::decode|services\.oidc\.(issuer|client_id)|\biss\b|\baud\b|\bazp\b" app/Providers/Socialite/GenericSocialiteProvider.php fd '^services\.php$' config -x rg -n "oidc|client_id|issuer" {}Expected result:
JWT::decodeis present, but there are noiss/aud/azpcomparisons insidedecodeIdToken().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Providers/Socialite/GenericSocialiteProvider.php` around lines 177 - 193, The decodeIdToken method currently only verifies the JWT signature; update decodeIdToken to also validate relying-party claims by fetching expected values from config('services.oidc.issuer') and config('services.oidc.client_id') and then (after JWT::decode) assert that the decoded claims contain 'iss' exactly equal to the configured issuer, that the 'aud' claim either equals the configured client_id or (if 'aud' is an array) contains the client_id, and if 'aud' is an array also verify 'azp' equals the configured client_id; on any mismatch throw a clear Exception (e.g., "Invalid issuer", "Invalid audience", "Invalid authorized party") so the token is rejected for wrong issuer/audience/azp. Ensure you reference the decoded payload returned by JWT::decode in these checks and keep signature validation intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Providers/Socialite/GenericSocialiteProvider.php`:
- Around line 111-112: The thrown InvalidStateException is resolving to the
current namespace instead of Socialite's class; add an import for Socialite's
exception (e.g., add "use Laravel\Socialite\Two\InvalidStateException;") at the
top of GenericSocialiteProvider.php so the throw in the hasInvalidState() check
references the correct Socialite exception class.
---
Duplicate comments:
In `@app/Providers/Socialite/GenericSocialiteProvider.php`:
- Around line 177-193: The decodeIdToken method currently only verifies the JWT
signature; update decodeIdToken to also validate relying-party claims by
fetching expected values from config('services.oidc.issuer') and
config('services.oidc.client_id') and then (after JWT::decode) assert that the
decoded claims contain 'iss' exactly equal to the configured issuer, that the
'aud' claim either equals the configured client_id or (if 'aud' is an array)
contains the client_id, and if 'aud' is an array also verify 'azp' equals the
configured client_id; on any mismatch throw a clear Exception (e.g., "Invalid
issuer", "Invalid audience", "Invalid authorized party") so the token is
rejected for wrong issuer/audience/azp. Ensure you reference the decoded payload
returned by JWT::decode in these checks and keep signature validation intact.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b45658be-46aa-485f-9b07-3db07bce143a
📒 Files selected for processing (1)
app/Providers/Socialite/GenericSocialiteProvider.php
Add OIDC claims validation to prevent authentication bypass.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/Providers/Socialite/GenericSocialiteProvider.php (1)
6-12:⚠️ Potential issue | 🔴 CriticalMissing
InvalidStateExceptionimport will cause a class-not-found error.Line 112 throws
InvalidStateException, but the class is not imported. PHP will attempt to resolve it within theApp\Providers\Socialitenamespace, resulting in a fatal error when state validation fails.Suggested fix
use GuzzleHttp\Exception\GuzzleException; use Illuminate\Support\Arr; use Laravel\Socialite\Two\AbstractProvider; +use Laravel\Socialite\Two\InvalidStateException; use Laravel\Socialite\Two\ProviderInterface; use Laravel\Socialite\Two\User; use Firebase\JWT\JWT; use Firebase\JWT\Key;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Providers/Socialite/GenericSocialiteProvider.php` around lines 6 - 12, The file throws InvalidStateException in the state validation path (see where InvalidStateException is thrown around the validation logic in GenericSocialiteProvider) but the class isn't imported; add the proper import for Laravel\Socialite\Two\InvalidStateException at the top of the file (or change the throw to a fully-qualified class name) so PHP resolves the exception instead of looking in the App\Providers\Socialite namespace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/Providers/Socialite/GenericSocialiteProvider.php`:
- Around line 6-12: The file throws InvalidStateException in the state
validation path (see where InvalidStateException is thrown around the validation
logic in GenericSocialiteProvider) but the class isn't imported; add the proper
import for Laravel\Socialite\Two\InvalidStateException at the top of the file
(or change the throw to a fully-qualified class name) so PHP resolves the
exception instead of looking in the App\Providers\Socialite namespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d5e2f254-97e3-4faf-9873-637a63d08f89
📒 Files selected for processing (1)
app/Providers/Socialite/GenericSocialiteProvider.php
|
Merci ! |
2 participants
Bonjour,
Ce PR ajoute la possibilité de configurer Deming pour utiliser le id_token (JWT) retourné par l'IdP afin d'en extraire directement les claims, au lieu d'appeler l'endpoint /userinfo.
Cette fonctionnalité est utile avec Apereo CAS, qui ne fournit pas l'endpoint /userinfo.
Le fonctionnement est contrôlé par l'option OIDC_USE_ID_TOKEN. Lorsqu'elle est activée, le id_token est décodé et ses claims sont utilisés pour construire l'utilisateur. Sinon, le comportement actuel (appel à /userinfo avec l'access_token) est conservé.
Je n'ai testé que l'algorithme RS256 avec une clé publique au format PEM. Le support de HS256 est également présent et devrait fonctionner correctement.
Deux points importants :
Mon serveur CAS (et probablement d'autres IdP) ne fournit pas la clé publique directement au format PEM mais au format JWKS (RSA). J'ai converti cette clé en PEM à l'aide de ce script Python :
https://akrabat.com/converting-jwks-json-to-pem-using-python/
Je n'ai pas réussi à récupérer et convertir automatiquement la clé depuis l'endpoint /jwks en PHP. Cette clé pourrait être récupérée dynamiquement plutôt que configurée manuellement. Ça pourrait faire l'objet d'une amélioration ultérieure.
Summary by CodeRabbit
New Features
Chores