Skip to content

feat(files_external/s3): Harden S3 cache invalidation prefix matching #58638

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
joshtrichards wants to merge 5 commits into
base: master
Choose a base branch
from
+23 −19
Open

feat(files_external/s3): Harden S3 cache invalidation prefix matching #58638

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
80f192b
refactor(Storage/S3): reduce duplicate code in invalidateCache()
joshtrichards Feb 28, 2026
474097d
refactor(Storage/S3): move needsPartFile()
joshtrichards Feb 28, 2026
4cbf8da
fix(Storage/S3): empty prefix guard for invalidCache()
joshtrichards Feb 28, 2026
407ce54
fix(Storage/S3): path normalization + boundary-aware matching
joshtrichards Feb 28, 2026
570deb4
chore(Storage/S3): final tidying
joshtrichards Feb 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 23 additions & 19 deletions apps/files_external/lib/Lib/Storage/AmazonS3.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?php

/**
* SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-FileCopyrightText: 2016-2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-FileCopyrightText: 2016 ownCloud, Inc.
* SPDX-License-Identifier: AGPL-3.0-only
*/
Expand Down Expand Up @@ -32,10 +32,6 @@ class AmazonS3 extends Common {

private LoggerInterface $logger;

public function needsPartFile(): bool {
return false;
}

/** @var CappedMemoryCache<array|false> */
private CappedMemoryCache $objectCache;

Expand Down Expand Up @@ -77,6 +73,10 @@ private function isRoot(string $path): bool {
return $path === '.';
}

public function needsPartFile(): bool {
return false;
}

private function cleanKey(string $path): string {
if ($this->isRoot($path)) {
return '/';
Expand All @@ -90,24 +90,28 @@ private function clearCache(): void {
$this->filesCache = new CappedMemoryCache();
}

private function invalidateCache(string $key): void {
unset($this->objectCache[$key]);
$keys = array_keys($this->objectCache->getData());
$keyLength = strlen($key);
foreach ($keys as $existingKey) {
if (substr($existingKey, 0, $keyLength) === $key) {
unset($this->objectCache[$existingKey]);
}
private function invalidateCache(string $prefix): void {
Copy link
Member

@CarlSchwan CarlSchwan Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private function invalidateCache(string $prefix): void {
/** @param non-empty-string $prefix */
private function invalidateCache(string $prefix): void {

if that should never happen

if ($prefix === '') {
return; // or throw InvalidArgumentException?
}
unset($this->filesCache[$key]);
$keys = array_keys($this->directoryCache->getData());
$keyLength = strlen($key);

$this->invalidateByPrefix($this->objectCache, $prefix);
$this->invalidateByPrefix($this->directoryCache, $prefix);

// FILES: exact match keys only (not hierarchical)
unset($this->filesCache[$prefix]);
}

private function invalidateByPrefix(CappedMemoryCache $cache, string $prefix): void {
$keys = array_keys($cache->getData());
$descendantPrefix = rtrim($prefix, '/') . '/';

foreach ($keys as $existingKey) {
if (substr($existingKey, 0, $keyLength) === $key) {
unset($this->directoryCache[$existingKey]);
// exact + normalized prefix matched keys
if ($existingKey === $prefix || str_starts_with($existingKey, $descendantPrefix)) {
unset($cache[$existingKey]);
}
}
unset($this->directoryCache[$key]);
}

private function headObject(string $key): array|false {
Expand Down
Loading