Conversation
Wraps the request values the sniff flags, choosing the escaper by what the value is: esc_url_raw() for REQUEST_URI, HTTP_REFERER, QUERY_STRING and SCRIPT_NAME, sanitize_text_field() for HTTP_HOST, HTTP_USER_AGENT, REMOTE_ADDR and the rest, with `?? ''` on the array access so a missing key is validated in the same expression. sanitize_text_field() is deliberately not used on the URL values. It strips percent-encoded octets in a loop, so it would quietly rewrite /tags/c%2B%2B/ to /tags/c/ and break routing for any encoded path; these values are compared, parsed and matched rather than printed, so mangling them changes behaviour rather than output. esc_url_raw() keeps the encoding and still drops CR and LF. Three files are left alone because WordPress is not loaded in them, so wp_unslash() and sanitize_text_field() would be fatal: the browse-happy endpoint and its test page, and the Slack security team list. Nineteen more lines already carry a `?? 'default'` that a blanket `?? ''` would have made unreachable, and the Rosetta contact form wants a pass of its own rather than a drive-by; both sets are left for the next tranches. Renames a local Akismet payload that shadowed the $comment global, and tidies the spacing and inline control structures that the longer lines surfaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
These are the lines the first pass skipped. Each already ended in `?? 'something'`, and wrapping them with a blanket `?? ''` would have made that fallback unreachable — SERVER_PROTOCOL would have stopped defaulting to HTTP/1.1, and the redirect paths to '/'. The existing default now sits inside the wrapper instead, so the behaviour is unchanged. block-config.php needed care: ?: binds tighter than ??, so the original default there is the whole ( wp_parse_url( … ) ?: 'localhost' ) expression, and it is preserved as one. The wporg-login referer fallback goes through isset() rather than ??, because the two are not equivalent once the value is wrapped. Renames the HelpScout request buffer, which was called $HTTP_RAW_POST_DATA. That is a PHP superglobal removed in 7.0, so the name reads as if the code depends on something that no longer exists; the value is really assigned from php://input two lines up and used only inside get_request(). Still left alone: the serve-happy endpoint and the Slack subgroup handler, which do not load WordPress. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Picks the sanitizer by what each value is rather than reaching for sanitize_text_field everywhere: sanitize_key() for action, post_type, post_status and the block directory verbs, absint() for post and comment ids, sanitize_user() for logins and the favourites author, esc_url_raw() for the plugin and readme URLs, sanitize_file_name() for the uploaded archive name, sanitize_textarea_field() for the upload comment, and wp_kses_post() for the author notice and internal comment bodies, which are allowed markup. $_FILES needs a different shape to satisfy the sniff: the cast has to sit directly on the array access, so the upload error and size are guarded with isset() and cast, while tmp_name and name go through the usual unslash-and-sanitize. The block validator's conditions tested $_POST for truthiness before checking a specific key. That test is what the nonce sniff was reporting, and it is redundant next to the ! empty( $_POST['key'] ) beside it, so it is gone and the branch needs no annotation at all. Three ignores remain, each with the reason it is true. The upload handler's reads happen after Upload::shortcode_handler() has verified the nonce, which the sniff cannot see across methods. The readme validator has no nonce because it stores nothing — it parses the submitted readme and echoes the result back. And $comment_post_ID keeps its name because the compact() below hands it to wp_insert_comment(), which expects that exact key. Also swaps a date() for gmdate() on a line this touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sanitizer by value again: sanitize_key() for the list table's view, orderby, order and bulk action, sanitize_user() for logins and the user column, sanitize_email() for the address fields, absint() for the IP block and allow durations and the pending ids, esc_url_raw() for every redirect target and the wporg_came_from cookie, and sanitize_textarea_field() for the block word, banned domain and IP list options, which are multi-line. The submitted password is the exception: it is only unslashed, with an ignore saying why. wp_set_password() has to receive it byte for byte, and sanitize_text_field() would strip characters out of anything containing a percent sequence, silently changing the password a visitor just chose. The nonce annotations here say what is actually true of these screens: the login and registration forms are served to logged-out visitors and have no nonce to verify, with reCAPTCHA doing the anti-automation work. The registration template's $user_login and $user_email hold submitted values rather than the current user, so they share one combined annotation rather than a stacked pair of line comments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Slugs, versions, locales and action names take sanitize_key() or sanitize_text_field(); the oEmbed target takes esc_url_raw(); the events endpoint's coordinates take floatval() and its result count absint(). Three payloads are deliberately only unslashed, with the reason recorded: the Slack interaction body, the Trac mentions body and the themes API request are JSON or serialized structures, and running a string sanitizer over them would corrupt the payload before it is decoded. Each is validated field by field after decoding, and the Slack request is signature-checked first. The nonce annotations on these files say what is true of a webhook: the request is authenticated by its signature, not by a nonce, because there is no browser session to carry one. Four files get an ignore instead of a fix because WordPress is not loaded in them at all, so wp_unslash() and the sanitizers do not exist: the browse-happy endpoint and its test page, the importers endpoint, and the Slack security team list. Their inputs are already constrained with preg_replace() or escaped with htmlspecialchars() on output. Renames two locals that shadowed WordPress globals, $comment in the Trac bot and $action in the themes API. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Clears WordPress.Security.ValidatedSanitizedInput across the support-forums plugin, the wptv2 theme and its bundled upload plugins, and the Rosetta contact page template. Values are unslashed and run through a sanitizer matched to what each one holds, rather than sanitize_text_field() across the board: sanitize_email() for addresses, sanitize_user() for usernames, esc_url_raw() for URIs whose percent-encoding sanitize_text_field() would strip, and sanitize_textarea_field() where line breaks are meaningful. Registers bbp_verify_nonce_request() as a nonce verification function in the ruleset. It wraps wp_verify_nonce() with an additional same-origin check, so the seven files that gate on it were verifying nonces that PHPCS could not see. The remaining nonce annotations record where bbPress or core verified the nonce upstream before the hook fired. Also replaces the contact form's validate_email(), deprecated since WordPress 3.0, with is_email(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reports page, list table, admin-post handlers, and menu UI read filter and routing values straight out of $_REQUEST. Each is now sanitized where it enters: sanitize_key() for the admin page slug, the action, and the SVN slug, which are all key-shaped and are matched against registered values anyway; absint() for revision numbers that only ever reach a %d placeholder; and sanitize_text_field() for the version, branch, author, and revision-range filters. The page slug was repeated across eight sinks in the reports page and two in the list table, so it is now read once into a local and reused, matching the other filter values already collected at the top of the function. Props usernames keep sanitize_text_field() rather than sanitize_user(), because find_user_id() also resolves a profiles.wordpress.org URL and sanitize_user() would strip it down to something unmatchable. The reports page escaping test asserted that a markup payload in the page slug reached the form escaped. It is now stripped on the way in instead, so the guard asserts the sanitized slug; the assertion it guards is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Rosetta roles screens and the Trac notifications plugin read user, project, and routing values out of $_REQUEST unsanitized. Each is now sanitized at the point it enters, with the sanitizer matched to the value: absint() for user and ticket IDs, sanitize_key() for actions, roles, order arguments, notice keys, and the Trac slug, and sanitize_text_field() for the search term and comma-separated project lists. The user lookup fields keep sanitize_text_field() rather than sanitize_user(), because both accept an email address as well as a login and sanitize_user() would strip it to something no lookup could match. The Trac HTTP server's request arguments stay raw: they are a JSON body that has to reach json_decode() as sent. The shared secret is likewise passed through unaltered and compared with hash_equals(), since sanitizing could change the bytes being compared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Post IDs become absint(), post types, statuses, and the queue-skip flags become sanitize_key(), and the moderation nonces are unslashed before wp_verify_nonce(). The contributor IP keeps filter_var() with FILTER_VALIDATE_IP, which already rejects anything that is not an address, and is only unslashed on the way in. The rejection reason fields now use sanitize_textarea_field(), which strips the tags the following wp_strip_all_tags() call used to take off, so that call is gone. Tag slugs are handed to sanitize_title() directly instead of being parked in an intermediate raw variable that nothing else read. The two upload checkbox guards spelled out "not set, or set but falsy", which is what empty() means; written that way they no longer read the unsanitized value at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Job tokens, categories, and the how-to-apply fields are sanitized where they enter, with each value matched to a sanitizer: absint() and sanitize_key() for IDs and key-shaped values, sanitize_title() for the category slug, and is_email()/esc_url_raw() for the how-to-apply address, which is checked as one or the other depending on the submitted method. Two values stay as submitted. create_job() hands the description and title to wp_insert_post(), which expects slashed data and filters the content through the content_save_pre filters the surrounding code swaps in, so unslashing them here would be wrong. validate_job_field() is a real sanitizer, but PHPCS only recognises global functions as such and this one is called statically, so the call is annotated instead. The remaining nonce annotations record where the nonce was already verified: save_job() calls check_admin_referer( 'jobswppostjob' ) before reaching any of the job creation code, and the Photo Directory upload checks run behind the Frontend Uploader plugin's fu_should_process_content_upload filter, which it applies only after verifying its own fu_nonce. Annotations that name a sniff are kept out of open phpcs:disable regions. A named ignore nested inside one does not take effect, which left a violation silently unreported while looking handled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sanitizes the remaining request reads across the public themes, wporg-learn, support-helphub and the meetings post type, matching the sanitizer to what each value holds: sanitize_key() for slugs and refinement keys, absint() for IDs, esc_url_raw() for URLs whose percent-encoding sanitize_text_field() would strip, and sanitize_text_field() elsewhere. Three of these were escaping on input rather than sanitizing. The meetings metabox stored its fields esc_textarea()'d and esc_url()'d and then escaped them again with esc_attr() when rendering the form back, so a team name containing an ampersand came back double-encoded; the values are now stored sanitized and escaped only on output. The HelpHub post type did the same with esc_url(), and its /* @codingStandardsIgnoreLine */ markers were doing nothing, both because PHP_CodeSniffer 3 dropped that syntax and because they sat on the line after the one they were meant to cover. The main site's search page sanitizes after urldecode() rather than around the raw value, since sanitize_text_field() strips percent-encoded octets and would empty out an encoded search term. The meetings plugin keeps its class inside an `if ( ! class_exists() ):` wrapper without the matching indent, so every line in the file reads as one level short. That is disabled at the top of the file with a note; phpcbf cannot fix it and reindenting the file does not belong in this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Clears the last WordPress.Security.ValidatedSanitizedInput violations: the SSO plugin, GlotPress routes and customizations, the bbPress version and resolution dropdowns, locale detection, the Rosetta network admin, the handbook, badge management, GitHub invites, the theme directory, and the smaller site plugins. Redirect targets and source URLs take esc_url_raw(), which keeps percent-encoding that sanitize_text_field() strips; IDs take absint(); slugs, actions and locales take sanitize_key(); everything else takes sanitize_text_field() or its textarea variant. The user importer's unmatched_authors field is an array keyed by old user ID rather than a block of text, so it is sanitized per element. A few reads stay raw on purpose. wporg-bad-request.php exists to reject malformed requests, so sanitizing the values it inspects would hide what it is looking for, and the theme upload hands the whole $_FILES entry to WPORG_Themes_Upload, which validates it before touching the file. WP Super Cache's advanced-cache.php drop-in on wp15.wordpress.net is excluded from the ruleset instead. It is third-party and runs before WordPress loads, so its sanitizers do not exist yet. The nonce annotations record where the nonce is verified upstream: core before save_post, bbPress before its topic hooks, and GlotPress's translations_post route, which verifies its add-translation nonce and returns a 403 before dispatching. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The role read added in trunk keeps the value exactly as core persists it and already carries an ignore saying so, but the annotation only named the sanitize sniff, leaving the unslash one reporting. Both are named now, which takes WordPress.Security.ValidatedSanitizedInput to zero across the repo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 178 files, which is 78 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (178)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
…itized-input # Conflicts: # api.wordpress.org/public_html/core/browse-happy/1.0/test.php # api.wordpress.org/public_html/core/serve-happy/1.0/index.php # api.wordpress.org/public_html/translations/plugins/1.0/index.php # api.wordpress.org/public_html/translations/themes/1.0/index.php # browsehappy.com/public_html/index.php # phpcs.xml.dist # wordpress.org/public_html/wp-content/themes/pub/wporg-login/admin/ui.php # wordpress.org/public_html/wp-content/themes/pub/wporg-login/pending-profile.php # wordpress.org/public_html/wp-content/themes/pub/wporg-showcase/functions.php
…ress loads The SSO class is reached from sunrise.php, which runs at wp-settings.php:161, before kses.php is loaded at 241. Every esc_*() helper routes through kses from there: esc_url() calls wp_kses_normalize_entities() directly, and esc_html() and esc_attr() reach it via _wp_specialchars(), which normalizes entities whenever $double_encode is false -- its default, and what both of those pass. That only fires for strings containing & < > " ', so it survives any test that boots WordPress normally and fatals in production on the first URL with a query string. REQUEST_URI and SCRIPT_NAME now go through wp_strip_all_tags(), which touches only preg_replace(), strip_tags() and trim(). _safe_redirect() guarded its esc_url() call on function_exists( 'esc_url' ), which is true at sunrise -- formatting.php loads at 115 -- so the htmlspecialchars() fallback beside it never ran. It now tests for wp_kses_normalize_entities() instead. The standalone API endpoints got the opposite correction: sanitizers were added to files that never load WordPress, where the calls would have been fatal. Those are back to raw reads with scoped ignores. Three more had the call ordered ahead of the bootstrap line that defines it. class-plugins-info-api.php stashes HTTP_HOST and REQUEST_URI to restore after wp-load.php, so sanitizing there would have changed what got put back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Where WordPress is never loaded, "its sanitizers are unavailable" is a property of the file, not of any line. These seven endpoints were repeating that sentence up to six times each, and none of them had a file doc comment at all, so the explanation was floating above the first statement with nothing to attach it to. Each now carries one file docblock holding the description, the reason and a phpcs:disable, which takes the annotation count from 24 to 5 and clears seven Squiz.Commenting.FileComment.Missing errors on the way. phpcs:disable works inside a docblock as long as it sits above the tag block -- anything after @Package reads as part of that tag to a doc parser. Package names follow whatever the neighbours already use: WordPressdotorg\API\Slack, WordPressdotorg\Plugin_Directory\Zip, trac. Notes that said something specific to the value -- that a pattern reduces it to digits and dots, that the JSON body has to reach json_decode() as Slack sent it -- stay as plain comments. subgroup.php and trac-notifications-http-server.php disable NonceVerification in the same annotation. Both authenticate every request by signature or shared secret, so nonce-absence is equally a file-level property; trunk already reported four uncovered NonceVerification.Missing errors in subgroup.php. Files that require wp-load.php partway through keep their scoped annotations: a file-level disable there would blind the sniff over the region after the bootstrap, where it is doing real work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…oints Same shape as the previous commit, applied to the endpoints that already sat on trunk: browse-happy (both the API and its debug page), serve-happy, the Composer notify-batch stub, and security-team.php, whose header already carried the pattern but disabled only the MissingUnslash half of the sniff. 23 annotations down to 13, and the copy-paste reason that claimed a JSONP callback was "restricted with preg_replace()" on a line reading HTTP_USER_AGENT is gone. Squiz.Commenting.FileComment.Missing keeps firing when the first statement after a file docblock is a require -- the sniff binds the comment to the include and then reports the file as uncommented. A single-line comment over the require separates them, which clears it in browse-happy's index.php and in parse.php, where it predated this branch. Left alone: class-composer-repository.php, which has two annotations in 529 lines and pulls in code that does load WordPress, and packages/p2 and themes/info/1.0, which require wp-load.php partway through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
obenland
force-pushed
the
update/validated-sanitized-input
branch
from
September 12, 2026 01:14
af85bb6 to
5cbbef2
Compare
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sanitize_textarea_field() strips tags, and this is a free-text message a visitor types. Anything containing "<" -- "a < b", or an address written as <user@example.org> -- was being silently truncated at four separate points: the required-field check, both re-render paths, and the payload Akismet scores. The message is now read once, as typed, and reused. Nothing depended on that sanitizer for safety: the email body still runs it through wp_kses() with an empty allowlist, Akismet urlencodes it into a query string it never renders, and both echoes are esc_textarea(). The empty check keeps rejecting whitespace-only messages via trim(), now with a strict comparison. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…es it left Ten of the twelve points applied; the readme-slug and consistency-search ones were already fixed in 21a5ef1. Broken behaviour: - themes/info/1.0 called wp_unslash() and sanitize_text_field() above load_wordpress() on line 140. send_error() is reached from seven call sites, all of them earlier, so every error path was a fatal, and unslashing a serialized payload no magic quotes had touched stripped real backslashes out of it and left the byte-length prefixes lying. Back to raw reads. - class-customizations.php ran wp_kses_post() unconditionally, ahead of the unfiltered_html check that decides whether kses applies at all. - jobswp.php unslashed a value add_post_meta() unslashes again, and handed validate_job_field() an unslashed value beside a still-slashed $_POST. - trac/oembed unslashed twice, the second time behind an is_string() test that esc_url_raw() had already made unreachable. Silently rewritten input: - register.php used sanitize_user()/sanitize_email() where the form means to validate: an accented name registered as its stripped form with no error shown. - wporg-redirects.php and the search term: sanitize_text_field() eats %XX, so ?s=100%ab redirected to /search/100/. urlencode() on the same line is what makes it safe. Paired checks that disagreed: - class-plugin-posts.php tested the raw post_status and indexed with the sanitized one. - trac-notifications.php inserted from sanitized keys and deleted by re-reading raw $_POST, so an altered value was inserted and deleted in one request. Both loops now read the same normalized list. - class-author-notice.php passed an array to wp_kses_post() and subscripted the result; set() already runs wp_kses() and sanitize_key() itself. Also removes five phpcbf scratch files committed at the repo root, and ignores that pattern so they cannot come back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…placed them Moving the inline ignores into file docblocks left the `?? ''` fallbacks and the odd blank line behind, none of which the sniff still asks for. Reverted to what trunk had wherever the index is already guaranteed: $_REQUEST['version'] inside its own isset(), $_GET['version'] inside a ! empty(), $_GET['locale'] on a branch only reachable when it is set, $_POST['payload'] inside an isset(), and SERVER_PROTOCOL / REQUEST_METHOD / REMOTE_ADDR, which the web SAPI always sets and which trunk has relied on for years. Kept the three HTTP_USER_AGENT fallbacks in the events API. That header is genuinely optional, and without them a request that omits it reaches str_starts_with( null, ... ), whose PHP 8.1 deprecation notice would be printed into the JSON response body. themes/info/1.0 had one more of the ordering bug fixed in the previous commit: sanitize_key() on line 89, 54 lines above load_wordpress(). The $api_action rename stays -- it is local to the file and wporg_themes_query_api() takes the value as a parameter -- but the sanitizer is gone. Also checked the rest of the directory: every file this branch adds a WordPress function to does load WordPress first, counting helpscout/webhook.php, which gets there through common.php. The $HTTP_RAW_POST_DATA and $comment renames stay; both silence errors that trunk already had on those lines, and the first is a global PHP removed in 7.0 that is only ever read inside its own file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rfaced phpcs-branch.php ran phpcs with -n at all three call sites, which is the shortcut for --warning-severity=0. Every warning-severity sniff was therefore invisible to CI -- including Universal.Operators.StrictComparisons, which is why loose comparisons have been landing on reviewed lines unchallenged. Dropping the n leaves -sq: still showing sniff codes, still quiet. With warnings on, this branch's changed lines reported 231. 163 of those were WordPress.Security.NonceVerification.Recommended, which fires on reading $_GET in a page that also handles a form. Most of this estate is read-only endpoints and admin list tables filtering by query string, where there is no nonce to check, so at that volume it hides everything else; it is now severity 0. NonceVerification .Missing, which fires on $_POST and is the one that matters, stays an error. The remaining 68 are fixed rather than silenced: 17 loose comparisons where both operands are provably strings, 8 parse_url() calls swapped for wp_parse_url(), 31 assignments realigned, 3 urlencode() to rawurlencode() where the value goes into a path, and 3 in_array() calls given their missing true. Four are annotated instead, because the sniff is wrong about them: a lowercase hostname regex read as a misspelling of the product name, a base64_decode() that decodes a readme the form posted, unreachable code in a file that exists to document usage, and the one parse_url() in the zip server, which runs outside WordPress where wp_parse_url() does not exist. Two things needed more than a mechanical swap. The comparison in class-plugin-directory.php takes parse_url()'s path, which is null for a URL without one, so it casts to string before comparing strictly -- null != '' was false and null !== '' would not have been. And the add_action() call at the end of wporg-login/admin/ui.php is reformatted rather than annotated: adding the missing trailing newline made its closing line ours, and the sniff wants the whole call laid out, not just that line. Also removes another phpcbf scratch file from the repo root and ignores the pattern there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e they replaced
Seven findings, all confirmed.
The SSO one is the same sunrise fatal twice over. sanitize_text_field() looks
self-contained, but once the value holds a "<" it calls wp_pre_kses_less_than(),
whose callback calls esc_html() -> _wp_specialchars() -> wp_kses_normalize_entities(),
which is not defined until kses.php loads well after sunrise.php. Host is
attacker-controlled, so "Host: <" was enough. HTTP_HOST now uses wp_strip_all_tags()
like the REQUEST_URI and SCRIPT_NAME lines beside it. I had accepted the opposite
about sanitize_text_field() earlier in this branch; it was wrong.
Badge management: tab keys are built as 'list_users:{slug}', and sanitize_key()
strips the colon, so no per-badge listing could ever match and every one fell back
to the first tab. The array_key_exists() check on the next line is the real
validation, so the value is read as submitted.
Four places lost their array guard ahead of a sanitizer that assumes a string:
esc_url_raw() ltrim()s and sanitize_email() strlen()s, both TypeErrors on array
input in PHP 8. Restored on the trac oembed endpoint, the SSO login, sso_token and
logout paths, and the Rosetta contact form. The contact form also validates what
was typed again: running sanitize_email() first stripped a non-ASCII domain and
let the stripped address through is_email(), so the visitor saw no error and the
mail went somewhere else.
update_post_meta() unslashes what it is given, so the values this branch started
unslashing lost a literal backslash on save. Re-slashed at the write in
wporg-meeting-posttype, rosetta-showcase, wporg-bbp-topic-resolution,
wporg-make-sites-cpt, wptv2 and support-helphub. My own audit for this missed
them: it traced wp_insert_post(), wp_new_comment() and wp_update_user() but not
the *_post_meta family, which has the same contract.
Last one is a dead branch the sweep touched but did not cause: get_query_var()
returns '' rather than null for a missing var, so ?? never reached the
favorites_user fallback. Now ?:.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tream Seven findings, all confirmed. Three are sanitize_text_field() eating %XX octets from a value that is a URL or goes into one. The came_from cookie took its redirect through it while the branch directly beside it used esc_url_raw(), so ?redirect_to=...?s=a%2Fb was stored corrupted; it now matches its sibling. The legacy /search/ redirect in the plugin directory has the same problem and is handled the way wporg-redirects.php already handles its search term -- raw, annotated, with rawurlencode() doing the escaping. The trackback guard was the sharpest of these. It exists to reject malformed trackbacks before core's mb_convert_encoding() falls over (Core #60261), which means deriving the charset exactly as wp-trackback.php does -- from the raw value. Sanitizing first normalised charset=<b>UTF-8</b> to UTF-8, passed mb_list_encodings(), and let core have the unsanitised string: precisely the request the guard blocks. It also disagreed with this file's own policy, which leaves queryVars and share-by-email raw for the same reason. Two more meta writers that unslash internally, missed when I swept for these last round because I only searched the *_post_meta family by name: update_user_option() in the forums moderator title, and one update_post_meta() in wporg-learn. The forum search themes unslash the query on the way in now, but kept a pre-existing stripslashes() a couple of lines below, which had been the unslash and is now just eating real backslashes. Dropped in both themes. buddypress.org gated admin-ajax on sanitize_key( action ) while admin-ajax.php dispatches on the raw action and the in_array() beside it also used the raw value, so the two halves of one condition disagreed and any nopriv action with an uppercase letter or a dot was redirected instead of run. Both halves now read the same value. Last, sanitize_user() runs remove_accents(), so the prefilled login field, the "username %s is not registered" error and the WordPress.tv producer field all showed jose to someone who typed josé. register.php already avoids it for exactly this reason; these three now match. Every one of them is escaped on output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…as built from Eight findings. Seven are code; the eighth belongs in the PR body. The pending-registrations screen was the real one. Its list table mints resend_/clear_/block_/delete_ nonces from the raw stored user_email, and wporg_get_pending_user() keys on that same string, so running the action parameter through sanitize_email() first broke both halves at once. An IDN or single-label domain, a trailing dot, an unusual local part -- user@munchen.de for user@münchen.de -- and neither the nonce nor the lookup matches; a malformed address becomes '' outright. That population is exactly what the screen exists to moderate. All four actions, and the user_login one beside them, now unslash only. While in there: the block handler tested $user after line 545 had already proved it non-empty, so a failed lookup fell through and dereferenced null. It tests $pending_user now. Two more double-unslashes. The Akismet loop in the contact form still applied stripslashes() to fields this branch had already unslashed at assignment, so the author email and URL reached the spam check unslashed twice. And the forums email check compared $user_email against the raw slashed $_POST['email'] while the two conditions beside it used the sanitized form, so any address containing a quote made the halves disagree; it reads one value now. The legacy /search/ redirect in wporg-redirects.php keeps urlencode() where its twin in class-plugin-directory.php moved to rawurlencode(); both are paths, so both use rawurlencode() now. The strict in_array() I added to the HelpHub select whitelist compares a string against array_keys(), and PHP hands back ints for integer-like keys, so a numeric option would never validate. It compares against strval()ed keys instead. Latent -- no such field exists in-tree -- but it was mine to begin with. Last, the Trac oembed api_key is an opaque credential forwarded verbatim, and sanitize_text_field() strips %XX octets out of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four, all collateral from the review fixes rather than anything new. The block comment in buddypress-dot-org.php landed directly under a pre-existing two-line // comment, which the sniff reads as a missing blank line; the two are merged into one block, which also retires a stacked // pair. contact.php reads the submitted address as typed and had no sanitizer for the sniff to see, so it now carries the reason as an annotation. Its Akismet query string moves to rawurlencode(), which the warning has been asking for since the stripslashes() removal put that line in the diff. make_sites_cpt.php lines up the third argument of three update_post_meta() calls; adding wp_slash() to two of them made that padding mine, so the block uses single spaces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The phpcs:ignore reasons had drifted into recounting the code they replaced -- "sanitize_text_field() strips %XX octets", "sanitize_email() rewrites IDN domains, so neither would match" -- which reads as a changelog and stops being true the moment someone rewrites the line. They state the constraint on the value instead: the search term keeps its percent-encoding, the nonce and the lookup key on the raw stored user_email, admin-ajax.php dispatches on the raw action. Two block comments folded into the annotation they sat above, so the reason travels with the directive rather than a few lines away from it, and the comment above the admin-ajax read is one line with a blank after it rather than a stacked pair. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-one phpcs:ignore and phpcs:disable lines carried no reason of their own and leaned on a block comment sitting above them. That separates the directive from its justification, and the two drift: delete or move one and the other silently stops making sense. Each reason now travels on the line it explains. File docblocks are left alone -- the contact form's file-level nonce disable still sits under its header, where the explanation is the header. The one genuine code comment caught up in this, "Prevent recursion." in trac-bot.php, stays a comment rather than becoming part of a lint annotation. The contact form's two reasons are also reworded to say what the value needs rather than what sanitizing it would have cost, matching the rest of the branch, and one of them no longer contains a "--" of its own inside a "--" reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Swapping sanitize_text_field() for esc_url_raw() on the login redirects took away the one thing the code above them says it relies on. The comments still read "Make sure value is a string", but esc_url_raw() opens with ltrim(), so ?redirect_to[]=x reached it as an array and fataled -- and wporg_remember_where_user_came_from() runs on init, so that was every request to login.wordpress.org, not a code path someone had to find. Guarded there and in wporg_login_wporg_is_starpress(), where from and redirect_to have the same shape. The upload-token tool looks its value up as a login, then an email, then a slug, so sanitize_user() was the wrong sanitizer for it: it strips accents and %XX, and josé@example.com matched nothing or the wrong account. It uses sanitize_text_field(), which is what the rest of this branch uses for lookup fields. The events number is more interesting than it looks. Trunk passed the raw value into max( 0, min( $number, 100 ) ), and PHP 8 compares a non-numeric string against 100 as a string, so ?number=abc clamped to 100. Casting it to int made that 0, which is a real value downstream and returns no events at all. It is now only set when the value is numeric, so garbage falls through to the documented default of 10 rather than either accident. Events suite still passes, 28 tests, 75 assertions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ntact Both are values this branch sanitized where the code needs them byte-for-byte. The upload-token tool resolves its input as a login, then an email, then a slug. sanitize_user() was wrong for it and sanitize_text_field(), which I swapped in last round, is wrong for the same reason a level down: it strips %XX, so the valid address person%ab@example.org becomes person@example.org. If that second account exists the token is issued against it, and if it does not the intended recipient cannot be found at all. It reads the value as submitted now, with a type guard, and it was already escaped where it is echoed. The BuddyPress search redirect has the same shape. Trunk passed the term through stripslashes() and handed it to add_query_arg(); sanitizing it stripped tags, so s=<iframe> redirected with an empty query and mixed queries quietly lost their HTML terms -- which are the point of a search on a technical site. add_query_arg() encodes the value for the URL, so there is nothing for a sanitizer to add here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The milestone route accepts one optional space in the name, written as a literal space, which worked while the URL arrived raw. esc_url_raw() turns that space into %20, so https://core.trac.wordpress.org/milestone/Awaiting Review stopped matching and came back 404 where it used to embed. The pattern takes either form now. Its shape is unchanged otherwise -- still one optional separator, so a two-space name is rejected exactly as before -- and the host allowlist and the rest of the route restrictions are untouched. Checked against Awaiting Review in both spellings, Future Release, a numeric milestone, a two-space name and a foreign host. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four of the seven. The contact form's type guard passed when the field was absent and the branch it guards then read $_POST['message'] with no default, so a request without the field warned on an undefined index. It tests isset() first, like the others. pre_user_url is a core filter that takes the slashed value; wp_filter_kses() sits on it and unslashes and re-slashes internally. Handing it an unslashed URL made a literal backslash disappear. It gets the value as submitted again, with a type guard, and the sniff annotation moves onto the line. submitted_name on a plugin upload records what the author actually sent, so sanitize_file_name() does not belong on the value stored there. The filename built from it still gets sanitized; the two are separate variables now, and the meta is re-slashed because update_post_meta() unslashes. The WordPress.tv event meta sanitized the same value twice; the second call went. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Leaving the value slashed for the filter fires MissingUnslash as well as InputNotSanitized, and the annotation named only the second. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The block plugin checker accepts a bare plugin slug and a git@github.com: address as well as a URL, and normalizes them itself; esc_url_raw() turned the first into http://hello-dolly and dropped the second entirely. prop_name_orig addresses an existing props row by the name the log parser took verbatim from the commit message. Sanitizing the lookup made edit and delete silently miss any row whose name carries the markup an administrator opened the screen to fix. The bb-base search helpers feed the plugins API and the bbPress query, so the term is searched for as typed. sanitize_text_field() emptied searches like <iframe> or %20, which turned them into an unfiltered listing and suppressed the no-results notice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lookup wporg-support-2024 carries its own bb_base_*_search_query() functions, so the fix to the BuddyPress base theme left the support forums still erasing searches like <iframe> or %20. They also lacked the scalar guard the other copies now have. find_user_id() resolves a login, a nicename, an email address, a profiles URL or a user ID, and a percent sign is legal in the local part of an address, so the identifier has to reach it exactly as typed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A repairing sanitizer in front of a validator moves the check onto a string nothing downstream sees. On the pending-profile form that was the worst of it: sanitize_email() turned a mistyped address into a different valid one, which the handler then saved, mailed the confirmation to, and charged against the single email correction a pending account gets. The same shape appears wherever a submitted identifier is checked and then stored or matched: the anonymous upload form validated a repaired address and username while recording the posted ones, the registrations list table ran an exact user_email query against a term with its percent-escapes stripped, and the forum profile handler screened a repaired address while bbPress went on to save the original. Percent signs are legal in the local part of an address, which is why the search terms and the user lookups have to keep them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…uard wp_kses_post() on the edited reply content made the ?: fallback reachable for input that was not empty to begin with, and the fallback dereferences bbp_get_reply( 0 ), which is null. The content is only tested for a block markup prefix, so the sanitizer was buying nothing; the fallback is now null-safe either way. esc_url_raw() percent-encoded the spaces in the site URL field, so no multi-word nsfw term could match it any more. It is free text a visitor typed and it is only ever searched, so it gets the same treatment as the tags field beside it. The Share By Email guard tested presence in $_POST and type in $_REQUEST, which are not the same array, and the annotation this PR put on it claimed otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…matching is_string( $_POST['x'] ?? '' ) is true when the field is absent, and the branch it guards then reads the key anyway. That is an undefined-key warning on every ordinary submission that omits the field, and the point of the guard was the opposite. Nine of these, all from this PR. The reviews link check has to run over the content bbPress is about to store. bbp_new_topic_pre_content takes the slashed value and bbp_filter_kses() on that hook unslashes and re-slashes it itself, so the read goes back to matching what bbPress passes; kses() ahead of the filters was also eating <https://example.org> before the check could see it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…anitized-input # Conflicts: # common/includes/wporg-sso/wp-plugin.php
…tion sweep Four follow-ups to the ValidatedSanitizedInput sweep: * Readme validator: drop `sanitize_text_field()` from around the posted base64 blob. `base64_decode()` in strict mode is already the validation for that field, and stripping tags first turned input the decoder would have rejected into input it accepts. * Trac oEmbed: the milestone pattern's literal-space branch is unreachable now that `esc_url_raw()` runs over the URL first - it rewrites any space to `%20`. * Plugins info API example: re-enable `Squiz.PHP.NonExecutableCode` at the end of the block so the rest of the file keeps its baseline. * Plugin scan: `$_ENV['PATH']` is process environment, not request input, so annotate it rather than run a text sanitizer over a value that has to be passed through verbatim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`esc_url_raw()` passes its argument to `ltrim()`, which is a TypeError on an array in PHP 8. The contact form's `blog_url` and the login language switcher's `redirect_to` reached it without a string check, so a request like `blog_url[]=x` fataled where it previously only emitted a notice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Clears
WordPress.Security.ValidatedSanitizedInputrepo-wide: 602 findings to 0, across 172 files.Based on current trunk. Unlike escaping, sanitizing changes the value, so the sanitizer is chosen per value rather than applied uniformly —
sanitize_text_field()everywhere would have quietly broken several of these.What changed
Request reads are unslashed and sanitized where they enter, matched to what each value holds:
absint()for IDs that only ever reach a%dplaceholder,sanitize_key()for slugs, actions and order arguments that are matched against registered values anyway,sanitize_email()/sanitize_user()/sanitize_textarea_field()where those fit, andesc_url_raw()for URIs —sanitize_text_field()strips%XXoctets in a loop, so it destroys percent-encoded URLs.Where a value was read at many sinks in one function — the Trac Watcher reports page had the admin page slug at eight — it is now read once into a local and reused, matching the filter values already collected alongside it.
What deliberately did not
trac-notifications-http-server.phpargumentsjson_decode()hash_equals(); sanitizing changes the bytes being comparedjobswp.phpdescription and titlewp_insert_post()expects slashed data and runs its owncontent_save_prefilterswporg-bad-request.phpinspected valuestheme-directory/upload.php$_FILESentryWPORG_Themes_Upload, which validates before touching the filewp15.wordpress.net'sadvanced-cache.phpis excluded in the ruleset instead — it is WP Super Cache's drop-in, third-party and running before WordPress loads, so its sanitizers do not exist yet.Bugs this turned up
class-locale-associations.phpvalidated the wrong field.action_add_association()checkedempty( $_POST['locale'] )twice; the second was meant to besubdomain, so a missing subdomain reached the insert unchecked.esc_textarea()'d andesc_url()'d, then escaped again withesc_attr()when the form rendered them back, so a team name containing&returned as&amp;. Input now getssanitize_*, output keepsesc_*./* @codingStandardsIgnoreLine */markers were inert. PHP_CodeSniffer 3 dropped that syntax, and they sat on the line after the one they were meant to cover — so those reads were both unsanitized and invisible.wporg-learn/inc/capabilities.phphad a half-covering annotation. The role read added in trunk names only the sanitize sniff, leaving the unslash one reporting; both are named now.Decisions worth a look
bbp_verify_nonce_requestis registered in the ruleset, not annotated away. It wrapswp_verify_nonce()with an added same-origin check, so seven files were verifying nonces PHPCS simply could not see. One entry cleared all of them.ContextHelper::is_in_function_call()rejects anything preceded by::or->, socustomSanitizingFunctionscannot reach$this->sanitize_wp_version(). Wrapping the superglobal in a global sanitizer first is honest and needs no annotation.jobswp'sself::validate_job_field()is the one place that could not be expressed that way and carries a scoped ignore saying why.sanitize_text_field(), notsanitize_user().find_user_id()and the Rosetta role screens also accept an email address or aprofiles.wordpress.orgURL;sanitize_user()would strip those to something no lookup could match.urldecode(). Around the raw value,sanitize_text_field()would strip the encoding before it is decoded and empty out the term. The sniff cannot see a sanitizer in that position, so the line is annotated with that reason.ScopeIndentdisable. Its class sits insideif ( ! class_exists() ):without the matching indent, so all 280 indented lines read one level short.phpcbfcannot fix it, and reindenting the file does not belong in this change.Two gaps I did not close
Both are NonceVerification territory rather than this sniff, so I annotated rather than changed behaviour, but they are worth someone's judgement:
global.wordpress.org/.../rosetta/contact.phphas no CSRF nonce at all — only! empty( $_POST['submit'] )gates the email send. I annotated it on the grounds that every logged-out visitor resolves the same nonce value so a nonce would not establish intent, with Akismet as the check that is actually there.jobswp.php:25only verifies the upload nonce whenwptv_uploaded_byis empty. A non-empty value skipswp_verify_nonce()and merely requires the field to be present.Two ruleset changes, and what they trade away
.github/bin/phpcs-branch.phpran phpcs with-nat all three call sites, the shortcut for--warning-severity=0, so every warning-severity sniff was invisible to CI. That is why loose comparisons have been landing on reviewed lines unchallenged. It now runs-sq.Turning warnings on surfaced 231 on this branch's changed lines, 163 of them
WordPress.Security.NonceVerification.Recommended, so that code is set to severity 0 inphpcs.xml.dist. This is a deliberate trade and it is not free:.Recommendedfires wherever$_GETor$_REQUESTis read on a page that also processes a form, and while most of this estate is read-only endpoints and admin list tables filtering by query string, it will also now stay quiet on the state-changing$_GEThandlers this codebase does have..Missing, which fires on$_POSTand is the one that matters, stays an error. If the nonce batch wants that signal back, remove the rule and take the 163.The remaining 68 warnings are fixed rather than silenced — 17 loose comparisons, 8
parse_url()towp_parse_url(), 31 realigned assignments, 3urlencode()torawurlencode(), 3 missingin_array()strictness — with four annotated where the sniff is wrong about them.Validation
With
BASE_REFset to trunk, the repository changed-line PHPCS check passes with zero violations across all 172 modified files, and every changed file passesphp -l. The fivemakeenvironment suites pass — Trac Watcher 18, Trac Notifications 36, o2 Posting Access 61, Badge Management 9, WP-CLI 66.I also confirmed the annotations actually suppress rather than being no-ops: a named
phpcs:ignorenested inside an openphpcs:disableregion silently does nothing, which had left one violation reported while looking handled. Control tests with the annotation stripped verified each remaining one, and the branch is checked for stacked//comments and unclosed disable regions.One test changed.
WPorg_Trac_Watcher_Reports_Page_Test::test_report_links_escape_a_page_from_the_requestasserted that a markup payload in the page slug reached the form escaped;sanitize_key()now strips it on the way in, so the guard asserts the sanitized slug. The assertion it guards — that no<imgreaches the output — is unchanged.🤖 Generated with Claude Code