Security: capability checks, term task binding, and output escaping - #774
Closed
ilicfilip wants to merge 3 commits into
Closed
Security: capability checks, term task binding, and output escaping#774ilicfilip wants to merge 3 commits into
ilicfilip wants to merge 3 commits into
Conversation
Five AIOSEO interactive task handlers verified only that AIOSEO was active and that the nonce was valid, with no capability check: - Archive_Author - Archive_Date - Crawl_Settings_Feed_Comments - Crawl_Settings_Feed_Authors - Media_Pages Every other settings-changing interactive handler in the plugin already requires manage_options, so these five were the outliers. They now call the existing verify_aioseo_ajax_security() helper, which runs the active check, the capability check and the nonce check together. The gap was reachable because Email_Sending::enqueue_scripts() overrode its parent without a capability check and enqueued on index.php, handing a valid 'progress_planner' nonce to any logged-in user. WordPress nonces are per-user and per-action but not per-handler, so a Subscriber could take that nonce and drive the capability-less AIOSEO handlers - e.g. noindexing author and date archives. The guard now uses $this->capability_required() rather than a hardcoded capability, so it resolves to the capability the provider actually declares (manage_options here, inherited from Tasks) and stays correct if a subclass overrides CAPABILITY. Also switch esc_attr() to esc_url() on the "Why is this important?" link. That href can be set remotely via branding links[].url, and esc_attr does not neutralize schemes, so a javascript: URI would have executed on click. The same pattern in Sample_Page and Hello_World is fixed too; those URLs come from get_permalink() and were not exploitable, but esc_url is the correct escaper for an href. Verified: PHPCS clean, parallel-lint clean, PHPStan clean, PHPUnit 398 tests / 1203 assertions passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Remove_Terms_Without_Posts and Update_Term_Description took term_id and
taxonomy straight from the request and acted on them after checking only
the capability and the nonce. The capability is edit_others_posts, and
wp_delete_term()/wp_update_term() perform no capability check of their
own, so the effective permission to delete or edit any term in any
taxonomy was Editor - bypassing each taxonomy's own delete_terms and
manage_terms gates. An Editor could delete a category full of posts or a
WooCommerce attribute term, neither of which these tasks should touch.
The constraints were already enforced when generating the tasks, just not
when acting on them: maybe_remove_irrelevant_tasks() skips non-public
taxonomies and drops tasks once a term has more than MIN_POSTS posts.
Both handlers now, after the existing capability and nonce checks:
- reject non-public taxonomies,
- require a task from the same provider that targets this exact
term_id/taxonomy pair,
- and, for deletion, re-check the post count at deletion time, since
the term may have gained posts after the task was created.
Binding rather than raising the capability keeps the feature working for
its intended audience: these tasks are meant for Editors, and the JS
already submits the task's own target_term_id/target_taxonomy, so the
legitimate flow is unchanged.
Adds regression tests covering both the rejected and the permitted paths.
Verified that all four security tests fail when the new guards are
removed, and that the two positive tests still pass - so they detect the
vulnerability rather than passing vacuously.
Verified: PHPCS clean, PHPStan clean, PHPUnit 404 tests / 1216
assertions passing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The site icon and Yoast organization logo pickers built their preview
image by concatenating the attachment's alt text into an HTML string and
assigning it to innerHTML. Media alt text is free-text set by any
upload-capable user (Author+), so a value like
" onerror="alert(document.cookie)
closed the alt attribute and added onerror as a real attribute. The
payload fires on the media library's `select` event - when an admin
clicks the image, before anything is confirmed or saved - and alt text
is not visible in the media grid, so there is nothing to notice
beforehand.
Reachability is narrow: an admin has to open this task's popover and
select that specific attachment. But it executes in an authenticated
admin session on an admin page, so a hit is full admin compromise.
Both previews now build the node with createElement and property
assignment. No HTML string is parsed, so the alt value stays a value and
cannot become markup - verified with jsdom that the payload serializes
to alt="" onerror="..." and hasAttribute('onerror') is false.
Note: the Yoast logo picker's alt fallback still reads "Site icon
preview", which looks like a copy-paste artifact. Left as-is here to
avoid mixing a user-visible string change into a security fix.
Verified: JS lint clean, PHPCS clean, PHPUnit 404 tests / 1216
assertions passing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Test on Playground |
Contributor
✅ Code Coverage Report
🎉 Great job maintaining/improving code coverage! 📊 File-level Coverage Changes (2 files)📈 Coverage Improved
ℹ️ About this report
|
Collaborator
Author
|
Superseded by a re-opened PR with the same changes; closing this one. |
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
1 participant
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.
No description provided.