Allow Windows Paths with validate_file() #489
Conversation
|
Warning Rate Limit Exceeded@srtfisher has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 13 minutes and 31 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe updates primarily involve the addition of the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (20)
- src/mantle/assets/class-asset-loader.php (1 hunks)
- src/mantle/assets/class-asset-service-provider.php (1 hunks)
- src/mantle/database/model/concerns/trait-has-events.php (1 hunks)
- src/mantle/database/model/concerns/trait-has-global-scopes.php (1 hunks)
- src/mantle/database/model/registration/trait-register-meta.php (1 hunks)
- src/mantle/database/model/registration/trait-register-rest-fields.php (1 hunks)
- src/mantle/database/query/class-post-query-builder.php (1 hunks)
- src/mantle/framework/helpers.php (1 hunks)
- src/mantle/http/class-request.php (1 hunks)
- src/mantle/http/class-uploaded-file.php (1 hunks)
- src/mantle/http/view/class-view-finder.php (1 hunks)
- src/mantle/http/view/class-view.php (1 hunks)
- src/mantle/scheduling/class-event.php (1 hunks)
- src/mantle/scheduling/class-schedule.php (1 hunks)
- src/mantle/scheduling/trait-manages-frequencies.php (1 hunks)
- src/mantle/support/class-arr.php (1 hunks)
- src/mantle/support/helpers/helpers-general.php (1 hunks)
- src/mantle/testing/concerns/trait-interacts-with-cron.php (1 hunks)
- src/mantle/view/engines/class-file-engine.php (1 hunks)
- src/mantle/view/engines/class-php-engine.php (1 hunks)
Files skipped from review due to trivial changes (15)
- src/mantle/database/model/concerns/trait-has-events.php
- src/mantle/database/model/concerns/trait-has-global-scopes.php
- src/mantle/database/model/registration/trait-register-meta.php
- src/mantle/database/model/registration/trait-register-rest-fields.php
- src/mantle/database/query/class-post-query-builder.php
- src/mantle/framework/helpers.php
- src/mantle/http/class-request.php
- src/mantle/http/class-uploaded-file.php
- src/mantle/http/view/class-view-finder.php
- src/mantle/http/view/class-view.php
- src/mantle/scheduling/class-event.php
- src/mantle/scheduling/class-schedule.php
- src/mantle/scheduling/trait-manages-frequencies.php
- src/mantle/support/class-arr.php
- src/mantle/testing/concerns/trait-interacts-with-cron.php
Additional comments: 8
src/mantle/assets/class-asset-loader.php (4)
16-16: The import of
validate_filefromMantle\Support\Helpersis correctly added to support enhanced file validation as per the PR objectives.16-16: Verify that the
validate_filefunction is correctly updated to handle Windows paths as intended in the PR objectives.16-16: Confirm that the comparison of
validate_filereturn value to0aligns with the new validation rules and the intended enhancements.16-16: Consider whether the new
$allowed_filesparameter should be utilized in thevalidate_filefunction call within this file, or if it's intended for use elsewhere.src/mantle/assets/class-asset-service-provider.php (2)
12-12: The import of
validate_filefromMantle\Support\Helpersis consistent with the PR's objective to enhance file validation.9-16: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [29-31]
The integration of
validate_filein theload_blocksmethod appears correct, assuming a return value of0fromvalidate_fileindicates the file is valid.src/mantle/view/engines/class-file-engine.php (1)
- 12-13: The import of
validate_filefromMantle\Support\Helpersis correctly added to use the updated function within this file.src/mantle/view/engines/class-php-engine.php (1)
- 13-17: The import of
validate_filefromMantle\Support\Helpersis correctly added to use the updated validation logic within the PHP engine.
| /** | ||
| * Validates a file name and path against an allowed set of rules. | ||
| * | ||
| * A return value of `1` means the file path contains directory traversal. | ||
| * | ||
| * A return value of `3` means the file is not in the allowed files list. | ||
| * | ||
| * @see validate_file() in WordPress core. | ||
| * | ||
| * @param string $file File path. | ||
| * @param string[] $allowed_files Optional. Array of allowed files. Default empty array. | ||
| * @return int 0 means nothing is wrong, greater than 0 means something was wrong. | ||
| */ | ||
| function validate_file( $file, $allowed_files = [] ) { | ||
| // Proxy back to the core function if it exists allowing Window drive paths. | ||
| if ( function_exists( 'validate_file' ) ) { | ||
| return in_array( \validate_file( $file, $allowed_files ), [ 0, 2 ], true ) ? 0 : 1; | ||
| } | ||
|
|
||
| if ( ! is_scalar( $file ) || '' === $file ) { | ||
| return 0; | ||
| } | ||
|
|
||
| // `../` on its own is not allowed: | ||
| if ( '../' === $file ) { | ||
| return 1; | ||
| } | ||
|
|
||
| // More than one occurrence of `../` is not allowed. | ||
| if ( preg_match_all( '#\.\./#', $file, $matches, PREG_SET_ORDER ) && ( count( $matches ) > 1 ) ) { | ||
| return 1; | ||
| } | ||
|
|
||
| // `../` which does not occur at the end of the path is not allowed. | ||
| if ( str_contains( $file, '../' ) && '../' !== mb_substr( $file, -3, 3 ) ) { | ||
| return 1; | ||
| } | ||
|
|
||
| // Files not in the allowed file list are not allowed. | ||
| if ( ! empty( $allowed_files ) && ! in_array( $file, $allowed_files, true ) ) { | ||
| return 3; | ||
| } | ||
|
|
||
| // Absolute Windows drive paths ARE allowed. | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
The updated validate_file function includes a new parameter $allowed_files and additional validation logic. It is important to ensure that the changes do not introduce security vulnerabilities, especially with the handling of Windows drive paths and directory traversal. The proxying to the core validate_file function should also be verified for correctness and security.
- The check for directory traversal using
../seems to be thorough, but it may be worth considering edge cases or encoding variations that could bypass these checks. - The proxy call to the core
validate_filefunction usesin_arrayto check the return value. Ensure that the core function's return values are compatible with this check and that no edge cases are missed. - The function now explicitly allows absolute Windows drive paths. Confirm that this does not introduce any security risks, such as allowing access to critical system files or directories.
|
|
||
| use Mantle\Contracts\View\Engine; | ||
|
|
||
| use function Mantle\Support\Helpers\validate_file; | ||
|
|
||
| /** | ||
| * File Engine to load raw view files. | ||
| */ |
There was a problem hiding this comment.
The validate_file function is called twice in a row with the same argument. This seems redundant and may be a mistake. Consider calling it once and storing the result in a variable or directly using the result in the if condition.
- if ( 0 === validate_file( $path ) && 0 === validate_file( $path ) ) {
+ if ( 0 === validate_file( $path ) ) {Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| use Mantle\Contracts\View\Engine; | |
| use function Mantle\Support\Helpers\validate_file; | |
| /** | |
| * File Engine to load raw view files. | |
| */ |
| @@ -10,6 +10,8 @@ | |||
| use Mantle\Contracts\View\Engine; | |||
| use Throwable; | |||
|
|
|||
| use function Mantle\Support\Helpers\validate_file; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
The validate_file function is called twice in succession with the same argument. If this is intentional, consider adding a comment to clarify the reason. If it's a mistake, remove the redundant call.
- if ( 0 === validate_file( $path ) && 0 === validate_file( $path ) ) {
+ if ( 0 === validate_file( $path ) ) {Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| /** | |
| if ( 0 === validate_file( $path ) ) { |
| @@ -13,6 +13,7 @@ | |||
| use Symfony\Component\Finder\SplFileInfo; | |||
|
|
|||
| use function Mantle\Support\Helpers\collect; | |||
| use function Mantle\Support\Helpers\validate_file; | |||
There was a problem hiding this comment.
Ensure that the validate_file function usage includes checks against a list of allowed files or directories to prevent potential security issues.
Co-authored-by: Kevin Fodness <[email protected]>
…ntle-framework into hotfix/validate-file
Fixes #487
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores