Repository navigation
fix(sentry): let an anonymous client error reach the tunnel, under a limit - #3149
Conversation
…limit The tunnel was not in publicPaths, so a browser error that happened before anyone signed in was answered with a 302 to the login page. The browser discards that, so those reports were not delayed, they were lost, and nothing anywhere said so. Safe to expose because the endpoint chooses nothing. Its destination comes from the server's configured Sentry project, the envelope's own DSN is only ever compared against it, an unconfigured deployment answers 404 rather than falling back, and redirects are never followed. That hardening landed in #2874, which is the work this finding had to wait for. Public does make it an anonymous POST relay into our own Sentry project, so sentryTunnelRateLimit bounds it per client address, scoped to that one path and placed before session() so an over-limit caller costs nothing it will not use. The ceiling is deliberately well above normal use: an error storm on a real page is when the reports matter most, so the limit must not be the thing that drops them. It is a speed bump rather than a control, since a caller with many addresses still gets through, and a Cloudflare rule remains the hard ceiling. The path was named in three places and a mismatch between them fails silently, so the client's tunnel option, the auth bypass and the limiter's scope now all read SENTRY_TUNNEL_PATH. Closes detail.dev D053.
|
Risk: No findings This incremental diff adds two protective bounds to the now-public Sentry tunnel: a 1 MiB Sentinel reviewed |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🩺 React Doctor — webapp✅ No new findings on the files changed by this PR. Run locally with |
![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: 310cf4d743
ℹ️ 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".
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. WalkthroughThe client and server use a shared Sentry tunnel path. The server limits POST envelope size and applies a per-IP rate limit of 300 requests per 60 seconds. The tunnel path is included in the public routes. ChangesSentry tunnel request limits
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant SentrySDK
participant WebServer
participant bodyLimit
participant sentryTunnelRateLimit
SentrySDK->>WebServer: POST SENTRY_TUNNEL_PATH with envelope
WebServer->>bodyLimit: Check envelope size
alt Envelope exceeds maximum
bodyLimit-->>SentrySDK: Return 413
else Envelope within maximum
bodyLimit->>sentryTunnelRateLimit: Check client IP request count
alt Rate limit exceeded
sentryTunnelRateLimit-->>SentrySDK: Return 429 JSON
else Within rate limit
sentryTunnelRateLimit-->>WebServer: Allow request
end
end
Merge Risk: ⚪ Minimal · up to The change is mergeable after normal checks. Verify that a logged-out browser error reaches Sentry after deployment. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…t sends one Three review findings on exposing the tunnel, all of them real. The route buffers the whole body, and the path is now reachable without a session, so an unbounded envelope was an unauthenticated way to make the server allocate. A per-minute request count cannot help, because one request can be arbitrarily large. The body is refused at 1 MiB before anything reads it. That ceiling is far above what this app sends: the browser SDK runs tracing only, no Replay and no attachments, so an envelope is an error event or a sampled transaction, measured in kilobytes. The limiter was mounted with `use`, which counts every method. A GET, HEAD or CORS preflight could therefore spend an address's budget and get that address's real reports refused. It is now scoped to POST, the only method that carries an envelope. The bucket is also coarser than one caller, and that is load-bearing rather than incidental. getClientIp trusts the edge header, which behind Cloudflare is the Cloudflare address, so in production one bucket covers everyone arriving through the same edge. client-ip.ts calls that acceptable because IP is only a fallback behind a signed-session key, and this limiter has no session to fall back from. A tight ceiling would throttle unrelated visitors, so the default moved to 300 and the reasoning is written down beside it: the ceiling says "clearly abnormal for one edge address", the body bound protects memory, and clipping during a genuine storm costs duplicates rather than the signal, since Sentry groups by fingerprint. The test that claimed one caller cannot silence another said more than the code delivers, so it now pins what is true, that each trusted address gets its own bucket, and records what that does not promise in production.
The only function the diff added without a docblock, which the review's docstring coverage gate picked up.
1 participant
Closes detail.dev D053, the last of the parked findings that needed code rather
than a decision.
The bug
/api/sentry-tunnelwas not inpublicPaths, so a browser error that happenedbefore anyone signed in got answered with a 302 to
/login. The browser discardsthat response, so those reports were not delayed, they were lost, and nothing
anywhere reported the loss. Pre-login is exactly where an unreported error hurts
most: a failure on the login or SSO path is invisible to us by construction.
Why it is safe to expose now
This finding was parked on purpose. Its tracker entry said it must not ship before
D071, because making the path anonymous while the tunnel's destination was
client-controlled would have converted an authenticated SSRF into an
unauthenticated one. That hardening shipped in PR #2874.
I re-read the route rather than trusting the note. The endpoint chooses nothing:
getConfiguredSentryTarget(), which reads server envenvelopeDsnMatches404rather than falling back to the enveloperedirect: "manual", so a 3xx is surfaced rather than chasedSo an anonymous caller gains no influence over where the request goes.
The residual, handled here
Public does make this an anonymous POST relay into our own Sentry project, so
sentryTunnelRateLimitbounds it per client address, scoped to that single pathand placed before
session()so an over-limit caller does not cost a sessionlookup it will not use.
The ceiling is deliberately generous (60/min/IP, matching the calendar feed).
An error storm on a legitimate page is the moment the reports matter most, so the
limiter must not become the thing that drops them. I have documented it as a speed
bump rather than a control: a caller with many addresses still gets through, and a
Cloudflare edge rule remains the hard ceiling, exactly as
mobileIpRateLimitalready states for its own bucket.
One change beyond the finding
The path was named in three places, and a mismatch between them fails silently:
the browser posts, gets a redirect, discards it, and no report arrives. Nothing in
a type check or a green suite can see that. So the client's
tunneloption, theauth bypass and the limiter's scope now all read
SENTRY_TUNNEL_PATHfromapp/utils/constants.ts.Tests
test/server/sentry-tunnel-exposure.test.ts, 8 cases.The contract half was run against the pre-fix
server/index.tsand the right twowent red (the bypass and the limiter scope). The file-text approach follows
test/routes-tests/api+/mobile-auth-contract.test.ts, which pins the mobileprefix's own
publicPathsbypass the same way.Verification
pnpm turbo typecheck --filter=@shelf/webapp, cleantest/server/,server/rate-limit.test.ts,test/utils/rate-limit.test.tsand the mobile-auth contract testAfter deploy
Worth a one-minute check that it works end to end, since no test can prove the
browser's side: open the app logged out, trigger a client error, and confirm the
event arrives in Sentry. Before this change it would not have.
Summary by CodeRabbit