fix(connector-kit): reject none prompt combined with other prompts - #9566
fix(connector-kit): reject none prompt combined with other prompts#9566Shivam8584 wants to merge 2 commits into
none prompt combined with other prompts#9566Conversation
OIDC Core 1.0 section 3.1.2.1 says a request containing `none` with any other prompt value is an error, since `none` asks for no UI at all. `oidcPromptsGuard` validated each element against the enum but never the combination, so the Admin Console accepted it and the connector joined the values verbatim into one `prompt` parameter. The identity provider was the first thing to reject it: Google returns `invalid_request: Invalid prompt: select_account consent none`, so the user lands on a provider error page instead of a sign-in screen. The guard is shared by the Google and Azure AD connectors, so fixing it in connector-kit covers both.
COMPARE TO
|
| Name | Diff |
|---|---|
| .changeset/rude-donuts-brake.md | 📈 +849 Bytes |
| packages/toolkit/connector-kit/src/types/social.test.ts | 📈 +1.57 KB |
| packages/toolkit/connector-kit/src/types/social.ts | 📈 +636 Bytes |
There was a problem hiding this comment.
🟡 Changes recommended
The new test calls oidcPromptsGuard.safeParse() with no argument, which will fail TypeScript type-checking and should be changed to safeParse(undefined).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes OIDC prompt validation in @logto/connector-kit so the mutually-exclusive none prompt is rejected when combined with other prompt values, aligning behavior with OIDC Core 1.0 §3.1.2.1 and preventing provider-side invalid_request errors from surfacing to end users.
Changes:
- Add a Zod
.refine()tooidcPromptsGuardto rejectnonewhen other prompts are present. - Add unit tests covering valid prompt combinations and the invalid
none+other cases. - Add a patch changeset for
@logto/connector-kit.
File summaries
| File | Description |
|---|---|
| packages/toolkit/connector-kit/src/types/social.ts | Tightens OIDC prompt validation by rejecting none when combined with other prompts. |
| packages/toolkit/connector-kit/src/types/social.test.ts | Adds Vitest coverage for the updated prompt guard, including invalid combinations. |
| .changeset/rude-donuts-brake.md | Adds a patch changeset describing the validation fix and its user impact. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| it('still accepts an empty array and undefined', () => { | ||
| expect(oidcPromptsGuard.safeParse([]).success).toBe(true); | ||
| expect(oidcPromptsGuard.safeParse().success).toBe(true); |
`safeParse` declares its data parameter as required, so calling it with no argument is a type error. Keep the explicit `undefined` and disable `unicorn/no-useless-undefined` on that line, matching how the rule is handled in packages/core/src/utils/zod.test.ts.
|
Good catch, thanks — fixed in d22d31d.
I've restored the explicit |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, aligns with the OIDC spec requirement, and is covered by targeted unit tests plus an appropriate changeset.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
| .nativeEnum(OidcPrompt) | ||
| .array() | ||
| .refine((prompts) => !prompts.includes(OidcPrompt.None) || prompts.length === 1, { | ||
| message: 'The `none` prompt cannot be combined with other prompts.', |
There was a problem hiding this comment.
The validation rule makes sense, but this guard is also used at runtime, so the impact is broader than rejecting config saves.
In the Google connector, getUserInfo and getTokenResponseAndUserInfo validate the full config before processing One Tap credentials. getAccessTokenByRefreshToken also validates the full config before making the token request. Neither flow sends the configured prompt parameter.
An existing config containing none alongside another prompt could therefore still work for One Tap or refresh-token operations today, but those operations would start failing with InvalidConfig after this change. This contradicts the PR note that existing invalid configs “keep working exactly as before.”
Could we clarify how existing configs should be handled and add regression coverage for these paths? If preserving those working flows is intended, we should separate save-time prompt validation from runtime validation for operations that do not use prompts. If rejecting the entire config at runtime is intentional, that behavior change should be explicitly acknowledged in the PR and release notes.
Summary
Closes #9565.
oidcPromptsGuardaccepted the OIDCnoneprompt alongside other values. Per OIDC Core 1.0 §3.1.2.1,nonerequests that no authentication or consent UI be shown, and a request containingnonewith any other value is an error.Since the combination passed validation it could be saved from the Admin Console, and the connector then joined the values into a single
promptparameter. The identity provider was the first thing in the chain to reject it, so the end user landed on a provider error page instead of a sign-in screen:The guard is shared by the Google and Azure AD connectors, so fixing it in
connector-kitcovers both.Change
The connector already encodes one provider rule next to this — it filters
loginout of Google's select items because Google does not support it — so invalid values were handled; only the mutually-exclusive combination was missed.Tests
New
packages/toolkit/connector-kit/src/types/social.test.tscovers:noneare accepted (select_account consent, and withlogin)nonewith any other prompt is rejected, in three orderingsundefinedare still acceptedI confirmed the test fails against the guard as it stands on
master— parsing["select_account","consent","none"]returnssuccess: truebefore this change andfalseafter, so the test genuinely covers the regression.Verified against
zod@3.24.3as pinned in the workspace root.Notes
@logto/connector-kit(patch).