diff --git a/.server-changes/clearer-whitelist-login-error.md b/.server-changes/clearer-whitelist-login-error.md new file mode 100644 index 0000000000..7d140f7370 --- /dev/null +++ b/.server-changes/clearer-whitelist-login-error.md @@ -0,0 +1,6 @@ +--- +area: webapp +type: improvement +--- + +Clearer login error when an email address is blocked by the WHITELISTED_EMAILS setting: the message now explains the address isn't allowed on this instance instead of the ambiguous "This email is unauthorized". diff --git a/.server-changes/consume-stale-login-errors.md b/.server-changes/consume-stale-login-errors.md new file mode 100644 index 0000000000..e43c559ab7 --- /dev/null +++ b/.server-changes/consume-stale-login-errors.md @@ -0,0 +1,6 @@ +--- +area: webapp +type: fix +--- + +Fixed stale login errors: an error from a previous login attempt (for example a rejected email address) no longer keeps reappearing on the login page and no longer makes later, successful attempts look like they failed. diff --git a/apps/webapp/app/routes/login._index/route.tsx b/apps/webapp/app/routes/login._index/route.tsx index c833de6ece..ce186c7346 100644 --- a/apps/webapp/app/routes/login._index/route.tsx +++ b/apps/webapp/app/routes/login._index/route.tsx @@ -23,7 +23,10 @@ import { isGithubAuthSupported, isGoogleAuthSupported } from "~/services/auth.se import { getLastAuthMethod } from "~/services/lastAuthMethod.server"; import { commitSession, setRedirectTo } from "~/services/redirectTo.server"; import { getUserId } from "~/services/session.server"; -import { getUserSession } from "~/services/sessionStorage.server"; +import { + commitSession as commitUserSession, + getUserSession, +} from "~/services/sessionStorage.server"; import { ssoController } from "~/services/sso.server"; import { flags as getGlobalFlags } from "~/v3/featureFlags.server"; import { requestUrl } from "~/utils/requestUrl.server"; @@ -111,8 +114,36 @@ export async function loader({ request }: LoaderFunctionArgs) { showSsoAuth = (globalFlags as Record).hasSso === true; } + // Read the flashed auth error and, when one exists, commit the session so + // the flash is consumed. Reading without committing leaves the flash in the + // cookie, so a stale error (e.g. a once-rejected email address) would + // resurface on every future /login visit and make later, successful login + // attempts look like they failed. + const userSession = await getUserSession(request); + const error = userSession.get("auth:error"); + // get() only auto-clears flash-stored values. Sessions written before this + // fix stored the error with set(), which survives get() + commit — unset it + // explicitly so those sessions heal too instead of showing the stale error + // until the cookie's one-year maxAge runs out. + userSession.unset("auth:error"); + + let authError: string | undefined; + if (error) { + if ("message" in error) { + authError = error.message; + } else { + authError = JSON.stringify(error, null, 2); + } + } + + const headers = new Headers(); + if (error) { + headers.append("Set-Cookie", await commitUserSession(userSession)); + } + if (redirectTo) { const session = await setRedirectTo(request, redirectTo); + headers.append("Set-Cookie", await commitSession(session)); return typedjson( { @@ -121,39 +152,26 @@ export async function loader({ request }: LoaderFunctionArgs) { showGoogleAuth: isGoogleAuthSupported, showSsoAuth, lastAuthMethod, - authError: null, + authError, notice, isVercelMarketplace: redirectTo.startsWith("/vercel/callback"), }, - { - headers: { - "Set-Cookie": await commitSession(session), - }, - } + { headers } ); } else { - const session = await getUserSession(request); - const error = session.get("auth:error"); - - let authError: string | undefined; - if (error) { - if ("message" in error) { - authError = error.message; - } else { - authError = JSON.stringify(error, null, 2); - } - } - - return typedjson({ - redirectTo: null, - showGithubAuth: isGithubAuthSupported, - showGoogleAuth: isGoogleAuthSupported, - showSsoAuth, - lastAuthMethod, - authError, - notice, - isVercelMarketplace: false, - }); + return typedjson( + { + redirectTo: null, + showGithubAuth: isGithubAuthSupported, + showGoogleAuth: isGoogleAuthSupported, + showSsoAuth, + lastAuthMethod, + authError, + notice, + isVercelMarketplace: false, + }, + { headers } + ); } } diff --git a/apps/webapp/app/routes/login.magic/magicLinkEmailCookie.server.ts b/apps/webapp/app/routes/login.magic/magicLinkEmailCookie.server.ts new file mode 100644 index 0000000000..8f1738d2aa --- /dev/null +++ b/apps/webapp/app/routes/login.magic/magicLinkEmailCookie.server.ts @@ -0,0 +1,15 @@ +import { createCookie } from "@remix-run/node"; +import { env } from "~/env.server"; + +// Carries the submitted email to the confirmation screen in a short-lived, +// httpOnly cookie rather than the URL, so the address never lands in access +// logs, browser history, or error-tracker breadcrumbs. Lives in a .server +// module: it calls createCookie at import time using server-only env, which +// throws if it ever evaluates in the client bundle. +export const magicLinkEmailCookie = createCookie("magiclink-email", { + maxAge: 60 * 10, + httpOnly: true, + sameSite: "lax", + secure: env.NODE_ENV === "production", + path: "/", +}); diff --git a/apps/webapp/app/routes/login.magic/route.tsx b/apps/webapp/app/routes/login.magic/route.tsx index 7ad40a04c6..cb164bcc91 100644 --- a/apps/webapp/app/routes/login.magic/route.tsx +++ b/apps/webapp/app/routes/login.magic/route.tsx @@ -34,6 +34,7 @@ import { ssoRedirectForEmail } from "~/services/ssoAutoDiscovery.server"; import { logger, tryCatch } from "@trigger.dev/core/v3"; import { env } from "~/env.server"; import { extractClientIp } from "~/utils/extractClientIp.server"; +import { magicLinkEmailCookie } from "./magicLinkEmailCookie.server"; export const meta: MetaFunction = ({ matches }) => { const parentMeta = matches @@ -73,14 +74,12 @@ export async function loader({ request }: LoaderFunctionArgs) { // confirmation renders the flashed error as magicLinkError below. const url = new URL(request.url); const sanitized = sanitizeRedirectPath(url.searchParams.get("redirectTo")); - // The email-link strategy stores the submitted address in the session - // (`auth:email`) alongside the magic-link key, so read it from there to name - // the confirmation — no address in the URL, and no separate cookie to leak - // into the client bundle. Validate before echoing it back. - const emailValue = session.get("auth:email"); + // The submitted address is carried in a short-lived cookie (not the URL) so + // the confirmation can name it. Validate before echoing it back. + const emailCookie = await magicLinkEmailCookie.parse(request.headers.get("Cookie")); const email = - typeof emailValue === "string" && z.string().email().safeParse(emailValue).success - ? emailValue + typeof emailCookie === "string" && z.string().email().safeParse(emailCookie).success + ? emailCookie : null; if (!session.has("triggerdotdev:magiclink")) { // Throw (not return) so the redirect doesn't widen the loader's return @@ -92,6 +91,9 @@ export async function loader({ request }: LoaderFunctionArgs) { } const error = session.get("auth:error"); + // Same migration hygiene as /login: pre-fix sessions stored this with + // set(), which get() doesn't clear — unset so the commit below removes it. + session.unset("auth:error"); const redirectTo = sanitized === "/" ? null : sanitized; const headers = new Headers(); @@ -142,7 +144,9 @@ export async function action({ request }: ActionFunctionArgs) { if (!result.success) { const session = await getUserSession(request); - session.set("auth:error", { + // flash, not set: a set() key survives every later read/commit, so the + // error would reappear on each /login visit long after the failed attempt. + session.flash("auth:error", { message: "Please enter a valid email address.", }); @@ -192,7 +196,8 @@ export async function action({ request }: ActionFunctionArgs) { : "Failed sending magic link. Please try again shortly."; const session = await getUserSession(request); - session.set("auth:error", { + // flash, not set — same one-shot semantics as the validation error above. + session.flash("auth:error", { message: errorMessage, }); @@ -216,13 +221,20 @@ export async function action({ request }: ActionFunctionArgs) { return redirect(ssoRedirect); } - // The email-link strategy stores the address in the session (`auth:email`) - // and throws its own redirect Response (with the committed session cookie), - // so return it directly — the confirmation reads the email from the session. - return await authenticator.authenticate("email-link", request, { - successRedirect: "/login/magic", - failureRedirect: "/login", - }); + // authenticator.authenticate throws its redirect Response; attach the + // sent-to email as a short-lived cookie so the confirmation can name it + // without putting the address in the URL. + try { + return await authenticator.authenticate("email-link", request, { + successRedirect: "/login/magic", + failureRedirect: "/login", + }); + } catch (thrown) { + if (thrown instanceof Response) { + thrown.headers.append("Set-Cookie", await magicLinkEmailCookie.serialize(email)); + } + throw thrown; + } } case "reset": default: { diff --git a/apps/webapp/app/services/emailAuth.server.tsx b/apps/webapp/app/services/emailAuth.server.tsx index 9771cae93c..9e8fbdb44b 100644 --- a/apps/webapp/app/services/emailAuth.server.tsx +++ b/apps/webapp/app/services/emailAuth.server.tsx @@ -18,11 +18,6 @@ const emailStrategy = new EmailLinkStrategy( secret, callbackURL: "/magic", sessionMagicLinkKey: "triggerdotdev:magiclink", - // Pin explicitly to the library default rather than relying on it: the - // /login/magic loader reads the submitted address via - // session.get("auth:email") to name it on the confirmation screen, so a - // future remix-auth-email-link default change can't silently break that. - sessionEmailKey: "auth:email", }, async ({ email, diff --git a/apps/webapp/app/utils/email.ts b/apps/webapp/app/utils/email.ts index ae2cf6d2cb..dca4e86aa8 100644 --- a/apps/webapp/app/utils/email.ts +++ b/apps/webapp/app/utils/email.ts @@ -7,6 +7,8 @@ export function assertEmailAllowed(email: string) { } if (!emailMatchesPattern(env.WHITELISTED_EMAILS, email)) { - throw new Error("This email is unauthorized"); + // Surfaced verbatim on the login page. Name the actual policy so a + // rejection on a restricted instance reads as configuration, not a bug. + throw new Error("This email address isn't allowed to sign in on this instance."); } }