Repository navigation
feat(content): 🔒 access rules for containers and Blog categories (work in progress) - #909
aXenDeveloper wants to merge 9 commits into
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 22 new issues in 12 files · 22 warnings · score 75 / 100 (Needs work) · 0 fixed · vs 22 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: 95d76ea4fe
ℹ️ 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".
| await Promise.all([ | ||
| invalidatePageCacheTags( | ||
| [ | ||
| contentAccessTag(family.id), |
There was a problem hiding this comment.
When a deployment uses a separate front end with its own page cache and configures content.revalidateOrigins, tightening a category or forum rule only invalidates the API process's cache here. Unlike the visibility effects, this path never calls dispatchContentRevalidation, and no listener for content.access.updated supplies that notification. Previously cached public HTML can therefore continue exposing the newly restricted content until it expires, without reaching the new access checks. Send hard invalidations to the configured front ends as part of the access change.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
| for (const [name, field] of Object.entries( | ||
| definition.fields as ContentFieldMap, | ||
| )) { | ||
| if (field.kind !== "file") continue; |
There was a problem hiding this comment.
For a published record with a file field omitted from publicApi.fields, this loop still accepts that field's file ID. The new public download route then streams it after checking only record access and attachment permissions, so a reader can retrieve an internal/private attachment by enumerating file IDs even though the public projection deliberately excludes it. The JSONB scan below similarly includes non-public rich-text fields. Limit membership checks to the fields exposed by the public API rather than every stored field.
Useful? React with 👍 / 👎.
| await db | ||
| .update(core_files) | ||
| .set({ visibility: "private" }) | ||
| .where(eq(core_files.id, id)); | ||
| // Last: if this fails the file is private anyway and the public copy is |
There was a problem hiding this comment.
If provider.delete fails after the database update succeeds, the original public object remains accessible, but the file is already marked private. A retry of makeFilePrivate immediately returns unchanged, and protectContentAccessFiles excludes private rows from its candidates, so neither path actually removes the orphan as the comment claims. The administrator can subsequently see no public files remaining while the supposedly protected bytes remain publicly downloadable. Preserve a retryable migration state or otherwise ensure deletion is retried before treating protection as complete.
Useful? React with 👍 / 👎.
…k in progress) Core access model (view, read, comment, download attachments), per-scope rules with inheritance and ancestor restrictions, enforcement in public Content Engine reads and writes, permission-aware search filtering and verification, Forum and Blog wiring, and the AdminCP access editor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnAiFc4gJkRmXr1TwfkSNX
…ope filtering Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnAiFc4gJkRmXr1TwfkSNX
…cache reset and article access notice Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnAiFc4gJkRmXr1TwfkSNX
- Generate the add_content_access migration: access rule tables, core_files.visibility and core_search_index.accessScopes with a GIN index - Ignore storage/private in the API app and the create-vitnode-app templates - Type the Blog editor's categoryId value for the effective-access notice - Read model definitions through contentDefinitionOf in access code - Forward a reported zero Content-Length again when streaming public files - Leave the search access filter off when nothing is restricted - Keep the middleware config when an identity change clears queries - Update tests for the new storage seam, container access flags and the identity reset contract Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnAiFc4gJkRmXr1TwfkSNX
…ttachments - Access rules: turn on access for a container collection and restrict forums - Category access: scope content types and access.from, with intersection rules - Protected attachments: private stores, private uploads and moving public files - How access is enforced: reads, writes, search, caching, notifications, sitemaps, moderators, root roles and troubleshooting - Access rules API reference, the content.access.updated event and the privateBucket option and private store in the storage docs Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnAiFc4gJkRmXr1TwfkSNX
…ages - Render a plugin route's notFound screen inside the route's own messages, and skip its breadcrumb when there is no loader data, so a hidden or missing forum shows "Forum not found" instead of a server error - Show a restricted attachment without the deleted-file strikethrough - Word the inherit hints in the Access sheet as what Inherit resolves to - Add real screenshots of the Access sheet, effective access, the guest forum list and sign-in state, the restricted attachment and the article editor notice to the access docs - Ignore storage/private in the web app Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnAiFc4gJkRmXr1TwfkSNX
Pin the access rules end to end with real seams (memory DB, context values, staff permission cache) and a shared access world fixture: - policy: prerequisites, audiences, deny-all for unreadable rules, root, inheritance order and code defaults - hierarchy: ancestor view/read rules, unknown scopes deny - multi-scope intersection, revoked roles and moved containers on the next request - public lists filtered before pagination, 404/403 detail, container visibility and flags, private caching, reply comment checks - admin access routes: errors, content.access.updated, permission matrix - search access filter, hit verification and skipAccess - protected downloads and attachment stripping - plugin route not-found messages and empty breadcrumbs Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnAiFc4gJkRmXr1TwfkSNX
- Saving a scope's access rules writes content.access_updated inside the same transaction as the rules: the forum or category name as the label, and the old and new rule of each changed action (inherit, everyone, members or roles with their names). Unchanged or rejected saves write nothing - Protect public files writes content.access_files_protected with the moved, shared and remaining counts - Register both actions with the Content Engine's audit actions, with labels and field names for the Activity Logs - Regenerate the add_content_access migration on top of canary's migrations (payments, SSO, presence, audit logs) - Document the entries in the access, activity logs and audit overview docs Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnAiFc4gJkRmXr1TwfkSNX
95d76ea to
74fe728
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74fe7286c3
ℹ️ 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".
| if (!(await contentRecordHasFile(c, accessModel, itemId, fileId))) { | ||
| return missing(); | ||
| } |
There was a problem hiding this comment.
A signed-in member can publish a topic in an open forum with an image or attachment node whose attrs.fileId names somebody else's private file, then download that file through this route using their own topic ID. The forum's public-write API accepts body, the rich-text schema permits arbitrary numeric attributes, and assertContentFileReferences validates only field.file() fields—not rich-text references. Consequently, contentRecordHasFile treats attacker-controlled JSON as proof of authorization, while the remaining checks concern only the attacker's topic. Validate newly assigned rich-text file references against uploader ownership or another trusted authorization source before allowing them to grant download access.
Useful? React with 👍 / 👎.
| metadata: { contentTypeId: definition.id, field }, | ||
| // Behind access rules, and the adapter can keep it private: never a | ||
| // public URL, only the record's authorized download route. | ||
| visibility: contentUploadVisibility(c, model), |
There was a problem hiding this comment.
With private-capable storage configured, new blog cover images are now stored privately, but automatic alt generation still calls prepareImage in api/lib/ai/alt.ts, which reads them through storage.readBytes(file.key, ...). That method reads only the public store and receives no visibility information, so these uploads are marked skipped with “The file could not be read or is too large” instead of receiving generated alt text. Update the AI read path to resolve the file's visibility and read from the appropriate store as part of switching these uploads to private storage.
Useful? React with 👍 / 👎.
- PUT /{id}/access writes one content.access_updated entry for a board
(container) and a category (scope content type): plugin, admin area,
staff actor, family as target type, scope id and name, content type id
and access_<action> changes with inherit/everyone/members/roles values
- Unchanged and rejected saves write no entry; rejected saves keep the
stored rules
- replaceContentAccessRules runs `within` after the rules in the same
transaction, so a failing audit write rolls both back
- recordContentAccessUpdate labels a localized scope with its default
translation's title
- recordContentAccessFilesProtected records moved/remaining/shared and
skips a run that moved and shared nothing
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnAiFc4gJkRmXr1TwfkSNX
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| let changed = 0; | ||
| for (const [itemType, keys] of byType) { | ||
| const rows = await db |
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.
| case "roles": | ||
| return ( | ||
| actor.kind === "member" && | ||
| actor.roleIds.some(roleId => audience.roleIds.includes(roleId)) |
There was a problem hiding this comment.
React Doctor · react-doctor/js-set-map-lookups (warning)
This scales poorly because array.includes() inside a loop scans the whole list every time. Use a Set for constant-time lookups.
Fix → Use a Set or Map when you check for the same items over and over. Array.includes/find scans the whole list each time
| // the outermost scope that keeps the actor out. | ||
| for (let index = chain.length - 1; index >= 1; index--) { | ||
| add(index, "view", "ancestor"); | ||
| if (needed.includes("read")) add(index, "read", "ancestor"); |
There was a problem hiding this comment.
React Doctor · react-doctor/js-set-map-lookups (warning)
This scales poorly because array.includes() inside a loop scans the whole list every time. Use a Set for constant-time lookups.
Fix → Use a Set or Map when you check for the same items over and over. Array.includes/find scans the whole list each time
| continue; | ||
| } | ||
|
|
||
| const state = await loadContentAccessFamilyState(c, family); |
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.
| for (const action of CONTENT_ACCESS_ACTIONS) { | ||
| const decision = decideContentAccess( | ||
| who, | ||
| await contentAccessRequirementsForScopes(c, scopes, action), |
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.
|
|
||
| return (changedFields ?? []).some(name => indexed.has(name)) | ||
| return (changedFields ?? []).some( | ||
| name => indexed.has(name) || access.includes(name), |
There was a problem hiding this comment.
React Doctor · react-doctor/js-set-map-lookups (warning)
This scales poorly because array.includes() inside a loop scans the whole list every time. Use a Set for constant-time lookups.
Fix → Use a Set or Map when you check for the same items over and over. Array.includes/find scans the whole list each time
| ...accessFieldsOf(definition), | ||
| ]; | ||
|
|
||
| return (changedFields ?? []).some(name => linked.includes(name)) |
There was a problem hiding this comment.
React Doctor · react-doctor/js-set-map-lookups (warning)
This scales poorly because array.includes() inside a loop scans the whole list every time. Use a Set for constant-time lookups.
Fix → Use a Set or Map when you check for the same items over and over. Array.includes/find scans the whole list each time
| const moved = (input.changedFields ?? []).some(name => indexed.has(name)); | ||
| const access = accessFieldsOf(definition); | ||
| const moved = (input.changedFields ?? []).some( | ||
| name => indexed.has(name) || access.includes(name), |
There was a problem hiding this comment.
React Doctor · react-doctor/js-set-map-lookups (warning)
This scales poorly because array.includes() inside a loop scans the whole list every time. Use a Set for constant-time lookups.
Fix → Use a Set or Map when you check for the same items over and over. Array.includes/find scans the whole list each time
| if ( | ||
| (input.operation === "update" || input.operation === "restore") && | ||
| (input.changedFields ?? []).some(name => | ||
| accessFieldsOf(definition).includes(name), |
There was a problem hiding this comment.
React Doctor · react-doctor/js-set-map-lookups (warning)
This scales poorly because array.includes() inside a loop scans the whole list every time. Use a Set for constant-time lookups.
Fix → Use a Set or Map when you check for the same items over and over. Array.includes/find scans the whole list each time
| @@ -363,10 +364,16 @@ const ThreadNodeView = memo( | |||
| ); | |||
|
|
|||
| export const ForumThread = ({ | |||
There was a problem hiding this comment.
React Doctor · react-doctor/no-high-complexity-react-function (warning)
ForumThread has cyclomatic complexity 15, cognitive complexity 17, and maximum nesting depth 3, so its React logic is hard to understand and change. Extract independent branches into components or hooks.
Fix → Extract independent render branches and state logic into focused components or hooks until the control flow is easy to follow.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24baa31c7f
ℹ️ 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".
| ((await canViewHiddenContentRecord(c, { | ||
| contentTypeId: definition.id, | ||
| itemId, | ||
| })) && |
There was a problem hiding this comment.
For a user with moderator can_view_hidden permission in a readable forum, this fallback also authorizes files belonging to draft, unpublished, scheduled, or deleted records. canViewHiddenContentRecord checks authorship/staff/moderator permissions but does not check the record's publication state, and canAccessContentRecord checks only scope access. Consequently, failing the published query above does not prevent that moderator from downloading unpublished attachments by record/file ID, even though the normal hidden-content read path still requires a published hidden record. Require the hidden publication predicate before accepting this fallback.
Useful? React with 👍 / 👎.
Core access model (view, read, comment, download attachments), per-scope
rules with inheritance and ancestor restrictions, enforcement in public
Content Engine reads and writes, permission-aware search filtering and
verification, Forum and Blog wiring, and the AdminCP access editor.
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01CnAiFc4gJkRmXr1TwfkSNX