Skip to content

Auth: unit tests for signSession and verifySession #172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
prachidarshi-2307 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Auth: unit tests for signSession and verifySession #172

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54e69cc
test: add unit tests for signSession and verifySession
prachidarshi-2307 Sep 27, 2026
f14f877
test: preserve JWT_SECRET with afterAll and restore existing test for…
prachidarshi-2307 Oct 3, 2026
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
72 changes: 70 additions & 2 deletions lib/auth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,28 @@
import { describe, it, expect, beforeAll } from "vitest";
import { describe, it, expect, beforeAll, afterAll, vi } from "vitest";
import type { SessionClaims } from "./auth";

// hashPassword/verifyPassword/generatePassword don't touch secret(),
// but auth.ts throws at import-time paths that do call secret() elsewhere
// in the module graph, so we set a dummy value before importing —
// never a real secret — using a dynamic import so it runs after this
// executes.
let auth: typeof import("./auth");
let originalJwtSecret: string | undefined;

beforeAll(async () => {
originalJwtSecret = process.env.JWT_SECRET;
process.env.JWT_SECRET = "test-only-dummy-secret-not-real-0123456789";
auth = await import("./auth");

@ishan-one8 ishan-one8 Oct 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A genuine question, and this comment predates your PR so it isn't a criticism of your work — I just couldn't verify the premise it states.

The comment says auth.ts throws at import-time via paths that call secret(). I tried importing it with JWT_SECRET unset and it imported fine; secret() appears to be called only inside signSession / verifySession, never at module scope.

If that's right, a plain static import would work and the let auth + dynamic import could go, which would also line up with lib/github-auth.test.ts that the issue points you at. But I'd rather ask than assume — is there an import path that calls secret() eagerly that I'm not seeing?

});

afterAll(() => {
if (originalJwtSecret === undefined) {
delete process.env.JWT_SECRET;
} else {
process.env.JWT_SECRET = originalJwtSecret;
}
});

describe("hashPassword / verifyPassword", () => {
it("hashes then verifies the correct password as true", async () => {
const hash = await auth.hashPassword("correct-horse-battery-staple");
Expand Down Expand Up @@ -52,4 +63,61 @@ describe("generatePassword", () => {
);
expect(passwords.size).toBe(100);
});
});
});

describe("signSession / verifySession", () => {

@ishan-one8 ishan-one8 Oct 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

verifySession doesn't only check the signature — it also passes { issuer: ISSUER } to jwt.verify (lib/auth.ts:77), and nothing in this block exercises that.

What that means concretely: if someone deleted issuer: ISSUER from the verify call, every test in this file would still pass. Given the issue's own framing — break the code on purpose and watch a test fail — that's the one condition in verifySession that can be removed without any test noticing.

I checked the behaviour is actually live before suggesting it: forging a token with the same secret but issuer: "someone-else" does return null, so this is a missing test rather than a bug.

// needs `import jwt from "jsonwebtoken";`
it("returns null for a token with a different issuer", () => {
  const forged = jwt.sign(claims, process.env.JWT_SECRET!, {
    expiresIn: "7d",
    issuer: "someone-else",
  });
  expect(auth.verifySession(forged)).toBeNull();
});

Worth adding while you're in here, since a same-secret token minted by anything else is exactly what that check exists to stop.

const claims: SessionClaims = {
usn: "1MS21CS001",
name: "Ada Lovelace",
role: "member",
};

it("signs then verifies returning the same claims", () => {
const token = auth.signSession(claims);
const verified = auth.verifySession(token);
expect(verified).toMatchObject(claims);
expect(verified?.usn).toBe(claims.usn);
expect(verified?.name).toBe(claims.name);
expect(verified?.role).toBe(claims.role);
});

it.each([
["undefined", undefined],
["empty string", ""],
["garbage string", "not-a-valid-token"],
["malformed jwt", "header.payload.signature"],
])("returns null for %s", (_, token) => {
expect(auth.verifySession(token)).toBeNull();
});

it("returns null for a tampered token", () => {
const token = auth.signSession(claims);
const lastChar = token.slice(-1);
const replacement = lastChar === "a" ? "b" : "a";
const tampered = token.slice(0, -1) + replacement;

@ThatKJ ThatKJ Oct 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The tamper step only changes the last character of the token, and for an HS256 signature that character is a bit odd. The signature is 32 bytes, i.e. 43 base64url characters, so the final character carries 4 data bits and 2 unused padding bits. Replacing it with a can leave the decoded signature bytes unchanged.

I generated 4000 tokens with jsonwebtoken@9 and applied this exact mutation. In 252 of them (about 6%, the ones whose last character is Y) the mutated signature decodes to the same bytes as the original. verify still rejected all 4000, so the test passes today, but that's because the library compares the encoded signature strings, not because the signature is cryptographically different. A change in how the library compares (or a bump away from v9) could make this test pass for the wrong reason or flake.

Altering a character in the middle of the signature segment, or in the payload, would test the same thing without depending on that detail, e.g. flip the first character of token.split(".")[2].

Question: was last-character tampering chosen deliberately, or is it just the easiest slice? If it's deliberate I'd like to understand what it covers that a middle-character change doesn't.

expect(auth.verifySession(tampered)).toBeNull();
});

it("returns null when token was signed with a different secret", () => {
const token = auth.signSession(claims);
const originalSecret = process.env.JWT_SECRET;
try {
process.env.JWT_SECRET = "different-dummy-secret-at-least-32-characters";
expect(auth.verifySession(token)).toBeNull();
} finally {
process.env.JWT_SECRET = originalSecret;
}
});

it("returns null for an expired token using fake timers", () => {
vi.useFakeTimers();
try {
const token = auth.signSession(claims);
// Advance fake time past 7-day TTL (e.g. 8 days)
vi.advanceTimersByTime(8 * 24 * 60 * 60 * 1000);
expect(auth.verifySession(token)).toBeNull();
} finally {
vi.useRealTimers();
}
});
});
Loading