Make WordPress Core

Opened 18 months ago

Closed 4 months ago

Last modified 3 months ago

#57336 closed defect (bug) (fixed)

Escape missing URLs and HTML element content in wp-activate.php

Reported by: rafiq91's profile rafiq91 Owned by: rajinsharwar's profile rajinsharwar
Milestone: 6.5 Priority: normal
Severity: major Version:
Component: Networks and Sites Keywords: has-patch has-testing-info needs-testing
Focuses: multisite Cc:

Description (last modified by rajinsharwar)

In the wp-activate.php file,

  1. We have some unescaped instances of the "network_site_url()" function.
  2. We have some unescaped URLs being used.
  3. We have some unescaped HTML element content.

This ticket escapes the missing unescaped instances.

Change History (18)

#2 @rajinsharwar
10 months ago

  • Keywords needs-dev-note added

Following on the feedback of the comment here: https://github.com/WordPress/wordpress-develop/pull/3771#discussion_r1050136612

Is it good to be closed @costdev?

#3 @costdev
10 months ago

  • Keywords 2nd-opinion added; needs-dev-note removed

Hi @rajinsharwar, thanks for the ping! I believe this should remain open as the PR covers some instances in this file, but not all. I'd also like to check out some of the history in this file and see why escaping isn't there already, as there may be valid reasons/decisions made about this.

#4 @rajinsharwar
9 months ago

  • Milestone changed from Awaiting Review to 6.4
  • Owner changed from rafiq91 to rajinsharwar

Hi @costdev, I have checked the historical data of those lines but didn't find any strong reason for not escaping. Here are the references:

https://core.trac.wordpress.org/changeset/44021#file0
https://core.trac.wordpress.org/changeset/12605/trunk/wp-activate.php
https://core.trac.wordpress.org/ticket/11644

Taking ownership of the ticket, and placing this for 6.4

This ticket was mentioned in PR #5046 on WordPress/wordpress-develop by @rajinsharwar.


9 months ago
#5

Escaping the URLs in the network_site_url function in the file of wp-activate.php

Trac ticket: https://core.trac.wordpress.org/ticket/57336

#6 @costdev
9 months ago

  • Component changed from Login and Registration to Networks and Sites
  • Focuses multisite added; privacy removed
  • Keywords 2nd-opinion removed

Thanks @rajinsharwar!

Indeed, escaping was intended - see the second item in this comment - but looks like it wasn't added.

Most other uses of network_site_url() are escaped in Core, and I don't see why these cases would be an exception.

I've left a comment on PR 5046 about some additional URL escaping needed, and whether we should expand the scope of this ticket and PR to handle other escaping in this file.


  • Updating the component to Networks and Sites and adding multisite focus, as this file is specifically for Multisite.
  • Removing privacy as the proposed change does not affect privacy.

#7 @rajinsharwar
9 months ago

Great thanks @costdev. Suggestion: Should we modify this ticket's title and description to include all escaping changes to this file? I guess that will be better for tracking purposes instead of splitting the task. What do you think?

#8 @costdev
9 months ago

@rajinsharwar I agree that one ticket makes sense for these changes, and the ticket title/description can be updated. Separate commits can be made if that's preferred by the final committer.

Last edited 9 months ago by costdev (previous) (diff)

#9 @oglekler
9 months ago

  • Keywords needs-testing added

#10 @oglekler
8 months ago

I cannot find any possible side effects from this, but I didn't find anything for wp-login.php, and screenshot are not available, so, it isn't clear what should be escaped. In this file, there are a lot of calls for URLs to look through 😅

#11 @nicolefurlan
8 months ago

  • Keywords needs-testing-info added

@rajinsharwar could you update the ticket title/description and add testing info to cover all of the changes in your PR?

This ticket was mentioned in Slack in #core by oglekler. View the logs.


8 months ago

#13 @rajinsharwar
8 months ago

  • Description modified (diff)
  • Keywords has-testing-info added; needs-testing needs-testing-info removed
  • Summary changed from Sanitize url and title missing to Escape missing URLs and HTML element content in wp-activate.php

Hi @nicolefurlan @oglekler, updated the title and description for the ticket. :)
Let me know if I missed anything.

#14 @oglekler
8 months ago

  • Milestone changed from 6.4 to 6.5

We are out of time before RC1, so, I am moving this ticket to the next milestone. Let's not procrastinate until the 6.5 RC1 and make it in the beginning of the alpha )

This ticket was mentioned in Slack in #core by rajinsharwar. View the logs.


4 months ago

#16 @rajinsharwar
4 months ago

  • Keywords needs-testing added

We likely need some test reports on this if this introduces any issues. Adding "needs-testing"

#17 @jorbin
4 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 57625:

Multisite: Escape urls and html elements in wp-activate.php

When WPMU was merged in [12603], the intent was to go back and make sure everything was escaped. This completes that intent.

Props rafiq91, rajinsharwar, costdev, oglekler, nicolefurlan, ryan, peterwilsoncc.
Fixes #57336.
See #11644.

@SergeyBiryukov commented on PR #5046:


3 months ago
#18

Thanks for the PR! This was merged in r57625.

Note: See TracTickets for help on using tickets.