diff --git a/apps/client/src/components/startup_checks.spec.ts b/apps/client/src/components/startup_checks.spec.ts index e03683f4b6..e3a16df284 100644 --- a/apps/client/src/components/startup_checks.spec.ts +++ b/apps/client/src/components/startup_checks.spec.ts @@ -6,7 +6,7 @@ import toast from "../services/toast"; import { showOAuthEnrollmentResultToast } from "./startup_checks"; vi.mock("../services/server", () => ({ default: { get: vi.fn() } })); -vi.mock("../services/toast", () => ({ default: { showMessage: vi.fn() } })); +vi.mock("../services/toast", () => ({ default: { showMessage: vi.fn(), showErrorTitleAndMessage: vi.fn() } })); // Echo interpolation values so assertions can verify the resolved account/provider. vi.mock("../services/i18n", () => ({ t: (key: string, opts?: Record) => (opts ? `${key} ${JSON.stringify(opts)}` : key) @@ -14,6 +14,7 @@ vi.mock("../services/i18n", () => ({ const serverGet = vi.mocked(server.get); const showMessage = vi.mocked(toast.showMessage); +const showErrorTitleAndMessage = vi.mocked(toast.showErrorTitleAndMessage); function setGlob(glob: Record | undefined) { (window as unknown as { glob?: unknown }).glob = glob; @@ -47,6 +48,22 @@ describe("showOAuthEnrollmentResultToast", () => { expect(showMessage).toHaveBeenCalledWith("multi_factor_authentication.oauth_connect_success_generic"); }); + it("shows the server's technical detail monospace, without probing for an account that was never connected", async () => { + const detail = "fetch failed ← caused by: self-signed certificate [DEPTH_ZERO_SELF_SIGNED_CERT]"; + setGlob({ oauthConnectionFailed: detail }); + + await showOAuthEnrollmentResultToast(); + + expect(showErrorTitleAndMessage).toHaveBeenCalledWith( + "multi_factor_authentication.oauth_connect_failed", + detail, + expect.any(Number), + { monospace: true } + ); + expect(serverGet).not.toHaveBeenCalled(); + expect(showMessage).not.toHaveBeenCalled(); + }); + it("does nothing without the bootstrap flag", async () => { setGlob({}); await showOAuthEnrollmentResultToast(); diff --git a/apps/client/src/components/startup_checks.ts b/apps/client/src/components/startup_checks.ts index 013824e4ec..c845c2f8fd 100644 --- a/apps/client/src/components/startup_checks.ts +++ b/apps/client/src/components/startup_checks.ts @@ -34,12 +34,27 @@ export class StartupChecks extends Component { } /** - * Shows a one-shot "account connected" toast after the OAuth provider round-trip redirects back to the - * app root (which drops the Settings modal). The signal rides in the server's bootstrap payload - * (`window.glob.oauthJustEnrolled`, set once by the OIDC afterCallback and cleared by /bootstrap), so - * nothing has to be stored on the client across the redirect. + * Shows a one-shot toast reporting the outcome of an OAuth provider round-trip once it redirects back + * to the app root (which drops the Settings modal): "account connected" on success, or a failure notice + * when the provider couldn't be reached at all. Both signals ride in the server's bootstrap payload + * (`window.glob.oauthJustEnrolled` / `oauthConnectionFailed`, set once server-side and cleared by + * /bootstrap), so nothing has to be stored on the client across the redirect. */ export async function showOAuthEnrollmentResultToast() { + const connectionFailure = window.glob?.oauthConnectionFailed; + if (connectionFailure) { + // The provider couldn't be reached at all, so there is no account to name. The server's technical + // detail (TLS trust, DNS, refused connection, …) is shown verbatim in monospace beneath the + // heading — it names the actual cause, which a generic "check the log" message never could. + toast.showErrorTitleAndMessage( + t("multi_factor_authentication.oauth_connect_failed"), + connectionFailure, + 15_000, + { monospace: true } + ); + return; + } + if (!window.glob?.oauthJustEnrolled) { return; } diff --git a/apps/client/src/services/toast.ts b/apps/client/src/services/toast.ts index b68878413d..003da7daff 100644 --- a/apps/client/src/services/toast.ts +++ b/apps/client/src/services/toast.ts @@ -84,13 +84,15 @@ export function showError(message: string, timeout = 10000) { }); } -function showErrorTitleAndMessage(title: string, message: string, timeout = 10000) { +function showErrorTitleAndMessage(title: string, message: string, timeout = 10000, opts?: { monospace?: boolean }) { console.log(utils.now(), "error: ", message); addToast({ title, icon: "bx bx-error-circle", message, + // Raw error strings are shown verbatim, so render them monospace. + messageMonospace: opts?.monospace, timeout }); } diff --git a/apps/client/src/translations/en/translation.json b/apps/client/src/translations/en/translation.json index dd920af36a..a19a8645f9 100644 --- a/apps/client/src/translations/en/translation.json +++ b/apps/client/src/translations/en/translation.json @@ -1952,6 +1952,7 @@ "oauth_disconnected": "Disconnected {{account}} from {{provider}}.", "oauth_connect_success": "Connected to {{provider}} as {{account}}.", "oauth_connect_success_generic": "Account connected successfully.", + "oauth_connect_failed": "Could not connect to the OpenID provider", "oauth_account_unknown": "your account" }, "shortcuts": { diff --git a/apps/server/src/express.d.ts b/apps/server/src/express.d.ts index 1c5938ef5e..ec71865b67 100644 --- a/apps/server/src/express.d.ts +++ b/apps/server/src/express.d.ts @@ -44,6 +44,15 @@ export declare module "express-session" { * after the post-enrollment redirect lands on the app root. */ ssoJustEnrolled?: true; + /** + * One-shot technical detail set when the OIDC provider round-trip fails outright (the provider + * is unreachable, its TLS certificate isn't trusted, the token exchange errors, …) rather than + * completing with a rejection. Read and cleared by /bootstrap so the client can show a single + * "connection failed" toast after we redirect back to the app root instead of leaving the user + * on a raw JSON error page. Always non-empty when set, since its presence is what marks the + * failure; bounded in length because it is held in the session store. + */ + ssoConnectionFailed?: string; /** * Transient state for the OneNote importer's delegated-Graph OAuth flow. During sign-in it * holds the PKCE verifier + state + redirect URI; after a successful callback it holds the diff --git a/apps/server/src/routes/index.ts b/apps/server/src/routes/index.ts index bfc45b382f..924daafc1b 100644 --- a/apps/server/src/routes/index.ts +++ b/apps/server/src/routes/index.ts @@ -91,6 +91,11 @@ export function bootstrap(req: Request, res: Response) { if (ssoError) { delete req.session.ssoError; } + // A round-trip that failed before the user was ever logged in lands here rather than on the + // authenticated bootstrap below, so consume its one-shot detail too. It isn't surfaced on the + // login screen (which has its own `ssoError` messaging); leaving it would otherwise fire as a + // stale, confusing toast after a later successful password login. + delete req.session.ssoConnectionFailed; res.send({ ...commonItems, loggedIn: false, @@ -126,6 +131,13 @@ export function bootstrap(req: Request, res: Response) { delete req.session.ssoJustEnrolled; } + // Likewise one-shot: the counterpart detail set when the provider round-trip failed outright, so the + // client can explain why the user was bounced back here instead of connecting. + const oauthConnectionFailed = req.session.ssoConnectionFailed; + if (oauthConnectionFailed) { + delete req.session.ssoConnectionFailed; + } + res.send({ ...commonItems, dbInitialized: true, @@ -133,6 +145,7 @@ export function bootstrap(req: Request, res: Response) { loggedIn: true, csrfToken, oauthJustEnrolled, + oauthConnectionFailed, hasNativeTitleBar: isElectron && nativeTitleBarVisible, hasBackgroundEffects: options.backgroundEffects === "true" && isElectron diff --git a/apps/server/src/services/open_id.spec.ts b/apps/server/src/services/open_id.spec.ts index d44ef150d3..75af5c03c4 100644 --- a/apps/server/src/services/open_id.spec.ts +++ b/apps/server/src/services/open_id.spec.ts @@ -550,10 +550,12 @@ describe("createReactiveOidcMiddleware", () => { async function run(middleware: RequestHandler) { const next = vi.fn() as unknown as NextFunction; - const req = {} as ExpressRequest; - const res = {} as ExpressResponse; + // A failed round-trip redirects back to the app root and flags the session, so both have to be + // present for the middleware to drive them. + const req = { method: "GET", url: "/authenticate", session: {} } as ExpressRequest; + const res = { headersSent: false, redirect: vi.fn() } as unknown as ExpressResponse; await middleware(req, res, next); - return { next, req, res }; + return { next, req, res, redirect: res.redirect as unknown as ReturnType }; } it("passes through without building the OIDC handler when OAuth is not selected", async () => { @@ -664,7 +666,11 @@ describe("createReactiveOidcMiddleware", () => { t.setConfigured(true); t.isRpInitiatedLogoutSupported.mockRejectedValueOnce(new Error("transient discovery failure")); - await expect(run(t.middleware)).rejects.toThrow("transient discovery failure"); + // The failure is reported to the user via the redirect-and-flag path rather than thrown at the + // generic error handler, which would answer this full-page navigation with raw JSON. + const failed = await run(t.middleware); + expect(failed.redirect).toHaveBeenCalledWith("/"); + expect(failed.req.session.ssoConnectionFailed).toContain("transient discovery failure"); expect(t.oidcHandler).not.toHaveBeenCalled(); // Second request: the probe succeeds and the handler is finally built and delegated to. @@ -672,6 +678,61 @@ describe("createReactiveOidcMiddleware", () => { expect(t.buildAuth).toHaveBeenCalledOnce(); expect(t.oidcHandler).toHaveBeenCalledWith(req, res, expect.any(Function)); }); + + // The provider round-trip failing outright (unreachable host, untrusted TLS certificate, token + // exchange error) used to fall through to the generic JSON error handler, stranding the user on a + // `{"message":"fetch failed"}` page mid-way through connecting an account. + it("redirects back to the app root and flags the session when the round-trip fails", async () => { + const t = setup(); + t.setConfigured(true); + // express-openid-connect reports a broken round-trip through next(err), not by rejecting. The + // shape mirrors undici's: an opaque top-level message with the real reason nested in `.cause`. + const tlsFailure = Object.assign(new Error("self-signed certificate"), { code: "DEPTH_ZERO_SELF_SIGNED_CERT" }); + t.oidcHandler.mockImplementation(((_req, _res, next) => + next(new Error("fetch failed", { cause: tlsFailure }))) as RequestHandler); + + const { req, redirect, next } = await run(t.middleware); + + expect(redirect).toHaveBeenCalledWith("/"); + // The detail travels to the client verbatim, so the actionable reason buried in the cause chain + // reaches the user rather than only the opaque top-level "fetch failed". + expect(req.session.ssoConnectionFailed).toBe("fetch failed ← caused by: self-signed certificate [DEPTH_ZERO_SELF_SIGNED_CERT]"); + // The error must not also continue down the chain, or Express would answer the request twice. + expect(next).not.toHaveBeenCalled(); + }); + + // The detail's presence is what marks the failure downstream, so an error that describes to nothing + // must still produce a non-empty string — otherwise the client would read it as "no failure" and the + // user would land back on the app root with no explanation at all. + it("still records a detail for an error that describes to nothing", async () => { + const t = setup(); + t.setConfigured(true); + // An object with neither a message nor any system/OAuth field — describeError yields null for it. + t.oidcHandler.mockImplementation(((_req, _res, next) => next({})) as RequestHandler); + + const { req, redirect } = await run(t.middleware); + + expect(redirect).toHaveBeenCalledWith("/"); + expect(req.session.ssoConnectionFailed).toBeTruthy(); + }); + + // Once the library has begun answering (e.g. it already started the redirect to the provider), we + // can't redirect on top of it — the error has to go to Express instead of causing a double response. + it("defers to the error chain when the response has already started", async () => { + const t = setup(); + t.setConfigured(true); + const failure = new Error("fetch failed"); + t.oidcHandler.mockImplementation(((_req, res, next) => { + (res as { headersSent: boolean }).headersSent = true; + next(failure); + }) as RequestHandler); + + const { req, redirect, next } = await run(t.middleware); + + expect(redirect).not.toHaveBeenCalled(); + expect(next).toHaveBeenCalledWith(failure); + expect(req.session.ssoConnectionFailed).toBeUndefined(); + }); }); /** diff --git a/apps/server/src/services/open_id.ts b/apps/server/src/services/open_id.ts index 76ae8834fe..7b9481c3ce 100644 --- a/apps/server/src/services/open_id.ts +++ b/apps/server/src/services/open_id.ts @@ -2,6 +2,7 @@ import { getLog, options } from "@triliumnext/core"; import type { NextFunction, Request, RequestHandler, Response } from "express"; import type { Session } from "express-openid-connect"; +import { describeError } from "../routes/error_handlers.js"; import config from "./config.js"; import openIDEncryption from "./encryption/open_id_encryption.js"; import sql from "./sql.js"; @@ -307,7 +308,7 @@ export function createReactiveOidcMiddleware(deps: Partial = { // discovery-probe failure, malformed config, etc.) would leave the rejected promise // cached and break every subsequent OAuth request until a server restart. oidcInit = null; - throw error; + return failRoundTrip(req, res, next, error); } } @@ -318,10 +319,51 @@ export function createReactiveOidcMiddleware(deps: Partial = { if (!oidcMiddleware) { return next(); } - return oidcMiddleware(req, res, next); + + // The library reports a broken round-trip by calling next(err) rather than by rejecting, so the + // interception has to happen on `next` — a try/catch around this call would never see it. A bare + // next() (the pass-through for non-OIDC routes) must still propagate untouched. + return oidcMiddleware(req, res, (error?: unknown) => { + if (!error) { + return next(); + } + return failRoundTrip(req, res, next, error); + }); }; } +/** + * Handles an OIDC round-trip that failed outright — as opposed to one that completed with a rejection + * (wrong account, not enrolled), which `afterCallback` already reports via `ssoError`. + * + * Previously such a failure fell through to the generic error handler, which answers with JSON. Since + * `/authenticate` is a full-page navigation, that stranded the user on a raw `{"message":"fetch failed"}` + * page with no way back — particularly unhelpful during first-time setup, where reaching the provider is + * exactly what's being configured. Instead we mirror the success path: flag the session and redirect to + * the app root, letting the client surface a toast carrying the technical detail. + */ +function failRoundTrip(req: Request, res: Response, next: NextFunction, error: unknown) { + // `describeError` unwraps the `.cause` chain, which is where the actionable reason actually lives: + // undici surfaces only "fetch failed" at the top level, with e.g. "self-signed certificate + // [DEPTH_ZERO_SELF_SIGNED_CERT]" nested underneath. Kept non-empty — its presence is what marks the + // failure downstream, so an error that describes to nothing must not read as "no failure". + const detail = describeError(error) || String(error) || "unknown error"; + getLog().error(`OAuth provider round-trip failed on ${req.method} ${req.url}: ${detail}`); + + // Nothing to redirect if the library already started answering; let Express finish it. + if (res.headersSent) { + return next(error); + } + + // Bounded: this rides in the session store until the next bootstrap consumes it, and a deep cause + // chain would otherwise be unbounded. + req.session.ssoConnectionFailed = detail.slice(0, MAX_CONNECTION_FAILURE_DETAIL_LENGTH); + return res.redirect("/"); +} + +/** Upper bound on the technical detail carried to the client; enough for a full cause chain. */ +const MAX_CONNECTION_FAILURE_DETAIL_LENGTH = 300; + export default { generateOAuthConfig, getOAuthStatus, diff --git a/packages/commons/src/lib/server_api.ts b/packages/commons/src/lib/server_api.ts index 9883b97caa..9c0782fe87 100644 --- a/packages/commons/src/lib/server_api.ts +++ b/packages/commons/src/lib/server_api.ts @@ -470,6 +470,13 @@ export type BootstrapDefinition = { * one-shot "account connected" toast once the post-enrollment redirect lands on the app root. */ oauthJustEnrolled?: boolean; + /** + * Set for exactly one bootstrap after an OAuth round-trip failed to reach the provider at all, + * letting the client explain the bounce back to the app root with a one-shot error toast. Carries + * the technical reason (e.g. `fetch failed ← caused by: self-signed certificate + * [DEPTH_ZERO_SELF_SIGNED_CERT]`), shown verbatim in monospace; non-empty whenever present. + */ + oauthConnectionFailed?: string; }; /**