feat(security): add authentication, rate limiting, and container hardening #9
Conversation
…ening - Add bearer token authentication to HTTP API (optional but recommended) - Add rate limiting with configurable limits (60 req/min default) - Fix path traversal vulnerability in serviceName parameter - Validate service names for Docker compatibility (1-63 chars, lowercase alphanumeric with hyphens) - Expose audit mode via HTTP API for sandboxed execution - Add non-root user (ignite:1001) to both Bun and Node Dockerfiles - Fix /health endpoint to bypass authentication for load balancer checks - Fix memory leak: clear rate limiter cleanup interval on server stop BREAKING CHANGE: None - all new features are opt-in
📝 WalkthroughWalkthroughThe pull request introduces Docker-compatible naming validation for services, adds API key authentication and rate limiting to the HTTP server, includes optional audit support for service execution requests, and hardens container runtimes by running them as non-root users. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/http/src/server.ts`:
- Around line 26-27: The inline comment above SERVICE_NAME_REGEX is incorrect:
it says "2-63 chars" while the regex alternation (SERVICE_NAME_REGEX) allows a
single-character name and the error message later states "1-63 chars"; update
the comment to match the regex and error text (e.g., "1-63 chars") so the
comment accurately reflects SERVICE_NAME_REGEX and the validation behavior.
- Around line 137-140: Replace the direct string comparison of token !== apiKey
with a constant-time comparison: convert both token and apiKey to Buffers
(ensuring lengths match) and use crypto.timingSafeEqual to compare them; handle
the case where token is missing or lengths differ by failing the check and
keeping the same response behavior (set.status = 401; throw new Error(...)).
Update the check around the token variable in the request handling block (the
code that currently uses token and apiKey) to perform this buffered, timing-safe
comparison and preserve the existing error message and status on mismatch.
- Around line 123-131: The current extraction of client IP in the .derive(({
request, set }) => { ... }) middleware reads
request.headers.get('x-forwarded-for') raw and falls back to 'unknown', which is
spoofable and improperly parsed; update this to (1) support a configuration flag
(e.g., trustProxy / trustedProxies) that determines whether to use proxied
headers or direct socket address, (2) when trusting proxies, use a proven
library like proxy-addr (or equivalent) with an explicit trustedProxies
whitelist to resolve the client IP instead of reading the header directly, (3)
if you cannot use proxy-addr, at minimum parse x-forwarded-for by taking the
first non-empty, trimmed token before any comma and validate it, (4) when not
trusting proxies, use request.socket.remoteAddress (or framework equivalent) as
the client IP, and (5) remove the 'unknown' shared-bucket fallback—require a
resolved IP and handle missing/invalid values by denying or using a distinct
per-request fallback; pass the normalized IP to rateLimiter.check(clientIp) and
keep setting set.status and Retry-After as before.
🧹 Nitpick comments (2)
packages/http/src/server.ts (2)
118-122: Consider adding a comment explaining why/healthis defined beforederive.The placement of
/healthbefore thederivemiddleware intentionally bypasses authentication and rate limiting for load balancer compatibility. A brief comment would make this intent explicit and prevent accidental reordering.Suggested comment
+ // Health check endpoint defined before derive() to bypass auth and rate limiting + // for load balancer compatibility .get('/health', (): HealthResponse => ({
246-260: Consider using static import for consistency.
readdiris dynamically imported here, butjoinandresolvefromnode:pathare statically imported at the top. Since this is server-side code, a static import would be more consistent.Suggested change
Add to imports at top:
-import { join, resolve } from 'node:path'; +import { join, resolve } from 'node:path'; +import { readdir } from 'node:fs/promises';Then simplify the handler:
.get('/services', async ({ set }): Promise<{ services: string[] } | ErrorResponse> => { try { - const { readdir } = await import('node:fs/promises'); const entries = await readdir(resolvedServicesPath, { withFileTypes: true });
| // Service name validation: lowercase alphanumeric with hyphens, 2-63 chars (Docker compatible) | ||
| const SERVICE_NAME_REGEX = /^[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$|^[a-z0-9]$/; |
There was a problem hiding this comment.
Minor: Comment states "2-63 chars" but regex allows single-character names.
The comment says "2-63 chars" but the regex alternation |^[a-z0-9]$ allows single-character names. The error message on line 44 correctly states "1-63 chars".
Suggested fix
-// Service name validation: lowercase alphanumeric with hyphens, 2-63 chars (Docker compatible)
+// Service name validation: lowercase alphanumeric with hyphens, 1-63 chars (Docker compatible)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Service name validation: lowercase alphanumeric with hyphens, 2-63 chars (Docker compatible) | |
| const SERVICE_NAME_REGEX = /^[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$|^[a-z0-9]$/; | |
| // Service name validation: lowercase alphanumeric with hyphens, 1-63 chars (Docker compatible) | |
| const SERVICE_NAME_REGEX = /^[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$|^[a-z0-9]$/; |
🤖 Prompt for AI Agents
In `@packages/http/src/server.ts` around lines 26 - 27, The inline comment above
SERVICE_NAME_REGEX is incorrect: it says "2-63 chars" while the regex
alternation (SERVICE_NAME_REGEX) allows a single-character name and the error
message later states "1-63 chars"; update the comment to match the regex and
error text (e.g., "1-63 chars") so the comment accurately reflects
SERVICE_NAME_REGEX and the validation behavior.
| .derive(({ request, set }) => { | ||
| const clientIp = request.headers.get('x-forwarded-for') || 'unknown'; | ||
| const rateLimitResult = rateLimiter.check(clientIp); | ||
|
|
||
| if (!rateLimitResult.allowed) { | ||
| set.status = 429; | ||
| set.headers['Retry-After'] = String(rateLimitResult.retryAfter); | ||
| throw new Error(`Rate limit exceeded. Retry after ${rateLimitResult.retryAfter} seconds`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
safely extract client IP x-forwarded-for header Node.js best practices
💡 Result:
Best practices (Node.js / Express) for safely extracting the real client IP
-
Treat
X-Forwarded-Foras untrusted input unless you control the proxy chain. Clients can send/spoofX-Forwarded-*headers; only trust them when your app is definitely behind known reverse proxies/load balancers you control. The same general warning applies to the standardizedForwardedheader too. [4] -
In Express, prefer
req.ip/req.ips+ correctly configuredtrust proxy(don’t manually splitX-Forwarded-For). Express will select the client IP by walking the chain and finding the first untrusted address (right-to-left) based on yourtrust proxyconfiguration. Misconfiguringtrust proxycan let clients influence what IP you see. [1] -
Configure
trust proxynarrowly (CIDRs / known proxy IPs / exact hop count), not justtrue, whenever possible.app.set('trust proxy', true)makes Express treat the left-mostX-Forwarded-Forvalue as the client IP (easy to spoof if requests can reach your app without going through your proxy). [1]- Prefer explicit IPs/CIDRs of your proxy tiers, or a fixed hop count only if the path length is guaranteed. Express documents pitfalls if multiple path lengths exist. [1]
-
If you’re not using Express, use a battle-tested parser (
proxy-addr) with a trust policy. Express usesproxy-addrinternally; it returns the closest untrusted address according to your trust function/list. [2] -
Prefer single-purpose “real IP” headers from your edge provider when available (and only accept them from that provider).
- Example (Cloudflare):
CF-Connecting-IP/True-Client-IPcontain a single IP and are recommended overX-Forwarded-Forfor origin apps/logs, but still must be trusted only when the request actually came from Cloudflare. [3]
- Example (Cloudflare):
Express example (recommended pattern)
import express from "express";
const app = express();
// Trust only your proxy/load balancer networks (examples; use YOUR real CIDRs/IPs).
app.set("trust proxy", [
"10.0.0.0/8",
"172.16.0.0/12",
"192.168.0.0/16",
]);
app.get("/", (req, res) => {
// Express picks the correct client IP based on trust proxy settings
res.json({ ip: req.ip, chain: req.ips });
});(How Express determines req.ip/req.ips and why correct trust proxy matters is described here. [1])
Plain Node / any framework example with proxy-addr
import proxyaddr from "proxy-addr";
const trust = proxyaddr.compile(["10.0.0.0/8", "172.16.0.0/12"]); // your proxies
function getClientIp(req) {
return proxyaddr(req, trust); // closest untrusted address
}(proxy-addr API and “closest untrusted address” behavior. [2])
Sources
[1] Express “Behind Proxies” (trust proxy, req.ip, right-to-left evaluation, hop-count caveats) (expressjs.com)
[2] proxy-addr documentation (closest untrusted address; compile trust) (npmjs.com)
[3] Cloudflare headers guidance (prefer CF-Connecting-IP / True-Client-IP; X-Forwarded-For behavior) (developers.cloudflare.com)
[4] RFC 7239 security considerations (forwarded headers can’t be relied on; whitelist trusted proxies) (pike.lysator.liu.se)
Citations:
- 1: https://expressjs.com/en/guide/behind-proxies.html?utm_source=openai
- 2: https://www.npmjs.com/package/proxy-addr?utm_source=openai
- 3: https://developers.cloudflare.com/fundamentals/reference/http-request-headers/?utm_source=openai
- 4: https://pike.lysator.liu.se/rfc7239.xml?utm_source=openai
🌐 Web query:
x-forwarded-for spoofing security risks rate limiting
💡 Result:
Why X‑Forwarded‑For (XFF) spoofing matters (especially for rate limiting)
X-Forwarded-For is just an HTTP request header. Unless you control and trust the proxy that sets it, the client can send any value they want, and many servers/frameworks will mistakenly treat it as the “real client IP” (often by taking the first IP in the list). This enables straightforward bypasses of IP-based controls like rate limiting and allow/deny lists. [2]
RFC 7239 (the standardized Forwarded header) explicitly warns that forwarding headers cannot be relied on because they can be modified by any node in the path (including the client), and even “whitelisting proxies” still doesn’t make the earlier IP chain trustworthy. [1]
Common security risks
- Rate-limit bypass / abuse amplification: attacker rotates fake XFF values to appear as many different clients, evading per-IP throttles. [2]
- Access-control bypass: systems that allow/deny based on “internal/trusted IPs” from XFF can be tricked by sending private/internal IPs in XFF. [4]
- Bad audit logs / forensics gaps: logs attribute actions to spoofed IPs, harming incident response and abuse detection. (Implied by the header’s lack of integrity.) [1]
- Incorrect geo/risk decisions: geo-fencing, fraud scoring, bot detection, and anomaly detection keyed to client IP become unreliable. [1]
How to do rate limiting safely when proxies/CDNs are involved
- Never trust XFF from the open Internet. Only use it if the request arrived from a trusted reverse proxy/CDN IP range that you control/explicitly trust. [1]
- At the edge, overwrite/normalize forwarding headers. Your edge proxy/CDN should set the “client IP” signal itself and ignore/strip client-supplied values (otherwise spoofed values remain at the front of XFF). [2]
- Prefer a single-IP header your provider guarantees at the origin. Example: Cloudflare recommends using
CF-Connecting-IP(single IP) rather than XFF (list that may include client-supplied values). [3] - Parse forwarding chains defensively. If you must consume XFF/
Forwarded, don’t blindly take the first IP; determine the client IP relative to your known trusted proxy chain (because only the portion added by trusted infrastructure is meaningful). [1][2] - Don’t key rate limits only on IP. Use stronger identifiers when available (API key, user ID, session, device fingerprint), with IP as a secondary signal—this was a key mitigation after real-world XFF-based rate-limit bypasses. [2]
Rule of thumb: if an attacker can send it as a header, it’s input—treat it as untrusted unless your network boundary guarantees otherwise. [1][2]
Sources: RFC 7239 security considerations [1]; StackHawk write-up on XFF rate-limit bypass and fixes [2]; Cloudflare header guidance (CF-Connecting-IP vs XFF) [3]; Acunetix advisory on XFF-based security bypass [4].
🌐 Web query:
x-forwarded-for comma separated multiple IPs format
💡 Result:
X-Forwarded-For (XFF) is a comma-separated list of IP addresses:
- Format / syntax:
X-Forwarded-For: <client>, <proxy1>, <proxy2>(commas may have optional surrounding whitespace). [1] - Ordering meaning (when proxies behave correctly):
Leftmost = original client, and each proxy that forwards the request appends its own address to the right; rightmost = most recent proxy. [1] - Multiple header lines:
A request can include multipleX-Forwarded-Forheaders; they must be treated as one combined list (effectively concatenated, in order, with commas). [1] - Security note:
Don’t trust the leftmost value for security decisions unless you’re only accepting traffic through trusted proxies and you validate/strip incoming XFF appropriately. [1]
Example:
X-Forwarded-For: 203.0.113.195, 2001:db8::1, 198.51.100.178 [1]
Related standard header (preferred when supported): Forwarded (RFC 7239). [2]
Sources: [1] MDN X-Forwarded-For header, [2] RFC 7239.
Rate limiting can be bypassed via spoofed x-forwarded-for header without a trusted proxy configuration.
The code extracts client IP directly from the x-forwarded-for header without validating that the request came through a trusted reverse proxy, allowing clients to send arbitrary header values to bypass rate limits. Additionally, x-forwarded-for is comma-separated (format: client, proxy1, proxy2), but the code uses the raw header value without parsing. The 'unknown' fallback also creates a shared rate limit bucket that could be exploited.
Address this by:
- Documenting that this service must run exclusively behind a trusted reverse proxy that overwrites untrusted
x-forwarded-forvalues - Using a library like
proxy-addr(or Express'strust proxyconfiguration) with an explicit whitelist of trusted proxy IPs/CIDRs rather than manual header extraction - If direct client connection without proxies must be supported, add a configuration option to switch IP extraction strategies (e.g., use
req.socket.remoteAddresswhen not proxied)
🤖 Prompt for AI Agents
In `@packages/http/src/server.ts` around lines 123 - 131, The current extraction
of client IP in the .derive(({ request, set }) => { ... }) middleware reads
request.headers.get('x-forwarded-for') raw and falls back to 'unknown', which is
spoofable and improperly parsed; update this to (1) support a configuration flag
(e.g., trustProxy / trustedProxies) that determines whether to use proxied
headers or direct socket address, (2) when trusting proxies, use a proven
library like proxy-addr (or equivalent) with an explicit trustedProxies
whitelist to resolve the client IP instead of reading the header directly, (3)
if you cannot use proxy-addr, at minimum parse x-forwarded-for by taking the
first non-empty, trimmed token before any comma and validate it, (4) when not
trusting proxies, use request.socket.remoteAddress (or framework equivalent) as
the client IP, and (5) remove the 'unknown' shared-bucket fallback—require a
resolved IP and handle missing/invalid values by denying or using a distinct
per-request fallback; pass the normalized IP to rateLimiter.check(clientIp) and
keep setting set.status and Retry-After as before.
| if (!token || token !== apiKey) { | ||
| set.status = 401; | ||
| throw new Error('Unauthorized: Invalid or missing API key'); | ||
| } |
There was a problem hiding this comment.
Timing attack vulnerability in API key comparison.
Direct string comparison with !== leaks timing information that could allow attackers to guess the API key character by character. Use a constant-time comparison function.
Suggested fix using crypto.timingSafeEqual
+import { timingSafeEqual } from 'node:crypto';
+
+function safeCompare(a: string, b: string): boolean {
+ if (a.length !== b.length) {
+ // Compare against itself to maintain constant time even on length mismatch
+ const buf = Buffer.from(a);
+ timingSafeEqual(buf, buf);
+ return false;
+ }
+ return timingSafeEqual(Buffer.from(a), Buffer.from(b));
+}
// Then in the derive block:
- if (!token || token !== apiKey) {
+ if (!token || !safeCompare(token, apiKey)) {🤖 Prompt for AI Agents
In `@packages/http/src/server.ts` around lines 137 - 140, Replace the direct
string comparison of token !== apiKey with a constant-time comparison: convert
both token and apiKey to Buffers (ensuring lengths match) and use
crypto.timingSafeEqual to compare them; handle the case where token is missing
or lengths differ by failing the check and keeping the same response behavior
(set.status = 401; throw new Error(...)). Update the check around the token
variable in the request handling block (the code that currently uses token and
apiKey) to perform this buffered, timing-safe comparison and preserve the
existing error message and status on mismatch.
Summary
This PR addresses critical and important security issues identified during codebase review:
Critical Fixes
../in service namesImportant Improvements
igniteuser (UID 1001)auditoption in HTTP API for sandboxed execution/healthendpoint now bypasses auth for load balancer compatibilityChanges
packages/http/src/server.tspackages/http/src/types.tsauditoption to request typepackages/core/src/service/load-service.tspackages/runtime-bun/Dockerfilepackages/runtime-node/DockerfileUsage
Testing
Breaking Changes
None - all new features are opt-in.
Summary by CodeRabbit
New Features
Security & Infrastructure
✏️ Tip: You can customize this high-level summary in your review settings.