mirror of
https://github.com/zadam/trilium.git
synced 2026-09-14 11:06:04 +05:00
fix(oauth): report a failed provider round-trip in the app instead of as raw JSON
A round-trip that failed outright — unreachable provider, untrusted TLS
certificate, token-exchange error — fell through to the generic error handler,
which answers with JSON. Since /authenticate is a full-page navigation, that
stranded the user on a bare {"message":"fetch failed"} page with no way back,
which is especially unhelpful during first-time setup, where reaching the
provider is exactly what is being configured.
Mirror the existing success path instead: record the failure on the session,
redirect back to the app root, and let the client surface a one-shot toast. The
toast carries the technical detail verbatim in monospace, taken from
describeError, so the actionable reason reaches the user rather than only the
opaque top-level message — undici reports "fetch failed" and keeps the real
cause ("self-signed certificate [DEPTH_ZERO_SELF_SIGNED_CERT]") in .cause.
Note that express-openid-connect reports a broken round-trip by calling
next(err) rather than by rejecting, so the interception is on next; a try/catch
around the handoff would never fire. Where the response has already started, the
error is passed on to Express instead, to avoid answering the request twice.
The detail is only ever shown to an authenticated user: a failure from a
logged-out session is consumed and discarded on the pre-auth bootstrap, which
keeps its existing generic ssoError messaging. That avoids disclosing issuer
hostnames and error codes to an unauthenticated visitor, and prevents the flag
lingering to fire as a stale toast after a later password login.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
5b0c075d78
commit
7ddc045ed4
@ -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<string, unknown>) => (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<string, unknown> | 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();
|
||||
|
||||
@ -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;
|
||||
}
|
||||
|
||||
@ -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
|
||||
});
|
||||
}
|
||||
|
||||
@ -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": {
|
||||
|
||||
9
apps/server/src/express.d.ts
vendored
9
apps/server/src/express.d.ts
vendored
@ -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
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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<typeof vi.fn> };
|
||||
}
|
||||
|
||||
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();
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
|
||||
@ -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<ReactiveOidcDeps> = {
|
||||
// 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<ReactiveOidcDeps> = {
|
||||
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,
|
||||
|
||||
@ -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;
|
||||
};
|
||||
|
||||
/**
|
||||
|
||||
Loading…
Reference in New Issue
Block a user