fix(user-profiles): require auth + ownership on profile read/write - #1022
Open
mmcintosh wants to merge 3 commits into
Open
fix(user-profiles): require auth + ownership on profile read/write#1022mmcintosh wants to merge 3 commits into
mmcintosh wants to merge 3 commits into
Conversation
GET/PUT /api/user-profiles/:userId performed no auth — any unauthenticated caller could read or overwrite any user's custom profile data by id (information disclosure + data tampering). Gate both routes with requireAuth() plus an ownership check: a signed-in user may act on their own profile, admins on any, else 403. GET /schema stays public (field definitions only). While securing these routes, type the Hono sub-app with Bindings/Variables (matching the analytics/email plugins) instead of the `(c.env as any).DB || (c as any).db` casts, and make canAccessProfile a `targetUserId is string` type-guard. The guard predicate also narrows userId, clearing the two pre-existing string|undefined errors on getCustomData/saveCustomData. Net for the file: 0 `any`, 0 tsc errors.
…canAccessProfile requireAuth() guarantees the user is present before canAccessProfile runs. A null user here means middleware was bypassed — throw loudly instead of masking the bug with a silent 403 Forbidden. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Ctx in test env Module-level caches in plugin-middleware, document-scalar-schema, and the Hono executionCtx getter (throws without a real Cloudflare execution context) caused 25 test failures in CI. Fix by: - Export resetScalarSchemaCaches() from document-scalar-schema.ts; call it in createTestD1().close() and in migrations-d45.test.ts beforeEach so each fresh in-memory DB gets a clean cache slate. - Call invalidatePluginStatusCache() in beforeEach of the three plugin-middleware describe blocks so mock DB lookups aren't shadowed by prior-test cache entries. - Add safeExecCtx() helper in api.ts that wraps c.executionCtx in try-catch and returns null when no ExecutionContext is set (test environment). scheduleKvWrite updated to accept null/undefined ctx and exit early — keeps KV scheduling a no-op in tests without changing production behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
2 participants
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.
Description
GET/PUT /api/user-profiles/:userId(inuser-profiles/index.ts) performed no authentication — any unauthenticated caller could read or overwrite any user's custom profile data by id (information disclosure + data tampering). OnlyGET /schemawas intentionally public.This gates both
:userIdroutes withrequireAuth()+ an ownership check, and — since it touches these handlers — types the sub-app instead of casting.Fixes #1021
Changes
requireAuth()toGET/PUT /api/user-profiles/:userId.canAccessProfile(c, userId)— a signed-in user may act on their own profile; admins (user.role === 'admin') on any; else403 Forbidden.GET /schemaunchanged (public field definitions).new Hono<{ Bindings; Variables }>()(same pattern as theanalytics/email-pluginroutes) replaces the(c.env as any).DB || (c as any).dbcasts with the typedc.env.DB— thec.dbfallback was dead (nothing sets it).canAccessProfileis atargetUserId is stringtype-guard whose predicate also narrowsuserId, which clears the two pre-existingstring | undefinederrors ongetCustomData/saveCustomData. Net for the file: 0any, 0 tsc errors.One file, +33/−5.
Testing
Type-checked against the repo toolchain (
tsc --noEmit, scoped to the changed file — clean). No unit/E2E tests added yet — opened as a draft for discussion of the auth model; happy to add a route test asserting401(unauthenticated) /403(cross-user) /200(owner) if preferred.Unit Tests
E2E Tests
Screenshots/Videos
N/A — API-only change.
Checklist