Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions server/middlewares/authentication.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from "@server/test/factories";
import { User } from "@server/models";
import { AuthenticationType } from "@server/types";
import { JWT_COOKIE_TTL_DAYS } from "@server/utils/authentication";
import auth, { FORWARDAUTH_SERVICE } from "./authentication";

function createCtx(overrides: any = {}) {
Expand Down Expand Up @@ -379,6 +380,47 @@ describe("Authentication middleware", () => {
expect(state.auth.type).toEqual(AuthenticationType.APP);
});

it("should issue the accessToken cookie with a 7-day expiry", async () => {
const team = await buildTeam();
const user = await buildUser({ teamId: team.id });
const state = {} as DefaultState;
const authMiddleware = auth();
const cookiesSet = jest.fn();
const before = Date.now();

await authMiddleware(
{
// @ts-expect-error mock request
request: {
get: jest.fn((header: string) => {
if (header === "x-auth-request-email") {
return user.email!;
}
return "";
}),
},
// @ts-expect-error mock cookies
cookies: { get: jest.fn(() => undefined), set: cookiesSet },
state,
ip: "127.0.0.1",
cache: {},
},
jest.fn()
);

const accessTokenCall = cookiesSet.mock.calls.find(
(call) => call[0] === "accessToken"
);
expect(accessTokenCall).toBeDefined();

const expires: Date = accessTokenCall![2].expires;
const ageMs = expires.getTime() - before;
const expectedMs = JWT_COOKIE_TTL_DAYS * 24 * 60 * 60 * 1000;
// Allow ±60s skew for test runtime.
expect(ageMs).toBeGreaterThan(expectedMs - 60_000);
expect(ageMs).toBeLessThan(expectedMs + 60_000);
});

it("should provision a new user when X-Auth-Request-Email is unknown", async () => {
await buildTeam();
const state = {} as DefaultState;
Expand Down
5 changes: 3 additions & 2 deletions server/middlewares/authentication.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { addMonths } from "date-fns";
import { addDays } from "date-fns";
import type { Next } from "koa";
import capitalize from "lodash/capitalize";
import { Op } from "sequelize";
Expand All @@ -18,6 +18,7 @@ import { User, Team, ApiKey, OAuthAuthentication } from "@server/models";
import { sequelize } from "@server/storage/database";
import type { AppContext } from "@server/types";
import { AuthenticationType } from "@server/types";
import { JWT_COOKIE_TTL_DAYS } from "@server/utils/authentication";
import { getUserForJWT } from "@server/utils/jwt";
import {
AuthenticationError,
Expand Down Expand Up @@ -56,7 +57,7 @@ export default function auth(options: AuthenticationOptions = {}) {
// that subsequent requests and cookie-dependent services (WebSocket,
// collaboration) use the fast JWT path instead of the header DB path.
if (service === FORWARDAUTH_SERVICE && !ctx.cookies.get("accessToken")) {
const expires = addMonths(new Date(), 3);
const expires = addDays(new Date(), JWT_COOKIE_TTL_DAYS);
ctx.cookies.set("accessToken", user.getJwtToken(expires, service), {
sameSite: "lax",
expires,
Expand Down
37 changes: 37 additions & 0 deletions server/routes/auth/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { buildUser, buildCollection } from "@server/test/factories";
import { getTestServer } from "@server/test/support";
import { JWT_COOKIE_TTL_DAYS } from "@server/utils/authentication";

const server = getTestServer();

Expand Down Expand Up @@ -43,4 +44,40 @@ describe("auth/redirect", () => {

expect(res.status).toEqual(401);
});

it("should mint an accessToken JWT carrying an expiresAt claim ~JWT_COOKIE_TTL_DAYS out", async () => {
const user = await buildUser();
const before = Date.now();

const res = await server.get(
`/auth/redirect?token=${user.getTransferToken()}`,
{
redirect: "manual",
}
);

expect(res.status).toEqual(302);

// Pull the `accessToken` cookie out of the Set-Cookie header(s).
const setCookie = res.headers.get("set-cookie") || "";
const match = setCookie.match(/accessToken=([^;,]+)/);
expect(match).not.toBeNull();
const jwt = match![1];

// Decode the JWT payload directly — no signature check needed, we're
// only inspecting the claim. JWT payload is the base64url middle segment.
const payload = JSON.parse(
Buffer.from(jwt.split(".")[1], "base64url").toString()
);

// Without the fix, getJwtToken(undefined, ...) would omit this claim
// entirely and the validator at utils/jwt.ts:47 would skip the check.
expect(payload.expiresAt).toBeDefined();

const ageMs = new Date(payload.expiresAt).getTime() - before;
const expectedMs = JWT_COOKIE_TTL_DAYS * 24 * 60 * 60 * 1000;
// Allow ±60s skew for test runtime.
expect(ageMs).toBeGreaterThan(expectedMs - 60_000);
expect(ageMs).toBeLessThan(expectedMs + 60_000);
});
});
23 changes: 18 additions & 5 deletions server/routes/auth/index.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
import passport from "@outlinewiki/koa-passport";
import { addMonths } from "date-fns";
import { addDays } from "date-fns";
import Koa from "koa";
import bodyParser from "koa-body";
import Router from "koa-router";
import { AuthenticationError } from "@server/errors";
import authMiddleware from "@server/middlewares/authentication";
import coalesceBody from "@server/middlewares/coaleseBody";
import { verifyCSRFToken } from "@server/middlewares/csrf";
import { Collection, Team, View } from "@server/models";
import AuthenticationHelper from "@server/models/helpers/AuthenticationHelper";
import type { AppState, AppContext, APIContext } from "@server/types";
import { verifyCSRFToken } from "@server/middlewares/csrf";
import { JWT_COOKIE_TTL_DAYS } from "@server/utils/authentication";
Comment on lines +9 to +13
import { getJWTPayload } from "@server/utils/jwt";

const app = new Koa<AppState, AppContext>();
const router = new Router();
Expand All @@ -32,18 +34,29 @@ void (async () => {

router.get("/redirect", authMiddleware(), async (ctx: APIContext) => {
const { user, service } = ctx.state.auth;
const jwtToken = user.getJwtToken(undefined, service);

if (jwtToken === ctx.state.auth.token) {
// This route is only for exchanging a short-lived transfer token for a
// session cookie. Reject anything else (in particular session JWTs being
Comment on lines +38 to +39
// replayed to extend their own life). The previous heuristic relied on
// a quirk where session JWTs were deterministic; with proper `expiresAt`
// claims they no longer are, so check the token type directly.
if (getJWTPayload(ctx.state.auth.token).type !== "transfer") {
throw AuthenticationError("Cannot extend token");
Comment on lines +40 to 44
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already addressed in the current revision — the comparison-based check has been replaced with an explicit type check exactly as suggested:

if (getJWTPayload(ctx.state.auth.token).type !== "transfer") {
  throw AuthenticationError("Cannot extend token");
}

This decodes the incoming token's payload and requires type === "transfer" for the route to proceed, rather than relying on the JWT-determinism heuristic that the expiresAt fix invalidated. The existing test "should prevent token extension by rejecting JWT tokens" exercises this — a session JWT now reaches the route handler and gets rejected at the type check.

The diff hunk Copilot is reading actually shows the fix already applied (the - line is the old comparison, the + lines are the type check). Looks like a review-rendering issue where the bot is treating the pre-image of the hunk as active.

}

// Mint the JWT with the same expiry the cookie will carry so the token
// and the cookie die together. Without an `expiresAt` claim the
// validator at `utils/jwt.ts:47` skips the expiry check, leaving the
// token replayable indefinitely if it ever leaves the cookie.
const expires = addDays(new Date(), JWT_COOKIE_TTL_DAYS);
const jwtToken = user.getJwtToken(expires, service);

// ensure that the lastActiveAt on user is updated to prevent replay requests
await user.updateActiveAt(ctx, true);

ctx.cookies.set("accessToken", jwtToken, {
sameSite: "lax",
expires: addMonths(new Date(), 3),
expires,
});
Comment on lines 56 to 60
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the JWT-no-expiresAt claim — but worth flagging that this is a pre-existing upstream bug, not introduced by this PR. The user.getJwtToken(undefined, service) call on line 36 is unchanged; only the cookie's expires line was modified.

Net effect on exposure window:

  • Before this PR: 3-month cookie + eternal JWT → 90-day replay window
  • After this PR: 7-day cookie + eternal JWT → 7-day replay window

So this PR actually shrinks the exposure ~13x. It doesn't introduce the mismatch.

Will file a separate follow-up to pass expires into getJwtToken at this call site (matching the pattern already used in middlewares/authentication.ts and utils/authentication.ts). Keeping this PR scoped to cookie TTL only.

const [team, collection, view] = await Promise.all([
Team.findByPk(user.teamId),
Expand Down
12 changes: 10 additions & 2 deletions server/utils/authentication.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import querystring from "node:querystring";
import { addMonths } from "date-fns";
import { addDays } from "date-fns";
import type { Context } from "koa";
import pick from "lodash/pick";
import { Client } from "@shared/types";
Expand All @@ -10,6 +10,14 @@ import { Event, Collection, View } from "@server/models";
import type { APIContext, AuthenticationResult } from "@server/types";
import { AuthenticationType } from "@server/types";

/**
* Lifetime of the JWT cookie (`accessToken`) issued after Cognito sign-in.
* 7 days matches the rest of the foss-server-bundle-devstack
* (Plane / Penpot / SurfSense / Twenty / oauth2-proxy). Single source of
* truth for all three call sites that mint this cookie.
*/
Comment on lines +13 to +18
Comment on lines +14 to +18
export const JWT_COOKIE_TTL_DAYS = 7;

/**
* Parse and return the details from the "sessions" cookie in the request, if
* any. The cookie is on the apex domain and includes session details for
Expand Down Expand Up @@ -88,7 +96,7 @@ export async function signIn(
);

const domain = getCookieDomain(ctx.request.hostname, env.isCloudHosted);
const expires = addMonths(new Date(), 3);
const expires = addDays(new Date(), JWT_COOKIE_TTL_DAYS);

// set a cookie for which service we last signed in with. This is
// only used to display a UI hint for the user for next time
Expand Down
Loading