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

[rafiq91]

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.

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

[rajinsharwar]

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?

[costdev]

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.

[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

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

[costdev]

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.

[rajinsharwar]

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?

[costdev]

@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.

[oglekler]

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 😅

[nicolefurlan]

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

[rajinsharwar]

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

[oglekler]

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 )

[rajinsharwar]

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

[jorbin]

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.

Thanks for the PR! This was merged in r57625.