Normalize Windows temp dir in Given a directory step#330
Normalize Windows temp dir in Given a directory step#330swissspidy wants to merge 2 commits intomainfrom
Given a directory step#330Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the directory validation logic in GivenStepDefinitions.php to better handle system temporary directories across different operating systems, including Mac OS X path prefixes and Windows case-insensitivity. The feedback suggests simplifying the repetitive path comparison logic by using a loop and a dynamic comparison function to improve readability and maintainability.
| $in_temp = 0 === ( $is_windows ? stripos( $dir, $temp_dir_original ) : strpos( $dir, $temp_dir_original ) ) | ||
| || 0 === ( $is_windows ? stripos( $dir, $temp_dir_real ) : strpos( $dir, $temp_dir_real ) ) | ||
| || 0 === ( $is_windows ? stripos( $dir, "/private{$temp_dir_original}" ) : strpos( $dir, "/private{$temp_dir_original}" ) ) | ||
| || 0 === ( $is_windows ? stripos( $dir, "/private{$temp_dir_real}" ) : strpos( $dir, "/private{$temp_dir_real}" ) ); |
There was a problem hiding this comment.
The logic for checking if the directory is within the temporary directory is quite repetitive and difficult to read. Since you are checking multiple candidates against the same directory using a conditional comparison function (stripos vs strpos), you can simplify this by using a loop. This improves maintainability and reduces the risk of errors when adding new candidates.
$compare = $is_windows ? 'stripos' : 'strpos';
$in_temp = false;
foreach ( [ $temp_dir_original, $temp_dir_real ] as $temp_path ) {
if ( 0 === $compare( $dir, $temp_path ) || 0 === $compare( $dir, "/private{$temp_path}" ) ) {
$in_temp = true;
break;
}
}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Updates the Behat Given an (empty|non-existent) <dir> directory step to handle Windows temp directory normalization more robustly by accounting for sys_get_temp_dir() vs realpath() differences and case-insensitive path comparisons.
Changes:
- Track both the original temp dir and the
realpath()’d temp dir and compare against both. - Use case-insensitive prefix checks on Windows to avoid temp-dir mismatches due to path casing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $in_temp = 0 === ( $is_windows ? stripos( $dir, $temp_dir_original ) : strpos( $dir, $temp_dir_original ) ) | ||
| || 0 === ( $is_windows ? stripos( $dir, $temp_dir_real ) : strpos( $dir, $temp_dir_real ) ) | ||
| || 0 === ( $is_windows ? stripos( $dir, "/private{$temp_dir_original}" ) : strpos( $dir, "/private{$temp_dir_original}" ) ) | ||
| || 0 === ( $is_windows ? stripos( $dir, "/private{$temp_dir_real}" ) : strpos( $dir, "/private{$temp_dir_real}" ) ); |
There was a problem hiding this comment.
The temp-dir guard uses a raw prefix check (strpos/stripos === 0). This can be bypassed by paths that merely share the same prefix (e.g. temp dir /tmp vs directory /tmpfoo), which could allow deleting directories outside the temp tree. Consider enforcing a path-boundary check by normalizing/rtrimming both sides and comparing with an added trailing separator (or otherwise ensuring the next character is a separator/end-of-string) before calling remove_dir().
| $is_windows = DIRECTORY_SEPARATOR === '\\'; | ||
|
|
||
| $in_temp = 0 === ( $is_windows ? stripos( $dir, $temp_dir_original ) : strpos( $dir, $temp_dir_original ) ) | ||
| || 0 === ( $is_windows ? stripos( $dir, $temp_dir_real ) : strpos( $dir, $temp_dir_real ) ) | ||
| || 0 === ( $is_windows ? stripos( $dir, "/private{$temp_dir_original}" ) : strpos( $dir, "/private{$temp_dir_original}" ) ) | ||
| || 0 === ( $is_windows ? stripos( $dir, "/private{$temp_dir_real}" ) : strpos( $dir, "/private{$temp_dir_real}" ) ); |
There was a problem hiding this comment.
This change is Windows-specific (case-insensitive matching + sys_get_temp_dir vs realpath handling), but there’s no corresponding functional test ensuring Given an empty/non-existent <absolute-path> directory works on Windows when the temp path casing differs. Adding a @require-windows Behat scenario that builds an absolute temp path with different casing (e.g. via php -r + save STDOUT as {VAR}) would help prevent regressions.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.