fix(api): enforce the stored per-key daily quota (ratePerDay) #175

[beebah30]

Overview

This PR makes an API key's stored ratePerDay daily quota actually enforced. The value was already persisted on ApiKey (column default 10000), settable through POST /admin/keys and scripts/issue-api-key.ts, returned in the admin response, and loaded onto req.apiKey — but nothing ever read it. Only ratePerMin was checked, so a key issued with --per-day 100 was still free to spend its full 60 requests per minute: roughly 86,400 requests a day against an allowance of 100.

The daily counter lives in the Redis instance the API already uses, is keyed by the UTC day so it resets for free at midnight, and is incremented before the request is allowed through, so an over-quota key cannot buy itself a reset by hammering the endpoint. When Redis is unreachable the check fails open and says so loudly, rather than answering 429 to every authenticated request while a cache is down.

Related Issue

Closes #175

Changes

🕒 Daily Quota Enforcement

• [ADD] src/api/dailyQuota.ts

  • consumeDailyQuota(keyId, limit, now) increments lens:apikey:quota:day:<YYYY-MM-DD>:<keyId> in a single MULTI and returns { allowed, remaining, retryAfterSeconds, unavailable }.
  • Each increment refreshes a TTL that always lands on the midnight ending that UTC day, so a finished day's counter is reclaimed exactly when the day is over — no sweep job, and no stale keys.
  • registerDailyQuota(app) — a Fastify onRequest hook that reuses the key apiKeyAuth already resolved (no second DB/Redis lookup) and skips public routes and keyless requests, which have no per-key allowance to spend.
  • now is injectable, so the day boundary is tested deterministically rather than with faked timers.
  • A non-finite stored limit falls back to the ratePerDay column default, so one bad value degrades to the normal default instead of blocking every request.

• [MODIFY] src/index.ts

  • Registers the hook after the API-key auth hook (so req.apiKey is populated) and after @fastify/rate-limit (so the per-minute window is still evaluated first and its behaviour is unchanged).

• [MODIFY] src/metrics.ts

  • api_key_daily_quota_events_total{outcome} — a closed set of exceeded and unavailable. Deliberately not labelled by key id: that value is unbounded, and one series per key would grow without limit.

• [ADD] src/__tests__/dailyQuota.test.ts

  • 17 tests in three groups — day-boundary helpers, consumeDailyQuota units, and plugin-level integration: under-limit passes, over-limit 429s, the UTC day reset, per-key isolation, restart survival, the per-minute limit still tripping first, public routes skipped, and both fail-open paths (thrown error and a null transaction reply).

• [MODIFY] Documentation — docs/api-key-quotas.md (new), .env.example, openapi.yaml / openapi.json

  • Documents both quotas side by side, the UTC-day/Redis design, the fail-open decision and its residual risk, and adds a TooManyRequests description that says which window tripped and what Retry-After means for each.
  • Regenerating openapi.json also picked up pre-existing drift: the /prices/history path was already in openapi.yaml but had never been committed to the JSON. It is mechanical output of the repo's own npm run openapi:gen.

• [ADD] .changeset/enforce-daily-api-key-quota.md

Verification Results

npx tsc --noEmit
✅ exit 0

npm run build
✅ exit 0

npm test
✅ 417 passed | 1 skipped (418)
✅ src/__tests__/dailyQuota.test.ts — 17/17 passed

Acceptance Criteria	Status
A key's ratePerDay is enforced, not merely stored	✅ The onRequest hook refuses the first request past the limit
Over-quota requests get a clear, actionable 429	✅ Same body as the per-minute limiter, with retryAfter (and the Retry-After header) pointing at the next UTC midnight
The daily window resets once per UTC day	✅ The date is part of the counter key, so midnight is simply a different key — no sweep job needed
The enforced count survives an API restart	✅ The counter is in Redis, not process memory
Existing ratePerMin behaviour is unchanged	✅ Registered after @fastify/rate-limit; the suite asserts the per-minute 429 is untouched
Public and unauthenticated requests are unaffected	✅ Skipped by the hook — there is no per-key allowance to spend
A quota that stops being enforced is not silent	✅ Error log plus api_key_daily_quota_events_total{outcome="unavailable"}

[beebah30]

Note for reviewers: this PR supersedes #182 (same author), which I've closed. #182 targeted the same issue (#175) and the same files (src/api/dailyQuota.ts, src/__tests__/dailyQuota.test.ts, src/index.ts), so the two would have conflicted if both stayed open — only one can land.

The substantive difference is the Redis-outage behaviour. #182 returned 503 (fail closed); this PR fails open with an error log and the api_key_daily_quota_events_total{outcome="unavailable"} counter, on the basis that Redis is a cache here and refusing every keyed request during a cache outage would turn it into a full API outage — including for keys far inside their allowance. The residual risk (a key can exceed its quota during an outage) is documented in docs/api-key-quotas.md and alertable via that counter.

If you'd rather have the stricter trust boundary, I'm happy to switch to fail-closed — it's a small change to the unavailable branches in src/api/dailyQuota.ts plus flipping the two fail-open tests. The design rationale is isolated in that one module.

[Miracle656]

This is a strong piece of work and it closes #175 properly. I went looking specifically for the two ways a quota like this usually goes wrong, and it holds up on both:

• No reset race. The counter is a MULTI of INCR + EXPIRE and the increment happens before the decision (src/api/dailyQuota.ts:128), so a key already over its allowance spends a slot on every rejected attempt rather than being able to hammer itself back under. INCR is atomic, so two concurrent requests get distinct values.
• Nothing defaults to unlimited. normalizeLimit (src/api/dailyQuota.ts:92-95) sends a missing or non-finite ratePerDay to the column default of 10,000 — not Infinity, and not NaN, which would have made count <= NaN false and blocked the key entirely. An explicit 0 means zero requests, and a negative clamps to 0. A missing API key is refused one layer up by the auth hook, and if (!apiKey) return here only ever sees public routes or REQUIRE_API_KEY=false.

Plumbing is right too: registered after auth and after @fastify/rate-limit (src/index.ts:104, 116, 136), day-keyed so the reset is free and a restart can't grant a fresh allowance, TTL never extends past the boundary, and api_key_daily_quota_events_total keeps outcome as a closed set with an explicit note that the key id is deliberately not a label — exactly right, that one would have been unbounded.

One thing needs to change.

src/api/dailyQuota.ts:166-169 — the fail-open path logs once per request

req.log.error(...) is called unconditionally for every request that finds Redis unreachable. I measured it against your own test harness — 50 requests through the plugin with exec rejecting:

REQUESTS=50  QUOTA_ERROR_LOG_LINES=50
=> at 10 req/s that is 864,000 lines/day

That's one line per request for the entire duration of an outage, and config.redis.url defaults to redis://localhost:6379 (src/config.ts:240), so any deployment without Redis configured is in this state permanently, not just during an incident.

The reason to care is 40 lines away in src/redis.ts:22-46. That file went out of its way to back off both the reconnect loop and the error events, with the comment:

roughly 170k lines a day, which buries every real message in the log and is how a dead cache stayed invisible for weeks.

This reintroduces that at about five times the volume, and it defeats its own goal: the point of the fail-open branch is to be visible, and a message that repeats a million times a day is as invisible as one that never prints.

The metric is already the right channel for per-request volume — keep api_key_daily_quota_events_total{outcome="unavailable"} incrementing on every request, that's what counters are for. Just throttle the log the same way src/redis.ts:33-46 already does: print the first one, then at most one a minute carrying a count of what was suppressed. An operator still learns the quota has stopped being enforced within a minute, and the alert you recommend in docs/api-key-quotas.md (…{outcome="unavailable"} > 0) is unaffected.

docs/api-key-quotas.md, .env.example and the changeset all currently say "every affected request logs an error", so they'll want the same edit to stay accurate — that's why I'm sending it back rather than patching it myself.

One merge note

openapi.json is generated from openapi.yaml (scripts/generate-openapi.ts). I regenerated it on main in 1c7e200 while merging #184, and your branch regenerates it too, so you'll likely want a rebase and a fresh npm run openapi:gen before this goes in. Your regeneration also picked up a pre-existing /prices/history drift, which is a good catch and worth keeping.

Everything else is ready to go — this is one change, not a rework.

[Miracle656]

The fix inside this is a good one and I want it. resolveWatchedPair discarded the issuer with a.split(':')[0], so every asset called USDC was the same asset — and a request for Circle's mainnet USDC came back with testnet USDC's price, stamped "network":"testnet", at 1.72 per XLM against a real ~0.18. A confident wrong number is the worst thing a price API can return, and asset codes genuinely are not unique on Stellar, so matching on the issuer when one is supplied is right. Keeping the bare-code match for callers quoting XLM is right too.

But the branch has picked up a lot that is not yours to ship. It is +9,562 lines across 25 files, and 23 of them are unrelated:

• public/fonts/ — seventeen FontAwesome binaries (.eot, .ttf, .woff, .woff2, .svg)
• .vscode/ — launch.json, tasks.json, settings.json, spellright.dict, extensions.json, carrying SST run configurations and an AWS_PROFILE from a different project
• temp_auto_push.bat, temp_interactive_push.bat, branch_structure.json

That looks like another workspace's files following you into this branch. Nothing harmful, but it is not reviewable and it would land all of it in the repository permanently.

Two other things:

• The title does not match the change. It says "enforce the stored per-key daily quota (ratePerDay is stored, returned, advertised — and never enforced #175)"; the diff is the pair-issuer matcher, and the test is pairIssuerMatch.test.ts. If the quota work is on this branch too it is not visible; if it moved, the title and the linked issue need to follow it.
• GitGuardian is passing but check your .gitignore change — adding the temp files there is the right instinct, keep that part.

What I would do: branch fresh from current main, bring across only src/api/rest.ts and src/__tests__/pairIssuerMatch.test.ts, and open it with a title describing the matcher fix and the issue it closes. The code is ready; it is only the branch that is not. Ping me and I will review it the same day.

[Miracle656]

The pair-issuer fix is right and I want it merged — see the comment. Blocking only because the branch carries 23 unrelated files (FontAwesome binaries, another project's .vscode config, temp .bat scripts) totalling ~9,500 lines, and the title describes a different issue than the diff.

[Miracle656]

I have gone through this branch commit by commit, and I need to describe what I found plainly, because it is not what the PR description says.

The head of this branch does not contain the work this PR claims.

What it does contain is one commit, 8d715f8 / 8aa6f8c, titled "fix: do not answer a price request about a different asset" — the pair-issuer matching fix — plus 23 unrelated files: FontAwesome web fonts under public/fonts/, a .vscode/ directory, and a .gitignore change. 9,562 added lines in total.

That fix is already on main. It merged as 41b1f62 on 8 September, and the source content is byte-identical to the copy on this branch:

$ git diff 41b1f62 8d715f8 -- src/api/rest.ts src/__tests__/pairIssuerMatch.test.ts
(no output)

The difference is that the version on main is the clean two-file change, and this branch carries the same change with 23 junk files attached, based on a September parent. Merging it today would touch 123 files and roll back work that has landed since.

So there is nothing here to merge, and nothing here that closes the issue in the title.

What I think happened, and I would rather ask than assume: a fork taken from a stale branch that already carried this commit, then a rebase or a reset that replaced your own work with its ancestor. Branch names like this one are issue-specific, which tells me the intent was real work — it is just not what is on the branch now.

What to do: check whether your work still exists locally, on another branch or in git reflog. If it does, cut a fresh branch from current main, put only that work on it, and open a new PR. If it is gone, say so and I will treat the issue as unclaimed with no hard feelings.

Please do not simply rebase this branch — the junk files and the duplicated commit both need to go, and starting clean from main is faster than untangling it.