feat: superadmin powers, ownership, runtime settings, API keys, rate-limiting (auth PR B) - #19
Merged
Conversation
…limiting PR B of the auth milestone — the superadmin half. Role powers now do something, config is admin-editable at runtime, non-browser clients get durable credentials, and the credential endpoints are throttled. Also folds in the home-page account-links gap from PR A. Ownership (owner-or-admin): - PATCH /assets/:id and PATCH /assets/:id/tags now require the uploader or an admin (was any authenticated user). Resumable upload-session ops (HEAD/PATCH/DELETE /uploads/:token) enforce ownership inside the service using the already-loaded session (zero extra queries). PATCH /tags/:name (set a tag's category) is admin-only. Runtime settings (upload caps): - New `settings` KV table + SettingsService: env values seed the defaults, DB rows override them, cached in-process (single-instance). GET/PATCH /settings (admin) edit the one-shot + resumable caps; the upload routes read the caps from settings, and the resumable route now correctly guards on the resumable cap. `ValidationError` (→ 400) rejects a one-shot cap above the request-body ceiling. API keys: - New `api_keys` table, folded into authService (Core owns Auth). `bnb_<hex>` tokens, sha256-hashed like sessions, full account powers, no expiry. `currentUser` dispatches by prefix (`bnb_` → key, else session). CRUD under /account/api-keys (key shown once); revoke is owner-scoped. Rate-limiting: - In-memory per-IP fixed-window limiter on POST /auth/login + /register (429). Single-instance deployment, so no Redis needed. Web: - Extracted AccountLinks into a shared component (fixes the home-page dead Login/Sign-up links), role-gated Admin nav, /admin (edit caps + set tag category) and /account (manage API keys) pages. Covered by db-integration (settings + api-key repos, FK behaviours), core-unit (settings-service, api-key create/resolve/revoke, upload ownership), and API-route tests (403/401/404 gating, settings, api-keys incl. Bearer-via-key, 429), plus an in-process Postgres e2e. Migration 0009 adds both tables. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR adds runtime-editable upload limits, API-key authentication, ownership checks for uploads/assets/tags, IP-based auth rate limiting, new settings/API-key persistence, and matching web admin/account pages. ChangesSettings, API keys, upload authorization feature
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 9
🧹 Nitpick comments (1)
packages/core/test/settings-service.test.ts (1)
50-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one test for the partial-write cache reset branch.
createSettingsService.updateUploadLimits()explicitly drops its cache when onerepo.set()lands and a later write throws. This suite covers the happy path, but not that recovery branch, so a regression there could leave stale upload limits in memory after a failed admin update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/settings-service.test.ts` around lines 50 - 58, The updateUploadLimits flow in createSettingsService needs coverage for the partial-write failure path where one repo.set succeeds and a later write throws, causing the cache to be cleared. Add a test alongside the existing updateUploadLimits cases that simulates the second write failing after the first succeeds, then verifies the service resets its cached upload limits by re-reading from the repo on the next getUploadLimits call and does not retain stale values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/lib/rate-limit.ts`:
- Around line 67-72: The clientIp logic in rate-limit.ts should not trust the
first X-Forwarded-For token when TRUST_PROXY is enabled, since that allows
spoofed multi-hop chains. Update clientIp to only accept a sanitized single-hop
XFF value or to derive the client IP from a trusted proxy hop, and explicitly
reject any multi-hop header before falling back to the server/IP resolver path.
In `@apps/web/src/lib/api-keys.ts`:
- Around line 16-23: The `useApiKeys()` cache is currently shared across users
because `API_KEYS_KEY` is only `["api-keys"]`, so stale key names can show after
logout/login. Update the query key in `useApiKeys` to include the authenticated
user identity (for example the current user id) or clear this cache on auth
changes. Keep existing prefix invalidations like `invalidateQueries({ queryKey:
["api-keys"] })` working by preserving `["api-keys", ...]` as the base key
shape.
In `@apps/web/src/routes/admin.tsx`:
- Around line 104-106: The admin upload hint in the JSX span renders stray
literal backticks because `POST /assets` is written as plain text in
`admin.tsx`. Update that inline text in the relevant `<span>` so the route label
is rendered without backticks, matching the neighboring resumable-cap hint
formatting and keeping the UI text consistent.
- Around line 73-82: The onSubmit handler in admin.tsx currently calls
update.mutate(patch) even when both maxUpload and maxResumable are invalid,
leaving patch empty and causing a false “Saved.” state. Update onSubmit to
validate the assembled patch before mutating, and return early (or otherwise
prevent submission) when neither maxUploadBytes nor maxResumableUploadBytes was
added. Use the existing onSubmit, update.isPending, and update.mutate flow to
keep the fix localized.
In `@apps/web/src/routes/home.tsx`:
- Around line 76-79: Make the divider conditional with AccountLinks in the home
page header so the stray separator does not render while /auth/me is still
loading. Update the home route JSX around AccountLinks and ThemeSwitcher to
either render the divider only when AccountLinks has content, or move the
separator into AccountLinks so both stay in the same auth-gated branch.
In `@packages/core/src/services/settings-service.ts`:
- Around line 74-110: The updateUploadLimits flow in SettingsService is
publishing an optimistic in-memory cache after writing to repo, which can leave
stale field values when concurrent PATCHes touch different upload limit keys.
After the awaited repo.set calls succeed, reload the authoritative UploadLimits
from storage (or otherwise refresh via currentLimits/repo) and assign that
result to cache before returning. Keep the existing validation and partial-write
cache invalidation behavior intact, and update the updateUploadLimits method so
cache always reflects the persisted state.
In `@packages/db/src/repositories/settings-repository.ts`:
- Around line 9-13: The settings update flow is doing two separate writes
through SettingsRepository, so a failure in the second write can leave only part
of the /settings PATCH persisted. Extend SettingsRepository with a transactional
batch-update method that can apply multiple key/value changes atomically, then
update SettingsService to use that new path instead of calling set for
maxUploadBytes and maxResumableUploadBytes separately. Keep the existing
single-key API if needed, but route the multi-setting update logic through the
new batch method so both values are committed or rolled back together.
In `@packages/db/test/settings-api-key-repositories.test.ts`:
- Around line 77-78: The test in apiKeys.listByUser is masking the repository’s
newest-first ordering contract by sorting the results before asserting. Update
the assertion to check the returned sequence directly on aliceKeys, using the
existing aliceKeys.map((k) => k.name) result, so it verifies the documented
order for these inserts is ["two", "one"].
In `@scripts/seed.ts`:
- Around line 123-126: The seed storage fallback in createCore(...) is still
pointing at a different default directory than the runtime config, so seeded
files end up outside what the API reads. Update the storage-root fallback used
in scripts/seed.ts to match the runtime default of <cwd>/storage, and keep the
change localized around the createCore configuration so the seed path resolution
stays consistent with the app’s env config.
---
Nitpick comments:
In `@packages/core/test/settings-service.test.ts`:
- Around line 50-58: The updateUploadLimits flow in createSettingsService needs
coverage for the partial-write failure path where one repo.set succeeds and a
later write throws, causing the cache to be cleared. Add a test alongside the
existing updateUploadLimits cases that simulates the second write failing after
the first succeeds, then verifies the service resets its cached upload limits by
re-reading from the repo on the next getUploadLimits call and does not retain
stale values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e9601cae-983a-4a28-8413-eff7fc32f055
📒 Files selected for processing (34)
.env.exampleapps/api/src/env-config.tsapps/api/src/index.tsapps/api/src/lib/http.tsapps/api/src/lib/rate-limit.tsapps/api/src/server.tsapps/api/test/server.test.tsapps/web/src/components/account-links.tsxapps/web/src/lib/api-keys.tsapps/web/src/lib/settings.tsapps/web/src/lib/tags.tsapps/web/src/router.tsxapps/web/src/routes/__root.tsxapps/web/src/routes/account.tsxapps/web/src/routes/admin.tsxapps/web/src/routes/home.tsxpackages/core/src/core.tspackages/core/src/errors.tspackages/core/src/index.tspackages/core/src/services/auth-service.tspackages/core/src/services/settings-service.tspackages/core/src/services/upload-service.tspackages/core/test/auth-service.test.tspackages/core/test/settings-service.test.tspackages/core/test/upload-service.test.tspackages/db/drizzle/0009_settings_and_api_keys.sqlpackages/db/drizzle/meta/0009_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/index.tspackages/db/src/repositories/api-key-repository.tspackages/db/src/repositories/settings-repository.tspackages/db/src/schema.tspackages/db/test/settings-api-key-repositories.test.tsscripts/seed.ts
Comment on lines
+123
to
+126
| // so these caps are effectively unused here; keep them generous. | ||
| maxUploadBytes: 100 * 1024 * 1024, | ||
| maxResumableUploadBytes: 100 * 1024 * 1024, | ||
| requestBodyCeilingBytes: 2 * 1024 * 1024 * 1024, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the seed storage fallback with the runtime default.
While touching this createCore(...) config, Line 121 still falls back to <cwd>/data/storage. The app runtime uses <cwd>/storage, so seeded files land in a different directory than the API reads from.
🔧 Suggested fix
- storageRoot: Bun.env.STORAGE_ROOT?.trim() || resolve(process.cwd(), "data/storage"),
+ storageRoot: Bun.env.STORAGE_ROOT?.trim() || resolve(process.cwd(), "storage"),Based on learnings, scripts/seed.ts should use the same storage-root fallback as the runtime env config (<cwd>/storage), not data/storage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/seed.ts` around lines 123 - 126, The seed storage fallback in
createCore(...) is still pointing at a different default directory than the
runtime config, so seeded files end up outside what the API reads. Update the
storage-root fallback used in scripts/seed.ts to match the runtime default of
<cwd>/storage, and keep the change localized around the createCore configuration
so the seed path resolution stays consistent with the app’s env config.
Source: Learnings
- settings: make multi-key updates atomic — SettingsRepository.set → setMany (one transaction), and the service reloads the authoritative DB state after a write instead of publishing its optimistic snapshot, so concurrent admin PATCHes to different keys can't leave the cache half-stale. - rate-limit: only trust a SINGLE-hop X-Forwarded-For under TRUST_PROXY; a multi-hop chain (possible spoof / extra proxy) falls back to the socket IP. - web: scope the API-key query cache by user id (no cross-account leak across a logout/login); gate the home-page divider on auth resolution (no lone "·"); guard the admin form against an empty patch; drop stray literal backticks. - tests: assert the api-key newest-first order directly (not sorted); update the settings repo/service tests for setMany. Not changed: the seed storage fallback already matches the runtime default (both `resolve(cwd, "data/storage")`), so that finding was a false positive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Addressed the review in
Not changed (false positive): the seed storage fallback already matches the runtime default — both are
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR B of the auth milestone — the superadmin half. PR A (#18) gave accounts + sessions + gated writes; this makes role powers actually do something, makes config admin-editable at runtime, gives non-browser clients a durable credential, and throttles the credential endpoints. It also folds in the small home-page account-links gap from PR A. (Delivered as one PR by request.)
What's in it
Ownership (owner-or-admin)
PATCH /assets/:idandPATCH /assets/:id/tagsnow require the uploader or an admin (was any authenticated user). Resumable upload-session ops (HEAD/PATCH/DELETE /uploads/:token) enforce ownership inside the service using the already-loaded session (zero extra queries), andbeginnow derives the owner from the authenticated user (the service is the source of truth, not a caller-supplied field).PATCH /tags/:name(set a tag's category) is admin-only (canModerate).Runtime settings (upload caps)
settingsKV table +SettingsService: env values seed the defaults, DB rows override them, cached in-process (single-instance).GET/PATCH /settings(admin) edit the one-shot + resumable caps; the upload routes read caps from settings (and the resumable route now correctly guards on the resumable cap). A one-shot cap above the request-body ceiling →400(ValidationError).API keys
api_keystable, folded intoauthService(Core owns Auth).bnb_<hex>tokens, sha256-hashed like sessions, full account powers, no expiry.currentUserdispatches by prefix (bnb_→ key, else session). CRUD under/account/api-keys(raw key shown once); revoke is owner-scoped. The hash never leaves the repository (ApiKeySummary).Rate-limiting
POST /auth/login+/register→429. Single-instance deployment, so no Redis. IP keying uses the socket address by default;X-Forwarded-Foris trusted only whenTRUST_PROXY=true.Web
AccountLinksinto a shared component (fixes the home-page dead Login/Sign-up links), role-gated Admin nav,/admin(edit caps + set tag category) and/account(manage API keys) pages.Schema (migration 0009)
settings(keyPK,value,updated_byFK→users set null,updated_at) andapi_keys(token_hashunique,user_idFK→users cascade,name,last_used_at,created_at, index onuser_id).Testing
typecheck✅,lint:boundaries✅ (no new edges), 257 tests pass, migration 0009 applies.Review
A pre-PR CodeRabbit CLI pass surfaced 10 findings; 9 are folded into this commit — cache-invalidation on partial settings write,
ApiKeySummary(keeptokenHashrepo-internal), owner-derived-in-begin, runtime-cap validation,TRUST_PROXY-gated XFF, hard rate-limit key cap, deterministic key ordering, whitespace-name rejection, and seed-once admin form. The 10th (settingsupdated_at"insert-only") is a non-issue — the repository setsupdated_aton every upsert.Deferred (future)
Per-key scopes/expiry; cookie-only web login now that API keys exist; GC-interval / session-expiry as runtime settings; Redis-backed rate-limiting if the app ever scales past one instance.
🤖 Generated with Claude Code
Summary by CodeRabbit
TRUST_PROXY.