diff --git a/api/controllers/openapi/oauth_device_sso.py b/api/controllers/openapi/oauth_device_sso.py index 0218d143309..79538f48059 100644 --- a/api/controllers/openapi/oauth_device_sso.py +++ b/api/controllers/openapi/oauth_device_sso.py @@ -13,8 +13,10 @@ handlers do redirects + cookie kwargs that don't fit the Resource shape. from __future__ import annotations import logging +import re import secrets from dataclasses import dataclass +from urllib.parse import urlencode from flask import jsonify, make_response, redirect, request from pydantic import ValidationError @@ -74,6 +76,21 @@ STATE_ENVELOPE_TTL_SECONDS = 15 * 60 # Canonical sso-complete path. IdP-side ACS callback URL must point here. _SSO_COMPLETE_PATH = "/openapi/v1/oauth/device/sso-complete" +_ALLOWED_SSO_ERRORS = {"sso_failed", "email_belongs_to_dify_account"} + +# user_code only ever reaches the redirect as a urlencoded query value; the +# charset bound additionally forbids the path/scheme separators a redirection +# attack would need, so an untrusted value cannot escape the fixed /device path. +_USER_CODE_RE = re.compile(r"\A[A-Z0-9-]{1,16}\Z") + + +def _device_error_redirect(code: str, user_code: str | None = None): + safe_code = code if code in _ALLOWED_SSO_ERRORS else "sso_failed" + params: dict[str, str] = {"sso_error": safe_code} + if user_code and _USER_CODE_RE.match(user_code): + params["user_code"] = user_code + return redirect(f"/device?{urlencode(params)}", code=302) + def _trusted_origin() -> str: base = (dify_config.CONSOLE_API_URL or "").rstrip("/") @@ -134,9 +151,21 @@ def sso_initiate(): @bp.route("/oauth/device/sso-complete", methods=["GET"]) @enterprise_only def sso_complete(): + try: + return _sso_complete_impl() + except Exception: + logger.exception("sso-complete: unhandled") + return _device_error_redirect("sso_failed") + + +def _sso_complete_impl(): + inbound_error = request.args.get("sso_error") + if inbound_error: + return _device_error_redirect(inbound_error, request.args.get("user_code")) + blob = request.args.get("sso_assertion") if not blob: - raise BadRequest("sso_assertion required") + return _device_error_redirect("sso_failed") keyset = jws.KeySet.from_shared_secret() @@ -144,25 +173,26 @@ def sso_complete(): raw_claims = jws.verify(keyset, blob, expected_aud=jws.AUD_EXT_SUBJECT_ASSERTION) except jws.VerifyError as e: logger.warning("sso-complete: rejected assertion: %s", e) - raise BadRequest("invalid_sso_assertion") from e + return _device_error_redirect("sso_failed") try: claims = ExtSubjectAssertionClaims.model_validate(raw_claims) except ValidationError as e: logger.warning("sso-complete: claim shape invalid: %s", e) - raise BadRequest("invalid_sso_assertion") from e - - if not consume_sso_assertion_nonce(redis_client, claims.nonce): - raise BadRequest("invalid_sso_assertion") + return _device_error_redirect("sso_failed") user_code = claims.user_code.strip().upper() + + if not consume_sso_assertion_nonce(redis_client, claims.nonce): + return _device_error_redirect("sso_failed", user_code) + store = DeviceFlowRedis(redis_client) found = store.load_by_user_code(user_code) if found is None: - raise Conflict("user_code_not_pending") + return _device_error_redirect("sso_failed", user_code) _, state = found if state.status is not DeviceFlowStatus.PENDING: - raise Conflict("user_code_not_pending") + return _device_error_redirect("sso_failed", user_code) if AccountService.has_active_account_with_email(db.session, claims.email): _emit_external_rejection_audit( @@ -170,7 +200,7 @@ def sso_complete(): _RejectedClaims(subject_email=claims.email, subject_issuer=claims.issuer), reason="email_belongs_to_dify_account", ) - return redirect("/device?sso_error=email_belongs_to_dify_account", code=302) + return _device_error_redirect("email_belongs_to_dify_account", user_code) iss = _trusted_origin() cookie_value, _ = mint_approval_grant( diff --git a/api/tests/unit_tests/controllers/openapi/test_device_sso.py b/api/tests/unit_tests/controllers/openapi/test_device_sso.py index 0125c583f04..655a5940f47 100644 --- a/api/tests/unit_tests/controllers/openapi/test_device_sso.py +++ b/api/tests/unit_tests/controllers/openapi/test_device_sso.py @@ -1,6 +1,7 @@ """SSO-branch device-flow endpoints under /openapi/v1/oauth/device/.""" import builtins +from unittest.mock import MagicMock, patch import pytest from flask import Flask @@ -77,3 +78,97 @@ def test_sso_complete_idp_callback_url_uses_canonical_path(): from controllers.openapi import oauth_device_sso assert oauth_device_sso._SSO_COMPLETE_PATH == "/openapi/v1/oauth/device/sso-complete" + + +# --------------------------------------------------------------------------- +# _device_error_redirect helper +# --------------------------------------------------------------------------- + + +def test_device_error_redirect_builds_relative_location(): + from controllers.openapi import oauth_device_sso + + app = Flask(__name__) + with app.test_request_context(): + resp = oauth_device_sso._device_error_redirect("sso_failed", "ABCD-1234") + assert resp.status_code == 302 + loc = resp.headers["Location"] + assert loc.startswith("/device?") + assert "sso_error=sso_failed" in loc + assert "user_code=ABCD-1234" in loc + + +def test_device_error_redirect_clamps_unknown_code(): + from controllers.openapi import oauth_device_sso + + app = Flask(__name__) + with app.test_request_context(): + resp = oauth_device_sso._device_error_redirect("totally-bogus") + assert "sso_error=sso_failed" in resp.headers["Location"] + + +def test_device_error_redirect_keeps_email_special_case(): + from controllers.openapi import oauth_device_sso + + app = Flask(__name__) + with app.test_request_context(): + resp = oauth_device_sso._device_error_redirect("email_belongs_to_dify_account", "ABCD-1234") + assert "sso_error=email_belongs_to_dify_account" in resp.headers["Location"] + + +def test_device_error_redirect_omits_empty_user_code(): + from controllers.openapi import oauth_device_sso + + app = Flask(__name__) + with app.test_request_context(): + resp = oauth_device_sso._device_error_redirect("sso_failed") + assert "user_code=" not in resp.headers["Location"] + + +def test_device_error_redirect_drops_malformed_user_code(): + from controllers.openapi import oauth_device_sso + + app = Flask(__name__) + with app.test_request_context(): + resp = oauth_device_sso._device_error_redirect("sso_failed", "https://evil.example/") + loc = resp.headers["Location"] + assert loc.startswith("/device?") + assert "user_code=" not in loc + assert "evil" not in loc + + +# --------------------------------------------------------------------------- +# sso_complete redirect behaviour +# --------------------------------------------------------------------------- + + +def _ee_features(): + from services.feature_service import LicenseStatus + + m = MagicMock() + m.license.status = LicenseStatus.ACTIVE + return m + + +@patch("libs.device_flow_security.FeatureService.get_system_features") +def test_sso_complete_relays_inbound_sso_error(ee_feat, openapi_app): + ee_feat.return_value = _ee_features() + client = openapi_app.test_client() + resp = client.get( + "/openapi/v1/oauth/device/sso-complete?sso_error=sso_failed&user_code=ABCD-1234", + follow_redirects=False, + ) + assert resp.status_code == 302 + loc = resp.headers["Location"] + assert "/device?" in loc + assert "sso_error=sso_failed" in loc + assert "user_code=ABCD-1234" in loc + + +@patch("libs.device_flow_security.FeatureService.get_system_features") +def test_sso_complete_missing_assertion_redirects_generic(ee_feat, openapi_app): + ee_feat.return_value = _ee_features() + client = openapi_app.test_client() + resp = client.get("/openapi/v1/oauth/device/sso-complete", follow_redirects=False) + assert resp.status_code == 302 + assert "sso_error=sso_failed" in resp.headers["Location"] diff --git a/api/tests/unit_tests/controllers/openapi/test_oauth_sso_claims.py b/api/tests/unit_tests/controllers/openapi/test_oauth_sso_claims.py index 9eb4cf01f41..9551a37142f 100644 --- a/api/tests/unit_tests/controllers/openapi/test_oauth_sso_claims.py +++ b/api/tests/unit_tests/controllers/openapi/test_oauth_sso_claims.py @@ -34,8 +34,9 @@ def test_sso_complete_rejects_assertion_missing_email(ee_feat, jws_mod, app: Fla jws_mod.VerifyError = Exception client = app.test_client() - resp = client.get("/openapi/v1/oauth/device/sso-complete?sso_assertion=blob") - assert resp.status_code == 400, resp.data + resp = client.get("/openapi/v1/oauth/device/sso-complete?sso_assertion=blob", follow_redirects=False) + assert resp.status_code == 302, resp.data + assert "sso_error=sso_failed" in resp.headers["Location"] @patch("controllers.openapi.oauth_device_sso.jws") @@ -48,8 +49,9 @@ def test_sso_complete_rejects_assertion_empty_issuer(ee_feat, jws_mod, app: Flas jws_mod.VerifyError = Exception client = app.test_client() - resp = client.get("/openapi/v1/oauth/device/sso-complete?sso_assertion=blob") - assert resp.status_code == 400 + resp = client.get("/openapi/v1/oauth/device/sso-complete?sso_assertion=blob", follow_redirects=False) + assert resp.status_code == 302 + assert "sso_error=sso_failed" in resp.headers["Location"] def test_verify_approval_grant_raises_on_missing_field(): diff --git a/web/app/device/__tests__/page-terminal.spec.tsx b/web/app/device/__tests__/page-terminal.spec.tsx index aba7b7391ac..e9fe1c0055a 100644 --- a/web/app/device/__tests__/page-terminal.spec.tsx +++ b/web/app/device/__tests__/page-terminal.spec.tsx @@ -121,40 +121,53 @@ describe('error_lookup_failed terminal state', () => { }) }) -describe('sso_error inline banner on the code-entry page', () => { - const SSO_BANNER_COPY = 'deviceFlow.ssoError.emailBelongsToDifyAccount' +describe('error_sso dedicated view', () => { + const TITLE = 'deviceFlow.errorSso.title' + const GENERIC = 'deviceFlow.ssoError.default' + const EMAIL_COPY = 'deviceFlow.ssoError.emailBelongsToDifyAccount' + const BACK_TO_LOGIN = 'deviceFlow.errorSso.backToLoginOptions' - it('shows the error banner with friendly copy when sso_error is present', async () => { - mockSearchParams = { sso_error: 'email_belongs_to_dify_account' } + it('renders the dedicated SSO error screen (not the code-entry page)', async () => { + mockSearchParams = { sso_error: 'sso_failed', user_code: 'ABCD-3456' } render() - expect(await screen.findByText(SSO_BANNER_COPY)).toBeInTheDocument() + expect(await screen.findByText(TITLE)).toBeInTheDocument() + expect(await screen.findByText(GENERIC)).toBeInTheDocument() + expect(screen.queryByRole('textbox')).not.toBeInTheDocument() }) - it('keeps the code-entry screen visible (error on main page, not a separate view)', async () => { - mockSearchParams = { sso_error: 'email_belongs_to_dify_account' } + it('shows the email special-case copy', async () => { + mockSearchParams = { sso_error: 'email_belongs_to_dify_account', user_code: 'ABCD-3456' } render() - await screen.findByText(SSO_BANNER_COPY) - expect(screen.getByRole('textbox')).toBeInTheDocument() - expect(screen.getByRole('button', { name: /deviceFlow.codeEntry.continue/i })).toBeInTheDocument() + expect(await screen.findByText(EMAIL_COPY)).toBeInTheDocument() }) - it('does not surface the raw backend error code', async () => { - mockSearchParams = { sso_error: 'email_belongs_to_dify_account' } + it('never surfaces the raw backend code', async () => { + mockSearchParams = { sso_error: 'email_belongs_to_dify_account', user_code: 'ABCD-3456' } render() - await screen.findByText(SSO_BANNER_COPY) + await screen.findByText(EMAIL_COPY) expect(screen.queryByText('email_belongs_to_dify_account')).not.toBeInTheDocument() }) - it('does not scrub the param on mount (regression: error was wiped by router.replace)', async () => { - mockSearchParams = { sso_error: 'email_belongs_to_dify_account' } + it('scrubs sso_error + user_code from the URL on mount', async () => { + mockSearchParams = { sso_error: 'sso_failed', user_code: 'ABCD-3456' } render() - await screen.findByText(SSO_BANNER_COPY) - expect(mockReplace).not.toHaveBeenCalled() + await screen.findByText(TITLE) + expect(mockReplace).toHaveBeenCalledWith('/device') }) - it('shows no banner when sso_error is absent', () => { + it('"Back to login options" re-checks the code and advances to the chooser', async () => { + mockSearchParams = { sso_error: 'sso_failed', user_code: 'ABCD-3456' } + mockDeviceLookup.mockResolvedValue({ valid: true }) + render() + await screen.findByText(TITLE) + fireEvent.click(screen.getByRole('button', { name: BACK_TO_LOGIN })) + await screen.findByText('chooser.subtitle') + expect(mockDeviceLookup).toHaveBeenCalledWith('ABCD-3456') + }) + + it('shows no SSO error screen when sso_error is absent', () => { render() expect(screen.getByRole('textbox')).toBeInTheDocument() - expect(screen.queryByText(SSO_BANNER_COPY)).not.toBeInTheDocument() + expect(screen.queryByText(TITLE)).not.toBeInTheDocument() }) }) diff --git a/web/app/device/page.tsx b/web/app/device/page.tsx index 048d7e44ae5..75d0be64b76 100644 --- a/web/app/device/page.tsx +++ b/web/app/device/page.tsx @@ -26,6 +26,7 @@ type View | { kind: 'error_expired' } | { kind: 'error_rate_limited' } | { kind: 'error_lookup_failed' } + | { kind: 'error_sso', code: string, userCode: string } export default function DevicePage() { const { t } = useTranslation('deviceFlow') @@ -74,6 +75,11 @@ export default function DevicePage() { useEffect(() => { if (view.kind !== 'code_entry' && view.kind !== 'chooser') return + if (ssoError) { + setView({ kind: 'error_sso', code: ssoError, userCode: urlUserCode }) // eslint-disable-line react/set-state-in-effect + router.replace(pathname) + return + } // Post-login bounce: chooser holds the typed code, account just loaded. // The URL was already scrubbed on the first effect run, so urlUserCode // is empty here — advance using the userCode stashed in view state. @@ -95,13 +101,11 @@ export default function DevicePage() { } if (consumed && (urlUserCode || ssoVerified)) router.replace(pathname) - }, [urlUserCode, ssoVerified, account, view, router, pathname]) + }, [urlUserCode, ssoVerified, ssoError, account, view, router, pathname]) - const onContinue = async () => { - if (!isValidUserCode(typed)) - return + const advanceFromCode = async (code: string) => { try { - const reply = await deviceLookup(typed) + const reply = await deviceLookup(code) if (!reply.valid) { setView({ kind: 'error_expired' }) return @@ -118,20 +122,20 @@ export default function DevicePage() { return } if (account) - setView({ kind: 'authorize_account', userCode: typed }) - else setView({ kind: 'chooser', userCode: typed }) + setView({ kind: 'authorize_account', userCode: code }) + else setView({ kind: 'chooser', userCode: code }) + } + + const onContinue = async () => { + if (!isValidUserCode(typed)) + return + await advanceFromCode(typed) } return ( <> {view.kind === 'code_entry' && (
- {ssoError && ( -
- -

{ssoErrorCopy(ssoError, t)}

-
- )}

{t('codeEntry.title')}

@@ -273,6 +277,31 @@ export default function DevicePage() {

)} + {view.kind === 'error_sso' && ( +
+
+
+

{t('errorSso.title')}

+

{ssoErrorCopy(view.code, t)}

+ + +
+ )} + {errMsg && (

{errMsg}

)} diff --git a/web/i18n/en-US/device-flow.json b/web/i18n/en-US/device-flow.json index 662a9734b39..18a69ff2217 100644 --- a/web/i18n/en-US/device-flow.json +++ b/web/i18n/en-US/device-flow.json @@ -36,6 +36,8 @@ "errorLookupFailed.title": "Could not verify the code", "errorRateLimited.body": "Wait a moment and try again.", "errorRateLimited.title": "Too many attempts", + "errorSso.backToLoginOptions": "← Back to login options", + "errorSso.title": "Sign-in could not be completed", "ssoError.default": "Single sign-on could not be completed. Try again.", "ssoError.emailBelongsToDifyAccount": "This identity is linked to a Dify account. Use “Sign in with Dify account” instead.", "success.goToConsole": "Go to Dify console →",