feat(security): add security headers and rate limiting middleware#629
feat(security): add security headers and rate limiting middleware#629theermite wants to merge 2 commits intoThe-Vibe-Company:mainfrom
Conversation
Security headers (all responses): - X-Content-Type-Options: nosniff - X-Frame-Options: DENY - Referrer-Policy: strict-origin-when-cross-origin - Permissions-Policy: restrict camera/mic/geo/payment - Content-Security-Policy: self + ws/wss + inline styles (Tailwind) - HSTS: 2 years with preload (HTTPS only, detected via X-Forwarded-Proto) Rate limiting (API routes): - 100 requests/minute per IP - X-RateLimit-Limit/Remaining/Reset headers on every response - 429 Too Many Requests with Retry-After header - In-memory store with periodic cleanup (60s interval) - IP extraction from X-Forwarded-For, X-Real-IP, or Bun requestIP Tests: 12 new tests (7 security headers, 5 rate limit), all passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@theermite is attempting to deploy a commit to the The Vibe Company Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR adds two Hono middlewares — Key findings:
Confidence Score: 4/5Safe to merge after fixing the X-Forwarded-For IP spoofing issue in the rate limiter, which currently allows unlimited bypass of the abuse-prevention control. One P1 defect: the rate limiter's IP extraction unconditionally trusts the client-supplied X-Forwarded-For header, making the limiter bypassable. All other findings are P2 style/hardening suggestions. The security-headers middleware and the middleware wiring are correct. web/server/middleware/rate-limit.ts — specifically the Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Hono as Hono App
participant SH as securityHeaders()
participant CORS as cors()
participant RL as rateLimit()
participant Route as API Route Handler
Client->>Hono: HTTP request /*
Hono->>SH: middleware (post-next)
SH->>CORS: next() [for /api/*]
CORS->>RL: next() [for /api/*]
RL->>RL: getClientIp() → trusts X-Forwarded-For
alt count <= max
RL->>Route: next()
Route-->>RL: response
RL-->>CORS: set X-RateLimit-* headers
else count > max
RL-->>CORS: 429 + Retry-After
end
CORS-->>SH: response
SH-->>SH: set security headers (X-Frame-Options, CSP, HSTS...)
SH-->>Hono: response
Hono-->>Client: final response
Prompt To Fix All With AIThis is a comment left during a code review.
Path: web/server/middleware/rate-limit.ts
Line: 65-71
Comment:
**X-Forwarded-For is unconditionally trusted, enabling rate-limit bypass**
`getClientIp` always prefers `X-Forwarded-For` when it is present. Because this header is set by the *client* in any direct-connection scenario (i.e. no trusted reverse proxy in front), an attacker can rotate through arbitrary IPs on every request:
```
GET /api/sessions HTTP/1.1
X-Forwarded-For: 10.0.0.1 # first request uses this bucket
X-Forwarded-For: 10.0.0.2 # second request gets a fresh bucket
...
```
This entirely defeats the rate limiter's stated goal of preventing abuse.
The conventional fix is to make proxy trust opt-in via a `trustProxy` flag (or a proxy-count parameter), and only fall back to the connection IP when that flag is not set. When a proxy *is* trusted, the correct IP to extract is the **rightmost untrusted IP** in the `X-Forwarded-For` chain — not the leftmost, which is also client-controlled even behind a proxy.
Since the server binds to `0.0.0.0` and may be directly internet-exposed (Fly.io is referenced in a comment), this is an active defect rather than a theoretical concern.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: web/server/middleware/security-headers.ts
Line: 42-53
Comment:
**`object-src` not explicitly set**
`default-src 'self'` acts as the fallback for `object-src`, which governs `<object>`, `<embed>`, and `<applet>` elements. The OWASP CSP cheat sheet recommends explicitly adding `object-src 'none'` to eliminate any reliance on the fallback and to be future-proof against browsers that might treat `object-src` differently.
```suggestion
"default-src 'self'",
"script-src 'self'",
"style-src 'self' 'unsafe-inline'",
"img-src 'self' data: blob:",
"font-src 'self'",
"connect-src 'self' ws: wss: blob:",
"worker-src 'self' blob:",
"object-src 'none'",
"frame-ancestors 'none'",
"base-uri 'self'",
"form-action 'self'",
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(security): add security headers and..." | Re-trigger Greptile |
| function getClientIp(c: { req: { header: (name: string) => string | undefined }; env?: unknown }): string | undefined { | ||
| // Trust X-Forwarded-For from nginx reverse proxy | ||
| const forwarded = c.req.header("x-forwarded-for"); | ||
| if (forwarded) return forwarded.split(",")[0].trim(); | ||
|
|
||
| const realIp = c.req.header("x-real-ip"); | ||
| if (realIp) return realIp; |
There was a problem hiding this comment.
X-Forwarded-For is unconditionally trusted, enabling rate-limit bypass
getClientIp always prefers X-Forwarded-For when it is present. Because this header is set by the client in any direct-connection scenario (i.e. no trusted reverse proxy in front), an attacker can rotate through arbitrary IPs on every request:
GET /api/sessions HTTP/1.1
X-Forwarded-For: 10.0.0.1 # first request uses this bucket
X-Forwarded-For: 10.0.0.2 # second request gets a fresh bucket
...
This entirely defeats the rate limiter's stated goal of preventing abuse.
The conventional fix is to make proxy trust opt-in via a trustProxy flag (or a proxy-count parameter), and only fall back to the connection IP when that flag is not set. When a proxy is trusted, the correct IP to extract is the rightmost untrusted IP in the X-Forwarded-For chain — not the leftmost, which is also client-controlled even behind a proxy.
Since the server binds to 0.0.0.0 and may be directly internet-exposed (Fly.io is referenced in a comment), this is an active defect rather than a theoretical concern.
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/server/middleware/rate-limit.ts
Line: 65-71
Comment:
**X-Forwarded-For is unconditionally trusted, enabling rate-limit bypass**
`getClientIp` always prefers `X-Forwarded-For` when it is present. Because this header is set by the *client* in any direct-connection scenario (i.e. no trusted reverse proxy in front), an attacker can rotate through arbitrary IPs on every request:
```
GET /api/sessions HTTP/1.1
X-Forwarded-For: 10.0.0.1 # first request uses this bucket
X-Forwarded-For: 10.0.0.2 # second request gets a fresh bucket
...
```
This entirely defeats the rate limiter's stated goal of preventing abuse.
The conventional fix is to make proxy trust opt-in via a `trustProxy` flag (or a proxy-count parameter), and only fall back to the connection IP when that flag is not set. When a proxy *is* trusted, the correct IP to extract is the **rightmost untrusted IP** in the `X-Forwarded-For` chain — not the leftmost, which is also client-controlled even behind a proxy.
Since the server binds to `0.0.0.0` and may be directly internet-exposed (Fly.io is referenced in a comment), this is an active defect rather than a theoretical concern.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
3 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="web/server/middleware/security-headers.ts">
<violation number="1" location="web/server/middleware/security-headers.ts:12">
P2: Security headers won’t be applied on error responses because header-setting runs only after `await next()` without a `try/finally`. If downstream throws, the middleware exits early and error responses lack CSP/XFO/etc.</violation>
<violation number="2" location="web/server/middleware/security-headers.ts:25">
P2: HSTS detection is too strict: exact `x-forwarded-proto === "https"` can miss valid HTTPS requests behind proxies.</violation>
</file>
<file name="web/server/middleware/rate-limit.ts">
<violation number="1" location="web/server/middleware/rate-limit.ts:67">
P1: Rate limiting trusts spoofable forwarding headers without a trusted-proxy check, allowing per-IP limit bypass and unbounded key growth in the in-memory store.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| */ | ||
| export function securityHeaders(): MiddlewareHandler { | ||
| return async (c, next) => { | ||
| await next(); |
There was a problem hiding this comment.
P2: Security headers won’t be applied on error responses because header-setting runs only after await next() without a try/finally. If downstream throws, the middleware exits early and error responses lack CSP/XFO/etc.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/server/middleware/security-headers.ts, line 12:
<comment>Security headers won’t be applied on error responses because header-setting runs only after `await next()` without a `try/finally`. If downstream throws, the middleware exits early and error responses lack CSP/XFO/etc.</comment>
<file context>
@@ -0,0 +1,56 @@
+ */
+export function securityHeaders(): MiddlewareHandler {
+ return async (c, next) => {
+ await next();
+
+ // Prevent MIME-type sniffing
</file context>
|
|
||
| // HSTS — enforce HTTPS for 2 years, include subdomains | ||
| // Only set when served over HTTPS (detected via X-Forwarded-Proto or direct TLS) | ||
| const proto = c.req.header("x-forwarded-proto") ?? ""; |
There was a problem hiding this comment.
P2: HSTS detection is too strict: exact x-forwarded-proto === "https" can miss valid HTTPS requests behind proxies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/server/middleware/security-headers.ts, line 25:
<comment>HSTS detection is too strict: exact `x-forwarded-proto === "https"` can miss valid HTTPS requests behind proxies.</comment>
<file context>
@@ -0,0 +1,56 @@
+
+ // HSTS — enforce HTTPS for 2 years, include subdomains
+ // Only set when served over HTTPS (detected via X-Forwarded-Proto or direct TLS)
+ const proto = c.req.header("x-forwarded-proto") ?? "";
+ if (proto === "https" || c.req.url.startsWith("https://")) {
+ c.header(
</file context>
…o CSP Make X-Forwarded-For trust opt-in via trustProxy option (default false) to prevent attackers from bypassing rate limiting by rotating the header value. When trustProxy is enabled, use the rightmost IP (proxy-appended) instead of the leftmost (client-controlled). Add object-src 'none' to CSP as defense-in-depth. Add tests covering IP spoofing prevention. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
The Companion currently serves no security headers on HTTP responses. This PR adds two Hono middlewares:
Security headers (all responses)
nosniffDENYstrict-origin-when-cross-origincamera=(), microphone=(), geolocation=(), payment=()default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; connect-src 'self' ws: wss: blob:; frame-ancestors 'none'max-age=63072000; includeSubDomains; preload(HTTPS only)CSP allows
'unsafe-inline'for styles (required by Tailwind) andws:/wss:for WebSocket connections.Rate limiting (API routes)
X-RateLimit-*headers on every response429 Too Many RequestswithRetry-Afterwhen exceededFiles added
web/server/middleware/security-headers.ts+ test (7 tests)web/server/middleware/rate-limit.ts+ test (5 tests)web/server/index.ts— middleware registration (3 lines)Test plan
Development methodology
This contribution was developed using MNK-GoRin, a structured AI-assisted
development methodology by The Ermite. Each change
went through: audit → plan validation → test-driven generation → human review.
🤖 Generated with Claude Code