Skip to content

Use crypto RNG for ticket IDs so harvestable display IDs resist enumeration - #270

Open
Ayush7614 wants to merge 1 commit into
CopilotKit:mainfrom
Ayush7614:feat/secure-ticket-ids
Open

Ayush7614 wants to merge 1 commit into
CopilotKit:mainfrom
Ayush7614:feat/secure-ticket-ids

Conversation

@Ayush7614

Copy link
Copy Markdown

Display ticket IDs (TKT-XXXXXXXX) are pasted into public threads and are therefore harvestable — the Postmark webhook handler already gates replies on participant checks for exactly this reason. But the IDs themselves were drawn with Math.random, a non-crypto RNG whose output is predictable enough to shrink the enumeration search space.

This PR switches generateTicketId to node:crypto randomInt (CSPRNG) with no API change — same TKT-XXXXXXXX format and alphabet.

Verification (all real, run locally):

  • pnpm vitest run shared/src/tests/utils.test.ts → 28/28 pass, including 3 new tests (Math.random independence, 2000-ID uniqueness, full-alphabet coverage)
  • tsc --project shared/tsconfig.json --noEmit → clean
  • eslint on both touched files → clean
  • Full-repo typecheck failures (queue/db module resolution) confirmed pre-existing on clean upstream/main

@NathanTarbert NathanTarbert left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this, and for the four others — that is a lot of careful work to land in one morning.

Before anything else: CI has not run on any of your five PRs. Fork contributions need a maintainer to approve the workflow runs, and nobody has yet, so every check is sitting at action_required. That is on us, not you, and it matters here because the one blocking problem below is invisible to pnpm test and only shows up in next build. I ran everything locally so you are not waiting on us to find out.

One blocker — the build

packages/outpost/shared/src/utils.ts:1 imports randomInt from node:crypto. That module is re-exported through the shared barrel, and the barrel is imported by client components — apps/web/src/components/tickets/tickets-view.tsx and thirteen others are 'use client'. So webpack tries to bundle a Node built-in for the browser and @copilotkit/outpost-web#build fails.

I confirmed it is a regression rather than pre-existing: reverting just utils.ts to main gives a green 10/10 build.

Web Crypto works in both runtimes and keeps the generator uniform:

const bytes = new Uint8Array(8);
globalThis.crypto.getRandomValues(bytes);
let id = '';
for (let i = 0; i < 8; i++) {
    // 256 is an exact multiple of 32, so masking is uniform.
    id += chars[bytes[i]! & 31];
}

With that applied I get 28/28 utils tests passing and a 10/10 build.

Worth knowing why this was invisible to you: pnpm test and pnpm typecheck both pass. Only next build catches it.

A framing note, and it does not reduce the value of the change

The description says a non-crypto RNG lets an attacker shrink the search space for ID enumeration. I traced every surface that consumes a display ID, and knowing an id is not sufficient to reach a ticket on any of them — the reply path gates on the sender already being a participant, and every web route checks the session before the lookup. The repo says so in a couple of places too.

So I would take this as good hygiene rather than a vulnerability fix, and soften the new doc comment accordingly — display ids are public, a CSPRNG keeps them unguessable, authorization lives at the gates. That is a better claim because it is one that holds.

Your instinct was not baseless, though. It pointed at something real one layer over, which I am filing separately.

Small

Three comments now describe the RNG they no longer match — two in apps/web/src/app/api/webhooks/postmark/route.ts and one in apps/web/src/__tests__/postmark-webhook.test.ts. They are load-bearing (they explain why the participant gates exist), so worth rewording rather than deleting: the point was always that the id is short and public, not that the RNG was weak.

What is good

The generator itself is correct — randomInt uses rejection sampling, so no modulo bias, and the length, alphabet and TKT- prefix are unchanged, which I checked against everything in the repo that parses that prefix.

And the tests are real. I mutation-tested them: shortening the id, reverting to Math.random, and making the last alphabet character unreachable are all caught, and two of those are caught only by the tests you added. That is a better-than-average test contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants