fix: enforce per-chart authorization in AI Builder endpoints - #1353
Open
lucadobrescu wants to merge 2 commits into
Open
fix: enforce per-chart authorization in AI Builder endpoints#1353lucadobrescu wants to merge 2 commits into
lucadobrescu wants to merge 2 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
lucadobrescu
marked this pull request as ready for review
July 29, 2026 10:46
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
Strengthens AI Builder authorization for chart operations and workflow polling.
Changes:
- Uses the shared per-chart authorization guard.
- Binds workflow IDs to their initiating users.
- Adds authorization E2E coverage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
classes/Visualizer/Module/AIBuilder.php |
Enforces chart and workflow ownership checks. |
tests/e2e/specs/ai-builder-auth.spec.js |
Tests AI Builder authorization behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+505
to
+506
| if ( ! empty( $workflow_id ) ) { | ||
| set_transient( 'viz_ai_wf_' . $workflow_id, get_current_user_id(), 6 * HOUR_IN_SECONDS ); |
| if ( adminChartId ) { | ||
| await requestUtils.rest( { method: 'DELETE', path: `/wp/v2/visualizer/${ adminChartId }`, params: { force: true } } ); | ||
| } | ||
| for ( const [ id, user ] of [ [ contributorId, CONTRIBUTOR ], [ editorId, EDITOR ] ] ) { |
|
|
||
| const created = await aiAjax( page, 'visualizer-ai-create', { nonce } ); | ||
| expect( created.body.success ).toBe( true ); | ||
| const ownChartId = created.body.data.chart_id; |
| */ | ||
| private function _verify_chart_access( $chart_id ): void { | ||
| if ( ! current_user_can( 'edit_post', $chart_id ) ) { | ||
| if ( ! self::can_edit_chart( $chart_id ) ) { |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
tests/e2e/specs/ai-builder-auth.spec.js:131
- The chart created here is never deleted.
afterAlldeletes the contributor withreassign: 1, which transfers this auto-draft to user 1 instead of removing it, so repeated runs leak charts and can affect later chart-library specs. Track this ID and explicitly delete it during teardown before deleting the user.
const created = await aiAjax( page, 'visualizer-ai-create', { nonce } );
expect( created.body.success ).toBe( true );
const ownChartId = created.body.data.chart_id;
tests/e2e/specs/ai-builder-auth.spec.js:22
- This fresh browser context has no
wordpress_test_cookie, but the form setstestcookie=1. WordPress rejects that first POST with the “cookies are blocked” login error (HTTP 200), so every test usingloginAs()fails before reaching the authorization assertions. Prime/wp-login.phponce (or explicitly add the test cookie) before posting credentials.
const response = await context.request.post( '/wp-login.php', {
tests/e2e/specs/ai-builder-auth.spec.js:117
- This test never creates a workflow owned by someone else; it only verifies that an unknown ID with no transient is rejected. It therefore still passes if generation never stores the owner or if status polling rejects owners too, leaving the new binding/positive path untested. Mock the Agents start/status responses, generate as one user, then assert that user can poll while a second user receives 403.
const { status, body } = await aiAjax( page, 'visualizer-ai-status', { nonce, workflow_id: 'foreign-workflow-id' } );
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
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.
The AI Builder's chart-targeted AJAX handlers used a weaker authorization check than the rest of the plugin —
edit_postwithout verifying the target is actually a chart — and the generation-status poller accepted any workflow ID from any logged-in user. This routes every chart-targeted handler through the sharedcan_edit_chart()guard and binds AI workflow IDs to the user who started them. Follow-up to the authorization hardening started in #1348.What changed
Chart access check —
_verify_chart_access()now delegates toVisualizer_Module::can_edit_chart(). Before:current_user_can( 'edit_post', $id )alone, so any editable post type was accepted —saveChartcould retitle and force-publish arbitrary posts, anduploadDatacould overwrite non-chart content. After: onlyvisualizerposts pass, and contributor-authors keep access to their own charts, matching the rule used by the chart editor endpoints.Workflow status polling —
generateChartstores the returned workflow ID in a user-scoped transient;chartStatusanswers 403 for IDs not bound to the current user. Before: any user withedit_postscould poll another user's AI generation and read its full payload (prompt, series, data, generated code).Upload validation — unchanged. The uploaded file is parsed in place by the CSV/XLSX source classes, which reject content without the required header/type rows, and is never written to a web-accessible path, so the extension check is dispatch logic, not a security boundary. MIME sniffing was left out because real-world CSVs often sniff as
text/plainand would be rejected.Note
saveChartkeeps the plugin's existing force-publish behavior for charts (same as the classic chart editor save), gated by the samecan_edit_chart()rule, so contributor-owned AI charts keep working.AI Builder request flow
flowchart LR A[AI Builder AJAX request] --> B{Nonce valid?} B -- no --> X[403] B -- yes --> C{Changed:<br/>can_edit_chart<br/>chart_id?}:::changed C -- no --> X C -- yes --> D{Action} D -- generate --> E[Start AI workflow] --> F[New:<br/>bind workflow ID<br/>to user transient]:::added D -- poll status --> G{New:<br/>workflow bound<br/>to user?}:::added G -- no --> X G -- yes --> H[Return status] D -- fetch / upload / save --> I[Operate on chart] classDef added fill:#1a7f37,color:#fff,stroke:#116329,stroke-width:3px classDef changed fill:#9a6700,color:#fff,stroke:#5c3d00,stroke-width:3px,stroke-dasharray:6 3Data changes
viz_ai_wf_<workflow_id>transientBefore → after: workflow IDs were unbound → each is bound to its creator for the generation's lifetime.
QA
WP Admin → Users → Add New: create a user with the Contributor role, then log in as that user in a private window.
WP Admin → Visualizer → Chart Library. In DevTools console, run the following with the ID of a chart owned by admin:
Expect:
{ success: false, data: { message: "Unauthorized." } }with HTTP 403. Before this change, the same request returned the chart's full data payload.action: 'visualizer-ai-status'and any made-upworkflow_id.Expect: the same
Unauthorized.response — before, the request was proxied to the AI service and returned its payload.visualizer-ai-fetchcall from step 2 against the draft chart ID returned by the builder.Expect: the request succeeds — contributors keep full access to charts they created.