fix: S3 exists() should not match directories for file paths

[ChiragAgg5k]

Summary

Fixes a bug in S3 exists() method where file paths would incorrectly match directories with the same prefix.

Problem

Previously, exists() would add a trailing slash to all paths when falling back to listObjects(). This caused:

• exists('/storage/builds/app-123/extracted/abc123/index.html') → checks for prefix /storage/builds/app-123/extracted/abc123/index.html/
• Returns true if any directory starts with that prefix, even if the file doesn't exist
• Results in NoSuchKey errors when trying to read the file

Solution

Only add trailing slash if the original path ends with /:

// Before
if (! empty($path) && ! str_ends_with($prefix, '/')) {
    $prefix .= '/';
}

// After
if (! empty($path) && str_ends_with($path, '/')) {
    $prefix = rtrim($prefix, '/') . '/';
}

Changes

• Updated src/Storage/Device/S3.php:639-641 to only add trailing slash when original path indicates a directory
• Added test testFileExistsDoesNotMatchDirectoryPrefix() to verify the fix

Testing

The new test verifies that:

• exists('file.html') returns false when the file doesn't exist
• Even if the parent directory exists with other files
• exists('directory/') still works correctly for directory checks

Summary by CodeRabbit

• Bug Fixes
  • Fixed an issue in cloud storage file detection where existence checks could incorrectly match directory prefixes instead of specific files when file paths lacked trailing slashes, ensuring more accurate file status queries.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Changes to the S3 storage device's exists() method modify how prefix normalization handles trailing slashes. The logic was inverted to check the original path parameter instead of the prefix, and now enforces a single trailing slash when the path ends with one. A corresponding test was added to verify that file paths without trailing slashes do not incorrectly match directory prefixes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• The prefix normalization logic inversion requires careful verification to ensure the condition change correctly addresses the intended behavior and doesn't introduce regressions
• Understanding the S3 path matching semantics and why the original approach was problematic is necessary
• The new test is straightforward but validates a specific edge case that merits review for completeness and coverage of related scenarios
• Consider whether additional edge cases (empty paths, multiple consecutive slashes, special characters) are properly handled

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the primary change: fixing S3's exists() method to prevent file paths from incorrectly matching directories.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-s3-exists-trailing-slash

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR fixes a bug in the S3 storage adapter's exists() method where file path existence checks were incorrectly being converted to directory prefix checks, leading to false positives.

Key Changes:

• Modified the logic to only append a trailing slash when the original path explicitly ends with / (indicating a directory check)
• Added comprehensive test coverage to verify files don't match directory prefixes

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/Storage/Device/S3.php	Fixed the exists() method to preserve file vs directory semantics by only adding trailing slashes for paths that explicitly end with /
tests/Storage/S3Base.php	Added test case testFileExistsDoesNotMatchDirectoryPrefix() to verify the fix prevents false positives when checking file existence in directories with similar prefixes

The implementation correctly addresses the reported issue. The fix ensures that:

• File existence checks like exists('builds/index.html') use prefix builds/index.html (no trailing slash)
• Directory existence checks like exists('builds/') use prefix builds/ (with trailing slash)
• Directory checks without trailing slash like exists('nested') still work because S3's listObjects with prefix nested matches both exact keys and nested paths like nested/file.txt

The new test appropriately validates the fix by creating two files in a directory, deleting one, and confirming that the deleted file correctly reports as non-existent even though the parent directory still contains other files. The test follows established patterns in the codebase and properly cleans up after itself.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/Storage/Device/S3.php (1)

639-647: Trailing‑slash fix is good, but non‑directory prefix behavior changed; please double‑check semantics

The new condition correctly standardizes a single trailing / only when the original $path ends with /, which matches the intent of treating “directory‑like” paths differently from file paths and avoids the previous unconditional folder behavior. That’s a solid improvement.

One subtle side effect, though: for paths that do not end with /, the fallback now calls listObjects() with $prefix without a trailing slash. Compared to the old logic (which always forced $prefix .= '/'), this means:

• Previously, for a failing HEAD on $path = 'foo', the fallback prefix was effectively foo/, so only keys under foo/… could make exists('foo') return true.
• Now, the fallback prefix is foo, so any key starting with foo (e.g. foo-bar, foo_tmp, etc.) will cause exists('foo') to return true, even if there is no foo object and no foo/… “directory”.

That’s a real behavior change for ambiguous names where the requested path is a strict prefix of other keys. It may be acceptable, but if you want to avoid new false positives while still keeping the directory fix, you might consider tightening the fallback for non‑directory paths, e.g., by only treating the fallback as true when at least one returned Key exactly matches the target key you used for HEAD rather than just sharing the prefix.

tests/Storage/S3Base.php (1)

168-185: Great regression test; consider aligning the comment and optionally covering the ‘file.html/…’ case explicitly

The new testFileExistsDoesNotMatchDirectoryPrefix() is a good addition and clearly validates the core regression you’re fixing: after deleting builds/index.html, exists('builds/index.html') stays false even though builds/ still contains other files.

Two small improvements you might want to consider:

• The docblock mentions “directory 'file.html/' exists”, but the test currently only uses a sibling file (builds/other.css). Either adjust the comment to describe the actual scenario (file absent, parent directory still populated) or
• Extend the test to also create something like builds/index.html/inner.txt and assert that exists('builds/index.html') is still false when only that “directory under file name” structure exists. That would more directly lock in the original bug description.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 10fe0c0 and d334eb4.

📒 Files selected for processing (2)

• src/Storage/Device/S3.php (1 hunks)
• tests/Storage/S3Base.php (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: E2E Test (WasabiTest)
• GitHub Check: E2E Test (LinodeTest)
• GitHub Check: E2E Test (BackblazeTest)
• GitHub Check: E2E Test (LocalTest)
• GitHub Check: E2E Test (S3Test)
• GitHub Check: E2E Test (DOSpacesTest)