Repository navigation
fix(security): close findings from the canary security audit - #913
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
React Doctor found 2 new issues in 1 file · 2 warnings · score 87 / 100 (Great) · 0 fixed · vs 2 warnings
Reviewed by React Doctor for commit |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa0490f0e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !byAuthor && | ||
| file.userId !== null && | ||
| ((await isStaff(c, { type: "admin", userId: file.userId })) || | ||
| (await isStaff(c, { type: "moderator", userId: file.userId }))); | ||
| if (!byAuthor && !byStaff) return missing(); |
There was a problem hiding this comment.
When a moderator uploads an attachment while editing another member's post, this check makes the attachment's availability depend on the uploader remaining staff forever. Removing their staff entry through admin/staff/routes/delete.route.ts makes both checks false, so existing protected images and attachments start returning 404 even though the post, file reference, and reader's access are unchanged. Deleting an uploader's account also breaks this path because core_files.userId uses onDelete: "set null". Validate and persist the attachment's association when it is added rather than requiring the uploader's current identity or staff status on every download.
Useful? React with 👍 / 👎.
- Content files, comments, SSO and payments audit findings - Pin cache generation per request, re-auth for passkeys - HTTPS-only update links in the AdminCP, wider CSV formula guard - Findings from the full, line-by-line and thematic core scans - Staff and user routes invalidate all staff permissions - Align passkey and timeline tests with the new security rules Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
06446aa to
59a65c4
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| const owned = new Set( | ||
| ( | ||
| await tx |
There was a problem hiding this comment.
React Doctor · react-doctor/async-await-in-loop (warning)
This for…of loop waits before starting the next iteration. If iterations perform independent asynchronous work, consider bounded concurrency; await syntax alone does not establish a speedup.
Fix → Consider concurrent calls only for independent asynchronous work. Shared queues or synchronous work may not benefit. Preserve resource limits, transaction ordering, and failure/cancellation semantics; observe all callback promises.
| if (owned.has(fileId)) continue; | ||
| if ( | ||
| itemId !== undefined && | ||
| (await contentRecordHasFile(c, accessModel, itemId, fileId)) |
There was a problem hiding this comment.
React Doctor · react-doctor/async-await-in-loop (warning)
This for…of loop waits before starting the next iteration. If iterations perform independent asynchronous work, consider bounded concurrency; await syntax alone does not establish a speedup.
Fix → Consider concurrent calls only for independent asynchronous work. Shared queues or synchronous work may not benefit. Preserve resource limits, transaction ordering, and failure/cancellation semantics; observe all callback promises.
Improving Documentation
pnpm lint:fixandpnpm lintpass on the branch.Description
What?
These are fixes from a multi-pass security review of
@vitnode/coreand the shipped plugins.High
colspanof 30,000,000 used up all the server-side renderer's memory and killed the process on every view. A member could cause this with one 187-byte post. Spans are now capped when rendering, and values above HTML's limits are rejected when saved.fileIdin rich text and download it through the protected file route. Rich-text file ids must now be the writer's own uploads or files already on the record. The download route also checks that the uploader is the record owner or staff.Medium
reauthentication_required). A stolen cookie can no longer plant a key that survives a password reset.emailVerified === trueis trusted. An omitted value fails closed.already_paidinstead of starting a second charge.field.file(): a match on content type is honoured for staff only. "Protect files" skips avatars, covers and other content types' files.pinContentPublicGeneration), so an access-rule change mid-request can't be cached for guests.Low and hardening
/.//hostis refused after parsing in notification targets andsanitizeReturnTo.useHrefNavigateonly leaves the app for http(s) and mailto.hrefscheme is re-checked./thread/branchesis no-store unless the access model is shareable.Vary: Accept-Language.notifications:can_manage.allowedScopesdenies an unknown family, andaccess.frompointing at a non-scope type fails at boot.application/octet-stream.zod4.6.7 andsanitize-html2.18.1, which fix published advisories.Tests: fixtures and expectations changed only where behaviour changed on purpose:
emailVerified: true;Why?
Each item was traced from a realistic request to its impact before it was fixed. Regression tests for the new behaviour should come in a follow-up PR (repo rule: tests in a separate session).
Left for a maintainer decision (not in this PR)
vitnode migrategenerates and applies migrations in production without asking.CRON_SECRETis sent tocontent.revalidateOrigins.storage.image./admin/ai/assisthas no specific permission.__Host-cookie prefix, CSP and HSTS.Generated by Claude Code