Make WordPress Core

Opened 22 months ago

Last modified 12 days ago

#58905 assigned defect (bug)

Ensure locate_template only loads theme files

Reported by: jorbin's profile jorbin Owned by: jorbin's profile jorbin
Milestone: 6.9 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch early has-unit-tests needs-dev-note changes-requested has-test-info
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 22 months ago.

Download all attachments as: .zip

Change History (28)

@jorbin
22 months ago

#1 @joemcgill
22 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
21 months ago

  • Keywords needs-refresh needs-unit-tests added

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


21 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
21 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
20 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
16 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
15 months ago

  • Milestone changed from 6.5 to 6.6

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


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


12 months ago

#10 @oglekler
11 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
10 months 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:


9 months 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 months 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.

#15 @peterwilsoncc
8 months ago

@jorbin I think the PR is good, are you still wanting to commit this or would you rather wait until the 6.7 branch is forked?

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


8 months ago

#17 @joemcgill
8 months ago

  • Owner set to jorbin
  • Status changed from new to assigned

Assigning to @jorbin for a final decision on committing or punting for 6.7.

#18 @jorbin
8 months ago

  • Milestone changed from 6.7 to 6.8

There have been some test failures and I don't think I will be able to get them resolved before beta 1, so I'm bumping this to 6.8

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


4 months ago

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


4 months ago

#21 @audrasjb
4 months ago

This ticket was reviewed during today's bug scrub. @jorbin do you think you can address this ticket for 6.8? As it is marked early, it should ideally be committed by next week.

@joemcgill commented on PR #6502:


3 months ago
#22

Updated this PR from trunk to try to address Unit Test failures, but something is causing these to error out before reporting failures in the workflow. I'm running them locally to see if I can diagnose what the issue is.

@joemcgill commented on PR #6502:


3 months ago
#23

I've also found that the reason the PHPUnit Tests workflow are not showing failures is because the test suite is failing silently on Tests_Embed_Template::test_oembed_output_post. This will also require investigation.

This ticket was mentioned in Slack in #core-test by krupajnanda. View the logs.


3 months ago

#25 @joemcgill
2 months ago

  • Keywords commit removed

Removing the commit keyword until the unit test failures can be addressed.

#26 @jorbin
2 months ago

  • Milestone changed from 6.8 to 6.9

I am not confident committing this during beta, I think it needs as much testing as possible so I am moving this to 6.9.

#27 @SirLouen
12 days ago

  • Keywords changes-requested has-test-info added; needs-testing-info removed

Combined Bug Reproduction and Patch Test Report

Description

❌ This report can't validate that the indicated patch works as expected.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/6502.diff

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.4.6
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.4.6)
  • Browser: Chrome 136.0.0.0
  • OS: Windows 10/11
  • Theme: My First WordPress Theme
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Bug Reproduction Steps

  1. Create a dummy Theme
  2. Make sure it has at least the files: style.css and index.php. Check a Basic Theme example here: https://developer.wordpress.org/themes/classic-themes/your-first-theme/#classic-theme
  3. Create another file inside the theme directory called test-non-template.txt
  4. Install this plugin https://raw.githubusercontent.com/SirLouen/testing-template-loading/refs/tags/Second-Version/testing-template-loading.php
  5. Create a new Post and add the short code to see the results: [test_locate_template]
  6. 🐞Bug reproduced: Wrongly loaded non-template file

Actual Results with Patch

  1. ❌ Issue is not resolved with the patch.

Additional Comments

If there are any concerns, please review the plugin for further testing.
The plugin tests:

  1. Locating a typical file in any classic theme
  2. Locating a random file that should not be located
  3. Locating a file outside the theme files, that could potentially be a security risk like wp-config.php

These tests can obviously be added as Unit-Tests (and should be), but for a regular tester that is not familiarized with PHPUnit this type of plugin-based testing is much more friendly and easier to be inspected.

Supplemental Artifacts

Link to the testing plugin repo: https://github.com/SirLouen/testing-template-loading/

Last edited 12 days ago by SirLouen (previous) (diff)
Note: See TracTickets for help on using tickets.