Harden calendar trashed-message Undo URL construction#944
Merged
Conversation
The calendar's trashed/untrashed notice interpolated the raw $_GET ['ids'] query parameter into the Undo link's URL and derived the post type from the first value of that list without checking that it was a registered type. esc_url() on the resulting link only makes the URL safe to emit into HTML; it does not stop a crafted ids value from injecting additional query arguments (e.g. ids=1%26action%3Dmalicious) or from pointing the undo to an arbitrary post type. Each id is now cast through absint() before the list is reassembled, the post type is rejected unless post_type_exists() confirms it, and add_query_arg() builds the URL so the ampersand separator cannot come from user input. The cast also flows into the translated message and into number_format_i18n() so both receive a known-integer value. While in the file, the stray unset( $_GET['undeleted'] ) is corrected to the parameter the branch actually guards on (untrashed), and the ValidatedSanitizedInput phpcs suppression is dropped because the input is now sanitised in code rather than by comment.
PHPCS flagged $_GET['ids'] as unsanitised input because the sniff does not recognise the subsequent absint/explode chain as a sanitisation path. Wrap the initial read in sanitize_text_field so the intent is explicit and CI stays clean; the resulting value is still fed through absint per id and post_type_exists on the inferred post type, so the security properties are unchanged.
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.
Summary
The calendar view renders a "post moved to the trash" notice with an Undo link when the admin arrives with
?trashed=N&ids=...in the URL. That notice was built by interpolating the raw\$_GET['ids']value straight into the link's href, deriving the post type from the first comma-separated value, and leaning onesc_url()to make the result safe.esc_url()only makes a URL safe to emit into HTML — it escapes the ampersand for HTML context so that reading the href in the source shows&, but it does not URL-encode values inside the query string. A crafted request like?trashed=1&ids=1%26action%3Dmaliciouswould therefore produce an Undo link whose query string contained an injectedaction=maliciousparameter alongsideids=1. The post type, read from the first value in the list with no validation, could similarly be any arbitrary string.The fix pushes every id through
absint(), reassembles the list from the resulting integers, and only proceeds whenpost_type_exists()confirms the derived post type is real. The URL itself is then built withadd_query_arg()so the&separator can never originate from user input. As tidying whilst in the file, the count values for both branches are cast tointonce and reused, a strayunset( \$_GET['undeleted'] )is corrected to the parameter the branch actually guards on (untrashed), and theValidatedSanitizedInputphpcs suppression is dropped now that sanitisation happens in code rather than by comment.Test plan
&ids=1%26action%3Dmaliciousto a?trashed=1calendar URL and confirm the rendered Undo link carries only a cleanids=1value and a registered post type