Repository navigation
feat(content): 🔒 access rules for containers and Blog categories (work in progress) - #909
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 31 new issues in 19 files · 31 warnings · score 75 / 100 (Needs work) · 0 fixed · vs 31 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 👍 / 👎.
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 👍 / 👎.
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 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99c3a8d34c
ℹ️ 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".
| const [row] = await db | ||
| .select({ one: sql`1` }) | ||
| .from(source) | ||
| .where(and(eq(idColumn, itemId), references(column))) |
There was a problem hiding this comment.
For a published blog article with a draft, unpublished, or scheduled translation, this query accepts files referenced exclusively by that translation. The download route checks only the base record's publication state, so any reader of the published article can retrieve those private files by record/file ID even though the translation itself is unavailable. Unlike the published-translation predicates in localized-public-service.ts, this scan has no translation publication condition. Require the referencing translation to be published before its rich-text references can authorize a public download.
Useful? React with 👍 / 👎.
| </div> | ||
| ); | ||
| } | ||
| if (!query.data) return null; |
There was a problem hiding this comment.
If the /access/files request fails, this silently removes the entire file-protection section while leaving the access-rule editor usable. Administrators lose both the warning that existing files remain publicly accessible and the control for protecting them, with no indication that the check failed or way to retry in place. Render an alert Empty state with a retry action rather than treating failed status loading as an absent section, as required by AGENTS.md.
AGENTS.md reference: AGENTS.md:L52-L52
Useful? React with 👍 / 👎.
- Access rules for Content Engine containers and Blog categories, with an access matrix and dialog in AdminCP - Protected attachments, private storage and search scope filtering - Access-aware notifications, approval queues, client cache reset and article access notice - Access rule changes recorded in Activity Logs - Structured error titles and descriptions in locales - Access migration, not-found pages and docs (access rules, category access, protected attachments) - Regression tests for access rules and their Activity Log entries Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ed497c1 to
1f74289
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed497c1974
ℹ️ 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".
| eq(columns.id, itemId), | ||
| contentPublicReadCondition(published, {}), | ||
| access, |
There was a problem hiding this comment.
For a published forum or blog comment whose parent topic/article has been hidden or unpublished, this query still authorizes an ordinary reader to download its private files. It checks only the comment's publication state; contentAccessCondition inherits the parent's scope rules but does not check the parent's publication or visibility. Normal comment reads enforce that additional check through createContentParentVisibility in content/server/parent-visibility.ts. Apply the same parent-visibility check here so previously obtained comment/file URLs stop serving protected bytes when the parent is withdrawn.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f74289613
ℹ️ 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 this.upsertRows(stale); | ||
| await this.provider().bulkIndex(this.c, stale); |
There was a problem hiding this comment.
When an external search provider fails during bulkIndex, the canonical rows have already received their new access scopes. Subsequent reconciliation attempts compare against those updated rows, find no stale documents, and skip the provider write permanently. For example, after an article moves from a restricted category to a public one, Elasticsearch can continue filtering it out even after the provider recovers and the hourly reconciliation runs. Preserve a retryable synchronization state or otherwise ensure the provider update is retried before considering these documents reconciled.
Useful? React with 👍 / 👎.
![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.
| * that a parent or a prerequisite still keeps out gets a lock explaining why, | ||
| * from the server's preview of the unsaved rules. | ||
| */ | ||
| export const ContentAccessMatrix = ({ |
There was a problem hiding this comment.
React Doctor · react-doctor/no-giant-component (warning)
Component "ContentAccessMatrix" is over 300 lines long, which is hard to read & change. Split it into a few smaller components.
Fix → Pull each section into its own component so the parent is easier to read, test, and change.
|
|
||
| return ( | ||
| <TooltipProvider> | ||
| <div aria-label={tMatrix("caption")} className="text-sm" role="table"> |
There was a problem hiding this comment.
React Doctor · react-doctor/prefer-tag-over-role (warning)
Screen reader users get more reliable semantics from <table> than role="table", so use <table> instead.
Fix → Use the matching HTML element when one exists so browsers and assistive tech get native semantics.
| (CONTENT_ROW_PINNED_ACTION_IDS as readonly string[]).includes(id), | ||
| ); | ||
| if (pinned.length > 0) { | ||
| return { inline: pinned, menu: ids.filter(id => !pinned.includes(id)) }; |
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
| <EmptyMedia variant="icon"> | ||
| <MessageSquareWarningIcon aria-hidden="true" /> | ||
| </EmptyMedia> | ||
| <EmptyTitle render={<h3 />}>{t("load_failed.title")}</EmptyTitle> |
There was a problem hiding this comment.
React Doctor · react-doctor/heading-has-content (warning)
Blind users can't use this heading to navigate because screen readers skip it empty, so add text, aria-label, or aria-labelledby.
Fix → Put readable text in every heading.
| @@ -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: c847f81304
ℹ️ 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".
| // AdminCP editor reads it as it is stored, so it points at the staff | ||
| // route now. Public reads replace it with the authorized route anyway. | ||
| for (const model of inside) { | ||
| await rewriteStoredFileSrc(c, model, id, contentPrivateAdminFileUrl(id)); |
There was a problem hiding this comment.
After an administrator protects existing forum files, this replaces their stored rich-text URLs with a staff-only download URL. However, createContentPublicWriteService.findEditable() returns the stored body unchanged through valuesOf(), and plugins/forum/src/pages/topic-edit-page.tsx passes that body directly into the editor. An ordinary author reopening their topic therefore gets broken images and inaccessible attachments despite still being allowed to read and edit the topic: the target route requires the AdminCP files.can_download permission. Resolve private-file URLs for the authorized member edit response rather than leaving it dependent on the staff-only URL.
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