Skip to content

Enforce password byte cap and throttle invite resends - #274

Open
Ayush7614 wants to merge 1 commit into
CopilotKit:mainfrom
Ayush7614:feat/password-policy-and-resend-throttle
Open

Ayush7614 wants to merge 1 commit into
CopilotKit:mainfrom
Ayush7614:feat/password-policy-and-resend-throttle

Conversation

@Ayush7614

Copy link
Copy Markdown

Two auth hardening gaps, one PR:

  1. bcrypt silently ignores password bytes past 72, so hashPassword accepted arbitrary-length input where the tail was security theater (a 90-char password verified identically to its 72-byte prefix), and a multi-KB password burned CPU per hash. All password entry points (setup, profile change, invite accept) enforced only a minimum length.
  2. POST /api/team/invite/resend did deleteMany + create with no cooldown, so a compromised admin session or retry loop could emit an unbounded stream of invite emails.

Changes:

  • packages/outpost/shared/src/auth/passwords.ts: MIN_PASSWORD_LENGTH (8), MAX_PASSWORD_BYTES (72), passwordByteLength (TextEncoder, no Node Buffer so it stays runtime-agnostic), validatePassword helper; hashPassword rejects out-of-policy input instead of storing a truncated-equivalent hash. verifyPassword untouched, so legacy hashes keep verifying.
  • setup, profile, and invite-accept routes enforce the cap with 400s (legacy messages preserved for the min-length cases).
  • resend route: 60s per-member cooldown based on the latest token's createdAt (memberId is unique, so one row per member); 429 + Retry-After when too soon.

Verification (all real, run locally):

  • packages/outpost suite: 67 files, 1290 tests pass — auth-passwords now covers byte-vs-char measurement (multibyte euro-sign boundary), rejection paths, exact-cap acceptance, legacy-hash compatibility
  • apps/web suite: 53 files, 756 tests pass — new tests for 72-byte rejection on all three password routes (hash never called), resend 429 with Retry-After, resend allowed after cooldown
  • tsc clean for both shared and web (shared dist rebuilt)

@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.

This is the strongest of the five, and the parts that are easy to get wrong are right.

First, the thing that is our fault rather than yours: CI has never run on any of your PRs. Fork contributions need a maintainer to approve the workflow runs, so every check is parked at action_required. I ran everything locally instead, which is how the blocker below turned up.

One blocker, and it is one word

apps/web/src/app/api/team/invite/resend/route.ts:12 exports INVITE_RESEND_COOLDOWN_MS. Next's App Router only allows HTTP verbs and a fixed set of config fields as route exports, so both pnpm build and pnpm typecheck fail:

Type error: "INVITE_RESEND_COOLDOWN_MS" is not a valid Route export field.

Dropping export fixes both — I verified a clean build and typecheck afterwards, and nothing imports the constant. It is the only non-verb export const in any route file in the repo.

pnpm test passes with the export in place, because vitest imports the module directly and never applies Next's route constraint. So there was no way for you to see this without CI.

Two other PRs of yours have the same shape — #273 exports sanitizeHistory and some constants from its route, and it is the same fix.

Worth fixing before merge

Two of the new tests are load-sensitive. auth-passwords.test.ts:72 and the backward-compat test below it timed out at 5000ms on a full run under load, reproduced 1 of 3 times, and do not fail on main. They add bcrypt cost-12 work, and the pre-existing multi-hash test in that same file already carries { timeout: 30000 } for exactly this reason. Adding the same override to the two new ones should settle it — CI runners are usually slower than a dev machine, so this would surface there.

Smaller

The password policy now exists in four places — validatePassword, the hand-mirrored mocks in the route tests, scripts/create-admin.ts:82, and apps/web/src/app/api/profile/route.ts:64-69. I mutated the real MAX_PASSWORD_BYTES and all 35 web route tests stayed green, because they assert the mirror rather than the policy. The real policy is covered at package level, so the gap is narrow, and you followed the existing convention here — but the "faithful mirror" comment is the only thing keeping them in sync. create-admin.ts and the profile route calling validatePassword would collapse two of the four.

Also: the throttle read-then-write is not atomic, so two concurrent resends can both pass. Admin-gated, so low impact. And invite create has no throttle at all — out of scope here, worth its own issue.

What I verified, so nobody re-checks it

The byte cap is a real byte cap, which is the thing this class of change usually gets wrong. Against the repo's actual bcrypt: a 24-character euro-sign password is 72 bytes and accepted; a 30-character CJK password is 90 bytes and correctly rejected. A character cap would have let the second through. passwordByteLength uses TextEncoder rather than Buffer, so it works in edge runtimes too.

Existing users cannot be locked out. verifyPassword is untouched and the login path never calls validatePassword — a legacy over-length password still verifies. The cap applies at set time only, which is the right call.

The throttle is the good version. Database-backed rather than in-memory, so it survives deploys and works across replicas. Keyed on member id rather than email, so there is no case or whitespace bypass. It fails closed. Retry-After is set.

Both central claims survive mutation. Turning the byte cap into a character cap fails your multibyte test; disabling the throttle fails the 429 test. The multibyte one is doing real work — it is exactly the assertion that separates a correct cap from a plausible-looking wrong one.

Not yours

prettier --check fails on 6 of the 10 files you touched — and all 6 fail on main too. The format check runs over whole changed files, so touching them pulled a pre-existing backlog into scope. We need to decide how to clear that; it is not something to ask you to absorb.

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