Skip to content

fix(device): surface SSO errors on /device and fix CLI null-account crash on external-SSO login#36781

Merged
wylswz merged 5 commits into
mainfrom
fix/device-sso-error-display
May 29, 2026
Merged

fix(device): surface SSO errors on /device and fix CLI null-account crash on external-SSO login#36781
wylswz merged 5 commits into
mainfrom
fix/device-sso-error-display

Conversation

@GareArc
Copy link
Copy Markdown
Contributor

@GareArc GareArc commented May 28, 2026

Summary

Two related fixes for the device-authorization (RFC 8628) SSO path.

Web — surface SSO errors on /device

When the SSO callback (SAML ACS / OIDC / OAuth2) rejects a login, the enterprise backend redirects the browser to /device?sso_error=<code>. The /device page read sso_verified from the URL but not sso_error, so the failure was silent — the user landed back on a blank code-entry form with no feedback.

The page now maps the sso_error code to friendly copy (ssoErrorCopy() dispatch table) and shows it as an inline banner on the code-entry page. The banner is derived directly from the URL param (no effect, no extra state), so it survives re-render/remount and shows the error on the main page rather than a separate full-screen view. The param is intentionally not scrubbed (non-sensitive error code; keeps the banner stable instead of being wiped by router.replace).

CLI — fix crash on external-SSO device login

The poll-success contract sends account: null for the external_sso subject type (external SSO has no console account). difyctl auth login guarded only !== undefined and then read .id on the result, crashing the SSO login with null is not an object (evaluating 'account.id'). Normalized null/undefined to a single path so the external_subject branch is taken, and widened the PollSuccess.account type to PollAccount | null to match the contract. Updated the device-flow mock's SSO scenario to send account: null so the test reproduces the real server response (the gap that hid the bug).

Tests

  • web page-terminal.spec.tsx — banner shows with friendly copy, code-entry stays visible, raw backend code not surfaced, no scrub-on-mount regression. 11/11 pass.
  • cli login.test.ts — SSO case reproduces the crash pre-fix, passes post-fix; whoami/status already cover both subject types. Full CLI suite 817/817 pass.

Checklist

  • This change requires a documentation update, included: Dify Document
  • I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.
  • I ran make lint && make type-check (backend) and cd web && pnpm exec vp staged (frontend) to appease the lint gods

@GareArc GareArc requested a review from iamjoel as a code owner May 28, 2026 09:21
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. javascript labels May 28, 2026
@github-actions github-actions Bot added the web This relates to changes on the web. label May 28, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.75%. Comparing base (f5ab5e7) to head (24518aa).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #36781   +/-   ##
=======================================
  Coverage   85.75%   85.75%           
=======================================
  Files        4546     4546           
  Lines      222191   222198    +7     
  Branches    40927    40931    +4     
=======================================
+ Hits       190535   190545   +10     
+ Misses      27988    27985    -3     
  Partials     3668     3668           
Flag Coverage Δ
dify-ui 95.57% <ø> (ø)
web 86.46% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

GareArc added 3 commits May 28, 2026 20:10
The prior fix set errMsg(ssoError) and router.replace(pathname) in the same
effect run. router.replace stripped the param and re-rendered, wiping the
just-set state and resetting view to code_entry — so the SSO error never
showed and the page silently bounced back to code entry.

Drive sso_error from a dedicated terminal error_sso view (matching the
existing error_* views), leave the non-sensitive param in the URL, and map
the code to friendly copy via a dispatch table. Add regression tests,
including one asserting router.replace is not called on mount.
Replace the standalone error_sso terminal view with an inline banner
derived directly from the sso_error query param on the code-entry
screen. The banner is pure-rendered from the URL (no effect, no extra
state), so it survives re-render/remount and the error is shown on the
main page instead of a separate view.
The poll-success contract sends account: null for the external_sso
subject type. login guarded only !== undefined and then read .id on
null, crashing the device-flow SSO login. Normalize null/undefined to
one path so the external_subject branch is taken.
@GareArc GareArc requested a review from a team as a code owner May 29, 2026 05:10
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels May 29, 2026
@GareArc GareArc changed the title fix(device): display sso_error query param on /device page fix(device): surface SSO errors on /device and fix CLI null-account crash on external-SSO login May 29, 2026
Replace per-consumer 'const account = s.account ?? undefined' normalization
with direct truthy checks on s.account. Handles both null and undefined in
one guard, drops duplicated coercion across renderLoggedIn/bundleFromSuccess.

Fixes external_subject being skipped on SSO logins (account: null), which
caused the token to be stored under the 'default' key instead of the SSO
email. Pin via store-key assertion in the sso test.
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels May 29, 2026
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label May 29, 2026
@wylswz wylswz added this pull request to the merge queue May 29, 2026
Merged via the queue into main with commit 30270b5 May 29, 2026
38 checks passed
@wylswz wylswz deleted the fix/device-sso-error-display branch May 29, 2026 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files. web This relates to changes on the web.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants