Opened 22 months ago
Last modified 12 days ago
#58905 assigned defect (bug)
Ensure locate_template only loads theme files
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (28)
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
@
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
@
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
@
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.
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
@
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
@
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.
@mukesh27 commented on PR #6502:
9 months ago
#13
@peterwilsoncc Updated https://github.com/WordPress/wordpress-develop/pull/6502#pullrequestreview-2259763696
@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
@
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
@
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
@
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
@
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
@
2 months ago
- Keywords commit removed
Removing the commit
keyword until the unit test failures can be addressed.
#26
@
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
@
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
- Create a dummy Theme
- Make sure it has at least the files:
style.css
andindex.php
. Check a Basic Theme example here: https://developer.wordpress.org/themes/classic-themes/your-first-theme/#classic-theme - Create another file inside the theme directory called
test-non-template.txt
- Install this plugin https://raw.githubusercontent.com/SirLouen/testing-template-loading/refs/tags/Second-Version/testing-template-loading.php
- Create a new Post and add the short code to see the results:
[test_locate_template]
- 🐞Bug reproduced: Wrongly loaded non-template file
Actual Results with Patch
- ❌ Issue is not resolved with the patch.
Additional Comments
If there are any concerns, please review the plugin for further testing.
The plugin tests:
- Locating a typical file in any classic theme
- Locating a random file that should not be located
- 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/
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 usevalidate_file()
instead here and avoid the multiple calls torealpath()
?