Repository navigation
fix(runtime-node): bound browser relay context and count body size in bytes - #20
Conversation
There was a problem hiding this comment.
🔴 Autter review in progress — running security, correctness & dependency checks on this PR. Follow live step-by-step progress on the autter/review-gate check in the merge box. Merge is blocked until the gate completes; Autter approves automatically when the review comes back clean, and releases this hold with a neutral review when it finds non-blocking issues.
| } | ||
| const text = await request.text(); | ||
| if (text.length > maxBody) { | ||
| if (byteLength(text) > maxBody) { |
There was a problem hiding this comment.
🟠 [ai] Fetch relay enforces the body limit only after fully buffering the request — Risk: 75/100
await request.text() buffers and decodes the entire request before byteLength(text) > maxBody is checked. The byte calculation is correct, but it cannot protect the fetch-style relay from memory/CPU spent on oversized requests. Reject a known oversized content-length before reading when available, and read request.body incrementally with a byte budget (cancelling once exceeded) so the configured limit is enforced during consumption.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Functions/symbols:
sanitizeBrowserPayload,forward,firstForwardedFor,IpWindow.allow,respond,createBrowserRelayFetchHandler,createBrowserRelayHandler - Dependent files:
node:http - Scopes:
@autter/runtime-node
🛠 AI fix prompt (copy & paste into your coding agent)
Enforce the body limit while streaming the request instead of after `request.text`. Read the request body incrementally, accumulate bytes as chunks arrive, and abort with 413 as soon as the byte budget is exceeded.
Flagged by Autter security & observability checks.
🚦 Pre-merge checks ·
|
| Check | Status | Explanation |
|---|---|---|
| Missing linked tracker issue | 1 potential issue(s) detected (max risk 45/100): packages/runtime-node/src/relay.ts:95. | |
| Missing security-team review on sensitive path | 1 potential issue(s) detected (max risk 75/100): packages/runtime-node/src/relay.ts:297. | |
| Comment contradicts or fabricates code behaviour | 1 potential issue(s) detected (max risk 35/100): packages/runtime-node/src/relay.ts:146. | |
| Code correctness issue | 1 finding(s) on changed lines. | |
| Dead export (no callers) | 1 finding(s) on changed lines. | |
| Complexity Guard | 1 finding(s) on changed lines. | |
| Release Notes Curator | 1 finding(s) on changed lines. |
✅ Passed checks (161)
| Check | Status | Explanation |
|---|---|---|
| Too many files changed | ✅ Passed | Changed 2 file(s), within the limit of 50. |
| Too many lines changed | ✅ Passed | Changed 385 line(s), within the limit of 1000. |
| Too many unrelated chapters | ✅ Passed | 3 chapter(s) detected, within the limit of 6. |
| Generated files hiding real changes | ✅ Passed | Generated-file volume (0 lines) does not obscure the 385 hand-written line(s). |
| Missing PR context | ✅ Passed | PR context looks sufficient. |
| Mixed concerns (refactor + behavior change) | ✅ Passed | The PR is a hardening fix with tests. Bounding context and counting body size in bytes are both behavior changes in the same area. No renames, moves, or pure refactors are mixed in; new helpers such as boundContext, byteLength and readBodyBounded exist only to implement the fixes. |
| Migration + app logic + UI combined in one PR | ✅ Passed | No database migrations or UI changes. Only relay.ts in the runtime package and its test file are touched. |
| Sensitive data in logs | ✅ Passed | No sensitive data in logs issues detected. |
| Log injection | ✅ Passed | No log injection issues detected. |
| Missing audit logging | ✅ Passed | No missing audit logging issues detected. |
| Removed observability | ✅ Passed | No removed observability issues detected. |
| Silent exception swallowing | ✅ Passed | No silent exception swallowing issues remaining after verification. |
| Unhandled promise rejection | ✅ Passed | No unhandled promise rejection issues detected. |
| Circuit breaker not detected | ✅ Passed | No circuit breaker not detected issues detected. |
| Stack trace leakage | ✅ Passed | No stack trace leakage issues detected. |
| Multi-write without detected transaction | ✅ Passed | No multi-write without detected transaction issues detected. |
| Possible TOCTOU in critical path | ✅ Passed | No possible toctou in critical path issues detected. |
| Idempotency key not detected | ✅ Passed | No idempotency key not detected issues detected. |
| Possible non-atomic read-modify-write | ✅ Passed | No possible non-atomic read-modify-write issues detected. |
| Optimistic locking not detected | ✅ Passed | No optimistic locking not detected issues detected. |
| Rate limiting not detected | ✅ Passed | No rate limiting not detected issues detected. |
| Rate limiting removed | ✅ Passed | No rate limiting removed issues detected. |
| Batch size limit not detected | ✅ Passed | No batch size limit not detected issues remaining after verification. |
| Pagination not detected | ✅ Passed | No pagination not detected issues detected. |
| Prompt injection risk | ✅ Passed | No LLM/AI-integration code touched by this diff. |
| LLM output used in a dangerous sink | ✅ Passed | No LLM/AI-integration code touched by this diff. |
| Sensitive data in prompt or system-prompt leakage | ✅ Passed | No LLM/AI-integration code touched by this diff. |
| Over-privileged LLM tool / excessive agency | ✅ Passed | No LLM/AI-integration code touched by this diff. |
| Missing validation on an LLM-driven decision | ✅ Passed | No LLM/AI-integration code touched by this diff. |
| Unbounded LLM usage (denial-of-wallet) | ✅ Passed | No LLM/AI-integration code touched by this diff. |
| Table exposed without row-level security | ✅ Passed | No row-level-security-related code touched by this diff. |
| Over-broad row-level security policy | ✅ Passed | No row-level-security-related code touched by this diff. |
| Code path that bypasses row-level security | ✅ Passed | No row-level-security-related code touched by this diff. |
| Privileged database credential reachable from the client | ✅ Passed | No row-level-security-related code touched by this diff. |
| Privileged query without row-level scoping | ✅ Passed | No row-level-security-related code touched by this diff. |
| Row-level security enabled with no policy | ✅ Passed | No row-level-security-related code touched by this diff. |
| Template-default gradient styling | ✅ Passed | No added frontend pages or design-slop markers in this diff. |
| Interchangeable AI marketing copy | ✅ Passed | No added frontend pages or design-slop markers in this diff. |
| Placeholder content shipped to users | ✅ Passed | No added frontend pages or design-slop markers in this diff. |
| Emoji standing in for an icon system | ✅ Passed | No added frontend pages or design-slop markers in this diff. |
| Call-to-action that goes nowhere | ✅ Passed | No added frontend pages or design-slop markers in this diff. |
| Templated page composition | ✅ Passed | No added frontend pages or design-slop markers in this diff. |
| Color outside the brand palette | ✅ Passed | No added frontend markup or brand-relevant style changes in this diff. |
| Hardcoded style bypassing design tokens | ✅ Passed | No added frontend markup or brand-relevant style changes in this diff. |
| Typography outside the brand type system | ✅ Passed | No added frontend markup or brand-relevant style changes in this diff. |
| One-off UI instead of the shared component | ✅ Passed | No added frontend markup or brand-relevant style changes in this diff. |
| Copy that does not match brand voice | ✅ Passed | No added frontend markup or brand-relevant style changes in this diff. |
| Screen does not match the rest of the product | ✅ Passed | No added frontend markup or brand-relevant style changes in this diff. |
| Merge-blocking marker left in the change | ✅ Passed | No pending-work markers added by this diff. |
| Known-defect marker shipped in code | ✅ Passed | No pending-work markers added by this diff. |
| Untracked TODO without an issue reference | ✅ Passed | No pending-work markers added by this diff. |
| Test disabled or left pending | ✅ Passed | No pending-work markers added by this diff. |
| PII in logs | ✅ Passed | No pii in logs issues detected. |
| PII or internals leaked in error response | ✅ Passed | No pii or internals leaked in error response issues detected. |
| PII stored without application-level encryption | ✅ Passed | No pii stored without application-level encryption issues detected. |
| User data stored without retention controls | ✅ Passed | No user data stored without retention controls issues detected. |
| PII sent to external / cross-border destination | ✅ Passed | No pii sent to external / cross-border destination issues detected. |
| Lockfile resolution / integrity tampered | ✅ Passed | No lockfile resolution / integrity tampered issues detected. |
| Dependency runs install-time lifecycle script | ✅ Passed | No dependency runs install-time lifecycle script issues detected. |
| Possible dependency-confusion attack | ✅ Passed | No possible dependency-confusion attack issues detected. |
| Lockfile resolves a dependency the manifest does not declare | ✅ Passed | No lockfile resolves a dependency the manifest does not declare issues detected. |
| Checked-in build artefact modified without source change | ✅ Passed | No checked-in build artefact modified without source change issues detected. |
| Dockerfile build-step is insecure | ✅ Passed | No dockerfile build-step is insecure issues detected. |
| External artefact pulled in without integrity pinning | ✅ Passed | No external artefact pulled in without integrity pinning issues detected. |
| Changed export, importer not updated | ✅ Passed | No changed export with an un-updated importer detected. |
| Missing CODEOWNERS reviewer approval | ✅ Passed | No missing codeowners reviewer approval issues detected. |
| Source changes without matching tests | ✅ Passed | No source changes without matching tests issues detected. |
| Migration missing rollback / down step | ✅ Passed | No migration missing rollback / down step issues detected. |
| Frontend importing database client directly | ✅ Passed | No frontend importing database client directly issues detected. |
| Route handler bypassing service layer | ✅ Passed | No route handler bypassing service layer issues detected. |
| Backend service importing UI module | ✅ Passed | No backend service importing ui module issues detected. |
| Cross-context internals import | ✅ Passed | No cross-context internals import issues detected. |
| Workspace package rule violation | ✅ Passed | No workspace package rule violation issues detected. |
| Inconsistent logging pattern | ✅ Passed | No inconsistent logging pattern issues detected. |
| Inconsistent error handling | ✅ Passed | No inconsistent error handling issues detected. |
| Endpoint missing input validation | ✅ Passed | No endpoint missing input validation issues detected. |
| Multi-write without transaction wrapper | ✅ Passed | No multi-write without transaction wrapper issues detected. |
| New feature shipped without feature flag | ✅ Passed | No new feature shipped without feature flag issues detected. |
| Module placed in the wrong workspace package | ✅ Passed | No module placed in the wrong workspace package issues detected. |
| Hallucinated import (package not installed) | ✅ Passed | No hallucinated import (package not installed) issues detected. |
| Nonexistent package (not found in registry) | ✅ Passed | No nonexistent package (not found in registry) issues detected. |
| Call to function that does not exist | ✅ Passed | No call to function that does not exist issues detected. |
| Generic placeholder identifier in production logic | ✅ Passed | No generic placeholder identifier in production logic issues remaining after verification. |
| Repetitive boilerplate (duplicated block) | ✅ Passed | No repetitive boilerplate (duplicated block) issues detected. |
| Overbroad try/catch swallowing all exceptions | ✅ Passed | No overbroad try/catch swallowing all exceptions issues detected. |
| TODO / FIXME on critical path | ✅ Passed | No todo / fixme on critical path issues detected. |
| Abstraction defined but never used | ✅ Passed | No abstraction defined but never used issues detected. |
| Code style differs from rest of codebase | ✅ Passed | No code style differs from rest of codebase issues detected. |
| Established pattern ignored | ✅ Passed | No established pattern ignored issues detected. |
| Unhandled edge case (null / empty / zero / boundary) | ✅ Passed | No unhandled edge case (null / empty / zero / boundary) issues detected. |
| Insecure training-data / doc-example pattern | ✅ Passed | No insecure training-data / doc-example pattern issues detected. |
| Dead code (defined but never referenced) | ✅ Passed | No dead code (defined but never referenced) issues detected. |
| Deprecated API call | ✅ Passed | No deprecated api call issues detected. |
| API pattern from wrong library version | ✅ Passed | No api pattern from wrong library version issues detected. |
| API endpoint removed | ✅ Passed | No api endpoint removed issues detected. |
| HTTP method changed (GET ↔ POST etc.) | ✅ Passed | No http method changed (get ↔ post etc.) issues detected. |
| New required field added to request | ✅ Passed | No new required field added to request issues detected. |
| Field removed from response schema | ✅ Passed | No field removed from response schema issues detected. |
| Response field type changed | ✅ Passed | No response field type changed issues detected. |
| HTTP status code changed | ✅ Passed | No http status code changed issues detected. |
| Auth requirement added / removed / changed | ✅ Passed | No auth requirement added / removed / changed issues detected. |
| Error response shape changed | ✅ Passed | No error response shape changed issues detected. |
| Pagination behaviour changed | ✅ Passed | No pagination behaviour changed issues detected. |
| Outbound webhook payload schema changed | ✅ Passed | No outbound webhook payload schema changed issues detected. |
| GraphQL field removed without deprecation | ✅ Passed | No graphql field removed without deprecation issues detected. |
| GraphQL enum value removed | ✅ Passed | No graphql enum value removed issues detected. |
| Vendor API consumer | ✅ Passed | This PR does not touch call sites of an open vendor API change. |
| SQL injection | ✅ Passed | No sql injection issues detected. |
| Cross-site scripting (XSS) | ✅ Passed | No cross-site scripting (xss) issues detected. |
| Path traversal | ✅ Passed | No path traversal issues detected. |
| Command injection | ✅ Passed | No command injection issues detected. |
| Insecure deserialization | ✅ Passed | No insecure deserialization issues detected. |
| Weak cryptography | ✅ Passed | No weak cryptography issues detected. |
| Hardcoded secret | ✅ Passed | No hardcoded secret issues detected. |
| Insecure randomness for security material | ✅ Passed | No insecure randomness for security material issues detected. |
| Unsafe file upload | ✅ Passed | No unsafe file upload issues detected. |
| Missing input validation | ✅ Passed | No missing input validation issues detected. |
| Unsafe CORS configuration | ✅ Passed | No unsafe cors configuration issues detected. |
| Unsafe / open redirect | ✅ Passed | No unsafe / open redirect issues detected. |
| Missing CSRF protection | ✅ Passed | No missing csrf protection issues detected. |
| Unsafe cookie / session settings | ✅ Passed | No unsafe cookie / session settings issues detected. |
| Sensitive data exposure | ✅ Passed | No sensitive data exposure issues detected. |
| Invisible Unicode / Trojan Source characters | ✅ Passed | No invisible unicode / trojan source characters issues detected. |
| API key in source | ✅ Passed | No api key in source issues remaining after verification. |
| Access token in source | ✅ Passed | No access token in source detected. |
| Private key in source | ✅ Passed | No private key in source detected. |
| Database connection URL with embedded credentials | ✅ Passed | No database connection url with embedded credentials detected. |
| Cloud credential in source | ✅ Passed | No cloud credential in source detected. |
| Webhook signing secret in source | ✅ Passed | No webhook signing secret in source detected. |
| OAuth client secret in source | ✅ Passed | No oauth client secret in source detected. |
| JWT signing secret in source | ✅ Passed | No jwt signing secret in source detected. |
| Hardcoded password | ✅ Passed | No hardcoded password detected. |
| Auth middleware removed from route | ✅ Passed | No auth middleware removed from route issues detected. |
| Route protection changed (protected → public) | ✅ Passed | No route protection changed (protected → public) issues detected. |
| Permission / RBAC check removed | ✅ Passed | No permission / rbac check removed issues detected. |
| Required role weakened | ✅ Passed | No required role weakened issues detected. |
| Admin-only route exposed to lower privilege | ✅ Passed | No admin-only route exposed to lower privilege issues detected. |
| Token validation skipped in middleware chain | ✅ Passed | No token validation skipped in middleware chain issues detected. |
| JWT verification weakened or changed | ✅ Passed | No jwt verification weakened or changed issues detected. |
| Session expiration / TTL changed | ✅ Passed | No session expiration / ttl changed issues detected. |
| Password reset flow changed | ✅ Passed | No password reset flow changed issues detected. |
| OAuth callback / redirect handling changed | ✅ Passed | No oauth callback / redirect handling changed issues detected. |
| Webhook endpoint missing signature verification | ✅ Passed | No webhook endpoint missing signature verification issues detected. |
| Public route touches private/PII data | ✅ Passed | No public route touches private/pii data issues detected. |
| Frontend performance issue | ✅ Passed | No additional explanation was reported. |
| Frontend security issue | ✅ Passed | No additional explanation was reported. |
| Frontend correctness issue | ✅ Passed | No additional explanation was reported. |
| Accessibility issue | ✅ Passed | No additional explanation was reported. |
| Frontend maintainability issue | ✅ Passed | No additional explanation was reported. |
| Runtime error risk | ✅ Passed | No runtime error risk issues remaining after verification. |
| Resource leak risk | ✅ Passed | No additional explanation was reported. |
| Data integrity risk | ✅ Passed | No additional explanation was reported. |
| Maintainability issue | ✅ Passed | No additional explanation was reported. |
| Co-change coupling | ✅ Passed | No additional explanation was reported. |
| Redundant alias / duplicate import | ✅ Passed | No additional explanation was reported. |
| Redundant type construct | ✅ Passed | No additional explanation was reported. |
| Unnecessary type assertion | ✅ Passed | No additional explanation was reported. |
| Module smell | ✅ Passed | No additional explanation was reported. |
| Code duplication / DRY violation | ✅ Passed | No additional explanation was reported. |
| Intent vs. implementation | ✅ Passed | Yes — the diff bounds context with a cycle-safe, fail-open copier and meters body size in real UTF-8 bytes, matching the PR's stated goals; test coverage was omitted from review. Requirements: 5 implemented. (Intent from product context, PR description.) |
| Bundle Size Monitor | ✅ Passed | No additional explanation was reported. |
This comment is updated automatically whenever Autter reviews a new PR revision.
💡 2 suggestion(s) — conventions, hardening and hygiene, not defectsInline comments are reserved for concrete defects. These are things worth knowing that the diff does not prove wrong — a missing hardening layer, a convention the repo usually follows, a file that historically changes alongside one you touched. Skim, adopt what fits, ignore the rest.
|
There was a problem hiding this comment.
Autter completed PR review for #20: 2 finding(s) remain below the merge-blocking bar, so this review stays neutral rather than approving. (Also detected: 30 finding(s) dismissed as likely false positives by verification.) See the findings below; the task checklist follows as the review's final comment.
🤖 Release Notes CuratorImpact: patch — fixes relay input handling and request-size accounting without changing the public API. Changelog entry: Fixed browser relay validation to safely bound context data and enforce request body limits using UTF-8 byte size. Custom agent · runs after review · configured in Autter |
| const obj = v as object; | ||
| if (seen.has(obj)) return undefined; | ||
| seen.add(obj); | ||
| if (Array.isArray(v)) { |
There was a problem hiding this comment.
🔴 [ai] Guard proxy classification and array length before relay sanitization — Risk: 80/100
boundContext promises never to throw, but both Array.isArray(v) and the subsequent v.length access execute Proxy traps outside the existing catches. A revoked Proxy (or an array Proxy with a throwing get trap for length) supplied as event.context therefore throws from the newly added sanitizer. createBrowserRelayFetchHandler invokes sanitizeBrowserPayload without catching that error, so its public Next.js/App Router handler rejects instead of returning a response; the parsed-body Node path likewise calls it outside a catch. This changes the callers’ valid-payload contract from accepting a context object to allowing a hostile context to abort the request, so it is not safe to ship.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/runtime-next/src/server.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Make the entire `boundContext` walk non-throwing: guard `Array.isArray`, array length inspection, `WeakSet` operations, and all other proxy-observable operations. On an inspection failure, drop that context value. Add cases for a revoked Proxy and an array Proxy whose `length` getter/trap throws through both fetch and parsed-body relay handlers.
Flagged by Autter security & observability checks.
| const obj = v as object; | ||
| if (seen.has(obj)) return undefined; | ||
| seen.add(obj); | ||
| if (Array.isArray(v)) { |
There was a problem hiding this comment.
🟠 [ai] Guard array classification and length reads in context sanitization — Risk: 78/100
boundContext promises never to throw, but Array.isArray(v) and the loop condition's v.length are both outside a catch. A revoked array Proxy makes Array.isArray throw, and an array Proxy with a throwing get trap for length throws when the loop condition is evaluated. sanitizeBrowserPayload calls this walker without an error boundary while building each event, so an already-parsed hostile body passed to createBrowserRelayHandler (or direct callers of the exported sanitizer) causes the handler to throw instead of returning its controlled 400/202 response. That bypasses the relay's failure handling and can leave the request unresponded. Treat array classification and length inspection as untrusted operations and drop that context value on failure.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Wrap `Array.isArray(v)` and obtaining a bounded array length in a try/catch before entering the loop. If either operation throws, return `undefined` for this context value. Keep each indexed read guarded as it is now, and add coverage for both a revoked array Proxy and an array Proxy whose `length` getter trap throws through `sanitizeBrowserPayload` and the Node handler path.
Flagged by Autter security & observability checks.
| const obj = v as object; | ||
| if (seen.has(obj)) return undefined; | ||
| seen.add(obj); | ||
| if (Array.isArray(v)) { |
There was a problem hiding this comment.
🟠 [ai] Guard array classification and length access in bounded context — Risk: 76/100
boundContext promises to never throw for hostile context graphs, but Array.isArray(v) is outside a guard and the loop condition then reads v.length outside the existing element-read guard. Array.isArray throws for a revoked Proxy, and an array Proxy can throw from its get trap for length; either exception propagates through sanitizeBrowserPayload rather than dropping the context. This is reachable through the package-exported sanitizeBrowserPayload and through createBrowserRelayHandler when a body parser supplies an object body, so the relay handler can fail its request instead of returning its controlled response. The stated fail-open behavior is therefore not safe to ship.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/runtime-node/src/index.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Wrap array classification and length inspection in the same fail-open boundary as element reads. If either operation throws, return `undefined` from `walk` so the offending context value is omitted; add coverage using a revoked Proxy and an array Proxy whose `length` read throws.
Flagged by Autter security & observability checks.
| for (let i = 0; i < keys.length && i < CONTEXT_MAX_KEYS; i++) { | ||
| if (nodes >= CONTEXT_MAX_NODES) break; | ||
| nodes++; | ||
| const key = keys[i]!; |
There was a problem hiding this comment.
🟠 [deterministic] Biome: lint/style/noNonNullAssertion — Risk: 55/100
Forbidden non-null assertion.
🛠 AI fix prompt (copy & paste into your coding agent)
Fix the Biome `lint/style/noNonNullAssertion` issue at packages/runtime-node/src/relay.ts:127: Forbidden non-null assertion.
Flagged by Autter security & observability checks.
🧪 Autter test runAutter checked This PR changes 2 source files. No project test command ran for the touched workspace. It also executed 7 checks from the PR's test plan. Targeted agent checks: Autter ran targeted checks against the files this PR changed — it executed uncovered test-plan items and, where a changed file had no matching test, wrote a temporary test to verify the new behavior. Execution summary: 11 checks executed · 10 passed · 1 failed. Project test commandsNo project test commands were detected in this repository. Autter targeted verification8 tests executed · 8 passed. Code-level simulationsRevision
Declared tests: 42 test file(s) found — 1 ran, 0 not observed in suite output, 41 did not run. The project's command reported 0 of them. Autter ran the other 1 file one by one so every declared test has a verdict (1 passed, 0 failed). "Did not run" means no test command that Autter executed covers the file, so nothing in it was checked.
|
| Changed file | Related test | Result |
|---|---|---|
packages/runtime-node/src/relay.ts |
packages/runtime-node/test/relay.test.mjs |
✅ existing test passes |
🤖 Coverage-check evidence
packages/runtime-node/src/relay.ts
Temporary tests are written under .autter/scratch/ for verification only — they are never committed to the repository.
Test plan (from the PR description)
- ✅ Run
npm run buildat the repo root and confirm it passes. — verified by agent execution - ✅ Run
npm test -w @autter/runtime-nodeand confirm allrelay.test.mjscases pass, including the Proxy, cycle, multibyte, and never-settlingcancel()cases. — verified by agent execution - ✅ Start an app that mounts
createBrowserRelayFetchHandlerwithmaxBodyBytes: 10. POST a body of six Tamil characters ("அ".repeat(6), 6 code units, 18 bytes) and confirm the response is 413. — verified by agent execution - ✅ POST a ~500-byte body with
maxBodyBytes: 100and confirm 413. POST a normal browser payload under the limit and confirm 202. — verified by agent execution - ✅ POST an event whose
contextis nested 30+ levels deep, one with a 9000-char string, and one with a 500-element array. Confirm the forwarded payload (inspect viaonError/a local ingester log) is truncated to the documented limits. — verified by agent execution - ✅ Call
sanitizeBrowserPayloadin a Node REPL with a cyclic context, a revokedProxy.revocableobject, and an array Proxy whoselengthtrap throws. Confirm none throw and the event is still returned. — verified by agent execution - ⬜ Send
context: { request: { headers: { cookie: "sid=1", authorization: "Bearer x" } }, note: "Bearer supersecrettoken12345", keep: "ok" }through the relay to a local ingester (docker composeon port 4318). Read the row back from ClickHouse and confirm the secret values are[redacted]andkeepis intact. — needs manual verification - ⬜ Send
context: { input_tokens: 500, sessions: 3 }and confirm the numeric values are preserved in the stored row. — needs manual verification - ✅ Optional gap check: send
context: { note: "sk-…" }or{ debug: "AKIA…" }and note whether the value survives (expected today, per the known follow-up). — verified by agent execution
🤖 Agent-executed checks
✅ Run npm run build at the repo root and confirm it passes.
Root build completed successfully through runtime-core, runtime-node, runtime-edge, runtime-browser, otlp-ingester, and runtime-next; exit 0.
✅ Run npm test -w @autter/runtime-node and confirm all relay.test.mjs cases pass, including the Proxy, cycle, multibyte, and never-settling cancel() cases.
All 124 tests passed; 0 failed, 1 skipped. relay.test.mjs passed Proxy, cycle, UTF-8 multibyte, and never-settling/rejecting cancel() cases.
✅ Start an app that mounts createBrowserRelayFetchHandler with maxBodyBytes: 10. POST a body of six Tamil characters ("அ".repeat(6), 6 code units, 18 bytes) and confirm the response is 413.
Six Tamil characters measured 6 code units / 18 bytes; POST returned 413 with maxBodyBytes=10.
✅ POST a ~500-byte body with maxBodyBytes: 100 and confirm 413. POST a normal browser payload under the limit and confirm 202.
500-byte body returned 413 with maxBodyBytes=100; valid 90-byte browser payload returned 202.
✅ POST an event whose context is nested 30+ levels deep, one with a 9000-char string, and one with a 500-element array. Confirm the forwarded payload (inspect via onError/a local ingester log) is truncated to the documented limits.
Forwarded context was bounded to depth 5, string length 4000, and array length 100.
✅ Call sanitizeBrowserPayload in a Node REPL with a cyclic context, a revoked Proxy.revocable object, and an array Proxy whose length trap throws. Confirm none throw and the event is still returned.
Cyclic context, revoked Proxy, and array Proxy with throwing length trap did not throw; each sanitizeBrowserPayload result contained the event.
✅ Optional gap check: send context: { note: "sk-…" } or { debug: "AKIA…" } and note whether the value survives (expected today, per the known follow-up).
The abbreviated sample values 'sk-…' and 'AKIA…' survived unchanged. Complete representative OpenAI and AWS-shaped values were [redacted] by the sanitizer.
⬜ items could not be verified automatically and still need a manual check.
There was a problem hiding this comment.
🔴 Autter review in progress — running security, correctness & dependency checks on this PR. Follow live step-by-step progress on the autter/review-gate check in the merge box. Merge is blocked until the gate completes; Autter approves automatically when the review comes back clean, and releases this hold with a neutral review when it finds non-blocking issues.
| : {}), | ||
| ...(typeof e.context === "object" && e.context !== null | ||
| ? { context: e.context } | ||
| ? { context: boundContext(e.context) } |
There was a problem hiding this comment.
🟠 Browser relay forwards arbitrary secret-bearing context fields — Risk: 68/100
boundContext bounds context size and structure but does not apply a key/value allowlist or redact sensitive values. Because sanitizeBrowserPayload accepts any object-valued e.context, callers can submit cookies, authorization headers, passwords, tokens, or session identifiers in nested context and the relay forwards them to telemetry. Apply an explicit context schema/allowlist or redact sensitive keys and values before forwarding.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Functions/symbols:
sanitizeBrowserPayload,forward,firstForwardedFor,IpWindow.allow,respond,createBrowserRelayFetchHandler,createBrowserRelayHandler - Dependent files:
node:http - Scopes:
@autter/runtime-node
🛠 AI fix prompt (copy & paste into your coding agent)
Tighten the browser relay context sanitizer so secret-bearing values are redacted or dropped before they can reach downstream telemetry/logging. Add explicit key-based redaction for common secret fields like password, token, sessionId, cookie, auth, and set-cookie, and ensure `sanitizeBrowserPayload` never retains raw secret strings in `context`.
Flagged by Autter security & observability checks.
| if (text.length > maxBody) { | ||
| let bounded: { tooLarge: true } | { tooLarge: false; text: string }; | ||
| try { | ||
| bounded = await readBodyBounded(request, maxBody); |
There was a problem hiding this comment.
🟡 [ai] Failed stream cancellation can turn an oversized body into a 400 response — Risk: 35/100
After total > maxBody, await reader.cancel() can reject. The outer catch then returns 400 { error: "invalid json" } even though the size limit was exceeded. Treat cancellation as best-effort (for example, catch its rejection locally) and return { tooLarge: true } so oversized requests consistently receive 413. Consider distinguishing stream-read failures from JSON parse failures as well.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Functions/symbols:
sanitizeBrowserPayload,forward,firstForwardedFor,IpWindow.allow,respond,createBrowserRelayFetchHandler,createBrowserRelayHandler - Dependent files:
node:http - Scopes:
@autter/runtime-node
🛠 AI fix prompt (copy & paste into your coding agent)
Differentiate body-read failures from JSON parse failures. Catch `readBodyBounded` errors separately and return the appropriate non-JSON error response (or rethrow into the existing transport error handling), while keeping `JSON.parse` failures mapped to 400 invalid json. Add a test for a request body stream that rejects or errors mid-read. Blast radius — if this defect reaches production it can fail the downstream usage that depends on this file: functions `sanitizeBrowserPayload`, `forward`, `firstForwardedFor`, `IpWindow.allow`, `respond`, `createBrowserRelayFetchHandler`, `createBrowserRelayHandler`; scopes `@autter/runtime-node`; dependent files `node:http`.
Flagged by Autter security & observability checks.
There was a problem hiding this comment.
Autter completed PR review for #20: 3 finding(s) remain below the merge-blocking bar, so this review stays neutral rather than approving. (Also detected: 34 finding(s) dismissed as likely false positives by verification.) See the findings below; the task checklist follows as the review's final comment.
🤖 Release Notes CuratorImpact: patch — fixes relay input handling and request-size enforcement without changing the public API. Changelog: Fixed browser relay validation to safely bound context data and enforce request body limits using UTF-8 byte size. Custom agent · runs after review · configured in Autter |
…y-size check The browser relay sanitiser length-caps every event field except context, which was forwarded as-is. Bound context with a depth, node, string, and array/key limit (cycle-safe, and proxy-safe: revoked or hostile Proxy traps on classification, length, key enumeration, or element reads are dropped rather than thrown out of the sanitizer). Enforce maxBodyBytes in the fetch handler while consuming the request stream, counting real UTF-8 bytes and cancelling once the limit is crossed, instead of buffering the whole body via request.text() first. Add the first tests for relay.ts. No public API or behaviour change.
52f9ba9 to
882b971
Compare
There was a problem hiding this comment.
🔴 Autter review in progress — running security, correctness & dependency checks on this PR. Follow live step-by-step progress on the autter/review-gate check in the merge box. Merge is blocked until the gate completes; Autter approves automatically when the review comes back clean, and releases this hold with a neutral review when it finds non-blocking issues.
| for (let i = 0; i < keys.length && i < CONTEXT_MAX_KEYS; i++) { | ||
| if (nodes >= CONTEXT_MAX_NODES) break; | ||
| nodes++; | ||
| const key = keys[i]!; |
There was a problem hiding this comment.
🟠 [deterministic] Biome: lint/style/noNonNullAssertion — Risk: 55/100
Forbidden non-null assertion.
🛠 AI fix prompt (copy & paste into your coding agent)
Fix the Biome `lint/style/noNonNullAssertion` issue at packages/runtime-node/src/relay.ts:141: Forbidden non-null assertion.
Flagged by Autter security & observability checks.
| @@ -0,0 +1,115 @@ | |||
| import { test } from "node:test"; | |||
| import assert from "node:assert/strict"; | |||
There was a problem hiding this comment.
🔴 [heuristic] Hallucinated import (package not installed) — Risk: 80/100
Import target node:assert/strict is not declared in the project's manifests (no matching entry in indexed external_dependencies for ecosystem npm).
References:
- https://pypi.org/project/pkgguard-cli/ — # pkgguard-cli v0.1.0 Vet the packages and repos your AI assistant recommended — before you install. Catches hallucinated/slopsquatted names, malware signals, license traps, dead r
- https://pypi.org/project/slopgate/ — # slopgate v0.1.0 A CI gate against slopsquatting and hallucinated dependencies (Python / npm / Go). - Author: Nullra - License: MIT - Python: >=3.11 - Package URL: https://pypi
🛠 AI fix prompt (copy & paste into your coding agent)
In `packages/runtime-node/test/relay.test.mjs` around line 2, this import target is not in the project's package manifests or lockfile and was not found in the indexed external_dependencies. Either install it via the package manager, fix the typo, or — if a local module was intended — use the correct relative path. AI agents commonly hallucinate plausible-looking but nonexistent packages.
Flagged by Autter security & observability checks.
| if (t === "number" || t === "boolean") return v; | ||
| if (t !== "object") return undefined; | ||
| if (depth >= CONTEXT_MAX_DEPTH || nodes >= CONTEXT_MAX_NODES) return undefined; | ||
| const obj = v as object; |
There was a problem hiding this comment.
🟡 [heuristic] Generic placeholder identifier in production logic — Risk: 45/100
Identifier obj is a generic placeholder; production logic should name the actual concept. Blast radius — if this AI-generated slop ships it cascades to the downstream usage that depends on this file: functions sanitizeBrowserPayload, forward, firstForwardedFor, IpWindow.allow, respond, createBrowserRelayFetchHandler, createBrowserRelayHandler; scopes @autter/runtime-node; dependent files node:http.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Functions/symbols:
sanitizeBrowserPayload,forward,firstForwardedFor,IpWindow.allow,respond,createBrowserRelayFetchHandler,createBrowserRelayHandler - Dependent files:
node:http - Scopes:
@autter/runtime-node
🛠 AI fix prompt (copy & paste into your coding agent)
In `packages/runtime-node/src/relay.ts` around line 98, this identifier uses a generic placeholder name (`data`/`result`/`handler`/`temp`/`item`) in production logic. Rename it to describe what it actually contains. Placeholder names are a hallmark of AI-generated scaffolding that was never edited for context. Blast radius — if this AI-generated slop ships it cascades to the downstream usage that depends on this file: functions `sanitizeBrowserPayload`, `forward`, `firstForwardedFor`, `IpWindow.allow`, `respond`, `createBrowserRelayFetchHandler`, `createBrowserRelayHandler`; scopes `@autter/runtime-node`; dependent files `node:http`.
Flagged by Autter security & observability checks.
There was a problem hiding this comment.
Autter completed PR review for #20: 9 finding(s) remain below the merge-blocking bar, so this review stays neutral rather than approving. See the findings below; the task checklist follows as the review's final comment.
…ream cancel fails
There was a problem hiding this comment.
🔴 Autter review in progress — running security, correctness & dependency checks on this PR. Follow live step-by-step progress on the autter/review-gate check in the merge box. Merge is blocked until the gate completes; Autter approves automatically when the review comes back clean, and releases this hold with a neutral review when it finds non-blocking issues.
| : {}), | ||
| ...(typeof e.context === "object" && e.context !== null | ||
| ? { context: e.context } | ||
| ? { context: boundContext(e.context) } |
There was a problem hiding this comment.
🔴 Sensitive data in logs — Risk: 86/100
sanitizeBrowserPayload now forwards e.context through boundContext(...), but this sanitizer still preserves arbitrary user-supplied context values instead of redacting secret-bearing fields. That means downstream relay telemetry from createBrowserRelayHandler / createBrowserRelayFetchHandler can still carry passwords, tokens, session IDs, cookies, or auth headers in nested context objects. Blast radius — if this logging/observability gap is exploited it affects: functions sanitizeBrowserPayload, forward, firstForwardedFor, IpWindow.allow, respond, createBrowserRelayFetchHandler, createBrowserRelayHandler; scopes @autter/runtime-node; dependent files node:http.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Functions/symbols:
sanitizeBrowserPayload,forward,firstForwardedFor,IpWindow.allow,respond,createBrowserRelayFetchHandler,createBrowserRelayHandler - Dependent files:
node:http - Scopes:
@autter/runtime-node
🛠 AI fix prompt (copy & paste into your coding agent)
Update `boundContext` in `packages/runtime-node/src/relay.ts` to explicitly drop or redact secret-like keys and values before they reach telemetry. Add a small allowlist or key-based redaction for fields such as `password`, `token`, `sessionId`, `cookie`, `authorization`, and `set-cookie`, and ensure nested objects/arrays cannot reintroduce raw secret strings. Add tests showing secret-bearing context values are removed from `sanitizeBrowserPayload` output.
Flagged by Autter security & observability checks.
| : {}), | ||
| ...(typeof e.context === "object" && e.context !== null | ||
| ? { context: e.context } | ||
| ? { context: boundContext(e.context) } |
There was a problem hiding this comment.
🟡 Browser relay forwards unredacted client-supplied context — Risk: 41/100
The relay accepts arbitrary object-valued event.context and boundContext only bounds its shape and size. It does not redact or allowlist sensitive keys or values, so a public client can submit cookies, authorization headers, passwords, tokens, or session identifiers that are forwarded into telemetry. Apply explicit context allowlisting or redact/drop sensitive keys and values before forwarding.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Functions/symbols:
sanitizeBrowserPayload,forward,firstForwardedFor,IpWindow.allow,respond,createBrowserRelayFetchHandler,createBrowserRelayHandler - Dependent files:
node:http - Scopes:
@autter/runtime-node
🛠 AI fix prompt (copy & paste into your coding agent)
Keep the public relay from ingesting secret-bearing context data. Either drop `context` entirely on this unauthenticated path or apply a strict allowlist/redaction step before persistence so cookies, auth tokens, session IDs, and similar values cannot be written downstream. Blast radius — if this auth regression ships it cascades to the downstream usage that depends on this guard: functions `sanitizeBrowserPayload`, `forward`, `firstForwardedFor`, `IpWindow.allow`, `respond`, `createBrowserRelayFetchHandler`, `createBrowserRelayHandler`; scopes `@autter/runtime-node`; dependent files `node:http`.
Flagged by Autter security & observability checks.
There was a problem hiding this comment.
Autter completed PR review for #20: 3 finding(s) remain below the merge-blocking bar, so this review stays neutral rather than approving. (Also detected: 28 finding(s) dismissed as likely false positives by verification.) See the findings below; the task checklist follows as the review's final comment.
🤖 Release Notes CuratorImpact: patch — fixes relay payload handling and request-size enforcement without changing the public API. Changelog: Fixed browser relay handling to safely bound event context and enforce request body limits by UTF-8 byte size. Custom agent · runs after review · configured in Autter |
| : {}), | ||
| ...(typeof e.context === "object" && e.context !== null | ||
| ? { context: e.context } | ||
| ? { context: boundContext(e.context) } |
There was a problem hiding this comment.
🔴 [ai] Nested credentials in public relay context are forwarded without redaction — Risk: 82/100
The public same-origin relay accepts arbitrary object-valued event.context and the new boundContext copy preserves every key and string value at every permitted nesting level. An unauthenticated browser caller can therefore submit a nested credential such as {context:{request:{authorization:"Bearer victim-token"}}}; the relay attaches the server's private ingest key and forwards it. The receiving /v1/browser handler authenticates that key and stores the payload. Its scrubContext only tests each top-level context key, so it preserves the nested request.authorization value. This creates a credential disclosure path into telemetry storage and the downstream sink under the server key, rather than dropping or masking browser-supplied auth material before privileged forwarding.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/otlp-ingester/src/server.ts,packages/otlp-ingester/src/normalize-browser.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Redact or drop sensitive keys recursively in boundContext before returning the copied context. Cover case-insensitive password, token, sessionId, cookie, auth, authorization, bearer, credential, api-key, and set-cookie variants at every nesting level, and add an end-to-end relay-to-normalization test proving raw secret strings never persist or reach sink payloads.
Flagged by Autter security & observability checks.
| // must still surface as "too large" (413), never fall through to | ||
| // the caller's JSON-error branch and become a 400. | ||
| try { | ||
| await reader.cancel(); |
There was a problem hiding this comment.
🟠 [ai] Do not await an unbounded stream cancellation before responding 413 — Risk: 78/100
When an oversized request body is detected, this path awaits reader.cancel() before returning { tooLarge: true }. A ReadableStream implementation may return a cancellation promise that never settles (for example while waiting on an unresponsive upstream). In that failure mode the catch is never entered, readBodyBounded never resolves, and createBrowserRelayFetchHandler never reaches its 413 response at lines 321-324. This is reachable through the public Next.js App Router wrapper, so a malicious or failed upload can leave a route request hanging indefinitely after the size limit has already been crossed.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/runtime-next/src/server.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Once the byte budget is exceeded, make cancellation strictly best-effort without awaiting an unbounded promise. Call `reader.cancel()` inside a synchronous try/catch and attach a rejection handler (for example `void reader.cancel().catch(() => {})`), then return `{ tooLarge: true }` immediately. Add a test whose `cancel()` returns a never-settling promise and assert the fetch handler resolves 413.
Flagged by Autter security & observability checks.
… from relay tests
There was a problem hiding this comment.
🔴 Autter review in progress — running security, correctness & dependency checks on this PR. Follow live step-by-step progress on the autter/review-gate check in the merge box. Merge is blocked until the gate completes; Autter approves automatically when the review comes back clean, and releases this hold with a neutral review when it finds non-blocking issues.
| * `false` to disable (e.g. when a WAF already rate-limits). | ||
| */ | ||
| perIpRateLimit?: number | false; | ||
| /** |
There was a problem hiding this comment.
🟠 Rate limiting not detected — Risk: 55/100
RelayOptions.trustProxy makes the public browser relay rate limit depend on X-Forwarded-For again: createBrowserRelayFetchHandler/createBrowserRelayHandler now key IpWindow.allow from a requester-controlled header when this flag is enabled. If a deployment flips it on without a trusted proxy that overwrites the header, an attacker can rotate the first forwarded IP per request and bypass the relay-side window, driving unbounded body parsing and authenticated forwards through forward under the server ingest key. Blast radius — abusing this cascades to the downstream usage that depends on this file: functions sanitizeBrowserPayload, forward, IpWindow.allow, firstForwardedFor, respond, createBrowserRelayFetchHandler, createBrowserRelayHandler, RelayOptions; scopes @autter/runtime-node; dependent files node:http.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Functions/symbols:
sanitizeBrowserPayload,forward,IpWindow.allow,firstForwardedFor,respond,createBrowserRelayFetchHandler,createBrowserRelayHandler,RelayOptions - Dependent files:
node:http - Scopes:
@autter/runtime-node
🛠 AI fix prompt (copy & paste into your coding agent)
Keep the relay keyed from a trusted client-IP source only. Remove the boolean opt-in, or require a trusted-proxy resolver that validates the immediate peer before honoring `X-Forwarded-For`. If trustworthy client IP cannot be established, continue using the shared bucket for fetch handlers and `req.socket.remoteAddress` for Node handlers, and add tests showing spoofed forwarded headers cannot create fresh rate-limit buckets. Blast radius — abusing this cascades to the downstream usage that depends on this file: functions `sanitizeBrowserPayload`, `forward`, `IpWindow.allow`, `firstForwardedFor`, `respond`, `createBrowserRelayFetchHandler`, `createBrowserRelayHandler`, `RelayOptions`; scopes `@autter/runtime-node`; dependent files `node:http`.
Flagged by Autter security & observability checks.
| : {}), | ||
| ...(typeof e.context === "object" && e.context !== null | ||
| ? { context: e.context } | ||
| ? { context: boundContext(e.context) } |
There was a problem hiding this comment.
🔴 [ai] Browser relay preserves opaque secret values under benign context keys — Risk: 80/100
Although boundContext redacts sensitive key names recursively, its value-based redaction only detects Bearer/Basic strings and JWTs. A browser caller can place other credential formats—or opaque passwords or session secrets—under benign keys such as note or debug; those strings are retained in context and forwarded through the authenticated relay. Use a strict context allowlist or extend value redaction to cover the credential formats protected by the server-side redactor.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Functions/symbols:
sanitizeBrowserPayload,forward,IpWindow.allow,firstForwardedFor,respond,createBrowserRelayFetchHandler,createBrowserRelayHandler,RelayOptions - Dependent files:
node:http - Scopes:
@autter/runtime-node
🛠 AI fix prompt (copy & paste into your coding agent)
Change `boundContext` to recursively redact or drop secret-bearing values at every nesting level before they reach `sanitizeBrowserPayload`. Use an explicit allowlist or broaden value-based secret detection so raw tokens, cookies, auth headers, session IDs, and credential-like strings are removed even when they appear under non-secret keys. Add regression tests covering nested objects and arrays. Blast radius — exposing this data cascades to the downstream consumers that depend on this code: functions `sanitizeBrowserPayload`, `forward`, `IpWindow.allow`, `firstForwardedFor`, `respond`, `createBrowserRelayFetchHandler`, `createBrowserRelayHandler`, `RelayOptions`; scopes `@autter/runtime-node`; dependent files `node:http`.
Flagged by Autter security & observability checks.
| // they can never be a credential, and this preserves usage counts such as | ||
| // `input_tokens`. | ||
| const REDACTED = "[redacted]"; | ||
| const SECRET_KEY_RE = |
There was a problem hiding this comment.
🟠 [ai] Browser relay retains API-key-shaped values under benign context keys — Risk: 57/100
boundContext redacts sensitive key names plus Bearer/Basic and JWT-shaped strings, but preserves other credential formats under benign keys. For example, browser-controlled context such as { note: "sk-..." } or { debug: "AKIA..." } passes through sanitizeBrowserPayload and is forwarded by the privileged relay. Apply value-pattern redaction equivalent to the runtime-node attribute redactor, or use a strict context allowlist, before forwarding.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Functions/symbols:
sanitizeBrowserPayload,forward,IpWindow.allow,firstForwardedFor,respond,createBrowserRelayFetchHandler,createBrowserRelayHandler,RelayOptions - Dependent files:
node:http - Scopes:
@autter/runtime-node
🛠 AI fix prompt (copy & paste into your coding agent)
Extend the relay context sanitizer to recursively drop or redact secret-shaped values, not just secret-looking keys. Reuse the existing `boundContext`/`sanitizeBrowserPayload` path so browser-supplied context cannot carry raw API-key literals into `forward`, and add tests covering benign-key nested API keys reaching both relay handlers. Blast radius — if this credential is exploited it cascades to the downstream usage that depends on this file: functions `sanitizeBrowserPayload`, `forward`, `IpWindow.allow`, `firstForwardedFor`, `respond`, `createBrowserRelayFetchHandler`, `createBrowserRelayHandler`, `RelayOptions`; scopes `@autter/runtime-node`; dependent files `node:http`.
Flagged by Autter security & observability checks.
| // caller has explicitly opted into a trusted-proxy deployment. | ||
| const ip = | ||
| firstForwardedFor(req.headers["x-forwarded-for"]) || | ||
| (opts.trustProxy === true |
There was a problem hiding this comment.
🟠 trustProxy accepts an unvalidated forwarded IP for relay rate limiting — Risk: 68/100
When trustProxy === true, the fetch and Node relay handlers key their local limiter from the first request-provided X-Forwarded-For value. The boolean option does not establish that the immediate peer is a trusted proxy or that the proxy overwrites this header. Deployments with a direct route or a proxy that preserves/appends client values allow callers to rotate the leading value and evade the per-IP limit. Use an adapter-supplied trusted client-IP resolver or validate the peer/proxy chain before honoring forwarded headers; otherwise retain the socket peer address (Node) or the shared fetch bucket.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Functions/symbols:
sanitizeBrowserPayload,forward,IpWindow.allow,firstForwardedFor,respond,createBrowserRelayFetchHandler,createBrowserRelayHandler,RelayOptions - Dependent files:
node:http - Scopes:
@autter/runtime-node
🛠 AI fix prompt (copy & paste into your coding agent)
Keep the default rate-limit key tied to a trusted client-IP source only. If proxy trust is needed, require an adapter-provided trusted-IP resolver or validated proxy chain instead of a boolean flag that reads `X-Forwarded-For` directly; otherwise fall back to the socket peer or a single shared bucket. Blast radius — if this auth regression ships it cascades to the downstream usage that depends on this guard: functions `sanitizeBrowserPayload`, `forward`, `IpWindow.allow`, `firstForwardedFor`, `respond`, `createBrowserRelayFetchHandler`, `createBrowserRelayHandler`, `RelayOptions`; scopes `@autter/runtime-node`; dependent files `node:http`.
Flagged by Autter security & observability checks.
🤖 Release Notes CuratorImpact: patch — fixes browser relay payload sanitization and byte-accurate request-size enforcement without changing the public API. Changelog: Hardened the browser telemetry relay to safely bound event context data, redact credentials, and enforce request size limits accurately for all character sets. Custom agent · runs after review · configured in Autter |
|
Hi Sagnik, thanks a lot for taking the time to review this — I really appreciate it. I've pushed fixes for the actionable findings on fix/relay-context-bounds: Context redaction — boundContext now recursively redacts secret-bearing keys (authorization, cookie, token, *_secret, *_key, session, jwt, and similar) at every nesting level, and scrubs Bearer/Basic/JWT-shaped string values even under benign keys. Numeric values such as usage counts (input_tokens) are preserved. All covered by tests (19 passing), and the earlier JWT-in-test flag is resolved. There are two remaining items where I'd really value your direction, since both feel like calls that should be yours rather than mine to assume: First — the "secret value under a benign key" case (e.g. an API-key-shaped string under a key like note). A blacklist can't fully close this; the complete fix is an allowlist of approved context keys, which would change context from free-form metadata into a fixed schema. Would you like me to move to an allowlist, or keep the current recursive redaction? Second — the rate limiter. Fully resolving the forwarded-IP concern points toward a trusted-proxy client-IP resolver, which is a larger change and touches code that predates this PR. Would you prefer I take that on here, keep the current safe-by-default behaviour, or handle it in a separate PR? I'm also happy to split this into focused commits/PRs if that's easier for you to review. Whichever direction you feel is right for Autter, I'm glad to follow. Thanks again — really enjoying working in this codebase. |
| const walk = (v: unknown, depth: number): unknown => { | ||
| if (v === null) return null; | ||
| const t = typeof v; | ||
| if (t === "string") return scrubSecretValue((v as string).slice(0, CONTEXT_MAX_STRING)); |
There was a problem hiding this comment.
🔴 [ai] Browser context still forwards credential formats under benign keys — Risk: 88/100
This public relay copies every string at a non-matching context key and only calls scrubSecretValue, which recognizes Bearer/Basic prefixes and JWT-shaped values. Consequently an unauthenticated caller can send a raw OpenAI key (sk-...), AWS access key (AKIA...), GitHub/Slack token, private-key block, URL credential, or email under a key such as note; it survives boundContext, is attached to the sanitized event, and is JSON-serialized in the authenticated forward. The server redactor already defines these formats, but this privileged browser-forwarding boundary does not use it, and the ingester's scrubContext only examines top-level keys and email values, so it does not close the nested path.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/runtime-node/src/redact.ts,packages/otlp-ingester/src/normalize-browser.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Apply the runtime-node redactor's value-format coverage recursively at the relay boundary (private key blocks, OpenAI/GitHub/AWS/Slack tokens, URL credentials, and email), or replace arbitrary browser context with an explicit allowlist. Add an end-to-end relay test using each representative secret under a benign nested key and assert the serialized forwarded payload contains no raw value.
Flagged by Autter security & observability checks.
| const walk = (v: unknown, depth: number): unknown => { | ||
| if (v === null) return null; | ||
| const t = typeof v; | ||
| if (t === "string") return scrubSecretValue((v as string).slice(0, CONTEXT_MAX_STRING)); |
There was a problem hiding this comment.
🔴 [ai] Public relay still forwards credential-shaped values under benign context keys — Risk: 84/100
The relay route is deliberately unauthenticated, yet every accepted event is forwarded with the server ingest key. boundContext only treats Bearer/Basic-prefixed strings and JWT-shaped text as secret values. It therefore preserves raw OpenAI (sk-...), AWS (AKIA.../ASIA...), GitHub, Slack, PEM private-key, URL-credential, and email values when a browser caller puts them beneath an innocuous nested key such as context.debug or context.request.note. The forwarded payload is then accepted by /v1/browser under the server key, whose normalizer only redacts values by key and email-shaped strings, so these secrets can be persisted and sent to the sink through this privileged route. This cannot be proven safe for the public-to-private forwarding boundary.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/otlp-ingester/src/server.ts,packages/otlp-ingester/src/normalize-browser.ts,packages/runtime-node/src/redact.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Make browser relay context redaction cover the same credential value formats as runtime-node redact.ts at every nesting level (private-key blocks, OpenAI/GitHub/AWS/Slack tokens, URL credentials, and email), or replace arbitrary context with a strict allowlist. Add end-to-end tests through sanitizeBrowserPayload and forwarding asserting values under benign nested keys never survive.
Flagged by Autter security & observability checks.
| // back to one shared bucket (a conservative global limit). | ||
| const ip = | ||
| firstForwardedFor(request.headers.get("x-forwarded-for")) || "unknown"; | ||
| opts.trustProxy === true |
There was a problem hiding this comment.
🔴 [ai] trustProxy makes the public relay limiter key attacker-controlled — Risk: 82/100
The intentionally public relay forwards accepted payloads with the server's private ingest key, so its local limiter is the guard before body parsing and privileged forwarding. Setting the newly introduced trustProxy: true keys that guard directly from the first request-supplied X-Forwarded-For value. The boolean neither identifies nor verifies the immediate peer as a controlled proxy, nor proves that the proxy overwrites the header. In a common proxy configuration that appends or preserves client headers, an unauthenticated caller can rotate the leading value to obtain unlimited fresh 120/min buckets and drive unbounded parsing and authenticated downstream requests. The downstream per-key ingester limit is shared by all relay clients and is only reached after this public endpoint has already accepted and initiated the work.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/otlp-ingester/src/server.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Replace the boolean trustProxy option with an adapter/deployment-supplied trusted client-IP resolver, or a trusted-proxy configuration that validates the immediate peer/proxy chain before consulting X-Forwarded-For. Retain socket peer identity (Node) or a conservative shared bucket (fetch) when trusted identity cannot be established, and add a test showing spoofed leading X-Forwarded-For values cannot create new buckets.
Flagged by Autter security & observability checks.
| // back to one shared bucket (a conservative global limit). | ||
| const ip = | ||
| firstForwardedFor(request.headers.get("x-forwarded-for")) || "unknown"; | ||
| opts.trustProxy === true |
There was a problem hiding this comment.
🔴 [ai] trustProxy directly re-enables a requester-spoofable rate-limit key — Risk: 82/100
Setting the new boolean to true makes both relay handlers derive the limiter key from the first X-Forwarded-For value without validating the immediate peer or proxy chain. Any deployment that enables the documented proxy mode behind a proxy which appends or preserves client headers permits an unauthenticated caller to rotate that leading value and acquire unlimited fresh 120/min buckets. The local relay still parses each body and initiates privileged forwards under the shared server ingest key before downstream limits apply, so this does not meet the intended abuse guard.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Do not accept a raw forwarded header based on a boolean. Accept a deployment-supplied trusted client-IP resolver, or verify a configured trusted-proxy boundary/chain before consuming X-Forwarded-For. Cover spoofed leading forwarded values in both fetch and Node handlers when proxy support is enabled.
Flagged by Autter security & observability checks.
| continue; | ||
| } | ||
| const rt = typeof raw; | ||
| out[key] = rt === "number" || rt === "boolean" ? raw : REDACTED; |
There was a problem hiding this comment.
🟠 [ai] Sensitive numeric context values are forwarded intact — Risk: 78/100
For every key matched by SECRET_KEY_RE, this branch deliberately preserves numeric and boolean values. The public request path accepts arbitrary object-valued event.context, so {context:{otp:123456}} or {context:{sessionId:987654}} reaches this branch, is attached to the sanitized event, and is sent in the privileged ingest request. Numeric OTPs and session identifiers are credentials; preserving all numbers because the key might be a usage counter leaves a direct telemetry disclosure path.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/runtime-node/src/redact.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Redact all values at sensitive keys by default. If usage counters must survive, allow only an exact set of canonical usage-counter keys and only finite non-negative numeric values, matching the server redactor's `USAGE_TOKEN_KEYS` behavior. Cover numeric OTP and numeric session ID through the forwarding path.
Flagged by Autter security & observability checks.
| opts.trustProxy === true | ||
| ? firstForwardedFor(request.headers.get("x-forwarded-for")) || | ||
| "unknown" | ||
| : "shared"; |
There was a problem hiding this comment.
🟠 [ai] Default fetch relay globally rate-limits all browser clients — Risk: 76/100
The documented Next adapter passes RelayOptions through unchanged, so its standard integration leaves trustProxy unset. The changed fetch handler then uses the literal shared rate-limit key for every caller. Consequently, 120 aggregate requests in a minute from any mix of clients exhaust the sole bucket and cause unrelated legitimate users to receive 429 responses. This is a production availability regression on the deliberately public browser telemetry endpoint rather than the advertised per-IP protection; the adapter exposes no trusted client-IP source and the caller cannot retain a client-specific default key without opting into the spoofable header path.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-next/src/server.ts,packages/runtime-node/src/relay.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Add an optional trusted client-IP resolver to RelayOptions and have supported fetch adapters provide a platform-trusted source where available. Otherwise make the shared limiter an explicit opt-in rather than the default, or document and require an external per-client rate-limit boundary. Add coverage through createAutterRelayRoute's default options that validates the chosen client-specific limiter behavior.
Flagged by Autter security & observability checks.
| const ip = | ||
| firstForwardedFor(request.headers.get("x-forwarded-for")) || "unknown"; | ||
| opts.trustProxy === true | ||
| ? firstForwardedFor(request.headers.get("x-forwarded-for")) || |
There was a problem hiding this comment.
🟠 [ai] trustProxy enables requester-controlled rate-limit keys without proxy verification — Risk: 76/100
Setting the new boolean to true makes the public fetch relay use the first X-Forwarded-For value directly. Neither handler establishes that the immediate peer is a configured proxy nor that the proxy overwrote this header, so a client can rotate the leading value and receive a new 120/min bucket for each request. This bypasses the relay's only pre-parse limit and permits repeated bounded parsing and authenticated forwards under the server ingest key. Documentation asking callers to enable it only behind a correct proxy is not an enforcement boundary; the same direct use is present in the Node handler.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Do not derive the limit key directly from a request header. Replace `trustProxy` with an adapter-supplied trusted client-IP resolver, or require trusted-proxy configuration that validates the socket peer/proxy chain before accepting a forwarded address. Add fetch and Node tests showing spoofed leading X-Forwarded-For values cannot produce distinct buckets when proxy verification is absent.
Flagged by Autter security & observability checks.
| opts.trustProxy === true | ||
| ? firstForwardedFor(request.headers.get("x-forwarded-for")) || | ||
| "unknown" | ||
| : "shared"; |
There was a problem hiding this comment.
🟠 [ai] Default Next relay collapses all browser clients into one rate-limit bucket — Risk: 76/100
Without trustProxy, the fetch handler unconditionally keys its 120/min limiter as shared. The standard Next adapter passes RelayOptions through unchanged, and its documented default example supplies only apiKey, so every normal createAutterRelayRoute installation reaches this branch. After any aggregate 120 POSTs in a minute, unrelated clients receive 429 despite the RelayOptions contract documenting a per-IP default limit. This is a caller-visible availability regression introduced by the rate-limit hardening.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/runtime-next/src/server.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Provide a trusted client-IP resolver in RelayOptions and have the Next adapter use a platform-trusted source where available. Otherwise require explicit opt-in to a shared fetch limiter or preserve an appropriate per-client default without reading an untrusted header. Add a default createAutterRelayRoute test showing independent clients do not exhaust one another's quota.
Flagged by Autter security & observability checks.
| continue; | ||
| } | ||
| const rt = typeof raw; | ||
| out[key] = rt === "number" || rt === "boolean" ? raw : REDACTED; |
There was a problem hiding this comment.
🟠 [ai] Sensitive numeric context values are explicitly forwarded — Risk: 74/100
For every key matched by SECRET_KEY_RE, this branch preserves any numeric or boolean value rather than redacting it. The public handlers pass object-valued event.context to boundContext, so {context:{otp:123456}} and {context:{sessionId:987654}} are valid browser payloads and are serialized into authenticated telemetry unchanged. Numeric OTPs and session identifiers are credentials; treating all numbers as safe also contradicts the key-based redaction invariant. The existing test codifies the issue by expecting sessions and token-named fields to survive instead of restricting preservation to exact non-secret usage-count keys.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/runtime-node/src/redact.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Redact all values under sensitive keys by default. If usage counters must survive, use an exact allowlist equivalent to redact.ts's USAGE_TOKEN_KEYS and require finite non-negative numeric values; add coverage for numeric otp and sessionId values through sanitizeBrowserPayload and a relay handler.
Flagged by Autter security & observability checks.
| // back to one shared bucket (a conservative global limit). | ||
| const ip = | ||
| firstForwardedFor(request.headers.get("x-forwarded-for")) || "unknown"; | ||
| opts.trustProxy === true |
There was a problem hiding this comment.
🟠 [ai] trustProxy enables request-controlled IP rotation on the public relay — Risk: 70/100
The public relay uses this limiter before parsing and privileged forwarding, but setting trustProxy: true makes its bucket key the first value from the request-supplied X-Forwarded-For header. The option has no trusted proxy boundary, peer/proxy-chain verification, or adapter-provided client-IP resolver; the comment merely relies on deployment behavior. In a deployment where a proxy forwards or appends rather than overwrites the header, an unauthenticated caller can rotate the leading value to bypass the only relay-side request limit and drive arbitrary authenticated forwards using the shared server ingest key. The downstream server-key limiter is not equivalent because it is reached after this public route has accepted, parsed, and initiated each request.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/otlp-ingester/src/server.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Replace the boolean with a trusted client-IP resolver supplied by the deployment adapter, or validate the immediate peer/proxy chain before using X-Forwarded-For. Keep the socket address (Node) or a conservative shared bucket (fetch) when trusted client IP cannot be established, and test spoofed leading forwarded values cannot create new buckets.
Flagged by Autter security & observability checks.
|
🔧 Autter is processing fixes for 18 unresolved issue(s) — requested by @sagnik11. Packaging: one PR with all unresolved fixes — every fix lands on branch Issues being fixed:
1 unresolved finding(s) need a human instead of a patch — they are left out of this run and listed in a separate checklist comment below. Autter will comment here as fixes are pushed and fix PRs open. Issues that can't be safely automated are reported instead of guessed at. Run ID: |
|
👤 1 unresolved issue(s) need a human — Autter won't auto-fix these — requested by @sagnik11. These can't be resolved by a code patch. Autter is handling the automatable findings separately (see the fix run above) — this checklist is yours:
Tick items off as you resolve them — Autter re-checks these findings on the next pushed revision. |
|
🛑 Autter finished this fix run without pushing a fix branch — requested by @sagnik11. The generated patch was withheld after adversarial self-review judged it unsafe to propose: The new BEARER_RE is narrower than the previous redaction: it only redacts Bearer [REDACTED] with at least 10 characters from a restricted character set. Valid short or opaque Bearer [REDACTED] (for example containing characters outside that set) previously matched Findings the agent reported it could not safely automate:
No fix branch or PR was created. To retry, uncheck and re-check the fix option on the task comment. Run ID: |
Resolve the add/add conflict in packages/runtime-node/test/relay.test.mjs by keeping both the relay bounding tests from this branch and the CSP-violation tests added on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🤖 Release Notes CuratorImpact: minor. The PR adds a new opt-in The PR now carries both Changelog entry
Custom agent · runs after review · configured in Autter |
|
|
||
| /** Redact a string that looks like a credential (auth header value / JWT). */ | ||
| function scrubSecretValue(s: string): string { | ||
| return SECRET_VALUE_RE.test(s) || JWT_RE.test(s) ? REDACTED : s; |
There was a problem hiding this comment.
🔴 [ai] Public relay still forwards secrets under benign context keys — Risk: 80/100
The relay is intentionally unauthenticated, but it attaches its configured server ingest key when forwarding browser events. Its value scrubber only recognizes Bearer/Basic prefixes and JWT-shaped strings, so values such as OpenAI/AWS/GitHub/Slack credentials, PEM blocks, URL credentials, or opaque secrets under benign nested keys (for example context.debug or context.request.note) survive this boundary. The secret-key branch also preserves numeric values such as context.otp or context.sessionId. These values are forwarded with the server credential and can be persisted: /v1/browser authenticates the forwarded request, while its context scrubber only checks top-level keys and email strings and does not recursively inspect nested values (nor does its key expression cover otp). This leaves a browser-controlled credential disclosure path into privileged telemetry.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/otlp-ingester/src/normalize-browser.ts,packages/otlp-ingester/src/server.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Use a strict allowlist for browser context, or recursively scrub values at the relay boundary with coverage equivalent to the runtime-node redactor (including private-key blocks, API tokens, URL credentials, and email). Drop all values under sensitive keys regardless of type, except for an exact allowlist of validated usage-count keys. Add relay-to-ingester coverage proving representative nested secrets and numeric OTP/session values are absent from persisted/sink payloads.
Flagged by Autter security & observability checks.
| const walk = (v: unknown, depth: number): unknown => { | ||
| if (v === null) return null; | ||
| const t = typeof v; | ||
| if (t === "string") return scrubSecretValue((v as string).slice(0, CONTEXT_MAX_STRING)); |
There was a problem hiding this comment.
🔴 [ai] Public relay still forwards credentials in browser context — Risk: 80/100
This public relay does not establish the claimed secret-redaction boundary: strings under benign context keys are scrubbed only when they start with Bearer/Basic or match the JWT shape, so values such as OpenAI/AWS/GitHub/Slack keys, PEM private keys, URL credentials, and nested email/credential strings remain raw. In addition, the sensitive-key branch explicitly preserves all numeric/boolean values, so inputs such as {context:{otp:123456}} or {context:{sessionId:987654}} also survive. sanitizeBrowserPayload attaches this result to the event and forward serializes it in a request authenticated with the server ingest key. The ingester accepts arbitrary nested context and scrubContext only checks each top-level key plus email strings, leaving these nested credential values (and numeric otp/sessionId) available for storage and sink delivery. Use a strict context allowlist or recursively apply equivalent server-side key/value redaction before privileged forwarding, without exempting numeric values under secret-bearing keys.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/otlp-ingester/src/normalize-browser.ts,packages/otlp-ingester/src/server.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Harden boundContext at the public relay boundary: use a strict allowlist or recursively redact/drop credential-shaped strings using the same formats as packages/runtime-node/src/redact.ts, and redact all value types under sensitive keys (except an exact allowlist of validated usage counters). Add relay-to-forwarding regression coverage for nested OpenAI/AWS/GitHub/Slack keys, private-key blocks, URL credentials, email, and numeric otp/sessionId values.
Flagged by Autter security & observability checks.
| opts.trustProxy === true | ||
| ? firstForwardedFor(request.headers.get("x-forwarded-for")) || | ||
| "unknown" | ||
| : "shared"; |
There was a problem hiding this comment.
🟡 [ai] Fetch relay rate limit shares one bucket across all clients — Risk: 40/100
The public fetch-style relay keys every request to the literal shared by default because a Fetch Request has no peer-IP field. Consequently, any unauthenticated caller can consume the default 120-request window and cause legitimate clients of that relay instance to receive 429 responses until the window rolls over; this is a trivial cross-client availability attack on the browser telemetry endpoint. The Node handler can key on the socket peer, but this fetch path is used by Next.js and other supported Fetch runtimes. Key the default limiter by a trustworthy platform-provided client IP, or require a trusted proxy configuration that supplies a non-spoofable client address.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/relay.ts,packages/otlp-ingester/src/server.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Change the fetch relay's default rate-limit key from a shared constant to a trustworthy per-client IP supplied by the hosting adapter/platform, or require an explicitly trusted proxy configuration for Fetch runtimes; never use a client-controlled header as the identity source.
Flagged by Autter security & observability checks.
|
🟠 [verified] Change fails its execution check: packages/runtime-node/src/relay.ts — Risk: 70/100 The execution check failed while exercising the changed behavior in Ran: 🛠 AI fix prompt (copy & paste into your coding agent)Flagged by Autter security & observability checks. |
Autter review completed — merge blockedAutter completed its review and found at least one confirmed merge-blocking finding. Autter blocked this PR: test; test_failure; 1 confirmed correctness/runtime finding(s). See the findings below and the full PR review for details. Other findings explain the additional warnings, but they did not independently block this PR. The merge gate remains blocked until the confirmed finding is resolved. RetryRetrying reruns the review on the same commit. If the commit is unchanged, the same finding and block may be reproduced.
Autter will post a new comment when the retry starts. |
…ctor - Keep today's per-client rate-limit keying. Defaulting trustProxy off put every visitor of a default Next.js install (and any Node app behind a proxy) into one shared 120/min bucket. - Bound context first, then scrub it with the relay's shared redactor from main instead of a second, narrower set of secret patterns. - Guard the array check behind boundContext so a revoked Proxy context is dropped instead of throwing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Autter approved #20: no unresolved blocking findings remain after review.
There was a problem hiding this comment.
Autter approved #20: no unresolved blocking findings remain after review.
| } catch { | ||
| child = undefined; | ||
| } | ||
| if (child !== undefined) out[key] = child; |
There was a problem hiding this comment.
🟡 [ai] Code correctness issue — Risk: 30/100
boundContext copies context property names verbatim (out[key] = child) without applying CONTEXT_MAX_STRING or any key-length limit, so a long key bypasses the per-string cap. Impact is bounded by the overall request body limit (maxBodyBytes), but consider truncating or dropping keys longer than a reasonable limit.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Functions/symbols:
byteLength,boundContext,sanitizeBrowserPayload,createBrowserRelayFetchHandler,readBodyBounded,relayContext - Dependent files:
../../otlp-ingester/src/compat.js,./compat.js,./redact.js,@opentelemetry/api,node:http - Scopes:
@autter/runtime-node
🛠 AI fix prompt (copy & paste into your coding agent)
Cap context property-name length before adding each property to `out` (or drop keys exceeding the limit). Add a test showing that an overlong context key is truncated or omitted by `sanitizeBrowserPayload`. Blast radius — if this defect reaches production it can fail the downstream usage that depends on this file: functions `byteLength`, `boundContext`, `sanitizeBrowserPayload`, `createBrowserRelayFetchHandler`, `readBodyBounded`, `relayContext`; scopes `@autter/runtime-node`; dependent files `../../otlp-ingester/src/compat.js`, `./compat.js`, `./redact.js`, `@opentelemetry/api`, `node:http`.
Flagged by Autter security & observability checks.
🤖 Release Notes CuratorImpact: patch. This is a bug fix that hardens the browser relay. It caps the size of browser-supplied context and counts the request body limit in bytes. No API or option was removed or changed. The PR also carries a Changelog entry
Custom agent · runs after review · configured in Autter |
There was a problem hiding this comment.
Autter approved #20 with notes: the merge gate is clear, and 7 non-blocking finding(s) remain as review detail. (Also detected: 10 finding(s) dismissed as likely false positives by verification.)
Autter task list
Generated from PR diff, blast radius, and context. Issues foundNo unresolved code findings remain for this revision — 10 finding(s) dismissed as likely false positives by verification. Dismissed findings stay listed with their verdicts in the Autter review dashboard. 9 additional non-blocking finding(s) are available in the Autter review. |
Summary
The browser relay's payload sanitiser (
sanitizeBrowserPayload) length-caps every event field it forwards —message,stack,name,filename,route— butcontextwas forwarded verbatim, with no bound. That quietly contradicts the sanitiser's own contract:contextis exactly the field a client can use to push an unbounded, arbitrarily-nested object through the relay. This PR closes that gap, fixes a related size-limit inaccuracy, and adds the first test coverage forrelay.ts.Changes
1. Bound
context(sanitizeBrowserPayload)contextnow passes throughboundContext, a bounded, cycle-safe deep copy that mirrors the caps already applied to the other fields: a depth limit, a total-node budget, a per-string length cap, and array-length / key-count caps. Cycles are detected (the repeated reference is dropped) so a self-referential object can't loop, and reads that throw (hostile getters) or non-serialisable values fail open — that value is dropped and sanitising continues, so it never throws on the capture path.2. Count body size in bytes (
createBrowserRelayFetchHandler)The fetch-handler size guard compared
text.length(UTF-16 code units) againstmaxBodyBytes, so a multi-byte payload (non-Latin text, emoji) could carry several times the configured byte budget. It now measures UTF-8 bytes, matching the option's name and the Node handler (which already counts real bytes).TextEncoderkeeps it portable to the edge runtimes this handler targets (noBuffer).3. First tests for
relay.tsrelay.tshad no test file. Addedtest/relay.test.mjscovering context depth / string / array bounding, cyclic context, non-serialisable values, and the byte-vs-code-unit size limit.Testing
npm run buildsucceeds; the newrelay.test.mjsand the existingredact/lifecyclesuites pass.Notes
Summary
Summary generated by Autter.
Why this change
The browser relay accepted arbitrary client-supplied
contextobjects, so a hostile or oversized payload could smuggle unbounded data (or crash the sanitizer via cycles or hostile Proxies) through the public route. The body size limit also counted UTF-16 code units rather than bytes, so multibyte payloads could slip pastmaxBodyBytes.What changes for users
Browser events forwarded by the relay now carry a bounded, cycle-safe copy of
context, with limits on depth, node count, string length, array length, and key count. Secret-looking keys and secret-shaped values in that context are redacted. Request body limits in the fetch handler are enforced in UTF-8 bytes, and oversized bodies return 413.Implementation and review notes
boundContextinrelay.tsdoes a bounded deep copy: max depth 6, 256 nodes, strings capped at 4000 chars, arrays and key counts capped at 100, with aWeakSetfor cycle detection. Anything over a limit, cyclic, or non-serialisable is dropped.Array.isArray, length reads, key enumeration and element reads are wrapped in try/catch, so revoked or hostile Proxies drop that value instead of throwing intosanitizeBrowserPayloador the handlers. Reviewers should confirm every Proxy-observable operation is covered.password,token,authorization,cookie) and value-based scrubbing of Bearer and JWT-shaped strings. Numeric usage counts undertoken/sessionkeys are preserved. Prior review feedback flags that value redaction does not yet cover other credential formats (e.g.sk-,AKIA, PEM blocks, URL credentials) under benign keys, so check that gap.byteLengthusesTextEncoder.readBodyBoundedhandles the fetch handler's body reading, and a test checks that a 413 is still returned when the stream'scancel()never settles.sanitizeBrowserPayloadandcreateBrowserRelayFetchHandlerwere changed;relay.test.mjsadds about 214 lines of tests, including multibyte (Tamil) body sizing and oversized-body rejection.Changes
packages/runtime-node/src/relay.ts(+163/-7)CONTEXT_MAX_DEPTH(6),CONTEXT_MAX_NODES(256),CONTEXT_MAX_STRING(4000),CONTEXT_MAX_ARRAY(100) andCONTEXT_MAX_KEYS(100).byteLength(text), which returns the UTF-8 byte length viaTextEncoderand works on edge runtimes.boundContext(value):WeakSetfor cycle detection and a shared node budget.Array.isArray,length, key enumeration, and element reads, so revoked or throwing Proxies drop only that value.token/session-style keys.sanitizeBrowserPayloadnow routese.contextthroughboundContextinstead of passing it through raw (small net change in the existing hunk, +2/-4).readBodyBoundedand uses it increateBrowserRelayFetchHandlersomaxBodyBytesis enforced in UTF-8 bytes while the stream is read. An oversized body returns 413 even if the stream'scancel()never settles.packages/runtime-node/test/relay.test.mjs(+214/-1)lengthtrap, and a hostile element getter.cancel()./v1/browserversion 1 payload shape is unchanged; onlycontextcontent is bounded and redacted before forwarding.Acceptance Criteria
sanitizeBrowserPayloadnever throws for anycontextvalue, including cyclic graphs, revoked Proxies, and Proxies with throwinglength,get, or enumeration traps. The offending value is dropped and the rest of the event is kept.contextis bounded: depth ≤ 6, ≤ 256 nodes, strings ≤ 4000 chars, arrays ≤ 100 elements, objects ≤ 100 keys.password,token,authorization,cookie, etc.) are[redacted]at top level and when nested. Bearer/Basic- and JWT-shaped values under benign keys are[redacted]. Plain strings and numeric usage counts (input_tokens,sessions, …) are unchanged.createBrowserRelayFetchHandlerreturns 413 when the body exceedsmaxBodyBytesmeasured in UTF-8 bytes. A 6-character Tamil string (18 bytes) is rejected atmaxBodyBytes: 10.cancel()never settles.npm run buildandnpm test -w @autter/runtime-nodepass.sk-…,AKIA…, GitHub/Slack tokens, PEM blocks, URL credentials, email) under benign keys. This PR does not close that gap.Test Plan
npm run buildat the repo root and confirm it passes.npm test -w @autter/runtime-nodeand confirm allrelay.test.mjscases pass, including the Proxy, cycle, multibyte, and never-settlingcancel()cases.createBrowserRelayFetchHandlerwithmaxBodyBytes: 10. POST a body of six Tamil characters ("அ".repeat(6), 6 code units, 18 bytes) and confirm the response is 413.maxBodyBytes: 100and confirm 413. POST a normal browser payload under the limit and confirm 202.contextis nested 30+ levels deep, one with a 9000-char string, and one with a 500-element array. Confirm the forwarded payload (inspect viaonError/a local ingester log) is truncated to the documented limits.sanitizeBrowserPayloadin a Node REPL with a cyclic context, a revokedProxy.revocableobject, and an array Proxy whoselengthtrap throws. Confirm none throw and the event is still returned.context: { request: { headers: { cookie: "sid=1", authorization: "Bearer x" } }, note: "Bearer supersecrettoken12345", keep: "ok" }through the relay to a local ingester (docker composeon port 4318). Read the row back from ClickHouse and confirm the secret values are[redacted]andkeepis intact.context: { input_tokens: 500, sessions: 3 }and confirm the numeric values are preserved in the stored row.context: { note: "sk-…" }or{ debug: "AKIA…" }and note whether the value survives (expected today, per the known follow-up).Rollback Plan
763e04e(merge or squash revert) onmain. The change is confined topackages/runtime-node/src/relay.tsandtest/relay.test.mjs, so the revert is clean and needs no data or schema action.schemaStatements()change, or wire-format change is involved, so ingester deployments (AWS ECS, single-server) need no rollback.@autter/runtime-node(independent semver) from the revert. Dependents such as@autter/runtime-next, which re-exportscreateBrowserRelayFetchHandler, can pin the previous@autter/runtime-nodeversion in the meantime.maxBodyBytes(the default stays 64 KB) or temporarily disable the relay route.contextthrough unbounded and counting body size in UTF-16 code units. KeepperIpRateLimitenabled and consider a WAF body-size limit in front of the route until a fix lands.Related Issues
No linked issue was identified in the branch name, commit metadata, or diff context.
Written for commit 763e04e. Summary will update on new commits.