fix(twenty-server): honor the identity provider's advertised id_token signing algorithm - #24165
fix(twenty-server): honor the identity provider's advertised id_token signing algorithm#24165suparikoli wants to merge 1 commit into
Conversation
… signing algorithm SSOService.getOIDCClient constructs the openid-client Client without setting id_token_signed_response_alg. When that field is omitted, openid-client doesn't look at what the issuer supports via discovery - it silently defaults to RS256, per the OIDC dynamic client registration default, not something the library infers from id_token_signing_alg_values_supported. Confirmed this empirically against the installed openid-client version: constructing a client against an issuer whose discovery document only advertises id_token_signing_alg_values_supported: ['ES384'] still comes back with metadata.id_token_signed_response_alg === 'RS256' unless the field is passed explicitly. So any identity provider that doesn't sign id_tokens with RS256 fails every login with "unexpected JWT alg received", even though the algorithm it used is one it published support for. Logto is the example in the issue (its default is ES384), but this breaks any OIDC provider with a non-RS256 default. Issuer.discover() (called in oidc-auth.guard.ts before getOIDCClient runs) already fetches and populates issuer.metadata, including the standard id_token_signing_alg_values_supported discovery field - the client just never reads it. Fix: a small helper picks an algorithm from what the issuer actually advertises - RS256 if it's in the supported list (keeping today's behavior for the common case), otherwise the first algorithm the issuer supports. If the issuer doesn't publish the field, nothing is passed and openid-client's own default takes over, so behavior for providers that don't advertise this is unchanged. Fixes twentyhq#22780.
|
👋 Thanks for contributing to Twenty! We're excited to have you on board. Your PR has been set to draft while you work on it. Once you're done, mark it as Ready for review and our automated checks will run. By submitting your Pull Request, you acknowledge that you agree with the terms of our Contributor License Agreement. |
Greptile SummaryThe PR configures OIDC clients from issuer discovery metadata so providers using non-RS256 ID-token signatures can authenticate.
Confidence Score: 4/5The PR should not merge until algorithm selection stops interpreting the first advertised capability as the issuer's actual signing choice. A provider advertising multiple non-RS256 algorithms can still have every login rejected when it signs with a supported algorithm that is not first in its discovery list; the test mock-verification issue is non-blocking. Files Needing Attention: packages/twenty-server/src/engine/core-modules/sso/utils/resolve-id-token-signed-response-alg.util.ts, packages/twenty-server/src/engine/core-modules/sso/services/tests/sso.service.spec.ts Important Files Changed
Reviews (1): Last reviewed commit: "fix(twenty-server): honor the identity p..." | Re-trigger Greptile |
| return undefined; | ||
| } | ||
|
|
||
| return stringAlgs.includes('RS256') ? 'RS256' : stringAlgs[0]; |
There was a problem hiding this comment.
First capability becomes required algorithm
If an identity provider advertises multiple non-RS256 signing algorithms but signs this client's ID tokens with an advertised algorithm other than the first, stringAlgs[0] becomes a hard validation constraint, causing every otherwise-valid SSO callback to fail with an unexpected JWT algorithm error.
| const buildSSOService = () => { | ||
| const twentyConfigService = { | ||
| get: jest.fn().mockReturnValue('https://twenty.example.com'), |
There was a problem hiding this comment.
Configuration mock calls remain unchecked
The new tests never verify that twentyConfigService.get is called exactly once with the expected key, so duplicated or incorrect configuration lookups can regress without these tests detecting them.
Rule Used: Check that mocked functions in tests are called ex... (source)
Learned From
twentyhq/twenty#12300
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-server/src/engine/core-modules/sso/utils/resolve-id-token-signed-response-alg.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/sso/utils/resolve-id-token-signed-response-alg.util.ts:47">
P2: When an issuer advertises multiple non-RS256 algorithms, this always selects the first one in id_token_signing_alg_values_supported. Discovery metadata order isn't guaranteed to reflect the algorithm the provider actually uses to sign id_tokens, so if the IdP signs with a different (but still advertised) algorithm, the client will still reject valid tokens with an unexpected JWT alg error.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return undefined; | ||
| } | ||
|
|
||
| return stringAlgs.includes('RS256') ? 'RS256' : stringAlgs[0]; |
There was a problem hiding this comment.
P2: When an issuer advertises multiple non-RS256 algorithms, this always selects the first one in id_token_signing_alg_values_supported. Discovery metadata order isn't guaranteed to reflect the algorithm the provider actually uses to sign id_tokens, so if the IdP signs with a different (but still advertised) algorithm, the client will still reject valid tokens with an unexpected JWT alg error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/core-modules/sso/utils/resolve-id-token-signed-response-alg.util.ts, line 47:
<comment>When an issuer advertises multiple non-RS256 algorithms, this always selects the first one in id_token_signing_alg_values_supported. Discovery metadata order isn't guaranteed to reflect the algorithm the provider actually uses to sign id_tokens, so if the IdP signs with a different (but still advertised) algorithm, the client will still reject valid tokens with an unexpected JWT alg error.</comment>
<file context>
@@ -0,0 +1,48 @@
+ return undefined;
+ }
+
+ return stringAlgs.includes('RS256') ? 'RS256' : stringAlgs[0];
+};
</file context>
Fixes #22780.
SSOService.getOIDCClientconstructs theopenid-clientClientwithout settingid_token_signed_response_alg:When that field is omitted,
openid-clientdoesn't look at what the issuer actually supports — it silently defaults toRS256(that's the OIDC dynamic client registration default, not something the library infers from discovery). I checked this empirically against the installedopenid-clientversion: constructing a client against an issuer whose discovery document only advertisesid_token_signing_alg_values_supported: ['ES384']still comes back withmetadata.id_token_signed_response_alg === 'RS256'unless you pass the field explicitly.So any identity provider that doesn't sign id_tokens with RS256 fails every login with
RPError: unexpected JWT alg received, even though the algorithm it used is one it published support for. Logto is the example in the issue (its default is ES384), but this breaks any OIDC provider with a non-RS256 default.Issuer.discover()(called inoidc-auth.guard.tsbeforegetOIDCClientruns) already fetches and populatesissuer.metadata, including the standardid_token_signing_alg_values_supporteddiscovery field — the client just never reads it.Fix: a small helper (
resolveIdTokenSignedResponseAlg) picks an algorithm from what the issuer actually advertises —RS256if it's in the supported list (keeping today's behavior for the common case), otherwise the first algorithm the issuer supports. If the issuer doesn't publish the field at all, we pass nothing and keep relying onopenid-client's own default, so behavior for providers that don't advertise this is unchanged.Verified with a new
SSOService.getOIDCClientspec that builds a realopenid-clientIssuer(no network calls, just the in-memory metadata object) withid_token_signing_alg_values_supported: ['ES384']and asserts what the constructed client resolves to. Ran it against the unpatched code first — it fails withReceived: "RS256", reproducing the bug exactly. After the fix it resolves toES384. Added two more cases: RS256 is still preferred when the issuer lists it alongside others, and behavior is unchanged (falls back to the library's RS256 default) when the issuer publishes no algorithm list at all.