fix(storage): guard owner re-uploads with If-Match and read the bumped version back

This commit is contained in:
Anthony Stirling
2026-08-30 10:46:26 +01:00
parent ac2c8f71ab
commit d0e1e2289b
10 changed files with 190 additions and 10 deletions
@@ -90,6 +90,10 @@ public interface StoredFileRepository extends JpaRepository<StoredFile, Long> {
+ "WHERE f.id = :id")
int bumpContentVersion(@Param("id") Long id);
// Scalar projection: reads the DB, not the stale first-level cache entity.
@Query("SELECT COALESCE(f.contentVersion, 0) FROM StoredFile f WHERE f.id = :id")
Long findContentVersionById(@Param("id") Long id);
// ---- storage encryption at rest ----------------------------------------------------
long countByEncryptionKeyIdIsNull();
@@ -376,7 +376,10 @@ public class FileStorageService {
newVersion = expectedVersion + 1;
} else {
storedFileRepository.bumpContentVersion(existing.getId());
newVersion = existing.contentVersionOrZero() + 1;
// Another writer may have bumped past us; the in-memory value would
// otherwise be flushed back over the SQL-computed one.
Long persisted = storedFileRepository.findContentVersionById(existing.getId());
newVersion = persisted != null ? persisted : existing.contentVersionOrZero() + 1;
}
existing.setContentVersion(newVersion);
return newVersion;
@@ -488,8 +491,16 @@ public class FileStorageService {
Long expectedVersion) {
// Share-aware lookup so EDITOR recipients can write back; replaceFile enforces the role.
StoredFile existing = getAccessibleFile(actor, fileId);
boolean nonOwnerWrite = !isOwner(existing, actor);
StoredFile updated =
replaceFile(actor, existing, file, historyBundle, auditLog, expectedVersion);
if (nonOwnerWrite) {
// Mirrors updateShareLinkResponse so both write paths are attributable.
recordShareAccess(
userShare(existing, actor),
SecurityContextHolder.getContext().getAuthentication(),
FileShareAccessType.EDIT);
}
return buildResponse(updated, actor);
}
@@ -175,6 +175,31 @@ class StoredFileMigrationQueriesDbTest {
.isEqualTo(1L);
}
@Test
void findContentVersionById_readsTheBumpedValueNotTheCachedEntity() {
StoredFile file = persistFile("k-projection", null);
file.setContentVersion(5L);
entityManager.merge(file);
entityManager.flush();
entityManager.clear();
StoredFile loaded = repository.findById(file.getId()).orElseThrow();
repository.bumpContentVersion(loaded.getId());
repository.bumpContentVersion(loaded.getId());
// The entity loaded before the bulk update stays stale; the projection reads the DB.
assertThat(loaded.getContentVersion()).isEqualTo(5L);
assertThat(repository.findContentVersionById(loaded.getId())).isEqualTo(7L);
}
@Test
void findContentVersionById_legacyNullVersionReadsAsZero() {
StoredFile file = persistFile("k-projection-legacy", null);
entityManager.clear();
assertThat(repository.findContentVersionById(file.getId())).isEqualTo(0L);
}
@SpringBootConfiguration
@AutoConfigurationPackage(basePackages = "stirling.software.proprietary")
static class TestApp {}
@@ -17,12 +17,15 @@ import java.util.Optional;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.mockito.junit.jupiter.MockitoSettings;
import org.mockito.quality.Strictness;
import org.springframework.http.HttpStatus;
import org.springframework.mock.web.MockMultipartFile;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;
import org.springframework.transaction.support.TransactionSynchronizationUtils;
@@ -34,6 +37,8 @@ import stirling.software.proprietary.security.model.User;
import stirling.software.proprietary.storage.crypto.StorageEncryptionException;
import stirling.software.proprietary.storage.crypto.StorageKeyRevokedException;
import stirling.software.proprietary.storage.model.FileShare;
import stirling.software.proprietary.storage.model.FileShareAccess;
import stirling.software.proprietary.storage.model.FileShareAccessType;
import stirling.software.proprietary.storage.model.ShareAccessRole;
import stirling.software.proprietary.storage.model.StoredFile;
import stirling.software.proprietary.storage.provider.StorageProvider;
@@ -123,6 +128,17 @@ class FileStorageServiceTest {
return s;
}
/** The service reads the acting principal off the security context to attribute writes. */
private void withAuthenticatedUser(User user, Runnable action) {
SecurityContextHolder.getContext()
.setAuthentication(new UsernamePasswordAuthenticationToken(user, "n/a", List.of()));
try {
action.run();
} finally {
SecurityContextHolder.clearContext();
}
}
// -------------------------------------------------------------------------
// getAccessibleFile
// -------------------------------------------------------------------------
@@ -571,6 +587,49 @@ class FileStorageServiceTest {
verify(storageProvider).store(owner, file);
}
@Test
void updateFileResponse_editorShare_recordsEditAccess() throws IOException {
when(storageProperties.getQuotas()).thenReturn(null);
User owner = user(1L);
User editor = user(2L);
StoredFile existing = ownedFile(owner);
existing.setStorageKey("old-key");
FileShare share = shareFor(existing, editor, ShareAccessRole.EDITOR);
existing.getShares().add(share);
when(storedFileRepository.findByIdWithShares(100L)).thenReturn(Optional.of(existing));
when(fileShareRepository.findByFileAndSharedWithUser(existing, editor))
.thenReturn(Optional.of(share));
when(storageProvider.store(any(), any())).thenReturn(storedObject("new-key"));
when(storedFileRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
MockMultipartFile file =
new MockMultipartFile("file", "test.pdf", "application/pdf", new byte[] {1});
withAuthenticatedUser(editor, () -> service.updateFileResponse(editor, 100L, file));
ArgumentCaptor<FileShareAccess> access = ArgumentCaptor.forClass(FileShareAccess.class);
verify(fileShareAccessRepository).save(access.capture());
assertThat(access.getValue().getAccessType()).isEqualTo(FileShareAccessType.EDIT);
assertThat(access.getValue().getFileShare()).isSameAs(share);
assertThat(access.getValue().getUser()).isSameAs(editor);
}
@Test
void updateFileResponse_owner_recordsNoShareAccess() throws IOException {
when(storageProperties.getQuotas()).thenReturn(null);
User owner = user(1L);
StoredFile existing = ownedFile(owner);
existing.setStorageKey("old-key");
when(storedFileRepository.findByIdWithShares(100L)).thenReturn(Optional.of(existing));
when(storageProvider.store(any(), any())).thenReturn(storedObject("new-key"));
when(storedFileRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
MockMultipartFile file =
new MockMultipartFile("file", "test.pdf", "application/pdf", new byte[] {1});
withAuthenticatedUser(owner, () -> service.updateFileResponse(owner, 100L, file));
verify(fileShareAccessRepository, never()).save(any());
}
@Test
void replaceFile_viewerShare_nonOwnerForbidden() {
User owner = user(1L);
@@ -646,12 +705,32 @@ class FileStorageServiceTest {
existing.setStorageKey("old-key");
when(storageProvider.store(any(), any())).thenReturn(storedObject("new-key"));
when(storedFileRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
// Another writer bumped past us, so the SQL-computed value is not loaded + 1.
when(storedFileRepository.findContentVersionById(100L)).thenReturn(9L);
MockMultipartFile file =
new MockMultipartFile("file", "test.pdf", "application/pdf", new byte[] {1});
StoredFile updated = service.replaceFile(owner, existing, file, null, null, null);
verify(storedFileRepository).bumpContentVersion(100L);
assertThat(updated.getContentVersion()).isEqualTo(9L);
}
@Test
void replaceFile_noExpectedVersion_versionProjectionMissing_fallsBackToLoadedPlusOne()
throws IOException {
when(storageProperties.getQuotas()).thenReturn(null);
User owner = user(1L);
StoredFile existing = ownedFile(owner);
existing.setStorageKey("old-key");
when(storageProvider.store(any(), any())).thenReturn(storedObject("new-key"));
when(storedFileRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
when(storedFileRepository.findContentVersionById(100L)).thenReturn(null);
MockMultipartFile file =
new MockMultipartFile("file", "test.pdf", "application/pdf", new byte[] {1});
StoredFile updated = service.replaceFile(owner, existing, file, null, null, null);
// Legacy null version reads as 0 and increments to 1.
assertThat(updated.getContentVersion()).isEqualTo(1L);
}
@@ -21,6 +21,7 @@ import { Z_INDEX_OVER_FILE_MANAGER_MODAL } from "@app/styles/zIndex";
import { useAppConfig } from "@app/contexts/AppConfigContext";
import type { StirlingFileStub } from "@app/types/fileContext";
import { uploadHistoryChains } from "@app/services/serverStorageUpload";
import { SharedFileConflictError } from "@app/services/sharedFileSave";
import { fileStorage } from "@app/services/fileStorage";
import { useFileActions } from "@app/contexts/FileContext";
import type { FileId } from "@app/types/file";
@@ -117,13 +118,24 @@ const BulkShareModal: React.FC<BulkShareModalProps> = ({
rootIds.length === 1 && remoteIds.length === 1
? remoteIds[0]
: undefined;
// Only guard when the selected files agree on one base version.
const baseVersions = Array.from(
new Set(
files
.filter((file) => file.remoteStorageId === existingRemoteId)
.map((file) => file.remoteVersionBase)
.filter((v): v is number => typeof v === "number"),
),
);
const {
remoteId: storedId,
updatedAt,
version,
chain,
} = await uploadHistoryChains(rootIds, existingRemoteId);
} = await uploadHistoryChains(rootIds, existingRemoteId, {
baseVersion: baseVersions.length === 1 ? baseVersions[0] : undefined,
});
const shareResponse = await createShareLink(storedId);
setShareToken(shareResponse.token ?? null);
@@ -152,6 +164,15 @@ const BulkShareModal: React.FC<BulkShareModalProps> = ({
await onShared();
}
} catch (error: unknown) {
if (error instanceof SharedFileConflictError) {
setErrorMessage(
t(
"storageCollab.conflictBody",
"Someone else saved a newer version since you last synced. You can fetch their version to merge manually, or overwrite it with yours.",
),
);
return;
}
console.error("Failed to generate share link:", error);
setErrorMessage(
t(
@@ -8,6 +8,7 @@ import { alert } from "@app/components/toast";
import { Z_INDEX_OVER_FILE_MANAGER_MODAL } from "@app/styles/zIndex";
import type { StirlingFileStub } from "@app/types/fileContext";
import { uploadHistoryChains } from "@app/services/serverStorageUpload";
import { SharedFileConflictError } from "@app/services/sharedFileSave";
import { fileStorage } from "@app/services/fileStorage";
import { useFileActions } from "@app/contexts/FileContext";
import type { FileId } from "@app/types/file";
@@ -57,10 +58,22 @@ const BulkUploadToServerModal: React.FC<BulkUploadToServerModalProps> = ({
);
const existingRemoteId =
remoteIds.length === 1 ? remoteIds[0] : undefined;
// Only guard when the selected files agree on one base version.
const baseVersions = Array.from(
new Set(
files
.filter((file) => file.remoteStorageId === existingRemoteId)
.map((file) => file.remoteVersionBase)
.filter((v): v is number => typeof v === "number"),
),
);
const { remoteId, updatedAt, chain } = await uploadHistoryChains(
const { remoteId, updatedAt, version, chain } = await uploadHistoryChains(
rootIds,
existingRemoteId,
{
baseVersion: baseVersions.length === 1 ? baseVersions[0] : undefined,
},
);
for (const stub of chain) {
@@ -69,12 +82,16 @@ const BulkUploadToServerModal: React.FC<BulkUploadToServerModalProps> = ({
remoteStorageUpdatedAt: updatedAt,
remoteOwnedByCurrentUser: true,
remoteSharedViaLink: false,
remoteVersionBase: version,
remoteVersionLatest: version,
});
await fileStorage.updateFileMetadata(stub.id, {
remoteStorageId: remoteId,
remoteStorageUpdatedAt: updatedAt,
remoteOwnedByCurrentUser: true,
remoteSharedViaLink: false,
remoteVersionBase: version,
remoteVersionLatest: version,
});
}
@@ -89,6 +106,15 @@ const BulkUploadToServerModal: React.FC<BulkUploadToServerModalProps> = ({
}
onClose();
} catch (error) {
if (error instanceof SharedFileConflictError) {
setErrorMessage(
t(
"storageCollab.conflictBody",
"Someone else saved a newer version since you last synced. You can fetch their version to merge manually, or overwrite it with yours.",
),
);
return;
}
console.error("Failed to upload files to server:", error);
// A 403 means the server has storage turned off (or login disabled,
// which gates storage). Say so plainly instead of the generic
@@ -21,6 +21,7 @@ import { Z_INDEX_OVER_FILE_MANAGER_MODAL } from "@app/styles/zIndex";
import { useAppConfig } from "@app/contexts/AppConfigContext";
import type { StirlingFileStub } from "@app/types/fileContext";
import { uploadHistoryChain } from "@app/services/serverStorageUpload";
import { SharedFileConflictError } from "@app/services/sharedFileSave";
import { fileStorage } from "@app/services/fileStorage";
import { useFileActions } from "@app/contexts/FileContext";
import type { FileId } from "@app/types/file";
@@ -126,7 +127,9 @@ const ShareFileModal: React.FC<ShareFileModalProps> = ({
updatedAt,
version,
chain,
} = await uploadHistoryChain(originalFileId, remoteId);
} = await uploadHistoryChain(originalFileId, remoteId, {
baseVersion: file.remoteVersionBase,
});
storedId = newStoredId;
for (const stub of chain) {
@@ -165,6 +168,15 @@ const ShareFileModal: React.FC<ShareFileModalProps> = ({
await onUploaded();
}
} catch (error: unknown) {
if (error instanceof SharedFileConflictError) {
setErrorMessage(
t(
"storageCollab.conflictBody",
"Someone else saved a newer version since you last synced. You can fetch their version to merge manually, or overwrite it with yours.",
),
);
return;
}
console.error("Failed to generate share link:", error);
setErrorMessage(
t(
@@ -47,8 +47,10 @@ export function useSharedFileActions() {
const fetchLatestCopy = useCallback(
async (file: StirlingFileStub): Promise<boolean> => {
try {
const blob = await downloadSharedBytes(file);
// Version first: if a writer commits between the two calls the bytes are
// newer than the base, which costs a spurious 409, never a lost update.
const latestVersion = await fetchLatestSharedVersion(file);
const blob = await downloadSharedBytes(file);
const latest = new File([blob], file.name, {
type: blob.type || file.type,
});
@@ -49,7 +49,7 @@ async function putExistingFile(
const response = await apiClient.put(
`/api/v1/storage/files/${existingRemoteId}`,
formData,
{ headers: buildUpdateHeaders(options), suppressErrorToast: true } as any,
{ headers: buildUpdateHeaders(options), suppressErrorToast: true },
);
return {
updatedAt: resolveUpdatedAt(response.data?.updatedAt),
@@ -79,7 +79,7 @@ export async function saveSharedFile(
const response = await apiClient.put(url, formData, {
headers,
suppressErrorToast: true,
} as any);
});
const version =
typeof response.data?.version === "number"
? response.data.version
@@ -104,7 +104,7 @@ export async function downloadSharedBytes(
const response = await apiClient.get(url, {
responseType: "blob",
suppressErrorToast: true,
} as any);
});
return response.data as Blob;
}
@@ -116,7 +116,7 @@ export async function fetchLatestSharedVersion(
if (stub.remoteSharedViaLink && stub.remoteShareToken) {
const response = await apiClient.get(
`/api/v1/storage/share-links/${stub.remoteShareToken}/metadata`,
{ suppressErrorToast: true, skipAuthRedirect: true } as any,
{ suppressErrorToast: true, skipAuthRedirect: true },
);
const version = response.data?.version;
return typeof version === "number" ? version : null;
@@ -124,7 +124,7 @@ export async function fetchLatestSharedVersion(
if (stub.remoteStorageId) {
const response = await apiClient.get(
`/api/v1/storage/files/${stub.remoteStorageId}`,
{ suppressErrorToast: true, skipAuthRedirect: true } as any,
{ suppressErrorToast: true, skipAuthRedirect: true },
);
const version = response.data?.version;
return typeof version === "number" ? version : null;