fix(security): allowlist library/draft writes + scope favorite deletes (P4.3) - #52
Merged
Conversation
Adds failing tests against current repository/route behavior: - updateCitation/updateClause/updateTemplate must not let clients set isVerified/isPublished (and createCitation/createClause/createTemplate must not let clients set them at creation either). - updateDraft must not let clients reassign userId. - removeClauseFavorite/removeCitationFavorite must not delete a favorite row owned by a different user (IDOR). Also extends the fake Prisma test client with deleteMany, needed by the upcoming userId-scoped favorite delete implementation. Assisted-by: Claude Code (Sonnet) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s by userId (#49) Clause/citation/template libraries stay user-editable by any authenticated user (owner decision: no role gate), but clients could previously mass- assign system/trust fields by spreading raw req.body into prisma create/ update calls: - Citation.isVerified, Clause.isPublished, Template.isPublished/version could be set directly by any client on create or update. - Draft.userId could be reassigned via PATCH /drafts/:id. - removeClauseFavorite/removeCitationFavorite deleted by row id with no ownership check (IDOR: any authenticated user could delete any other user's favorite by guessing/enumerating ids). Adds a small pick(obj, keys) helper (server/src/repositories/pick.js) and per-model allowlists of client-writable content fields, built from prisma/schema.prisma: - Citation: title, citation, type, court, year, volume, reporter, page, pinpoint, jurisdiction, codeTitle, section, subdivision, shortForm, parenthetical, url, category, tags, notes. - Clause: title, content, description, category, subcategory, tags, jurisdiction, documentTypes, variations, author, isFavorite, notes, placeholders. - Template: category, name, skeletonContent, defaultMetadata, placeholders, sections, variables. - Draft (update only): title, content, metadata, intakeData, status. usageCount/lastUsedAt (managed by the dedicated /usage endpoints), isVerified/isPublished/version/publishedAt/parentTemplateId (trust/version fields), createdBy, and all ids/timestamps/relations are excluded from client writes on all three library models. removeClauseFavorite/removeCitationFavorite now take a userId and delete via deleteMany({ where: { id, userId } }), so a mismatched user deletes nothing; the DELETE /favorites/:id routes now pass req.user.id. Added deleteMany to the fake Prisma test client to support this. Adjusted templateRepository.test.js and clauseRepository.test.js fixtures that previously set isPublished via createClause/updateTemplate directly (now seeded via a raw prisma.*.update call instead, since that field is no longer client-writable through the repository functions). Assisted-by: Claude Code (Sonnet) Co-Authored-By: Claude Fable 5 <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
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.
Closes #49
Summary
Closes the mass-assignment / trust-flag-spoofing and favorite-IDOR findings from the Phase-4 review. Shared libraries stay user-editable (owner decision D18) — the fix restricts which fields clients may write, not who may write.
pick()helper) on create+update for citation, clause, template repositories. Excludes trust/system fields:isVerified,isPublished,publishedAt,version,usageCount/lastUsedAt,createdBy,id, timestamps, relation refs. A normal user can no longerPOST/PATCH {isVerified:true}a fabricated "verified" citation or publish arbitrary templates.title, content, metadata, intakeData, statusonly —userId/idcan no longer be reassigned viaPATCH /drafts/:id(document-planting closed).removeClauseFavorite/removeCitationFavoritenowdeleteMany({ where: { id, userId } })with the authenticatedreq.user.id; a mismatched user deletes 0 rows.No publish/verify/version endpoints exist in the server, so locking these fields breaks no legitimate flow (verified by cold review).
Red-first / tests
Red commit
8bfde53(12 failing tests: trust-flag ignored on create+update per model, draft userId immutable, cross-user favorite IDOR — repo + route level) → green311cfb5.Verification
server/npm test: 232 passed (220 baseline + 12).Gates
/simplify+/security-review+/code-reviewvia fresh cold reviewer, no implementation context — verdict clean, no functional regression, no remaining raw-body spread for these 4 models. Secret scan clean.Follow-up noted (separate issue): auditLog/collaborator/shareLink repos still spread raw data, but those writes are field-guarded at the route layer.
Assisted-by: Claude Code (Fable orchestrator + Sonnet implementer/reviewer)