diff --git a/README.md b/README.md index 71c5f7b..0ac1164 100644 --- a/README.md +++ b/README.md @@ -1,19 +1,123 @@ # prism-api -HTTP API that wraps [prism](https://github.com/hidetzu/prism) — a tool that +HTTP API that wraps [`prism`](https://github.com/hidetzu/prism), a tool that decomposes Pull Requests into structured, AI-review-ready context. -> **Status:** Phase 1 (v0.1.0-dev). Currently provides liveness, readiness, -> and version endpoints only. The `/v1/analyze` and `/v1/prompt` endpoints -> are planned for the next iteration. +> **Status:** Phase 2 (v0.2.0). Provides health, analyze, and prompt +> endpoints for public GitHub Pull Requests. + +## Who is this for? + +- **Multi-language CI/CD pipelines** — call from Python, Node, or Ruby + without shipping a Go binary to your CI image. +- **GitHub Actions and bot implementations** — automate PR analysis from + workflows, Slack/Teams bots, or any service that already speaks HTTP. +- **Distribution layer for the `prism` CLI** — one hosted endpoint, + rate-limited and bounded, serving multiple callers. ## Endpoints -| Method | Path | Description | -|--------|------------|------------------------| -| `GET` | `/healthz` | Liveness probe | -| `GET` | `/readyz` | Readiness probe | -| `GET` | `/version` | API and Go version | +| Method | Path | Description | +|--------|-----------------|------------------------------------------------| +| `GET` | `/healthz` | Liveness probe | +| `GET` | `/readyz` | Readiness probe | +| `GET` | `/version` | API and Go runtime version | +| `POST` | `/v1/analyze` | Decompose a GitHub PR into structured context | +| `POST` | `/v1/prompt` | Generate an AI review prompt from a GitHub PR | + +### `POST /v1/analyze` + +Request: + +```json +{ + "pull_request_url": "https://github.com/owner/repo/pull/123" +} +``` + +Response: + +```json +{ + "result": { + "pull_request": { "provider": "github", "repository": "owner/repo", "id": "123", "title": "...", "author": "...", "url": "..." }, + "analysis": { "change_type": "feature", "risk_level": "low", "review_axes": ["..."], "summary": "..." }, + "changed_files": [ + { "path": "a.go", "status": "modified", "additions": 10, "deletions": 2, "language": "go" } + ] + } +} +``` + +Example: + +```bash +curl -sS -X POST http://localhost:8080/v1/analyze \ + -H 'Content-Type: application/json' \ + -d '{"pull_request_url":"https://github.com/owner/repo/pull/123"}' +``` + +### `POST /v1/prompt` + +Request: + +```json +{ + "pull_request_url": "https://github.com/owner/repo/pull/123", + "mode": "light", + "language": "en" +} +``` + +`mode` and `language` are optional. When omitted, `prism` applies its own +defaults (`light` mode, `en` language). Accepted modes: `light`, `detailed`, +`cross`. Accepted languages: `en`, `ja`. + +Response: + +```json +{ + "prompt": "You are a code reviewer performing a quick screening..." +} +``` + +Example (Japanese detailed mode): + +```bash +curl -sS -X POST http://localhost:8080/v1/prompt \ + -H 'Content-Type: application/json' \ + -d '{"pull_request_url":"https://github.com/owner/repo/pull/123","mode":"detailed","language":"ja"}' +``` + +### Error format + +Every error response follows this envelope: + +```json +{ + "error": { + "code": "invalid_input", + "message": "pull_request_url: must be a github.com URL", + "request_id": "01HXXXXXXXX" + } +} +``` + +The `X-Request-Id` response header carries the same id. Log the header +(or the body field) when reporting issues so the server-side entry can be +correlated quickly. + +| HTTP | Code | When it fires | +|------|--------------------------|------------------------------------------------| +| 400 | `invalid_input` | Request body is malformed or fails validation | +| 400 | `unsupported_provider` | Provider URL parses but is not allowed | +| 401 | `auth_required` | Upstream requires authentication | +| 413 | `payload_too_large` | Request body exceeds `MAX_REQUEST_BYTES` | +| 429 | `rate_limited` | Per-IP rate limit exceeded | +| 502 | `upstream_failure` | Upstream provider (GitHub) returned an error | +| 503 | `service_unavailable` | Server-wide concurrency cap exceeded | +| 504 | `timeout` | Processing exceeded `REQUEST_TIMEOUT` | +| 500 | `internal_error` | Unknown failure | ## Quick Start @@ -22,7 +126,7 @@ make build make run ``` -In another terminal: +Test the health endpoints in another terminal: ```bash curl -sS http://localhost:8080/healthz @@ -44,28 +148,52 @@ curl -sS http://localhost:8080/version ## Configuration -All configuration is supplied via environment variables. The table below -lists the variables wired in Phase 1; see -[`docs/development_rules.md`](docs/development_rules.md) §12 for the full -Phase 1 configuration surface. - -| Variable | Default | Purpose | -|----------------------|----------|--------------------------------------| -| `PORT` | `8080` | Listen port | -| `LOG_LEVEL` | `info` | slog level (debug/info/warn/error) | -| `REQUEST_TIMEOUT` | `30s` | Per-request processing timeout | -| `SHUTDOWN_TIMEOUT` | `25s` | Graceful shutdown wait | -| `ALLOWED_PROVIDERS` | `github` | Comma-separated provider allowlist | - -## Provider Support - -prism-api currently supports **GitHub** only. For AWS CodeCommit, use the -prism CLI directly or self-host prism-api in your own AWS environment. +All configuration is supplied via environment variables. Values are +validated at startup; invalid inputs cause `prism-api` to exit loudly +rather than silently start with broken state. + +| Variable | Default | Purpose | +|---------------------------|----------|---------------------------------------------------| +| `PORT` | `8080` | Listen port (0–65535) | +| `LOG_LEVEL` | `info` | slog level (`debug` / `info` / `warn` / `error`) | +| `REQUEST_TIMEOUT` | `30s` | Per-request processing timeout | +| `SHUTDOWN_TIMEOUT` | `25s` | Graceful shutdown wait | +| `ALLOWED_PROVIDERS` | `github` | Comma-separated provider allowlist | +| `MAX_REQUEST_BYTES` | `262144` | Request body size limit (256 KiB). `0` disables | +| `RATE_LIMIT_RPM` | `10` | Per-IP rate limit (requests/minute). `0` disables | +| `RATE_LIMIT_BURST` | `20` | Per-IP burst capacity | +| `MAX_CONCURRENT_REQUESTS` | `50` | Global in-flight request cap. `0` disables | + +See [`docs/development_rules.md`](docs/development_rules.md) §12 for the +full configuration surface including variables reserved for future phases. + +## Known constraints (v0.2.0) + +- **No authentication.** Any caller who can reach the deployed URL can + submit any public GitHub PR for analysis. Do not deploy the public + image for private-repository workflows. +- **Public GitHub PRs only.** Per-request GitHub token passthrough is on + the roadmap but not yet wired. +- **GitHub unauthenticated rate limit.** The upstream GitHub API limits + unauthenticated callers to **60 requests per hour per source IP**. + Heavy CI/CD usage will hit this limit before `prism-api`'s own + per-IP `RATE_LIMIT_RPM` fires. +- **`related_files` is heuristic.** The `analysis.related_files` list in + `/v1/analyze` responses can include paths that are not present in the + PR; they are heuristic review-time suggestions from `prism`'s + classifier, not authoritative statements about the PR contents. Treat + them as hints. +- **No caching.** Re-analyzing the same PR re-runs the full GitHub + fetch. Caching is on the roadmap but not in Phase 2. +- **Rate limit state grows unbounded.** The per-IP limiter map is + append-only for the lifetime of a single `prism-api` process. Fly.io + machines auto-stop on idle so this rarely matters in practice, but + long-running deployments should plan to restart periodically. ## Deployment (Fly.io) -`fly.toml` is pre-configured for a single `shared-cpu-1x` / 256MB machine in -`nrt` (Tokyo) with `auto_stop_machines` enabled so idle instances stop +`fly.toml` is pre-configured for a single `shared-cpu-1x` / 256MB machine +in `nrt` (Tokyo) with `auto_stop_machines` enabled so idle instances stop billing. ```bash diff --git a/internal/app/app_chain_test.go b/internal/app/app_chain_test.go index 1f1525a..d0c6d9c 100644 --- a/internal/app/app_chain_test.go +++ b/internal/app/app_chain_test.go @@ -197,9 +197,8 @@ func TestChain_RateLimitRejectsExcessFromSameIP(t *testing.T) { } func TestChain_RateLimitKeysByXForwardedFor(t *testing.T) { - // Two clients arriving through the same Fly edge (shared RemoteAddr) - // must be keyed independently by X-Forwarded-For. Without XFF keying, - // the second client would inherit the first's exhausted bucket. + // Non-Fly fallback: two clients arriving through a reverse proxy + // (no Fly-Client-IP) must be keyed independently by X-Forwarded-For. cfg := chainConfig() cfg.RateLimitRPM = 60 cfg.RateLimitBurst = 1 @@ -225,6 +224,36 @@ func TestChain_RateLimitKeysByXForwardedFor(t *testing.T) { } } +func TestChain_RateLimitResistantToXFFSpoofing(t *testing.T) { + // SECURITY: On Fly.io the edge sets Fly-Client-IP with the real + // client address. An attacker who rotates X-Forwarded-For must NOT + // get a fresh rate-limit bucket per request if Fly-Client-IP is the + // same. The rate limiter keys by Fly-Client-IP when present, making + // XFF spoofing ineffective. + cfg := chainConfig() + cfg.RateLimitRPM = 60 + cfg.RateLimitBurst = 1 + a := New(cfg, slog.New(slog.NewJSONHandler(io.Discard, nil))) + + send := func(flyClientIP, xff string) int { + req := httptest.NewRequest(http.MethodGet, "/healthz", nil) + req.Header.Set("Fly-Client-IP", flyClientIP) + req.Header.Set("X-Forwarded-For", xff) + req.RemoteAddr = "10.0.0.1:1234" + return serveRequest(a, req).Code + } + + // Same real IP (Fly-Client-IP), rotated spoofed XFF. + if send("198.51.100.1", "fake-ip-1") != http.StatusOK { + t.Fatalf("first request: want 200") + } + // If rate limiting used XFF, "fake-ip-2" would be a fresh bucket → 200. + // Since Fly-Client-IP is preferred, the same real IP is rate-limited → 429. + if code := send("198.51.100.1", "fake-ip-2"); code != http.StatusTooManyRequests { + t.Errorf("second request with spoofed XFF: status = %d, want 429 (Fly-Client-IP must be used)", code) + } +} + func TestChain_AnalyzeEndpointSuccess(t *testing.T) { stubA := &stubAnalyzeUsecase{ result: prism.Result{ diff --git a/internal/httpapi/handler/health.go b/internal/httpapi/handler/health.go index 5dd2af4..1ed0e32 100644 --- a/internal/httpapi/handler/health.go +++ b/internal/httpapi/handler/health.go @@ -9,7 +9,7 @@ import ( ) // apiVersion is the current prism-api release marker. Bump at tag time. -const apiVersion = "0.1.0-dev" +const apiVersion = "0.2.0" // HealthHandler serves liveness, readiness, and version endpoints. type HealthHandler struct { diff --git a/internal/httpapi/middleware/clientip.go b/internal/httpapi/middleware/clientip.go index bd1227a..9d96ff1 100644 --- a/internal/httpapi/middleware/clientip.go +++ b/internal/httpapi/middleware/clientip.go @@ -6,12 +6,25 @@ import ( "strings" ) -// clientIP extracts the client IP address from the request. It prefers the -// first entry in X-Forwarded-For when present (behind a trusted proxy such -// as Fly.io's edge) and falls back to RemoteAddr otherwise. The helper is -// package-internal because logging and rate limiting both need the same -// extraction logic and neither wants to drift from the other. +// clientIP extracts the client IP address from the request. It is used by +// both the logging and rate-limiting middleware and must return a +// consistent, non-spoofable value so the per-IP rate limiter cannot be +// bypassed by a malicious X-Forwarded-For header. +// +// Priority order: +// +// 1. Fly-Client-IP — set by Fly.io's edge proxy with the real client +// address. The client cannot inject or override this header because +// Fly strips any client-supplied value before setting its own. This +// is the primary source when running on Fly.io. +// 2. X-Forwarded-For (first entry) — fallback for non-Fly environments +// (local development, other reverse proxies that are trusted). +// 3. RemoteAddr — direct-connect fallback. func clientIP(r *http.Request) string { + // Prefer Fly-Client-IP so rate limiting is resistant to XFF spoofing. + if fci := r.Header.Get("Fly-Client-IP"); fci != "" { + return strings.TrimSpace(fci) + } if xff := r.Header.Get("X-Forwarded-For"); xff != "" { if idx := strings.IndexByte(xff, ','); idx >= 0 { return strings.TrimSpace(xff[:idx]) diff --git a/internal/httpapi/middleware/clientip_test.go b/internal/httpapi/middleware/clientip_test.go index c5426c6..55b926d 100644 --- a/internal/httpapi/middleware/clientip_test.go +++ b/internal/httpapi/middleware/clientip_test.go @@ -7,16 +7,44 @@ import ( ) // TestClientIP covers every extraction path the helper supports, with -// particular attention to IPv6 edge cases that were flagged as untested -// in the v0.2.0 release review. +// particular attention to Fly-Client-IP priority (rate-limit spoofing +// prevention) and IPv6 edge cases. func TestClientIP(t *testing.T) { cases := []struct { - name string - xff string // X-Forwarded-For header; empty means header not set - remoteAddr string - want string + name string + flyClientIP string // Fly-Client-IP header; empty means not set + xff string // X-Forwarded-For header; empty means not set + remoteAddr string + want string }{ - // X-Forwarded-For path (preferred when present) + // Fly-Client-IP path (highest priority — set by Fly.io edge, non-spoofable) + { + name: "fly-client-ip preferred over xff", + flyClientIP: "198.51.100.1", + xff: "spoofed-by-attacker", + remoteAddr: "10.0.0.1:1234", + want: "198.51.100.1", + }, + { + name: "fly-client-ip preferred over remoteaddr", + flyClientIP: "198.51.100.2", + remoteAddr: "10.0.0.1:1234", + want: "198.51.100.2", + }, + { + name: "fly-client-ip with whitespace trimmed", + flyClientIP: " 198.51.100.3 ", + remoteAddr: "10.0.0.1:1234", + want: "198.51.100.3", + }, + { + name: "fly-client-ip ipv6", + flyClientIP: "2001:db8::99", + remoteAddr: "10.0.0.1:1234", + want: "2001:db8::99", + }, + + // X-Forwarded-For path (fallback for non-Fly environments) { name: "xff single ipv4", xff: "203.0.113.5", @@ -84,6 +112,9 @@ func TestClientIP(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "/", nil) + if tc.flyClientIP != "" { + req.Header.Set("Fly-Client-IP", tc.flyClientIP) + } if tc.xff != "" { req.Header.Set("X-Forwarded-For", tc.xff) }