fix(security): harden deployment auth tokens#4760
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview
Adds Reviewed by Cursor Bugbot for commit f9e565b. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThis PR hardens deployment auth cookies by introducing versioned (
Confidence Score: 3/5The auth hardening logic is sound and the HMAC construction is correct, but a gap in hasValidTimestamp means forward-dated tokens pass expiry validation without restriction, and the test suite has not been run against the actual source. The dual-path validator correctly routes v2 and legacy tokens, password binding is wired into the HMAC input rather than the cookie payload, and the test helpers faithfully mirror the production HMAC scheme. However, hasValidTimestamp returns true whenever Date.now() - createdAt is negative — any token carrying a future timestamp bypasses the 24-hour window. Non-browser callers or manually set cookies are unprotected by the browser maxAge bound. apps/sim/lib/core/security/deployment.ts — the hasValidTimestamp function; apps/sim/lib/core/security/deployment.test.ts — confirm the suite passes in CI before merging. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming token cookie] --> B[base64 decode]
B --> C[split on lastColon: payload + sig]
C --> D{parts0 === v2?}
D -- Yes-v2 --> E{parts.length === 4?}
E -- No --> REJECT1[return false]
E -- Yes --> F[HMAC over payload + sha256-encryptedPassword]
F --> G{sig matches?}
G -- No --> REJECT2[return false]
G -- Yes --> H{storedId === deploymentId?}
H -- No --> REJECT3[return false]
H -- Yes --> I{hasValidTimestamp?}
I -- No --> REJECT4[return false]
I -- Yes --> ACCEPT1[return true]
D -- No-legacy --> J{parts.length === 4?}
J -- No --> REJECT5[return false]
J -- Yes --> K[HMAC over payload only]
K --> L{sig matches?}
L -- No --> REJECT6[return false]
L -- Pass --> M{storedId === deploymentId?}
M -- No --> REJECT7[return false]
M -- Yes --> N{pwSlot matches?}
N -- No --> REJECT8[return false]
N -- Yes --> O{hasValidTimestamp?}
O -- No --> REJECT9[return false]
O -- Yes --> ACCEPT2[return true]
subgraph note [hasValidTimestamp - future timestamps accepted]
I
O
end
Reviews (1): Last reviewed commit: "fix(security): harden deployment auth to..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function hasValidTimestamp(timestamp: string): boolean { | ||
| const createdAt = Number.parseInt(timestamp, 10) | ||
| if (!Number.isFinite(createdAt)) return false | ||
|
|
||
| return Date.now() - createdAt <= AUTH_TOKEN_TTL_MS | ||
| } |
There was a problem hiding this comment.
hasValidTimestamp accepts tokens with a future createdAt. When createdAt > Date.now(), the expression Date.now() - createdAt is negative, which always satisfies <= AUTH_TOKEN_TTL_MS (86 400 000), so the check returns true unconditionally. In a browser context the maxAge cookie attribute enforces the wall-clock bound, but any server-side use, non-browser client, or manually constructed cookie bypasses that. Adding a lower bound rejects forward-dated timestamps regardless of context.
| function hasValidTimestamp(timestamp: string): boolean { | |
| const createdAt = Number.parseInt(timestamp, 10) | |
| if (!Number.isFinite(createdAt)) return false | |
| return Date.now() - createdAt <= AUTH_TOKEN_TTL_MS | |
| } | |
| function hasValidTimestamp(timestamp: string): boolean { | |
| const createdAt = Number.parseInt(timestamp, 10) | |
| if (!Number.isFinite(createdAt)) return false | |
| const elapsed = Date.now() - createdAt | |
| return elapsed >= 0 && elapsed <= AUTH_TOKEN_TTL_MS | |
| } |
| it('rejects expired signed tokens', () => { | ||
| const expiredTimestamp = Date.now() - 24 * 60 * 60 * 1000 - 1 | ||
| const token = signedV2Token('dep_test', 'encrypted-password', expiredTimestamp) | ||
|
|
||
| expect(validateAuthToken(token, 'dep_test', 'encrypted-password')).toBe(false) | ||
| }) |
There was a problem hiding this comment.
The expiry suite tests a timestamp 1 ms past 24 h but has no counterpart asserting that a token with timestamp = Date.now() + someOffset is rejected. Given the <= boundary issue in hasValidTimestamp, a test like signedV2Token('dep_test', 'encrypted-password', Date.now() + 60_000) would currently pass validation (returns true) rather than being rejected, confirming the bug in the implementation.
| import { createHash, createHmac } from 'node:crypto' | ||
| import { createEnvMock } from '@sim/testing' | ||
| import type { NextResponse } from 'next/server' | ||
| import { describe, expect, it, vi } from 'vitest' | ||
|
|
||
| vi.mock('@/lib/core/config/env', () => | ||
| createEnvMock({ | ||
| BETTER_AUTH_SECRET: 'deployment-auth-test-secret-32-chars', | ||
| }) | ||
| ) | ||
|
|
||
| vi.mock('@/lib/core/config/feature-flags', () => ({ | ||
| isDev: true, | ||
| })) | ||
|
|
||
| import { setDeploymentAuthCookie, validateAuthToken } from './deployment' |
There was a problem hiding this comment.
The PR explicitly notes npx vitest run … could not be started due to missing workspace dependencies. The test helpers (signedLegacyToken, signedV2Token) faithfully replicate the production HMAC logic (UTF-8 encoding matches sha256Hex/hmacSha256Hex in @sim/security), so the tests are structurally correct. However, for a security-hardening change the lack of a green CI run is a gap worth closing before merge, especially for the new rejects expired signed tokens and rejects tampered signed token payloads cases.
1 participant
Summary
v2tokensFixes #4759
Testing
git diff --check -- apps/sim/lib/core/security/deployment.ts apps/sim/lib/core/security/deployment.test.tsnpx --yes @biomejs/biome@2.0.0-beta.5 check --write --no-errors-on-unmatched --files-ignore-unknown=true apps/sim/lib/core/security/deployment.ts apps/sim/lib/core/security/deployment.test.tsNot run
npx --yes vitest@3.0.8 run apps/sim/lib/core/security/deployment.test.ts --config apps/sim/vitest.config.tscould not start because this checkout has no installed workspace dependencies (@vitejs/plugin-reactand@sim/tsconfig/nextjs.jsonwere missing). The expected project runner is Bun, butbunis not installed on this machine andnpx bun@1.3.13fails because npm has postinstall scripts disabled.