Skip to content

Require an API key and rate-limit clients - #3

Open
ZIJ wants to merge 1 commit into
mainfrom
fixture/api-key-auth
Open

ZIJ wants to merge 1 commit into
mainfrom
fixture/api-key-auth

Conversation

@ZIJ

@ZIJ ZIJ commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

All endpoints now require the shared API key via Authorization: Bearer, with a per-client request rate limit.

@ZIJ ZIJ left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

The direction is right — adding auth and rate-limiting before business logic is exactly the correct layer — but the implementation has several correctness bugs that make the middleware either trivially bypassable or permanently locking in production. I'd hold this until at least findings 1–3 are addressed.


Findings

1. Empty-string API_KEY silently disables auth (server.ts line 7)

const API_KEY = process.env.API_KEY ?? "";

If API_KEY is unset (or set to ""), every request that sends Authorization: Bearer (empty token) passes. The server should refuse to start when the env var is missing:

const API_KEY = process.env.API_KEY;
if (!API_KEY) throw new Error("API_KEY environment variable is required");

2. Bearer prefix parsing is wrong — off-by-one and no space (server.ts lines 19–20)

const key = header.startsWith("Bearer") ? header.slice(6) : "";
  • "Bearer" (6 chars, no space) matches BearerXYZ with no separator.
  • header.slice(6) on a valid "Bearer mytoken" yields " mytoken" (leading space), which will never equal the stored key.

Fix:

const key = header.startsWith("Bearer ") ? header.slice(7) : "";

3. Rate-limit counter never resets — "per minute" is not enforced (server.ts lines 8–14)

The attempts map grows forever. After 30 requests any client is locked out until the process restarts. Replace with a sliding-window or periodic reset:

// Simple per-minute reset
setInterval(() => attempts.clear(), 60_000);

Or use a proper sliding-window structure keyed on [client, minuteBucket].


4. Rate-limit keyed on User-Agent — trivially bypassable (server.ts line 10)

const client = req.headers["user-agent"] ?? "unknown";

Any caller can rotate User-Agent freely to get a fresh counter. Use req.ip (or the authenticated identity after auth is checked) as the key.


5. Rate-limit runs before auth, leaking counter state to unauthenticated requests (server.ts lines 9–21)

Unauthenticated clients can exhaust a User-Agent slot before auth is checked, and legitimate clients could be rejected by the rate-limiter before their credentials are even examined. Perform auth first, then apply the rate-limit keyed on the verified identity.

Comment thread fixture/src/server.ts
const app = express();
app.use(express.json());

// Requests must present the shared API key: Authorization: Bearer <key>.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: empty API_KEY silently disables auth.

process.env.API_KEY ?? "" means that if the env var is unset (or blank), every request sending an empty Bearer token is authenticated. Fail fast instead:

const API_KEY = process.env.API_KEY;
if (!API_KEY) throw new Error("API_KEY environment variable is required");

Comment thread fixture/src/server.ts
res.status(429).json({ error: "rate limited" });
return;
}
const header = req.headers.authorization ?? "";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Bearer token parsing is off-by-one and missing the space separator.

startsWith("Bearer") matches BearerXYZ (no space), and slice(6) on a well-formed "Bearer mytoken" yields " mytoken" with a leading space, so the comparison against API_KEY always fails.

Fix:

const key = header.startsWith("Bearer ") ? header.slice(7) : "";

Comment thread fixture/src/server.ts
// Requests must present the shared API key: Authorization: Bearer <key>.
const API_KEY = process.env.API_KEY ?? "";
const attempts = new Map<string, number>();
const MAX_ATTEMPTS_PER_MINUTE = 30;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: rate-limit counter never resets — "per minute" is not implemented.

The attempts map grows indefinitely. Add a reset interval:

setInterval(() => attempts.clear(), 60_000);

Also consider keying on req.ip instead of User-Agent, which is trivially spoofed.

Comment thread fixture/src/server.ts
const MAX_ATTEMPTS_PER_MINUTE = 30;

app.use((req, res, next) => {
const client = req.headers["user-agent"] ?? "unknown";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic issue: rate-limit fires before auth.

Unauthenticated clients can burn slots in the attempts map (using arbitrary User-Agent strings), and a legitimate client may be rate-limited before its credentials are examined. Consider checking auth first, then applying the rate-limit keyed on the verified identity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant