Fix chart creation nonce verification mismatch#1260
Closed
vytisbulkevicius wants to merge 23 commits intodevelopmentfrom
Closed
Fix chart creation nonce verification mismatch#1260vytisbulkevicius wants to merge 23 commits intodevelopmentfrom
vytisbulkevicius wants to merge 23 commits intodevelopmentfrom
Conversation
This commit implements two major AI features for the Visualizer plugin: Feature 1: AI Image-to-Chart Creation - Added upload interface on chart type selection page - Implemented AI image analysis to extract chart data and styling - Automatic chart type detection and data population - Styling extraction (colors, fonts, layout) from reference images - Support for OpenAI and Anthropic Claude vision models Feature 2: AI Configuration Assistant - Added AI chat interface in chart editor sidebar - Intelligent intent detection (action vs informational queries) - Smart auto-apply: applies configs for action requests, shows preview for questions - Chat history for conversational configuration - Deep merge of new configurations with existing settings Technical Changes: - New module: classes/Visualizer/Module/AI.php (AJAX handlers, API integration) - New page: classes/Visualizer/Render/Page/AISettings.php (API key management) - Modified: classes/Visualizer/Module/Chart.php (error suppression in uploadData) - Modified: classes/Visualizer/Render/Page/Types.php (image upload UI) - Modified: classes/Visualizer/Render/Sidebar.php (AI chat interface) - Modified: css/frame.css (AI interface styling) - New: js/ai-chart-from-image.js (image upload and analysis) - New: js/ai-chart-data-populate.js (chart data population after analysis) - New: js/ai-config.js (AI chat interface and intent detection) Related to #[ISSUE_NUMBER]
- Fix indentation in Types.php (use tabs instead of spaces) - Fix double quotes to single quotes in AI.php - Remove extensive debug logging from Chart.php - Keep essential error suppression for AJAX responses
- Add @return void annotations to all methods - Fix array type hints to array<string, mixed> - Replace DOING_AJAX constant with wp_doing_ajax() - Fix ini_set parameter type from int to string - Remove error suppression operators (@) - Remove unnecessary isset() check on explode() result
Adjust spacing in @param declarations to align parameter names correctly when using longer type declarations like array<string, mixed>
Critical fixes for AI features not appearing: 1. **Add AI Settings menu item**: - Register submenu page in Admin::registerAdminMenu() - Add Admin::renderAISettingsPage() method - Menu now appears under Visualizer menu 2. **Fix AI chat sidebar visibility**: - Call _renderAIConfigurationGroup() from _renderGeneralSettings() - Added to both Google.php and ChartJS.php sidebars - AI chat now shows in chart editor for all chart types 3. **Fix page URL references**: - Changed 'viz-ai-settings' to 'visualizer-ai-settings' - Updated links in Sidebar.php and Types.php - "Configure AI Settings" buttons now work correctly Fixes reported issues: - AI Settings menu not visible - Permission denied error (wrong page slug) - AI chat interface not showing in sidebar
Three UX improvements for better AI feature accessibility: 1. Image Upload Lock Icon - Fixed alignment and popup navigation - Repositioned lock overlay to top of container (40px padding) - Changed lock icon to display:block for proper centering - Added onclick handler to close popup and navigate to AI Settings in parent window 2. AI Chat Sidebar - Improved Settings/Chat section behavior - Added Settings section with conditional collapse (collapsed when API key exists) - Made Chat section grayed out when no API key configured - Fixed incorrect URL (viz-ai-settings → visualizer-ai-settings) - Added popup escape to all AI Settings links 3. Chart Creation Modal - Fixed auto-scroll behavior - Removed scrollIntoView() that was hiding image upload section - Modal now stays scrolled to top, making AI image upload visible by default Files modified: - classes/Visualizer/Render/Page/Types.php (lock icon alignment) - classes/Visualizer/Render/Sidebar.php (Settings/Chat sections) - js/frame.js (removed auto-scroll) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Two refinements to improve AI feature UX: 1. Lock Icon Centering (Types.php) - Re-added flexbox with center alignment (justify-content: center) - Positioned near top using align-items: flex-start with 60px padding - Icon is now centered horizontally and positioned at top without text overlap 2. Scroll Position Fix (Types.php + frame.js) - Added inline JavaScript to force scroll to top on page load - Uses both DOMContentLoaded and load events to ensure scroll stays at top - Overrides any cached JavaScript that might cause auto-scroll - Cleaned up frame.js by removing obsolete commented code This ensures the AI image upload section is always visible when opening the chart creation modal, regardless of browser caching. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Two critical fixes for improved UX and security: 1. Enhanced Scroll-to-Top Mechanism (Types.php) - Added multiple setTimeout calls at 100ms, 300ms, 500ms, and 1000ms intervals - Ensures scroll stays at top regardless of async content loading - Handles both window and parent window (iframe) scrolling - Prevents auto-scroll to chart library section that was hiding image upload 2. Secure API Key Input Fields (AISettings.php) - Changed input type from "text" to "password" for all API key fields - Removed insecure data attributes that exposed full keys in HTML - Added toggle button with eye icon to show/hide keys when needed - Added autocomplete="off" to prevent browser autofill exposure - Keys are now properly hidden and cannot be copied by simply clicking Security improvements: - API keys no longer visible in page source or DOM inspector - Keys cannot be accidentally copied when clicking input field - Keys remain hidden unless explicitly toggled visible by user - Proper password field behavior prevents casual exposure 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Two critical fixes per user requirements: 1. API Keys Fully Secured (AISettings.php) - Removed toggle button - keys are now completely non-retrievable - Changed all input fields to always show empty value (never display stored key) - Added green checkmark indicator when key is configured - Only update database if new non-empty value is entered - If user forgets key, they must enter new one (security by design) Security model: - Once saved, API keys cannot be viewed in dashboard - No visibility toggle, no masked display, completely hidden - Placeholder shows "API key is set (enter new key to replace)" - Maximum security - keys only retrievable from database, not UI 2. Aggressive Scroll Lock (Types.php) - Added scroll event listener that prevents ANY scrolling for 2.5 seconds - Multiple setTimeout intervals: 0, 50, 100, 200, 300, 500, 800, 1000, 1500, 2000ms - Forces scroll to 0,0 on both window and parent window (iframe) - Scroll lock automatically releases after 2.5 seconds for user control - Overrides any lazy-loaded JavaScript causing auto-scroll This ensures AI image upload section stays visible when modal opens, regardless of any async content loading or cached JavaScript. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Per user feedback - two critical fixes: 1. Enhanced Scroll Prevention (Types.php) - Removed for="chart-library" from label (prevents auto-focus scroll) - Added tabindex="-1" to select element (prevents focus-based scroll) - More aggressive scroll lock with 17 timeout intervals (0-2500ms) - Added document.body.scrollTop and documentElement.scrollTop resets - Added readystatechange listener for early DOM changes - Listens to both document and window scroll events - Extends lock to 3 seconds with console logging for debugging - Forces scroll on both iframe and parent window This should finally catch whatever async code is causing the scroll. 2. Restored Masked API Key Display (AISettings.php) - Removed green checkmark indicator (user feedback) - Keys now display masked: first 6 chars + asterisks + last 4 chars - Fields are readonly by default showing masked value - "Change Key" button makes field editable and clears it - "Cancel" button restores masked value and readonly state - Save only updates if value changed (not masked version) - Secure but provides visual confirmation that key exists 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…roll) Per user feedback - simplified both fixes: 1. API Key Fields - Simple and Clean (AISettings.php) - Removed "Change Key" button completely - Removed readonly attribute - fields are now editable - Removed all JavaScript for button handling - Fields show masked values (first 6 chars + *** + last 4 chars) - Users can click and type directly to change keys - Save button validates key is not masked version before saving Simple UX: See masked key, click field, type new key, save. 2. Scroll Fix - Root Cause Solution (Types.php) - IDENTIFIED ROOT CAUSE: Checked radio button causes browser autoscroll - Script finds checked radio buttons BEFORE browser can scroll - Temporarily removes "checked" attribute during page load - Forces scroll to top immediately and continuously - On DOMContentLoaded, restores checked state WITHOUT scrolling - Multiple setTimeout intervals as backup (0-2000ms) - Scroll lock for 2 seconds then releases for user control This directly prevents the browser's native scroll-to-checked behavior which was causing the view to scroll down to the chart library section. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Per user debugging - scroll happens in load-scripts.php (WordPress core) after scripts finish loading. Previous attempts to prevent scroll failed because they ran too late or didn't catch the right API calls. This implements a LOW-LEVEL scroll blocker: 1. Overrides window.scrollTo and window.scroll globally - Returns immediately if scroll is blocked - Logs blocked attempts to console for debugging 2. Overrides Element.prototype.scrollTop setter - Blocks ANY scrollTop assignment to ANY element - This catches document.body.scrollTop, documentElement.scrollTop, etc. - Logs blocked attempts to console 3. Blocks for 3 seconds after page load - Long enough for all WordPress scripts to finish loading - Then restores original functions - User can scroll normally after unlock Console logging will show: - "[Visualizer] Scroll blocker activated" on load - "[Visualizer] Blocked scrollTop set to X" when scroll is attempted - "[Visualizer] Scroll blocker deactivated" after 3 seconds This should catch the load-scripts.php scroll that the user debugged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Based on AI debugging analysis, the issue was NOT JavaScript scroll: - NO scroll events were triggered - NO JavaScript scroll calls happened - NO autofocus was causing scroll THE REAL ISSUE: CSS Layout Shift What happens: 1. library.js:140 opens modal with view.open() 2. WordPress focuses modal container immediately 3. Modal iframe loads Types.php content 4. Content renders with "Create Chart from Image" at top 5. BUT viewport has already anchored to visible content 6. Result: Appears scrolled to "Select Library" section THE FIX: Removed: - 51 lines of useless scroll-blocking JavaScript - API overrides for window.scrollTo - Element.prototype.scrollTop overrides - All the "nuclear option" code that didn't work Added: - Simple CSS to prevent layout shift - scroll-behavior: auto (disable smooth scroll) - min-height: 100vh on #type-picker - Simple scroll-to-top on DOMContentLoaded and load events This addresses the ACTUAL problem: ensuring viewport stays at top when iframe content finishes rendering, not blocking non-existent scrolls. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Final debugging revealed the smoking gun: The inline script I added in the previous commit was CAUSING the jump, not fixing it. What was happening: 1. Modal opens with iframe loading Types.php 2. My inline script runs: window.scrollTo(0, 0) on DOMContentLoaded/load 3. This forced scroll INSIDE the iframe triggers layout recalculation 4. WordPress modal parent detects the change and jumps visually The Fix: REMOVE the inline script completely. Stop fighting the browser. Let the modal and iframe render naturally without forced scrolling. Removed: - All CSS attempting to prevent layout shift - All inline JavaScript scroll commands - window.scrollTo(0, 0) on DOMContentLoaded - window.scrollTo(0, 0) on load - scrollTop assignments The modal should now open smoothly without jumping. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…action Enhanced the AI vision analysis to better recognize chart types, extract precise data values, and match visual styling: Chart Type Recognition: - Added comprehensive list of all supported chart types (pie, line, bar, column, area, scatter, bubble, geo, gauge, candlestick, timeline, combo, radar, polarArea) - Improved COMBO chart detection with explicit instructions to identify mixed visualization types (columns + lines) - Added chart type mapping for all Visualizer-supported types in parser Data Accuracy Improvements: - Instructed AI to interpolate values between gridlines instead of rounding - Added guidance to study Y-axis scale and gridline intervals carefully - Emphasized 5-10% accuracy requirement for data values - Provided examples of correct interpolation (e.g., 60% between 10-20 = 16) Visual Styling Detection: - Enhanced legend position detection (right/left/top/bottom) - Improved color extraction in exact order with hex codes - Added chart-type-specific styling instructions (pie slice text, 3D detection, donut detection) - Included styling examples for pie, bar/column, line/area, combo, bubble, geo, gauge, and scatter charts Combo Chart Support: - Added explicit combo chart detection rules - Instructed AI to use "seriesType" and "series" object for mixed types - Provided CSV and styling examples for combo charts - Added combo chart-specific analysis instructions Context and Safety: - Added professional context to prevent OpenAI safety filter rejections - Clarified this is for legitimate data extraction and visualization purposes - Maintained concise prompt while preserving all essential instructions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Changed the border color of the AI image upload section to always use the blue (#0073aa) border, regardless of whether API keys are configured or PRO features are locked. This makes it clear that this is a distinct section even when the lock overlay is shown. Previously, the border would change to light gray (#ddd) when locked, making the section boundary unclear. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Match development branch changes to resolve merge conflicts. The security updates in development removed this type hint for PHP compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Match development branch security updates by adding current_user_can('edit_posts') check before allowing chart creation. This prevents unauthorized users from accessing the chart creation interface.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Revert _getChartArray type hint from `$chart = null` back to `?WP_Post $chart = null` to match the state BEFORE the broken security PR was merged to development. The security PR in development has a bug where nonce creation and verification don't match. Our code maintains the working pre-security-PR state. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The security PR that was merged to development added nonce action parameters to creation and uploadData verification, but forgot to update _handleTypesPage verification. This caused a mismatch where:
- Nonces were created WITH action: wp_create_nonce('visualizer-upload-data')
- But _handleTypesPage verified WITHOUT action: wp_verify_nonce($nonce)
- Result: Nonce verification failed, chart creation broken
This fix ensures ALL nonce creation and verification use the same action parameter consistently.
Security improvements (from original security PR):
- Specific nonce action instead of generic -1
- Added current_user_can('edit_posts') capability check
- Added per-chart ownership check: current_user_can('edit_post', $chart_id)
Additional cleanup:
- Removed debug error_log statements
- Removed debug HTML error messages
- Cleaned up code formatting
Fixes vulnerability where subscribers could modify any chart's data.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
💂♂️ PR Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Contributor
Contributor
Author
|
Closing this PR as it incorrectly contains all AI feature changes. Creating a clean PR with only the security fix. |
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
2 participants
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.
Summary
Fixes the nonce verification mismatch introduced by the security PR that broke chart creation in the development branch.
Problem
The security PR added nonce action parameters but had a bug:
wp_create_nonce('visualizer-upload-data')_handleTypesPage()verified WITHOUT action:wp_verify_nonce($nonce)Solution
Updated
_handleTypesPage()to verify WITH the action parameter to match nonce creation.Security Improvements Maintained
✅ Specific nonce action
'visualizer-upload-data'instead of generic-1✅ Added
current_user_can('edit_posts')capability check✅ Added per-chart ownership check:
current_user_can('edit_post', $chart_id)✅ Prevents subscribers from modifying charts owned by admins (CVE fix)
Files Modified
classes/Visualizer/Module/Chart.php- Fixed nonce verification in_handleTypesPage()and enhanced security inuploadData()classes/Visualizer/Render/Page/Types.php- Updated nonce creationclasses/Visualizer/Render/Layout.php- Updated nonce creation (2 locations)Testing
✅ Tested locally - charts can be created successfully
✅ Security vulnerability remains fixed
🤖 Generated with Claude Code