Make WordPress Core

Opened 14 months ago

Last modified 9 days ago

#58905 new defect (bug)

Ensure locate_template only loads theme files

Reported by: jorbin's profile jorbin Owned by:
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch early needs-testing-info commit has-unit-tests needs-dev-note
Focuses: Cc:

Description

It's possible to have locate_template load files that are not template files which could be unexpected behavior that breaks the display of a page.

The chance this breaks something is, I think, not infinitesimal. Therefore, this should go in early.

An initial patch is attached.

Attachments (1)

58905.diff (2.1 KB) - added by jorbin 14 months ago.

Download all attachments as: .zip

Change History (15)

@jorbin
14 months ago

#1 @joemcgill
14 months ago

Thanks @jorbin. The use of realpath() in this diff can have a measurable negative performance impact. Given that this function has assumed that the template names were being concatenated directly with the various constants previously, I wonder if we could use validate_file() instead here and avoid the multiple calls to realpath()?

#2 @JeffPaul
13 months ago

  • Keywords needs-refresh needs-unit-tests added

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


13 months ago
#3

  • Keywords needs-refresh removed

Uses validate_file to ensure that the paths processed by locate_template are not directory traversals, Windows drive paths, etc...

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

#4 @pypwalters
13 months ago

Option added that uses validate_file(). I removed the unit tests because the result was always coming up empty. I believe I am running into some complications because of the use of STYLESHEETPATH and TEMPLATEPATH in the locate_template function. Is it possible that these are not available to phpunit?

#5 @oglekler
12 months ago

  • Milestone changed from 6.4 to 6.5

Because this is an early ticket, I am moving it into the 6.5 milestone.

#6 @swissspidy
8 months ago

  • Keywords needs-refresh added

locate_template did see some updates in r56635 which deprecated the usage of these constants. Patch needs to be refreshed.

#7 @swissspidy
7 months ago

  • Milestone changed from 6.5 to 6.6

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


4 months ago
#8

  • Keywords needs-refresh removed

Added refresh patch for Validate url in locate template.

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


4 months ago

#10 @oglekler
3 months ago

  • Keywords needs-testing-info added
  • Milestone changed from 6.6 to 6.7

This patch still needs unit-tests and is marked as early, that means that it is to late for now for it in the current milestone. In addition, it will be nice for testers to have testing instructions.

#11 @jorbin
6 weeks ago

  • Keywords commit has-unit-tests needs-dev-note added; needs-unit-tests removed

I've added some automated tests and am planning to commit the code in https://github.com/WordPress/wordpress-develop/pull/6502/ soon. As the chance of something breaking is not infinitesimal, I also am going to publish a dev note soon after to encourage testing.

@peterwilsoncc commented on PR #6502:


10 days ago
#12

@mukeshpanchal27 Are you able to edit the comment above to note the raw difference. As the benchmarking script is using hrtime() (nanoseconds) I think the raw numbers could be more helpful than a percentage.

@peterwilsoncc commented on PR #6502:


9 days ago
#14

Converting the raw times from nanoseconds to milliseconds is showing a showdown of between 0.7ms to 2.5ms over 1000 iterations so I don't think it's worth considering the performance impacts as a reason to block the benefits of avoiding using the function to include files outside of the template paths.

Note: See TracTickets for help on using tickets.