From 7840712b44b273af30d274773f287fa008b0e8ef Mon Sep 17 00:00:00 2001 From: Aleksander <170264518+t-aleksander@users.noreply.github.com> Date: Fri, 16 Oct 2026 13:54:00 +0200 Subject: [PATCH 1/5] better proxy error codes --- src/error.rs | 84 ++++++++++++++++--- src/tests/mfa_config.rs | 2 + src/tests/mfa_flow.rs | 2 +- src/tests/support.rs | 1 + web/messages/en.json | 6 +- web/src/routes/openid/callback.test.ts | 83 ++++++++++++++++++ web/src/routes/openid/callback.tsx | 4 +- web/src/routes/openid/error.tsx | 2 + web/src/routes/openid/mfa/callback.test.ts | 44 +++++++++- web/src/routes/openid/mfa/callback.tsx | 14 +++- web/src/shared/api/apiErrorMessages.ts | 21 +++++ .../shared/components/PageInfo/PageInfo.tsx | 8 ++ web/src/shared/hooks/useOpenIdStore.tsx | 2 + 13 files changed, 251 insertions(+), 22 deletions(-) create mode 100644 web/src/routes/openid/callback.test.ts create mode 100644 web/src/shared/api/apiErrorMessages.ts diff --git a/src/error.rs b/src/error.rs index 6c44cfa0..320336bf 100644 --- a/src/error.rs +++ b/src/error.rs @@ -6,7 +6,7 @@ use axum::{ use serde_json::json; use tonic::{Code, Status, metadata::errors::InvalidMetadataValue}; -use crate::proto::CoreError; +use crate::proto::{CoreError, CoreErrorCode}; /// Core's message prefix for an OIDC MFA poll made before the browser round trip completes. const OIDC_NOT_COMPLETED: &str = "OIDC authentication not completed"; @@ -45,16 +45,13 @@ pub enum ApiError { PostureRejected(Vec), #[error("Failed to get client IP address")] ClientIpError, + #[error("{0}")] + WithCode(Box, CoreErrorCode), } -impl IntoResponse for ApiError { - fn into_response(self) -> Response { - if matches!(self, Self::OidcNotCompleted(_)) { - debug!("{self}"); - } else { - error!("{self}"); - } - let (status, error_message) = match self { +impl ApiError { + fn status_and_message(self) -> (StatusCode, String) { + match self { Self::Unauthorized(msg) => (StatusCode::UNAUTHORIZED, msg), Self::BadRequest(msg) => (StatusCode::BAD_REQUEST, msg), Self::PermissionDenied(msg) => (StatusCode::FORBIDDEN, msg), @@ -67,15 +64,36 @@ impl IntoResponse for ApiError { } Self::NotFound(msg) => (StatusCode::NOT_FOUND, msg), Self::PostureRejected(reasons) => (StatusCode::FORBIDDEN, reasons.join(", ")), + Self::WithCode(inner, _) => inner.status_and_message(), _ => ( StatusCode::INTERNAL_SERVER_ERROR, "Internal server error".to_string(), ), - }; + } + } +} - let body = Json(json!({"error": error_message})); +/// The web UI matches on the lowercase name without the enum prefix, e.g. `oidc_account_mismatch`. +fn code_name(code: CoreErrorCode) -> String { + code.as_str_name() + .trim_start_matches("CORE_ERROR_CODE_") + .to_ascii_lowercase() +} + +impl IntoResponse for ApiError { + fn into_response(self) -> Response { + if matches!(self, Self::OidcNotCompleted(_)) { + debug!("{self}"); + } else { + error!("{self}"); + } + let code = match &self { + Self::WithCode(_, code) => Some(code_name(*code)), + _ => None, + }; + let (status, error_message) = self.status_and_message(); - (status, body).into_response() + (status, Json(json!({"error": error_message, "code": code}))).into_response() } } @@ -92,9 +110,10 @@ impl From for ApiError { if is_oidc_not_completed(&core_error) { return ApiError::OidcNotCompleted(core_error.message); } + let code = core_error.code(); // convert to tonic::Status first let status = Status::new(Code::from(core_error.status_code), core_error.message); - match status.code() { + let error = match status.code() { Code::Unauthenticated => ApiError::Unauthorized(status.message().to_string()), Code::InvalidArgument => ApiError::BadRequest(status.message().to_string()), Code::PermissionDenied => ApiError::PermissionDenied(status.message().to_string()), @@ -106,6 +125,10 @@ impl From for ApiError { Code::Unavailable => ApiError::CoreTimeout, Code::NotFound => ApiError::NotFound(status.to_string()), _ => ApiError::Unexpected(status.to_string()), + }; + match code { + CoreErrorCode::Unspecified => error, + code => ApiError::WithCode(Box::new(error), code), } } } @@ -118,10 +141,45 @@ mod tests { CoreError { status_code: Code::FailedPrecondition as i32, message: message.into(), + ..Default::default() } .into() } + async fn response_json(error: ApiError) -> (StatusCode, serde_json::Value) { + let response = error.into_response(); + let status = response.status(); + let bytes = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + (status, serde_json::from_slice(&bytes).unwrap()) + } + + #[tokio::test] + async fn test_core_error_code_reaches_response_body() { + let coded = CoreError { + status_code: Code::Unauthenticated as i32, + message: "unauthorized".into(), + code: CoreErrorCode::OidcAccountMismatch.into(), + }; + let (status, body) = response_json(coded.into()).await; + assert_eq!(status, StatusCode::UNAUTHORIZED); + assert_eq!( + body, + json!({"error": "unauthorized", "code": "oidc_account_mismatch"}) + ); + + // Older Core versions send no code. + let uncoded = CoreError { + status_code: Code::InvalidArgument as i32, + message: "invalid state data".into(), + ..Default::default() + }; + let (status, body) = response_json(uncoded.into()).await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert_eq!(body, json!({"error": "invalid state data", "code": null})); + } + #[test] fn test_oidc_not_completed_maps_to_own_variant() { // Core's MFA config and client MFA wordings. diff --git a/src/tests/mfa_config.rs b/src/tests/mfa_config.rs index 50eef3ad..9af189f1 100644 --- a/src/tests/mfa_config.rs +++ b/src/tests/mfa_config.rs @@ -171,6 +171,7 @@ async fn test_mfa_config_core_error_maps_to_http_status() { core_response::Payload::CoreError(CoreError { status_code: tonic::Code::Unauthenticated as i32, message: "invalid token".into(), + ..Default::default() }) }); @@ -186,6 +187,7 @@ async fn test_mfa_config_core_error_maps_to_http_status() { core_response::Payload::CoreError(CoreError { status_code: tonic::Code::FailedPrecondition as i32, message: "OIDC authentication not completed".into(), + ..Default::default() }) }); diff --git a/src/tests/mfa_flow.rs b/src/tests/mfa_flow.rs index 54e2c24b..f2f398a8 100644 --- a/src/tests/mfa_flow.rs +++ b/src/tests/mfa_flow.rs @@ -87,7 +87,7 @@ async fn test_start_maps_posture_rejection_to_forbidden() { let (status, body) = post_json(&app, "/api/v1/mfa-flow/start", &request).await; assert_eq!(status, StatusCode::FORBIDDEN); - assert_eq!(body, json!({"error": "OS version"})); + assert_eq!(body, json!({"error": "OS version", "code": null})); } #[tokio::test] diff --git a/src/tests/support.rs b/src/tests/support.rs index 7bceb2ec..84cdeb56 100644 --- a/src/tests/support.rs +++ b/src/tests/support.rs @@ -74,6 +74,7 @@ where core_response::Payload::CoreError(CoreError { status_code: tonic::Code::Internal as i32, message: format!("fake Core panicked: {message}"), + ..Default::default() }) }); responder.resolve_test_response(request.id, response); diff --git a/web/messages/en.json b/web/messages/en.json index e606ca2a..cd5412ae 100644 --- a/web/messages/en.json +++ b/web/messages/en.json @@ -104,9 +104,13 @@ "openid_mfa_redirect_error_title": "Authentication Error", "openid_mfa_redirect_error_message": "No token provided in the URL. Please ensure you have a valid token to proceed with OpenID authentication.", "openid_mfa_redirect_error_missing_args": "Missing code or state in the callback's URL. The provider might not be configured correctly.", + "openid_mfa_callback_error": "We couldn't verify your sign-in. Make sure you signed in with the account this Defguard VPN Client belongs to, then try connecting again.", + "api_error_oidc_account_mismatch": "You signed in with a different account than the one this Defguard VPN Client belongs to. Sign out of your identity provider, then try connecting again with the correct account.", + "api_error_oidc_verification_failed": "We couldn't verify your sign-in with your identity provider. Make sure you use the account linked to your Defguard user, then try connecting again.", + "api_error_mfa_session_expired": "This sign-in request has expired or was already used. Start connecting again from the Defguard VPN Client.", "openid_mfa_complete_title": "Authentication Completed", "openid_mfa_complete_subtitle": "You have been successfully authenticated. Please close this window and get back to the Defguard VPN Client", - "openid_generic_error": "OpenID authentication error", + "openid_generic_error": "Sign-in with your identity provider failed. Try again, or contact your administrator if the problem continues.", "open_desktop_title": "Open the desktop app to continue", "open_desktop_description": "We tried to open the desktop app automatically, but it didn't respond. This can happen if the browser blocks the request or the app didn't start in time.", "open_desktop_button": "Open desktop app", diff --git a/web/src/routes/openid/callback.test.ts b/web/src/routes/openid/callback.test.ts new file mode 100644 index 00000000..ce7515c4 --- /dev/null +++ b/web/src/routes/openid/callback.test.ts @@ -0,0 +1,83 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { Route } from './callback'; + +const mocks = vi.hoisted(() => ({ + callback: vi.fn(), + setError: vi.fn(), +})); + +vi.mock('../../shared/api/api', () => ({ + api: { + openId: { + enrollmentCallback: { + callbackFn: mocks.callback, + }, + }, + enrollment: { + start: { + callbackFn: vi.fn(), + }, + }, + }, +})); +vi.mock('../../paraglide/messages', () => ({ + m: { + openid_generic_error: () => 'Sign-in failed', + }, +})); +vi.mock('../../shared/hooks/useEnrollmentStore', () => ({ + useEnrollmentStore: { + setState: vi.fn(), + }, +})); +vi.mock('../../shared/hooks/useOpenIdStore', () => ({ + useOpenidStore: { + setState: mocks.setError, + }, +})); + +const beforeLoad = () => + ( + Route.options.beforeLoad as (args: { + search: { code: string; state: string }; + }) => Promise + )({ + search: { code: 'code', state: 'state' }, + }); + +describe('OpenID enrollment callback route', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('shows a friendly error with the provider error as details', async () => { + mocks.callback.mockRejectedValue({ + isAxiosError: true, + response: { + data: { + error: 'invalid_client: The provided client secret is invalid.', + }, + }, + }); + + await expect(beforeLoad()).rejects.toMatchObject({ + options: { replace: true, to: '/openid/error' }, + }); + expect(mocks.setError).toHaveBeenCalledWith({ + error: 'Sign-in failed', + details: 'invalid_client: The provided client secret is invalid.', + }); + }); + + it('shows no details when the failure has no error text', async () => { + mocks.callback.mockRejectedValue(new Error('Network Error')); + + await expect(beforeLoad()).rejects.toMatchObject({ + options: { replace: true, to: '/openid/error' }, + }); + expect(mocks.setError).toHaveBeenCalledWith({ + error: 'Sign-in failed', + details: undefined, + }); + }); +}); diff --git a/web/src/routes/openid/callback.tsx b/web/src/routes/openid/callback.tsx index 67ba7cbf..405b3024 100644 --- a/web/src/routes/openid/callback.tsx +++ b/web/src/routes/openid/callback.tsx @@ -2,6 +2,7 @@ import { createFileRoute, redirect } from '@tanstack/react-router'; import z from 'zod'; import { m } from '../../paraglide/messages'; import { api } from '../../shared/api/api'; +import { getApiErrorDetails } from '../../shared/api/apiErrorMessages'; import { useEnrollmentStore } from '../../shared/hooks/useEnrollmentStore'; import { useOpenidStore } from '../../shared/hooks/useOpenIdStore'; @@ -29,7 +30,8 @@ export const Route = createFileRoute('/openid/callback')({ }) .catch((e) => { useOpenidStore.setState({ - error: e.response?.data?.error ?? m.openid_generic_error(), + error: m.openid_generic_error(), + details: getApiErrorDetails(e), }); throw redirect({ to: '/openid/error', replace: true }); }); diff --git a/web/src/routes/openid/error.tsx b/web/src/routes/openid/error.tsx index 30d207a9..72f1343c 100644 --- a/web/src/routes/openid/error.tsx +++ b/web/src/routes/openid/error.tsx @@ -11,11 +11,13 @@ function RouteComponent() { const openIdError = useOpenidStore( (s) => s.error ?? m.openid_mfa_redirect_error_message(), ); + const details = useOpenidStore((s) => s.details); return ( ); diff --git a/web/src/routes/openid/mfa/callback.test.ts b/web/src/routes/openid/mfa/callback.test.ts index e7bdfccd..e938df82 100644 --- a/web/src/routes/openid/mfa/callback.test.ts +++ b/web/src/routes/openid/mfa/callback.test.ts @@ -17,8 +17,11 @@ vi.mock('../../../shared/api/api', () => ({ })); vi.mock('../../../paraglide/messages', () => ({ m: { - openid_mfa_redirect_error_message: () => 'MFA callback error', + openid_mfa_callback_error: () => 'MFA callback error', openid_mfa_redirect_error_missing_args: () => 'Missing callback arguments', + api_error_oidc_account_mismatch: () => 'Wrong account', + api_error_oidc_verification_failed: () => 'Verification failed', + api_error_mfa_session_expired: () => 'Session expired', }, })); vi.mock('../../../shared/components/PageInfo/PageInfo', () => ({ @@ -69,7 +72,10 @@ describe('OpenID MFA callback route', () => { to: '/openid/error', }, }); - expect(mocks.setError).toHaveBeenCalledWith({ error: 'MFA callback error' }); + expect(mocks.setError).toHaveBeenCalledWith({ + error: 'MFA callback error', + details: 'provider rejected', + }); }); it('uses the MFA error when the response has no error message', async () => { @@ -84,7 +90,34 @@ describe('OpenID MFA callback route', () => { to: '/openid/error', }, }); - expect(mocks.setError).toHaveBeenCalledWith({ error: 'MFA callback error' }); + expect(mocks.setError).toHaveBeenCalledWith({ + error: 'MFA callback error', + details: undefined, + }); + }); + + it('shows the message for the error code Core sent', async () => { + for (const [code, message] of [ + ['oidc_account_mismatch', 'Wrong account'], + ['oidc_verification_failed', 'Verification failed'], + ['mfa_session_expired', 'Session expired'], + ['code_from_a_newer_core', 'MFA callback error'], + ['constructor', 'MFA callback error'], + ]) { + mocks.setError.mockClear(); + mocks.callback.mockRejectedValue({ + isAxiosError: true, + response: { data: { error: 'unauthorized', code } }, + }); + + await expect(load({ code: 'code', state: 'state' })).rejects.toMatchObject({ + options: { replace: true, to: '/openid/error' }, + }); + expect(mocks.setError).toHaveBeenCalledWith({ + error: message, + details: 'unauthorized', + }); + } }); it('uses the MFA error for non-Axios callback failures', async () => { @@ -96,6 +129,9 @@ describe('OpenID MFA callback route', () => { to: '/openid/error', }, }); - expect(mocks.setError).toHaveBeenCalledWith({ error: 'MFA callback error' }); + expect(mocks.setError).toHaveBeenCalledWith({ + error: 'MFA callback error', + details: undefined, + }); }); }); diff --git a/web/src/routes/openid/mfa/callback.tsx b/web/src/routes/openid/mfa/callback.tsx index c4952f9f..b6873a69 100644 --- a/web/src/routes/openid/mfa/callback.tsx +++ b/web/src/routes/openid/mfa/callback.tsx @@ -2,6 +2,10 @@ import { createFileRoute, redirect } from '@tanstack/react-router'; import z from 'zod'; import { m } from '../../../paraglide/messages'; import { api } from '../../../shared/api/api'; +import { + getApiErrorDetails, + getApiErrorMessage, +} from '../../../shared/api/apiErrorMessages'; import { PageInfo } from '../../../shared/components/PageInfo/PageInfo'; import { useOpenidStore } from '../../../shared/hooks/useOpenIdStore'; @@ -13,7 +17,10 @@ const searchSchema = z.object({ export const Route = createFileRoute('/openid/mfa/callback')({ validateSearch: searchSchema, onError: () => { - useOpenidStore.setState({ error: m.openid_mfa_redirect_error_missing_args() }); + useOpenidStore.setState({ + error: m.openid_mfa_redirect_error_missing_args(), + details: undefined, + }); throw redirect({ to: '/openid/error', replace: true }); }, loaderDeps: ({ search }) => ({ search }), @@ -28,7 +35,10 @@ export const Route = createFileRoute('/openid/mfa/callback')({ }); } catch (e) { console.error('OIDC MFA callback failed', e); - useOpenidStore.setState({ error: m.openid_mfa_redirect_error_message() }); + useOpenidStore.setState({ + error: getApiErrorMessage(e, m.openid_mfa_callback_error()), + details: getApiErrorDetails(e), + }); throw redirect({ to: '/openid/error', replace: true }); } }, diff --git a/web/src/shared/api/apiErrorMessages.ts b/web/src/shared/api/apiErrorMessages.ts new file mode 100644 index 00000000..6a35b246 --- /dev/null +++ b/web/src/shared/api/apiErrorMessages.ts @@ -0,0 +1,21 @@ +import { isAxiosError } from 'axios'; +import { m } from '../../paraglide/messages'; + +export type ApiErrorBody = { + error?: string; + code?: string | null; +}; + +const apiErrorMessages = new Map([ + ['oidc_account_mismatch', m.api_error_oidc_account_mismatch], + ['oidc_verification_failed', m.api_error_oidc_verification_failed], + ['mfa_session_expired', m.api_error_mfa_session_expired], +]); + +export const getApiErrorMessage = (error: unknown, fallback: string): string => { + const code = isAxiosError(error) ? error.response?.data?.code : undefined; + return apiErrorMessages.get(code ?? '')?.() ?? fallback; +}; + +export const getApiErrorDetails = (error: unknown): string | undefined => + (isAxiosError(error) && error.response?.data?.error) || undefined; diff --git a/web/src/shared/components/PageInfo/PageInfo.tsx b/web/src/shared/components/PageInfo/PageInfo.tsx index 6a05392c..e5e3a5cf 100644 --- a/web/src/shared/components/PageInfo/PageInfo.tsx +++ b/web/src/shared/components/PageInfo/PageInfo.tsx @@ -13,6 +13,7 @@ import { Page } from '../Page/Page'; type Props = { title: string; subtitle: string; + details?: string; linkText?: string; link?: string; icon?: IconKindValue; @@ -23,6 +24,7 @@ export const PageInfo = ({ link, linkText, subtitle, + details, title, icon = 'check-circle', imageSrc, @@ -38,6 +40,12 @@ export const PageInfo = ({ {subtitle} + {isPresent(details) && ( + <> +
+ {details} + + )} {isPresent(linkText) && isPresent(link) && ( <> diff --git a/web/src/shared/hooks/useOpenIdStore.tsx b/web/src/shared/hooks/useOpenIdStore.tsx index 4839c05c..1b6e4512 100644 --- a/web/src/shared/hooks/useOpenIdStore.tsx +++ b/web/src/shared/hooks/useOpenIdStore.tsx @@ -4,10 +4,12 @@ type Store = StoreValues; type StoreValues = { error?: string; + details?: string; }; const defaults: StoreValues = { error: undefined, + details: undefined, }; export const useOpenidStore = create(() => ({ From f336c7ba429801650951bc269203954a2aa8d9bd Mon Sep 17 00:00:00 2001 From: Aleksander <170264518+t-aleksander@users.noreply.github.com> Date: Fri, 16 Oct 2026 13:58:32 +0200 Subject: [PATCH 2/5] proto --- proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proto b/proto index f1565967..cb018253 160000 --- a/proto +++ b/proto @@ -1 +1 @@ -Subproject commit f1565967e17d6ca714b66246685af1f8f6200ee4 +Subproject commit cb01825320fc1fc49e5fc1cbfc2be2cd676b90d5 From 67655fdef80051f6e574692e3fe9df1b2b751052 Mon Sep 17 00:00:00 2001 From: Aleksander <170264518+t-aleksander@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:08:50 +0200 Subject: [PATCH 3/5] fix vitest --- web/vitest.config.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/web/vitest.config.ts b/web/vitest.config.ts index 644e94c0..02aced3b 100644 --- a/web/vitest.config.ts +++ b/web/vitest.config.ts @@ -1,8 +1,16 @@ +import { paraglideVitePlugin } from '@inlang/paraglide-js'; import react from '@vitejs/plugin-react-swc'; import { defineConfig } from 'vitest/config'; export default defineConfig({ - plugins: [react()], + plugins: [ + paraglideVitePlugin({ + project: './project.inlang', + outdir: './src/paraglide', + strategy: ['localStorage', 'preferredLanguage', 'baseLocale'], + }), + react(), + ], test: { environment: 'node', }, From e3100f983dbe22227fcfc382c0c62bb997693aed Mon Sep 17 00:00:00 2001 From: Aleksander <170264518+t-aleksander@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:24:31 +0200 Subject: [PATCH 4/5] fix --- web/src/shared/components/PageInfo/PageInfo.tsx | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/web/src/shared/components/PageInfo/PageInfo.tsx b/web/src/shared/components/PageInfo/PageInfo.tsx index e5e3a5cf..23b9e6ad 100644 --- a/web/src/shared/components/PageInfo/PageInfo.tsx +++ b/web/src/shared/components/PageInfo/PageInfo.tsx @@ -40,13 +40,12 @@ export const PageInfo = ({ {subtitle} - {isPresent(details) && ( - <> -
- {details} - - )} + {isPresent(details) && ( + + {details} + + )} {isPresent(linkText) && isPresent(link) && ( <> From c4eef6c2ca57ab9bc8a63bfd11e34317f310f7ee Mon Sep 17 00:00:00 2001 From: Aleksander <170264518+t-aleksander@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:46:02 +0200 Subject: [PATCH 5/5] bump deps --- Cargo.lock | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c2cc78ff..63ded0f7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -71,9 +71,9 @@ checksum = "683d7910e743518b0e34f1186f92494becacb047c7b6bf616c96772180fef923" [[package]] name = "ammonia" -version = "4.2.0" +version = "4.2.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3f7ed3ac4252be4d3e36fb589ed5659356842587b21a31596a9dc89d3b2f5e2a" +checksum = "061e83b03c2681c18a6787d956e355c74e0b98ba7ba3d69b0822ade1e6f1d716" dependencies = [ "cssparser", "html5ever", @@ -4117,9 +4117,9 @@ dependencies = [ [[package]] name = "rustls" -version = "0.23.45" +version = "0.23.46" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0d41d731c7d2f962d1ccc364cec258de3c0e93b38c2fb3ba97ac74513048d634" +checksum = "48e13bd8c0e9365c43cfa5c9e8f9ad49d3c8444926c9aac819e0e4dc503c8fdf" dependencies = [ "aws-lc-rs", "log", @@ -4991,7 +4991,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" dependencies = [ "fastrand", - "getrandom 0.4.3", + "getrandom 0.3.4", "once_cell", "rustix", "windows-sys 0.61.2",