Skip to content

Escape URLs interpolated into markup - #814

Open
obenland wants to merge 13 commits into
WordPress:trunkfrom
obenland:trac/escape-url-output
Open

Escape URLs interpolated into markup#814
obenland wants to merge 13 commits into
WordPress:trunkfrom
obenland:trac/escape-url-output

Conversation

@obenland

@obenland obenland commented Aug 18, 2026

Copy link
Copy Markdown
Member

Escapes URLs on their way into markup, clearing 541 WordPress.Security.EscapeOutput violations (3,030 → 2,489).

Where a wrap put a line into the diff, the other violations the changed-lines linter reports for that line are fixed alongside it, which is what most of the non-esc_url() changes are.

Commit by commit:

  1. Wraps URL-returning function calls in esc_url(), and normalises call spacing and statement semicolons on the lines involved.
  2. Swaps get_stylesheet_directory_uri() concatenation for get_theme_file_uri() in the three themes it appeared in. Two of them are children of bb-base, and get_theme_file_uri() falls back to the parent when the child has no file of that name, so every asset they reference was checked to exist in the child directory first. The output is unchanged. Query strings stay outside the call so the file lookup still matches.
  3. Registers wporg_login_wordpress_url() as pre-escaped. It returns esc_url( $url ) already, so a ruleset entry suits it better than a second wrap at each of its nine call sites.
  4. Covers two files whose URLs sit on lines carrying unrelated violations. Browse Happy loses its IE 9 and older conditional markup, the PNG fix script that only ran on IE 6, and that script's now unused asset. The bb-base nav sets its current class with a ternary rather than an inline conditional block.
  5. Handles the URLs that reach output through a variable. Each was traced back to its assignment first. In the showcase home template the value was already escaped when assigned, which doubled as the validity check, so validation moves to esc_url_raw() and both the link and the displayed text are escaped where they are output.
  6. get_target_url() appended REQUEST_URI to the redirect target without sanitising it, and printed the result into an HTML comment when redirects are off.
  7. Escapes variables holding prebuilt anchors with wp_kses_post() rather than esc_url(). The notable users list turned out to hold plain URLs despite its name, and takes esc_url().
  8. The rest: forum permalinks inside a translated link, a host component that wants esc_html(), and a core function that assembles its own escaped markup, which is left unwrapped. The bounced emails script writes a terminal report, so the sniff is turned off for that file.
  9. Escapes plugin tags and handbook titles where they are assembled rather than at the outlet, so markup no longer survives in them.
  10. The link functions returning a plain URL that the first sweep did not match on name.
  11. Review follow-ups: the .ie6 rules Browse Happy no longer has a class for, esc_url_raw() on the Openverse debug comment so it matches the URL the redirect uses, the misnamed guard variable in the review notice, a string cast where get_edit_post_link() can return null, and get_sample_permalink_html() moved from the ruleset to a scoped phpcs:ignore at its one call site.

Left for follow-ups:

  • Functions like gp_link_get() and the bbPress author links return markup, so they want wp_kses_post() or a ruleset entry once each has been checked.
  • Exception messages are untouched. There are 45 of them and none currently escape, so that is one decision rather than a few.

🤖 Generated with Claude Code

obenland and others added 2 commits August 17, 2026 19:10
Wrap URL-returning function calls in esc_url() across templates and plugin
files, and normalize call spacing and statement semicolons on the lines
involved so the changed-lines linter stays clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces get_stylesheet_directory_uri() concatenation. Query strings stay
outside the call so the child-theme file lookup still matches a real file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 18, 2026 02:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props obenland.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

The function returns esc_url( $url ), so its call sites were being flagged
for output that is already escaped. Registering it stops nine wrapped-twice
call sites rather than adding a redundant esc_url() to each.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@obenland
obenland force-pushed the trac/escape-url-output branch 2 times, most recently from 907e64c to 15094e0 Compare August 18, 2026 02:29
Drops the IE 9 and older conditional markup from the Browse Happy template,
along with the PNG fix script it loaded and that script's now unused asset,
and marks the file as loading its assets inline. Rewrites the bb-base nav
highlighting as ternaries so the current class no longer needs an inline
conditional block.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@obenland
obenland force-pushed the trac/escape-url-output branch from 15094e0 to cf85f83 Compare August 18, 2026 02:32
obenland and others added 9 commits August 17, 2026 19:47
Covers the call sites where the URL reaches output through a variable rather
than a function call, so the sniff cannot see where it came from. Each one was
traced back to its assignment first; the variables that hold prebuilt anchors
rather than URLs are left for a separate pass.

In the showcase home template the value was already escaped when assigned,
which also served as the validity check. Validation moves to esc_url_raw() so
that both the link and the displayed text can be escaped where they are output.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
get_target_url() appended REQUEST_URI to the target without sanitizing it,
and the result was printed into an HTML comment unescaped when redirects are
turned off. Sanitize the path where it is read, and escape the URL where it
is printed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
These hold anchors assembled earlier rather than plain URLs, so they need
wp_kses_post() rather than esc_url(). Each variable was traced back to what
builds it; the notable users list on the front page turned out to hold plain
URLs despite its name, and takes esc_url() instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Covers what was left after the function and variable passes: forum permalinks
built into a translated link, a host component that wants esc_html() rather
than esc_url(), and a core function that assembles its own escaped markup,
which is registered as pre-escaped instead of being wrapped.

The bounced emails script writes a terminal report rather than markup, so the
escaping sniff is turned off for that file rather than escaping its output.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tag list and the breadcrumb trail escaped their URLs but not the names
and titles placed next to them, so those values reached the page as written.
wp_kses_post() on the assembled string kept scripts out but still let markup
through; escaping each value as it goes in settles it at the source.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rounds out the earlier passes with the link functions that return a plain URL
rather than markup, which the first sweep did not match on name.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Browse Happy loses the .ie6 rules that were left behind when the
conditional markup and the PNG fix script went away.

Openverse prints the redirect target with esc_url_raw(), so the debug
comment shows the URL the redirect actually uses rather than one with
its ampersands entity encoded.

Support Forums initialises $edit_url; the guard set an $edit_link that
nothing reads, leaving the escaped variable undefined for any compat
other than plugin or theme.

get_edit_post_link() returns null when the viewer cannot edit that post,
so its callers cast to string before escaping.

get_sample_permalink_html() moves out of the ruleset and into a scoped
phpcs:ignore at its one call site.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Apply late escaping in the wptv2 theme templates by wrapping the header search form action URL with `esc_url()` and sanitizing the rendered OGG link with `wp_kses_post()`. This hardens output handling and avoids printing unescaped link data in single-video sidebar metadata.
The changed-lines linter flags the initialiser once the line is touched,
since Squiz.PHP.DisallowMultipleAssignments wants one assignment per line.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants