Make WordPress Core

Opened 10 months ago

Last modified 3 days ago

#57336 assigned defect (bug)

Sanitize url and title missing

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

Description

  1. The login title in wp-login.php is not sanitized properly https://prnt.sc/MJLEeeUWf7BE
  2. Network site URL is also not properly sanitized https://prnt.sc/6rFVD0ClxbO-

Change History (10)

#2 @rajinsharwar
7 weeks 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
7 weeks 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
6 weeks 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.


6 weeks 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
6 weeks 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
6 weeks 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
6 weeks 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 6 weeks ago by costdev (previous) (diff)

#9 @oglekler
5 weeks ago

  • Keywords needs-testing added

#10 @oglekler
3 days 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 😅

Note: See TracTickets for help on using tickets.