Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove all functions from the auto escape list that don't return a value #1349

Closed
wants to merge 1 commit into from
Closed

Remove all functions from the auto escape list that don't return a value #1349

wants to merge 1 commit into from

Conversation

johnbillion
Copy link
Member

Fixes #1348

@jrfnl jrfnl mentioned this pull request Jun 25, 2018
10 tasks
@GaryJones
Copy link
Member

Has a merge conflict.

@NielsdeBlaauw
Copy link
Contributor

I've reviewed the functions removed in the current pull request. I agree with all functions removed in 1ffc427

I've also gone through the list of remaining items in the 'autoEscapedFunctions' property.
I've found the follow functions that should be reconsidered for this pull-request:

  • bloginfo - only echoes, no return
  • body_class - only echoes, no return
  • get_search_query - with improper usage this function might not escape
  • post_type_archive_title - prefix does not actually seem to be escaped
  • single_cat_title - prefix does not actually seem to be escaped
  • single_month_title - prefix does not actually seem to be escaped
  • single_post_title - prefix does not actually seem to be escaped
  • single_tag_title - prefix does not actually seem to be escaped
  • the_date - first parameter is not escaped, which might be vulnerable.
  • vip_powered_wpcom - Seems to be exclusively part of VIP. Does this touch the Deprecate VIP ruleset, and remove from WordPress ruleset #1309 issue?

wp_list_*, wp_nav_menu, wp_register functions don't escape some parts, but those are on purpose. However, they don't filter out dangerous html properties.

@jrfnl
Copy link
Member

jrfnl commented Aug 30, 2018

@johnbillion Thank you for the PR. Would you mind rebasing it on the current develop to make it mergable again ?

@NielsdeBlaauw Thank you for your detailed review & I look forward to your pull request for your other finds!

@jrfnl
Copy link
Member

jrfnl commented Oct 31, 2018

@johnbillion Just checking - did you see my previous message asking to rebase ?

@GaryJones
Copy link
Member

Closing in favour of #1547 since @johnbillion hasn't fixed the merge conflict.

@GaryJones GaryJones closed this Dec 16, 2018
@GaryJones
Copy link
Member

@NielsdeBlaauw Sorry - I missed your comment. If you want to open a PR with your recommended additional changes here, then we can take a look :-) Thanks!

@jrfnl jrfnl added this to the 1.x Next Release milestone Dec 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants