Security: Fix 24 vulnerabilities across auth, rate limiting, and uploads - #783
Open
aXenDeveloper wants to merge 5 commits into
Open
Security: Fix 24 vulnerabilities across auth, rate limiting, and uploads#783aXenDeveloper wants to merge 5 commits into
aXenDeveloper wants to merge 5 commits into
Conversation
A security review of the API surface. Each fix has tests covering the behaviour that was wrong. Access control - The admin user-update route guarded only the primary role, so `secondaryRoleIds` could attach a root role without `can_edit_admin` - `loadStaffPermissions` reads primary and secondary roles alike, so an administrator holding `users:can_edit` could make themselves root. Every role being assigned now goes through the guard, which also recognises root and moderator-granting roles. - The admin queue list selected every column, including `payload` - for `send-email` jobs the fully rendered message, live password-reset links included - for anyone with `queue:can_view`. It now selects the columns its response schema declares. - `POST /admin/notifications/send` required only an admin session, letting any restricted administrator push arbitrary in-product notifications to any user. Gated on `dashboard:can_edit`, like its sibling widget route. Credentials - Password-reset tokens were written to the database in plaintext (the hashing helper existed and was never called), so any read of the table was account takeover. Only the digest is stored now, and a completed reset revokes the user's sessions. - `CRON_SECRET` falls back to a constant published in this repository, so an install that never set it ran every cron job for anyone. Refused outside development, along with the scaffolded `.env.example` placeholder; the comparison is timing-safe and the `Bearer` prefix is matched rather than substring-replaced. - Sign-in answered "no such email" without hashing, timing-disclosing which addresses hold accounts. Both paths now derive a key. - `verifyPassword` continued after rejecting and threw a 500 on a malformed stored hash; the salt widens to 16 bytes for new hashes. Rate limiting and identity - The limiter was registered before the middleware that set `ipAddress`, so every request in the deployment shared one bucket named `undefined` - no per-client throttling, and a global kill switch at 80 requests a minute. Its unit test set `ipAddress` first, the opposite of the real wiring. - The client address was read from the first of sixteen client-settable headers, so any caller could choose their own bucket and their own line in the audit trail. Resolution is socket-based unless `trustProxy` says how many proxies are in front, and counts from the right so a forged chain is stepped over. Runtimes with no connection info now warn. Uploads and transport - The stored extension came from the client filename while the type came from the client `Content-Type`, so a file accepted as `image/gif` could be written as `.html` and served as a page from the app's own origin. The extension is now bound to the validated media type, and the uploads mount sends `Content-Security-Policy: sandbox` and `nosniff`. - The `/api/ws` handshake is cookie-authenticated but validated no Origin, and `csrf()` does not cover a GET - any site could open a socket as a visiting user. Added an origin check. - Auth cookies stated no `SameSite`; set to `Lax` explicitly. - The reCAPTCHA token was interpolated unencoded into the verification URL alongside the secret key; both now travel in a form-encoded body, and a missing secret key fails closed. - Swagger UI and the OpenAPI document were served unconditionally, publishing the whole attack surface. Off in production unless asked for. - Removed `POST /users/test`, an unauthenticated debug route that wrote a log row per call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014QtoqPREn6SsE5M4oFnQH5
Canonicalising a locale prefix strips it off the front of the path, so `/en//evil.example` became `//evil.example` - which is not a path but a protocol-relative URL, and a browser following that `Location` reads everything after the two slashes as a host. The site answered a request for one of its own URLs with a permanent redirect to somebody else's: a phishing link genuinely hosted on the real domain, and a way past any allowlist that trusts a same-origin-looking link. Leading slashes now collapse to one, backslashes included - browsers treat those as separators here even though the URL parser does not. The new test fails on all five payloads without the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014QtoqPREn6SsE5M4oFnQH5
…e session cache on a role change
Three findings from a second pass over the same review.
- `GET /admin/roles/{id}` was reachable on an admin session alone. `list` is
deliberately ungated - a role picker has to work for an administrator who
cannot open the roles screen - but that reasoning does not extend to one
role's full record, which only the edit screen reads. Gated on
`roles:can_view`, which `can_edit` already depends on, so nobody who could
open the screen loses access. The parity test's expectations move with it.
- The Postgres search adapter turned the client's `cursor` into the query's
`OFFSET` with no validation: `Number("abc")` is `NaN`, which Postgres
rejects as a 500 rather than a bad request, and a large one is a full scan
anybody can ask for by editing a URL. Now a checked integer, capped.
- A role change expired the staff-permission cache but not the session
cache, and `resolveStaffPermissions` reads the primary role off the cached
user object - so recomputing reached the same answer it had just thrown
away. Somebody demoted out of an administrator role kept its powers for
about a minute after the AdminCP said otherwise. Both caches now go, on
both write paths (a request changing only roles takes the second one).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014QtoqPREn6SsE5M4oFnQH5
…, and the rest of the second sweep Findings from the audit's completeness pass, each verified against the code. - Nothing bounded a request body anywhere in the stack, and `POST /sign_in` buffers its JSON and then runs scrypt on it unconditionally - so an unauthenticated caller chose how much memory and CPU to spend. Added a 25 MB default with `maxBodySize` to move it. Uploads keep their own per-field `maxBytes`; this is the outer wall. - `SessionModel.getUser` resolved the device before it knew the session was real, and resolving created one. Any request carrying a made-up `vitnode_auth` cookie therefore inserted a `core_sessions_known_devices` row - unauthenticated, one per request, unbounded. Split the model into `getExistingDeviceId` (a read, used by session resolution, where a missing device already means no session) and `getOrCreateDeviceId` (sign-in and sign-up, where minting one is the point). - The public search endpoint passed `Number(authorId)` and `new Date(from)` straight into the query builder, so `?authorId=abc` became `NaN` and Postgres answered with a 500 - which also wrote a `core_logs` row. A filter that cannot be parsed is now a filter that was not asked for. - The Discord SSO adapter never read Discord's `verified` flag, so an unconfirmed address could open an account keyed on it. Google already refuses this; Discord now matches. - The dev docker-compose files published Postgres and Redis on every interface with a default password of `root`. Bound to loopback. - Both reference apps read `TRUST_PROXY` for the hop count. The web app needs it most: its Start mount hands Hono a bare `Request` with no socket behind it, so without it every visitor resolves to the same fallback address and shares one rate-limit bucket. Documented in both `.env.example` files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014QtoqPREn6SsE5M4oFnQH5
Covers all 24 findings fixed across the four commits on this branch, with the attack path and the reasoning for each, plus eight issues reported and deliberately left alone because they need a product or deployment decision rather than a patch. Two things the document is careful about. It records what was checked and found sound, which is half of what a review is for - and several of those were places the obvious guess would have been wrong. And it is explicit about its own limits: no running instance, no database, no browser, no CVE scan, and machine verdicts that ran against already-patched code, so their "refuted" is not evidence of anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014QtoqPREn6SsE5M4oFnQH5
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa5293a247
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+143
to
+149
| const parsedCursor = params.cursor ? Number(params.cursor) : undefined; | ||
| const cursorValue = | ||
| parsedCursor !== undefined && | ||
| Number.isSafeInteger(parsedCursor) && | ||
| parsedCursor >= 0 | ||
| ? Math.min(parsedCursor, MAX_SEARCH_OFFSET) | ||
| : undefined; |
There was a problem hiding this comment.
Once core_search_index.id exceeds 10,000, this also clamps keyset cursors used by the newest and oldest branches. For example, a first page ending at ID 25,000 requests the next page with that cursor, but the query is changed to ID 10,000, silently skipping or repeating thousands of results. Preserve the validated ID for keyset pagination and apply MAX_SEARCH_OFFSET only inside the relevance branch.
Useful? React with 👍 / 👎.
Comment on lines
+56
to
+59
| if ( | ||
| INSECURE_CRON_SECRETS.includes(cronSecret) && | ||
| !CONFIG.node_development | ||
| ) { |
There was a problem hiding this comment.
With the repository's normal pnpm dev flow, apps/api/package.json runs tsx watch src/index.ts without setting NODE_ENV, so CONFIG.node_development is false. Because both the built-in fallback and the .env.example placeholder are in INSECURE_CRON_SECRETS, a fresh development checkout now rejects every scheduled cron request with 403 despite this explicit development exception; set the environment in the dev command or use a development signal that the shipped workflow actually provides.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
What?
This PR fixes 24 security vulnerabilities identified in a comprehensive security review of the VitNode codebase. The issues range from critical privilege escalation and world-runnable cron jobs to high-severity rate limiter failures and plaintext password reset tokens.
Critical fixes:
secondaryRoleIdsfield bypass (finding Block space for name #1)High-severity fixes:
undefined, creating a single global bucket (finding #3)Medium-severity fixes:
Low-severity fixes:
Why?
The security review identified systemic issues that could allow attackers to escalate privileges, bypass rate limiting, access sensitive data, and perform denial-of-service attacks. These fixes address the root causes rather than symptoms, with particular attention to:
All fixes include comprehensive test coverage. No database migrations required; no existing credentials invalidated.
Test Plan
cron-auth.middleware.test.tscovering secret comparison and insecure defaultsassert-edit-user-permission.test.tscovering role assignment guardsclient-ip.test.tscovering IP resolution with various proxy configurationsdevice.test.tscovering device creation guardspassword.test.tscovering hash verification and salt handlingtanstack/i18n/request.test.tscovering redirect validationwebsocket-origin.middleware.test.tscovering origin validationupload.test.tsandadmin-permission-parity.test.tshttps://claude.ai/code/session_014QtoqPREn6SsE5M4oFnQH5