feat: endpoint security hardening — auth, rate limiting, info leak fixes#118
feat: endpoint security hardening — auth, rate limiting, info leak fixes#118TerrifiedBug merged 11 commits intomainfrom
Conversation
BREAKING CHANGE: /api/metrics now always requires a service account token with metrics.read permission. The METRICS_AUTH_REQUIRED env var is removed.
BREAKING CHANGE: /api/health no longer returns the 'db' field. Health check callers should use the HTTP status code (200/503).
cb3ee50 to
2a6fc86
Compare
|
@greptile |
Greptile SummaryThis PR hardens 7 previously unauthenticated endpoints identified in a security audit, adding always-on Bearer token auth to One security defect found:
Confidence Score: 4/5Safe to merge after adding the All rate-limiting and session-guard changes are correct. The single P1 finding is that the metrics route authenticates callers but never enforces the src/app/api/metrics/route.ts — missing Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Request] --> B{Endpoint}
B --> C["/api/agent/enroll"]
B --> D["/api/webhooks/git"]
B --> E["/api/setup GET/POST"]
B --> F["/api/metrics"]
B --> G["/api/v1/docs"]
B --> H["/api/v1/openapi.json"]
B --> I["/api/health"]
C --> C1{IP rate limit 10 req/min}
D --> D1{IP rate limit 30 req/min}
E --> E1{IP rate limit 5 req/min}
C1 -->|exceeded| R429[429 Too Many Requests]
D1 -->|exceeded| R429
E1 -->|exceeded| R429
C1 -->|allowed| C2[Enrollment token validation]
D1 -->|allowed| D2[HMAC signature verification]
E1 -->|allowed| E2[Setup handler]
F --> F1{authenticateApiKey Bearer token}
F1 -->|invalid| R401[401 Unauthorized]
F1 -->|valid but metrics.read not checked| F2[Collect and return Prometheus metrics]
G --> G1{NextAuth session?}
H --> H1{NextAuth session?}
G1 -->|no| R401
H1 -->|no| R401
G1 -->|yes| G2[Serve Swagger UI + CSP header]
H1 -->|yes| H2[Return OpenAPI JSON spec]
I --> I2[DB ping]
I2 -->|ok| I3[200 status ok]
I2 -->|fail| I4[503 status error]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/app/api/metrics/route.ts
Line: 14-20
Comment:
**`metrics.read` permission never checked**
`authenticateApiKey` verifies the token is valid and returns the `ServiceAccountContext` (which includes `permissions`), but the route never calls `hasPermission(ctx, "metrics.read")`. Any service account holding any valid token — `pipelines.deploy`, `secrets.manage`, etc. — can currently read the Prometheus metrics data. The PR description and docs explicitly state that only accounts with `metrics.read` should be granted access.
```suggestion
const authHeader = request.headers.get("authorization");
const ctx = await authenticateApiKey(authHeader);
if (!ctx || !hasPermission(ctx, "metrics.read")) {
return new Response("Unauthorized\n", {
status: 401,
headers: { "Content-Type": "text/plain; charset=utf-8" },
});
}
```
Also import `hasPermission` at the top of the file:
```ts
import { authenticateApiKey, hasPermission } from "@/server/middleware/api-auth";
```
**Rule Used:** ## Security & Cryptography Review Rules
When revi... ([source](https://app.greptile.com/review/custom-context?memory=7cb20c56-ca6a-40aa-8660-7fa75e6e3db2))
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "docs: document metrics endpoint auth req..." | Re-trigger Greptile |
| const ctx = await authenticateApiKey(authHeader); | ||
| if (!ctx) { | ||
| return new Response("Unauthorized\n", { | ||
| status: 401, | ||
| headers: { "Content-Type": "text/plain; charset=utf-8" }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
metrics.read permission never checked
authenticateApiKey verifies the token is valid and returns the ServiceAccountContext (which includes permissions), but the route never calls hasPermission(ctx, "metrics.read"). Any service account holding any valid token — pipelines.deploy, secrets.manage, etc. — can currently read the Prometheus metrics data. The PR description and docs explicitly state that only accounts with metrics.read should be granted access.
| const ctx = await authenticateApiKey(authHeader); | |
| if (!ctx) { | |
| return new Response("Unauthorized\n", { | |
| status: 401, | |
| headers: { "Content-Type": "text/plain; charset=utf-8" }, | |
| }); | |
| } | |
| const authHeader = request.headers.get("authorization"); | |
| const ctx = await authenticateApiKey(authHeader); | |
| if (!ctx || !hasPermission(ctx, "metrics.read")) { | |
| return new Response("Unauthorized\n", { | |
| status: 401, | |
| headers: { "Content-Type": "text/plain; charset=utf-8" }, | |
| }); | |
| } |
Also import hasPermission at the top of the file:
import { authenticateApiKey, hasPermission } from "@/server/middleware/api-auth";Rule Used: ## Security & Cryptography Review Rules
When revi... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/metrics/route.ts
Line: 14-20
Comment:
**`metrics.read` permission never checked**
`authenticateApiKey` verifies the token is valid and returns the `ServiceAccountContext` (which includes `permissions`), but the route never calls `hasPermission(ctx, "metrics.read")`. Any service account holding any valid token — `pipelines.deploy`, `secrets.manage`, etc. — can currently read the Prometheus metrics data. The PR description and docs explicitly state that only accounts with `metrics.read` should be granted access.
```suggestion
const authHeader = request.headers.get("authorization");
const ctx = await authenticateApiKey(authHeader);
if (!ctx || !hasPermission(ctx, "metrics.read")) {
return new Response("Unauthorized\n", {
status: 401,
headers: { "Content-Type": "text/plain; charset=utf-8" },
});
}
```
Also import `hasPermission` at the top of the file:
```ts
import { authenticateApiKey, hasPermission } from "@/server/middleware/api-auth";
```
**Rule Used:** ## Security & Cryptography Review Rules
When revi... ([source](https://app.greptile.com/review/custom-context?memory=7cb20c56-ca6a-40aa-8660-7fa75e6e3db2))
How can I resolve this? If you propose a fix, please make it concise.
Summary
Secures 7 unauthenticated endpoints identified in a security audit:
/api/metrics— now requires Bearer token auth (BREAKING:METRICS_AUTH_REQUIREDenv var removed, auth is always on)/api/agent/enroll— IP rate limited to 10 req/min/api/webhooks/git— IP rate limited to 30 req/min/api/setup— IP rate limited to 5 req/min/api/v1/docs— requires NextAuth session + CSP header added/api/v1/openapi.json— requires NextAuth session, wildcard CORS removed/api/health— strippeddbfield from response (BREAKING: status code-only health checks unaffected)New infrastructure
src/app/api/_lib/ip-rate-limit.ts— IP-based rate limit helper reusing existingRateLimiterwith rightmostx-forwarded-forextractionRateLimiter.checkKey()method for explicit key-based rate limiting without tier suffixBreaking changes
/api/metricsmetrics.readservice account token to Prometheus scrape config/api/healthdbfield in response/api/v1/openapi.jsonTest plan
/api/metrics— 4 tests (401 without token, 401 invalid token, 200 with valid token, 500 on error)/api/health— 2 tests (200 without db field, 503 without db field)tsc --noEmitcleanpnpm lintclean