test(file-safety): pin null-byte contract for cleanup helpers#53
Merged
Conversation
Covers the three helpers in htdocs/include/file_safety.php - xoops_safe_basename(), xoops_chmod_quietly(), xoops_remove_file_quietly() - against the null-byte case that motivated the defensive shape: basename() does not throw on "\0" in PHP 8.2-8.4 but chmod()/unlink() do, and the helpers must collapse both into a single E_USER_WARNING without propagating ValueError. Also pins the backslash-normalisation and XOOPS_ROOT_PATH stripping branches of xoops_file_label().
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds PHPUnit test coverage for four file-safety helpers in ChangesFile Safety Test Suite
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #53 +/- ##
============================================
+ Coverage 18.04% 18.12% +0.07%
- Complexity 7845 7854 +9
============================================
Files 665 666 +1
Lines 43083 43142 +59
============================================
+ Hits 7775 7820 +45
- Misses 35308 35322 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds PHPUnit coverage to lock in the “best-effort / non-propagating” behavior of htdocs/include/file_safety.php helpers, especially around null-byte paths that can trigger ValueError in PHP 8+ filesystem functions. This helps ensure cleanup/atomic-write code paths don’t crash and emit consistent project-standard warnings.
Changes:
- Introduces unit tests for
xoops_safe_basename(),xoops_chmod_quietly(), andxoops_remove_file_quietly()focusing on null-byte handling. - Adds tests for
xoops_safe_basename()backslash normalization and forxoops_file_label()root-prefix stripping + basename fallback. - Verifies warning emission behavior via a local error handler (without
@suppression).
Comment on lines
+54
to
+70
| set_error_handler(static function (int $level, string $msg) use (&$captured): bool { | ||
| if (E_USER_WARNING === $level) { | ||
| $captured = $msg; | ||
| return true; | ||
| } | ||
| return false; | ||
| }); | ||
|
|
||
| try { | ||
| $result = xoops_chmod_quietly("bad\0path", 0644, 'test'); | ||
| } finally { | ||
| restore_error_handler(); | ||
| } | ||
|
|
||
| $this->assertFalse($result, 'chmod on a null-byte path must report failure'); | ||
| $this->assertNotNull($captured, 'helper must still emit one E_USER_WARNING on failure'); | ||
| $this->assertStringContainsString('invalid-path', (string) $captured); |
Comment on lines
+75
to
+78
| // The pre-check file_exists() / is_link() does not throw on a | ||
| // "\0"-bearing path in PHP 8.2-8.4 (it returns false), so the | ||
| // helper returns early without ever reaching unlink(). The | ||
| // contract being tested is just "no exception escapes". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/htdocs/include/FileSafetyTest.php`:
- Around line 54-71: The test currently only records the last E_USER_WARNING in
$captured, so update the error handler used around
xoops_chmod_quietly("bad\0path", 0644, 'test') to count warnings and capture the
message: add a counter (e.g. $warningCount = 0) in the scope, increment it
inside the set_error_handler callback whenever E_USER_WARNING is seen and still
set $captured to the first (or last) message as desired; after
restore_error_handler(), assert both that $result is false and that
$warningCount === 1 (exactly one warning emitted) in addition to the existing
assertNotNull/assertStringContainsString checks referencing xoops_chmod_quietly
and the handler variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dea9560c-e30a-4e6b-86dd-0acc5e5579e6
📒 Files selected for processing (1)
tests/unit/htdocs/include/FileSafetyTest.php
… helper Address PR XOOPS#53 review notes from Copilot and CodeRabbit: - testXoopsChmodQuietlyDoesNotPropagateOnNullBytePath() now asserts exactly one E_USER_WARNING and explicitly rejects any non-E_USER_WARNING leak, pinning the "collapse both into a single warning" contract. - Reword the testXoopsRemoveFileQuietlyDoesNotPropagateOnNullBytePath() comment: the pre-check may return false OR throw depending on PHP/ runtime; the helper's catch(\Throwable) exists for forward-compat and userland error handlers. The contract is just "no exception escapes"
Comment on lines
+112
to
+120
| // xoops_file_label() keeps install-relative context (unlike | ||
| // xoops_safe_basename()) for atomic-write call sites that | ||
| // benefit from knowing WHICH file failed. Spot-check the | ||
| // strip-prefix branch and the basename fallback. | ||
| $absUnderRoot = rtrim(XOOPS_ROOT_PATH, '/\\') . '/uploads/foo.png'; | ||
| $this->assertSame('uploads/foo.png', xoops_file_label($absUnderRoot)); | ||
|
|
||
| $absOutside = sys_get_temp_dir() . '/foo.png'; | ||
| $this->assertSame('foo.png', xoops_file_label($absOutside)); |
Comment on lines
+9
to
+17
| /** | ||
| * Coverage for the three side-effect-free file helpers in | ||
| * htdocs/include/file_safety.php. The helpers are documented as | ||
| * best-effort / non-propagating: on any failure they emit a single | ||
| * E_USER_WARNING (or return early) and continue. These tests pin that | ||
| * contract specifically against null-byte payloads, which trigger | ||
| * ValueError in the underlying filesystem calls on PHP 8+ — the case | ||
| * that motivated the explicit catch(\Throwable) wrappers and the | ||
| * xoops_safe_basename() defensive shape. |
…lock Address PR XOOPS#53 second round of Copilot review notes: - testXoopsFileLabelStripsXoopsRootPrefix() now asserts that a backslash-separated path under XOOPS_ROOT_PATH collapses to a forward-slash relative label, exercising the str_replace('\\\\', '/') branch in xoops_file_label() that was previously untested. The outside- root branch is intentionally left to forward slashes only because it falls through to basename(), which is platform-dependent on '\\'. - Reword class docblock: the suite covers four helpers, not three (xoops_file_label() was added but the count was not).
Comment on lines
+85
to
+86
| xoops_remove_file_quietly("bad\0path"); | ||
| $this->assertTrue(true, 'xoops_remove_file_quietly returned without throwing'); |
| }); | ||
|
|
||
| try { | ||
| xoops_remove_file_quietly(sys_get_temp_dir() . '/xoops_definitely_missing_' . uniqid() . '.tmp'); |
…uffix Address PR XOOPS#53 third-round Copilot polish notes: - testXoopsRemoveFileQuietlyDoesNotPropagateOnNullBytePath() replaces the assertTrue(true, ...) placeholder with expectNotToPerformAssertions(), PHPUnit's idiomatic form for the "no exception escapes" contract that also avoids the always-true assertion some linters flag. - testXoopsRemoveFileQuietlyIsNoOpForMissingPath() switches uniqid() to bin2hex(random_bytes(16)) for the temp suffix. PHP 8.2+ is the baseline; this makes parallel-runner collision concerns disappear.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Covers the three helpers in htdocs/include/file_safety.php -
xoops_safe_basename(), xoops_chmod_quietly(), xoops_remove_file_quietly() -
against the null-byte case that motivated the defensive shape: basename()
does not throw on "\0" in PHP 8.2-8.4 but chmod()/unlink() do, and the
helpers must collapse both into a single E_USER_WARNING without propagating
ValueError. Also pins the backslash-normalisation and XOOPS_ROOT_PATH
stripping branches of xoops_file_label().
Summary by CodeRabbit