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 |
|---|---|---|
| Mixed concerns (refactor + behavior change) | This PR mixes a no-behavior hardening/refactor of payload sanitization (bounding and cycle-safe copying of context) with behavior changes to request-size enforcement and relay trust/rate-limit behavior in the same transport-boundary module. Split the cleanup/hardening from behavior changes: one PR for the `sanitizeBr ... [truncated 350 chars] |
|
| Removed observability | 1 potential issue(s) detected (max risk 55/100): packages/runtime-node/src/relay.ts:367. | |
| Silent exception swallowing | 1 potential issue(s) detected (max risk 80/100): packages/runtime-node/src/relay.ts:240. | |
| Possible non-atomic read-modify-write | 1 potential issue(s) detected (max risk 55/100): packages/runtime-node/src/relay.ts:233. | |
| Rate limiting not detected | 1 potential issue(s) detected (max risk 55/100): packages/runtime-node/src/relay.ts:24. | |
| Batch size limit not detected | 1 potential issue(s) detected (max risk 58/100): packages/runtime-node/src/relay.ts:134. | |
| Over-permissive IAM policy | 1 potential issue(s) detected (max risk 74/100): packages/runtime-node/src/relay.ts:354. | |
| PII in logs | 1 potential issue(s) detected (max risk 80/100): packages/runtime-node/src/relay.ts:295. | |
| Missing linked tracker issue | 1 potential issue(s) detected (max risk 60/100): packages/runtime-node/src/relay.ts:24. | |
| Missing CODEOWNERS reviewer approval | 1 potential issue(s) detected (max risk 70/100): packages/runtime-node/src/relay.ts:24. | |
| Missing security-team review on sensitive path | 1 potential issue(s) detected (max risk 85/100): packages/runtime-node/src/relay.ts:88. | |
| Source changes without matching tests | 1 potential issue(s) detected (max risk 63/100): packages/runtime-node/src/relay.ts:76. | |
| Migration missing rollback / down step | 1 potential issue(s) detected (max risk 12/100): packages/runtime-node/src/relay.ts:107. | |
| Hallucinated import (package not installed) | 1 potential issue(s) detected (max risk 80/100): packages/runtime-node/test/relay.test.mjs:2. | |
| Generic placeholder identifier in production logic | 1 potential issue(s) detected (max risk 45/100): packages/runtime-node/src/relay.ts:128. | |
| Repetitive boilerplate (duplicated block) | 1 potential issue(s) detected (max risk 31/100): packages/runtime-node/src/relay.ts:76. | |
| Missing input validation | 1 potential issue(s) detected (max risk 52/100): packages/runtime-node/src/relay.ts:34. | |
| API key in source | 1 potential secret(s) detected (max risk 57/100): packages/runtime-node/src/relay.ts:97. | |
| Route protection changed (protected → public) | 1 potential issue(s) detected (max risk 82/100): packages/runtime-node/src/relay.ts:445. | |
| Runtime error risk | 6 finding(s) on changed lines. | |
| Excessive complexity | 3 finding(s) on changed lines. | |
| Code duplication / DRY violation | 1 finding(s) on changed lines. | |
| Complexity Guard | 3 finding(s) on changed lines. | |
| Release Notes Curator | 2 finding(s) on changed lines. |
✅ Passed checks (153)
| Check | Status | Explanation |
|---|---|---|
| Too many files changed | ✅ Passed | Changed 2 file(s), within the limit of 50. |
| Too many lines changed | ✅ Passed | Changed 490 line(s), within the limit of 1000. |
| Too many unrelated chapters | ✅ Passed | 1 chapter(s) detected, within the limit of 6. |
| Generated files hiding real changes | ✅ Passed | Generated-file volume (0 lines) does not obscure the 490 hand-written line(s). |
| Missing PR context | ✅ Passed | PR context looks sufficient. |
| Migration + app logic + UI combined in one PR | ✅ Passed | No database migration file or UI layer change is present in this PR. |
| 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. |
| 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. |
| Optimistic locking not detected | ✅ Passed | No optimistic locking not detected issues detected. |
| Rate limiting removed | ✅ Passed | No rate limiting removed issues detected. |
| Pagination not detected | ✅ Passed | No pagination not detected issues detected. |
| Publicly exposed storage | ✅ Passed | No publicly exposed storage issues detected. |
| Security group open to the internet | ✅ Passed | No security group open to the internet issues detected. |
| Unencrypted storage at rest | ✅ Passed | No unencrypted storage at rest issues detected. |
| Infrastructure missing access logging | ✅ Passed | No infrastructure missing access logging issues detected. |
| Hardcoded secret in IaC | ✅ Passed | No hardcoded secret in iac issues detected. |
| Infrastructure misconfiguration | ✅ Passed | No infrastructure misconfiguration issues detected. |
| Deprecated Kubernetes API version | ✅ Passed | No deprecated kubernetes api version issues detected. |
| Compound IaC attack chain | ✅ Passed | No compound iac attack chain 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. |
| 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 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. |
| 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. |
| Direct env-var access bypasses config module | ✅ Passed | No direct env-var access bypasses config module 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. |
| 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. |
| Comment contradicts or fabricates code behaviour | ✅ Passed | No comment contradicts or fabricates code behaviour 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. |
| Doc-copy code with insecure defaults | ✅ Passed | No doc-copy code with insecure defaults 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. |
| 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. |
| 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. |
| 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. |
| Code correctness issue | ✅ Passed | No additional explanation was reported. |
| 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. |
| Simplifiable code | ✅ Passed | No additional explanation was reported. |
| Unnecessary type assertion | ✅ Passed | No additional explanation was reported. |
| Module smell | ✅ Passed | No additional explanation was reported. |
| Dead export (no callers) | ✅ Passed | No additional explanation was reported. |
| Bundle Size Monitor | ✅ Passed | No additional explanation was reported. |
This comment is updated automatically whenever Autter reviews a new PR revision.
|
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 wrote 1 temporary test for changed files that had no matching test, so the new behavior is still checked. It also executed 5 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: 6 checks executed · 5 passed · 1 failed. Project test commandsNo project test commands were detected in this repository. Autter targeted verification6 tests executed · 5 passed · 1 failed. Declared tests: 10 test file(s) found — 0 ran, 0 not observed in suite output, 10 did not run.
|
| Changed file | Related test | Result |
|---|---|---|
packages/runtime-node/src/relay.ts |
packages/runtime-node/test/relay.test.mjs |
✅ no declared test — temporary test passed |
🤖 Coverage-check evidence
packages/runtime-node/src/relay.ts
Ran: npm ci --include=dev && npm run build -w @autter/runtime-node && node --test --test-name-pattern='relay:' test/relay.test.mjs; node .autter/scratch/relay-node-rate-limit.test.mjs
Declared relay suite: 19 tests passed, 0 failed. It directly exercised context bounds/redaction, hostile Proxy handling, fetch byte limits/cancel failures, and fetch trustProxy behavior. Supplemental Node-handler test passed: strict true honors distinct X-Forwarded-For buckets; non-boolean trustProxy uses the socket bucket and second request returned 429.
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 test -w @autter/runtime-node. — agent execution observed the wrong behavior - ✅ Send valid browser events containing nested context and verify bounded context is forwarded without changing the event shape. — verified by agent execution
- ✅ Send context containing sensitive key names, Bearer/Basic values, and JWT-like strings; verify raw recognized secret values are not present in the sanitized payload. — verified by agent execution
- ✅ POST a multibyte UTF-8 body just above
maxBodyBytesand verify the relay responds with413. — verified by agent execution - ✅ Exercise the Node and fetch relay handlers with distinct
X-Forwarded-Forvalues and verify they share the default rate-limit bucket unlesstrustProxy: trueis explicitly configured. — verified by agent execution
🤖 Agent-executed checks
❌ Run npm test -w @autter/runtime-node.
Ran: npm test -w @autter/runtime-node
npm run build failed: sh: 1: tsup: not found (npm error code 127).
✅ Send valid browser events containing nested context and verify bounded context is forwarded without changing the event shape.
Ran: npx --yes tsx .autter/scratch/verify-relay.ts
item1: status 202; local sink captured context {ui:{panel:'checkout',flags:['new',{compact:true}]},count:3}, equal to submitted nested context.
✅ Send context containing sensitive key names, Bearer/Basic values, and JWT-like strings; verify raw recognized secret values are not present in the sanitized payload.
Ran: npx --yes tsx .autter/scratch/verify-relay.ts
item2: status 202; captured forwarded payload contains [redacted] for password, nested authorization, Bearer value, and JWT-like value; no raw recognized value was present.
✅ POST a multibyte UTF-8 body just above maxBodyBytes and verify the relay responds with 413.
Ran: npx --yes tsx .autter/scratch/verify-relay.ts
item3: UTF-8 body bytes=172, maxBodyBytes=171, response status=413.
✅ Exercise the Node and fetch relay handlers with distinct X-Forwarded-For values and verify they share the default rate-limit bucket unless trustProxy: true is explicitly configured.
Ran: npx --yes tsx .autter/scratch/verify-relay.ts
fetch default [400,429], fetch trustProxy:true [400,400], Node default [400,429], Node trustProxy:true [400,400] for distinct X-Forwarded-For values.
⬜ 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.
|
Autter found 1 issue(s) it could not attach to the current diff (the anchor line is not part of a diff hunk, or the PR advanced during the review): 🔴 [ai] Rate limiter trusts a spoofable forwarded-IP header (risk 82/100)
The public browser relay's only default abuse control is the per-IP 120/min limit, but the fetch handler keys it directly from a request-supplied
|
|
🔧 Autter is processing fixes for 5 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 context redaction is incomplete. It only drops keys matching a narrow blacklist and string values that look like Bearer/Basic credentials. Sensitive fields such as No fix branch or PR was created. To retry, uncheck and re-check the fix option on the task comment. Run ID: |
… IP source, detach oversized-body cancel
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: 88/100
sanitizeBrowserPayload now forwards e.context through boundContext, but the new redaction only covers a key-name regex and a couple of string patterns. Nested browser-supplied context can still carry raw secret values under benign keys, and createBrowserRelayFetchHandler / createBrowserRelayHandler will persist them in downstream telemetry via the public sanitizeBrowserPayload path. Blast radius — if this logging/observability gap is exploited it affects: 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)
Strengthen `boundContext` so any browser-supplied `context` value that looks secret-bearing is removed or redacted before it reaches telemetry. Cover nested objects/arrays recursively, and add tests proving raw values for passwords, tokens, session IDs, cookies, authorization headers, bearer/basic auth strings, and JWT-like strings never survive through `sanitizeBrowserPayload` into the relay output.
Flagged by Autter security & observability checks.
| const ctx = sanitizeContext({ | ||
| note: "Bearer supersecrettoken12345", | ||
| jwtish: | ||
| "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c", |
There was a problem hiding this comment.
🔴 [deterministic] JWT signing secret in source — Risk: 88/100
A JWT secret appears to be hard-coded in source. Treat the credential as compromised and rotate it now.
🛠 AI fix prompt (copy & paste into your coding agent)
In `packages/runtime-node/test/relay.test.mjs` around line 126 a JWT secret is hard-coded in source. Treat the literal as compromised even if the PR is closed without merging. Rotate the signing key, invalidate every issued token (bump the `kid` or drop the cache), and reject the old key on the verifier. Move the value to your project's secret manager (AWS Secrets Manager / GCP Secret Manager / Azure Key Vault / HashiCorp Vault / Doppler / 1Password) and load it at runtime via an env var. Diff-only: rotation + amending this PR (drop the literal) is sufficient. Affected services / downstream impact: none resolved (re-run the indexer for full context)..
Flagged by Autter security & observability checks.
| * that overwrites the header. When off, the fetch handler keys a single | ||
| * shared bucket, and the Node handler keys the real socket peer address. | ||
| */ | ||
| trustProxy?: boolean; |
There was a problem hiding this comment.
🟡 [ai] Non-boolean trustProxy values can enable spoofable forwarded-IP rate-limit keys — Risk: 35/100
Both relay handlers use opts.trustProxy ? ... : ..., so any truthy runtime value enables X-Forwarded-For handling. In particular, configuration code that passes process.env.TRUST_PROXY may pass "false", which is truthy and causes the public relay to key its limiter from an attacker-controlled header. Treat the setting as enabled only for opts.trustProxy === true (and optionally reject non-boolean option values) so malformed configuration retains the safe default.
⚠ 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)
Validate relay configuration at construction time and document the trusted-proxy requirement next to `trustProxy`. If this option is meant to be user-facing, prefer a strict schema or explicit boolean parsing so only a deliberate `true` enables forwarded-header trust. Blast radius — if this 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.
🤖 Release Notes CuratorPatch — Fixes browser relay payload handling to enforce byte-based body limits and safely bound forwarded context. Changelog: Improved browser relay reliability and safety by enforcing request limits by actual byte size and constraining client context before forwarding telemetry. Custom agent · runs after review · configured in Autter |
| 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] Secret values under benign context keys still reach privileged telemetry — Risk: 78/100
boundContext retains every string whose key does not match SECRET_KEY_RE, and scrubSecretValue only recognizes Bearer/Basic prefixes and JWT-shaped text. A public relay caller can therefore submit a credential under an innocuous key, for example {context:{note:"sk-<OpenAI key>"}}, {context:{debug:"AKIA..."}}, or a PEM/private connection string. That string is retained at line 122, incorporated into the sanitized event at line 293, and serialized into the authenticated fetch to the configured ingester at lines 312-320. The downstream browser normalizer only redacts values by key and email-shaped strings, so it also preserves this value. This remains a browser-controlled secret-disclosure path into telemetry despite the new recursive traversal.
⚠ 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
🛠 AI fix prompt (copy & paste into your coding agent)
Use an explicit allowlist for browser context fields, or extend recursive value redaction to cover the credential formats already protected by `packages/runtime-node/src/redact.ts` (private-key blocks, OpenAI/GitHub/AWS/Slack tokens, URL credentials, and email as applicable). Add an end-to-end relay test proving a secret value under a benign nested key is absent or redacted in the forwarded payload.
Flagged by Autter security & observability checks.
| const ip = opts.trustProxy | ||
| ? firstForwardedFor(request.headers.get("x-forwarded-for")) || | ||
| "unknown" | ||
| : "shared"; |
There was a problem hiding this comment.
🟠 [ai] Default Next relay now globally rate-limits all browser clients — Risk: 76/100
createAutterRelayRoute passes its documented default RelayOptions through unchanged, so trustProxy is absent for every standard Next integration. The changed fetch handler consequently uses the literal "shared" key for every request. Once any aggregate of clients sends 120 requests in the minute, all unrelated clients receive 429, rather than the advertised built-in per-IP limit. This is a caller-visible availability regression in the public Next relay path; the wrapper has no way to supply a trusted platform client address and no default opt-out, so the new handler contract is not safe for its existing callers.
⚠ 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-next/README.md,packages/runtime-node/src/relay.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Preserve a client-specific default rate-limit key for fetch deployments without trusting request-controlled headers. Add an optional trusted client-IP resolver to RelayOptions (and have the Next adapter provide a platform-trusted source where available), or make the inability to identify a peer an explicit opt-in shared limiter rather than the default. Cover createAutterRelayRoute's default configuration with requests from distinct clients.
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] Numeric credentials are intentionally forwarded from browser context — Risk: 74/100
boundContext explicitly retains every number or boolean whose key matches SECRET_KEY_RE. This is reachable through the public relay: sanitizeBrowserPayload accepts an object-valued event context and places boundContext(e.context) in the forwarded payload, then both handlers call forward with that payload. A browser can therefore submit {context:{otp:123456}} or {context:{sessionId:987654}}; both key names match the redaction expression, but line 183 preserves the raw numeric value and it is serialized into privileged downstream telemetry. Numeric one-time passcodes and numeric session identifiers are credentials, so the stated invariant that context must never carry secrets is not met.
⚠ 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 exempt numeric values solely because they are numeric for sensitive key names. Drop or redact every value under a matched secret-bearing key, or replace the broad regex with an explicit non-secret usage-count allowlist (for example exact `input_tokens`, `output_tokens`, and `total_tokens`) before applying redaction. Add relay sanitizer coverage for numeric `otp`, `sessionId`, and other numeric identifiers.
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 |
There was a problem hiding this comment.
🟠 [ai] trustProxy lets callers bypass the public relay rate limit — Risk: 68/100
The browser relay is deliberately public and uses its local fixed-window limiter before body parsing and privileged forwarding. When trustProxy is enabled, however, the limiter key is taken directly from the first request-supplied X-Forwarded-For value (lines 442-443). This boolean does not validate that the immediate peer is a configured proxy or that the proxy overwrote the header. Any deployment that enables the documented proxy mode while its proxy merely appends/preserves the header allows an unauthenticated caller to rotate the first value and obtain a fresh 120/min bucket for each request, defeating the only relay-side abuse guard while causing bounded parsing and authenticated forwards with the server ingest key.
⚠ 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)
Replace the boolean trustProxy switch with a trusted client-IP resolver supplied by the deployment adapter, or accept a trusted-proxy configuration that verifies the immediate peer/proxy chain before reading X-Forwarded-For. If trusted client IP cannot be established, retain the socket peer address (Node) or shared fetch bucket. Add tests proving a spoofed leading X-Forwarded-For value cannot create separate buckets when a proxy appends rather than overwrites the header.
Flagged by Autter security & observability checks.
Autter product walkAutter did not drive this change in a browser. This PR does not change a page Autter can boot and click through in a browser. Autter exercised the change with targeted checks instead of a live browser walk. |
… 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 task list
Generated from PR diff, blast radius, and context. Issues found
Also detected but not listed above: 28 finding(s) dismissed as likely false positives by verification — see the Autter review dashboard for their verdicts. 🛠 Fix optionsCheck one option and Autter will start a fix run for the unresolved issues above.
Checking a box triggers the fix run immediately — Autter comments back with the issues being fixed and the branch created for each. |
|
🔧 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: |
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.
Harden the public same-origin browser relay by bounding browser-supplied event context and enforcing request-size limits using UTF-8 byte counts. The relay continues to fail open for telemetry delivery while limiting untrusted payload complexity before forwarding under the server’s private ingest key.
Changes
event.context:byteLength()helper and use byte size—not JavaScript character count—when enforcingmaxBodyBytes.trustProxybehavior:RelayOptionsgains an additive optionaltrustProxysetting; enabling it remains deployment-sensitive because forwarded headers must be overwritten by a trusted proxy.Acceptance Criteria
contextconstrained by the configured depth, node, key, array, and string limits.sanitizeBrowserPayloador either relay handler to throw.maxBodyBytesreceive a413response, including bodies whose character length is smaller than their byte length.req.socket.remoteAddressby default; forwarded IPs are considered only whentrustProxy === true.Test Plan
npm test -w @autter/runtime-node.maxBodyBytesand verify the relay responds with413.X-Forwarded-Forvalues and verify they share the default rate-limit bucket unlesstrustProxy: trueis explicitly configured.Rollback Plan
Related Issues
No linked issue was identified.
Written for commit 8716ca7. Summary will update on new commits.