fix(security): the publisher code scanning alerts - #42
Merged
Merged
Conversation
Closes #25. **Rate limits, keyed on the visitor rather than the proxy.** The embed page and the API both read the disk on every request, and nothing capped either. `middlewares/rateLimits.ts` on `express-rate-limit`: - The public embed page: 300 a minute per visitor. - The API: **only failures count**, 60 a minute, and the limit runs before the token check so a wrong guess is one. A batch publish of short clips over a LAN is several good requests a second, and a ceiling on those would eventually refuse the owner. - `/media` is not limited: a video that seeks is a burst of Range requests, and the edge answers most of them. The key is `CF-Connecting-IP`, then the first `X-Forwarded-For` hop, then the socket. This runs behind a reverse proxy, so `req.ip` is the proxy for every request and a limit keyed on it would be one bucket for the whole internet. Forging either header buys a fresh bucket, which is no worse than no limit, and never gets anybody in. **The bearer header is read by hand.** `/^Bearer\s+(.+)$/` let `\s+` and `.+` fight over the same spaces, quadratic on the one header anybody on the internet can send. A test holds the new parse under 100 ms on 200,000 spaces. **Two log lines no longer use the filename as a format string.** A `%s` in an uploaded name would have been read as a directive. Gates: `tests/unit/publisher`, 12 new cases. `publisher-views-check.mjs` gained three checks (the 61st wrong guess is a 429, another visitor still gets in, 80 good requests in a row are all answered), 20 in all, exit 0. `discord-webhook-check.mjs` exit 0. `npm run check`: 689 tests. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
DarrellVS
added
publisher
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
1 participant
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.
Closes #25. Code scanning alerts 3, 10, 11, 12, 13, 14.
Missing rate limiting (alerts 10, 11, 12)
publisher/src/middlewares/rateLimits.ts, onexpress-rate-limit:/medianot limited (a seeking video is a burst of Range requests).Key =
CF-Connecting-IP, then firstX-Forwarded-Forhop, then socket. Behind a reverse proxyreq.ipis the proxy for everyone, and keying on it would be one bucket for the whole internet. Forging a header buys a fresh bucket (no worse than no limit) and never gets anybody in.Polynomial regex (alert 3)
/^Bearer\s+(.+)$/replaced with a hand parse; test: under 100 ms on 200,000 spaces.Tainted format string (alerts 13, 14)
Paths carrying the uploaded filename moved out of the first
console.warnargument.Gates
tests/unit/publisher: 12 new cases.node scripts/publisher-views-check.mjs: 20 checks, exit 0, including 61st wrong guess = 429, another visitor still 200, 80 good requests all 200.node scripts/discord-webhook-check.mjs: exit 0.npm run check: 689 tests.Before you redeploy
New dependency in
publisher/package.json, so the image needs a rebuild. If your proxy is not Cloudflare and does not setX-Forwarded-For, every visitor shares one bucket; nginx and Nginx Proxy Manager set it by default.🤖 Generated with Claude Code