fix: enforce per-chart authorization in AI Builder endpoints - #1353
Conversation
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.
| 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 ) ) { |
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' } );
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
76187de to
09e61d0
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
classes/Visualizer/Module/AIBuilder.php:103
- The regression suite does not exercise this new post-type restriction. Its denied case uses an admin-owned chart, which the old
edit_postcheck already rejected, while the own auto-draft and Editor cases also passed under the old check. Add a Contributor-owned regular post and verify the chart-targeted handlers reject it; otherwise reverting this line would leave the tests green.
if ( ! self::can_edit_chart( $chart_id ) ) {
classes/Visualizer/Module/AIBuilder.php:507
- The new test only checks a fabricated, unbound ID. It never verifies that a successful generation creates this binding and lets its owner poll, so a broken
set_transient()call—or an implementation that rejects every poll—would pass. Mock the workflow start/status responses and assert owner success plus cross-user denial.
if ( ! empty( $workflow_id ) ) {
set_transient( 'viz_ai_wf_' . $workflow_id, get_current_user_id(), 6 * HOUR_IN_SECONDS );
}
tests/e2e/specs/ai-builder-auth.spec.js:131
- This chart is never deleted.
afterAlldeletes the Contributor withreassign: 1, which leaves the generated chart reassigned to admin and pollutes later specs. Track cleanup in afinallyblock using the adminrequestUtilsfixture.
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/wizard-auth.spec.js:30
- This fresh browser context has not visited
wp-login.php, so it has nowordpress_test_cookie. Postingtestcookie=1makes WordPress reject the login before the authorization assertion runs. Prime the cookie jar before submitting the form.
testcookie: '1',
tests/e2e/specs/ai-builder-auth.spec.js:22
- A fresh context has no
wordpress_test_cookie, so sendingtestcookie=1causes WordPress to reject every login before these authorization cases execute. Visit the login endpoint first so its test cookie is stored.
testcookie: '1',
The suite passed identically with can_edit_chart() reverted to current_user_can( 'edit_post' ): every case it exercised agreed under both checks. Add the two rows that differ — a contributor's own published chart (allowed only by the author branch) and a contributor's own non-chart post (allowed only by edit_post) — plus a workflow start/poll test that pins the transient binding, so a no-op set_transient() no longer passes. Start the e2e login contexts from an empty storage state. browser.newContext() inherits the project's admin cookies, so a login that silently failed left the requests running as admin; that also drops wordpress_test_cookie, hence no testcookie field. Delete the chart the contributor spec creates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
tests/e2e/specs/ai-builder-auth.spec.js:100
useris never read, so this destructuring introduces an unused variable and can fail the JavaScript lint rule. Iterate over the IDs directly.
for ( const [ id, user ] of [ [ contributorId, CONTRIBUTOR ], [ editorId, EDITOR ] ] ) {
classes/Visualizer/Module.php:653
- This stored-XSS hardening, along with the setup-wizard capability/nonce changes, materially expands the PR beyond the AI Builder authorization work described in the title and description. Please document these security changes and their QA impact, or split them into a focused PR so they can be reviewed and released with the correct scope.
$css .= wp_strip_all_tags( '.' . $class_name . ' {' . $properties . ' !important;}' );
|
🎉 This PR is included in version 4.0.7 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.