From 558ff4b4c68bebc81e50d6ddf0bb7f3c50421a77 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Fri, 19 Jun 2026 15:55:08 -0400 Subject: [PATCH] fix: close TOCTOU in CREATE path; add anti-forgery, loose-track, and case-sensitivity tests FindOrCreateRelease now returns (ReleaseDto, bool WasCreated); the CREATE path in UploadAsync rejects WasCreated=false as a duplicate rather than silently attaching on a lost race. --- DeepDrftAPI/Services/UnifiedTrackService.cs | 22 ++++++-- DeepDrftData/ITrackService.cs | 6 ++- DeepDrftData/TrackManager.cs | 18 +++---- DeepDrftTests/MediumWritePathTests.cs | 18 +++---- .../UploadDuplicateDetectionTests.cs | 53 +++++++++++++++++-- 5 files changed, 90 insertions(+), 27 deletions(-) diff --git a/DeepDrftAPI/Services/UnifiedTrackService.cs b/DeepDrftAPI/Services/UnifiedTrackService.cs index 1ed985d..5c7ef52 100644 --- a/DeepDrftAPI/Services/UnifiedTrackService.cs +++ b/DeepDrftAPI/Services/UnifiedTrackService.cs @@ -170,11 +170,13 @@ public class UnifiedTrackService CreatedByUserId = createdByUserId, }; - // FindOrCreateRelease's find branch still backstops a concurrent insert of the same - // (title, artist) between the duplicate peek and this call — it returns the winning row - // rather than throwing. Medium and every other field apply only on the create it performs. + // FindOrCreateRelease either creates a fresh release (WasCreated = true) or returns the + // row the concurrent winner just inserted (WasCreated = false). In the CREATE path the + // duplicate peek above already verified no pre-existing row exists — so WasCreated = false + // means we lost a concurrent-insert race. Treat that as the duplicate condition: reject + // rather than silently attaching, keeping the DB unique index as the final safety net. var releaseResult = await _sqlTrackService.FindOrCreateRelease(album, artist, releaseData, ct); - if (!releaseResult.Success || releaseResult.Value is null) + if (!releaseResult.Success) { var error = releaseResult.Messages.FirstOrDefault()?.Message ?? "Unknown error"; _logger.LogError( @@ -183,7 +185,17 @@ public class UnifiedTrackService return ResultContainer.CreateFailResult($"Track was uploaded but could not be saved: {error}"); } - resolvedReleaseId = releaseResult.Value.Id; + var (resolvedRelease, wasCreated) = releaseResult.Value; + if (!wasCreated) + { + // The winning concurrent upload created this release between our peek and our insert. + // Reject with the same marker the pre-flight peek uses so the controller maps it to 409. + return ResultContainer.CreateFailResult( + $"{DuplicateReleaseMarker}A release titled '{resolvedRelease.Title}' by {resolvedRelease.Artist} already " + + "exists. The upload form creates new releases only — use the edit tools to change an existing one."); + } + + resolvedReleaseId = resolvedRelease.Id; } var trackDto = TrackConverter.Convert(unpersisted); diff --git a/DeepDrftData/ITrackService.cs b/DeepDrftData/ITrackService.cs index 2845bba..e55f334 100644 --- a/DeepDrftData/ITrackService.cs +++ b/DeepDrftData/ITrackService.cs @@ -59,8 +59,12 @@ public interface ITrackService /// Resolve the release matching + , creating /// one from when none exists. Backs the upload flow's FK /// resolution so a track lands on a shared release rather than duplicating release-cardinal data. + /// The WasCreated flag in the result is when a new row was inserted + /// and when an existing row was found (including after a lost concurrent-insert + /// race). The CREATE path in UnifiedTrackService.UploadAsync uses this to turn a + /// "found existing" outcome into a duplicate rejection rather than a silent attach. /// - Task> FindOrCreateRelease( + Task> FindOrCreateRelease( string title, string artist, ReleaseDto releaseData, CancellationToken cancellationToken = default); /// diff --git a/DeepDrftData/TrackManager.cs b/DeepDrftData/TrackManager.cs index a04673c..2e826db 100644 --- a/DeepDrftData/TrackManager.cs +++ b/DeepDrftData/TrackManager.cs @@ -164,14 +164,14 @@ public class TrackManager } } - public async Task> FindOrCreateRelease( + public async Task> FindOrCreateRelease( string title, string artist, ReleaseDto releaseData, CancellationToken cancellationToken = default) { try { var existing = await Repository.GetReleaseByTitleAndArtistAsync(title, artist, cancellationToken); if (existing is not null) - return ResultContainer.CreatePassResult(TrackConverter.Convert(existing)); + return ResultContainer<(ReleaseDto, bool)>.CreatePassResult((TrackConverter.Convert(existing), false)); // The natural key (title + artist) is authoritative — override whatever the caller put // in releaseData so a typo upstream cannot create a release that won't be found again. @@ -186,21 +186,21 @@ public class TrackManager try { var added = await Repository.AddReleaseAsync(entity, cancellationToken); - return ResultContainer.CreatePassResult(TrackConverter.Convert(added)); + return ResultContainer<(ReleaseDto, bool)>.CreatePassResult((TrackConverter.Convert(added), true)); } catch (ClassifiedDbException ex) when (ex.Error.Category == DbErrorCategory.UniqueViolation) { // Concurrent upload inserted the same (title, artist) between our read and write. - // Re-query and return the winning row. Should not return null here since the - // constraint just fired, but re-throw if it does so the caller sees an error. + // Re-query and return the winning row as WasCreated=false so the caller (UploadAsync + // CREATE path) treats the lost race as a duplicate rather than silently attaching. var race = await Repository.GetReleaseByTitleAndArtistAsync(title, artist, cancellationToken); if (race is null) throw; - return ResultContainer.CreatePassResult(TrackConverter.Convert(race)); + return ResultContainer<(ReleaseDto, bool)>.CreatePassResult((TrackConverter.Convert(race), false)); } } catch (Exception e) { - return ResultContainer.CreateFailResult(e.Message); + return ResultContainer<(ReleaseDto, bool)>.CreateFailResult(e.Message); } } @@ -302,13 +302,13 @@ public class TrackManager if (newTrack.Release is { } release && !string.IsNullOrWhiteSpace(release.Title)) { var resolved = await FindOrCreateRelease(release.Title, release.Artist, release); - if (!resolved.Success || resolved.Value is null) + if (!resolved.Success) { var error = resolved.Messages.FirstOrDefault()?.Message ?? "Failed to resolve release."; return ResultContainer.CreateFailResult(error); } - newTrack.ReleaseId = resolved.Value.Id; + newTrack.ReleaseId = resolved.Value.Release.Id; } var added = await Repository.AddAsync(TrackConverter.Convert(newTrack)); diff --git a/DeepDrftTests/MediumWritePathTests.cs b/DeepDrftTests/MediumWritePathTests.cs index 6fd3ffd..038ea98 100644 --- a/DeepDrftTests/MediumWritePathTests.cs +++ b/DeepDrftTests/MediumWritePathTests.cs @@ -60,9 +60,9 @@ public class MediumWritePathTests "Live at the Vault", "Artist A", ReleaseData("Live at the Vault", "Artist A", ReleaseMedium.Session)); Assert.That(result.Success, Is.True); - Assert.That(result.Value!.Medium, Is.EqualTo(ReleaseMedium.Session)); + Assert.That(result.Value.Release.Medium, Is.EqualTo(ReleaseMedium.Session)); - var stored = await CreateRepository().GetReleaseByIdAsync(result.Value.Id); + var stored = await CreateRepository().GetReleaseByIdAsync(result.Value.Release.Id); Assert.That(stored!.Medium, Is.EqualTo(ReleaseMedium.Session)); } @@ -75,7 +75,7 @@ public class MediumWritePathTests var result = await manager.FindOrCreateRelease( "Sunset Set", "DJ B", ReleaseData("Sunset Set", "DJ B", ReleaseMedium.Mix)); - Assert.That(result.Value!.Medium, Is.EqualTo(ReleaseMedium.Mix)); + Assert.That(result.Value.Release.Medium, Is.EqualTo(ReleaseMedium.Mix)); } // 9.5.A — a Cut upload (the default) creates a release carrying Medium == Cut. @@ -87,7 +87,7 @@ public class MediumWritePathTests var result = await manager.FindOrCreateRelease( "Studio Album", "Artist C", ReleaseData("Studio Album", "Artist C", ReleaseMedium.Cut)); - Assert.That(result.Value!.Medium, Is.EqualTo(ReleaseMedium.Cut)); + Assert.That(result.Value.Release.Medium, Is.EqualTo(ReleaseMedium.Cut)); } // 9.5.A — a second upload to an existing release does NOT mutate the stored medium. The first @@ -105,10 +105,10 @@ public class MediumWritePathTests var found = await manager.FindOrCreateRelease( "Live at the Vault", "Artist A", ReleaseData("Live at the Vault", "Artist A", ReleaseMedium.Cut)); - Assert.That(found.Value!.Id, Is.EqualTo(created.Value!.Id), "same release row is returned"); - Assert.That(found.Value.Medium, Is.EqualTo(ReleaseMedium.Session), "medium stays as first set"); + Assert.That(found.Value.Release.Id, Is.EqualTo(created.Value.Release.Id), "same release row is returned"); + Assert.That(found.Value.Release.Medium, Is.EqualTo(ReleaseMedium.Session), "medium stays as first set"); - var stored = await CreateRepository().GetReleaseByIdAsync(created.Value.Id); + var stored = await CreateRepository().GetReleaseByIdAsync(created.Value.Release.Id); Assert.That(stored!.Medium, Is.EqualTo(ReleaseMedium.Session), "DB row unchanged"); } @@ -207,9 +207,9 @@ public class MediumWritePathTests var result = await manager.FindOrCreateRelease("Studio Album", "Artist C", data); Assert.That(result.Success, Is.True); - Assert.That(result.Value!.Description, Is.EqualTo(prose)); + Assert.That(result.Value.Release.Description, Is.EqualTo(prose)); - var stored = await CreateRepository().GetReleaseByIdAsync(result.Value.Id); + var stored = await CreateRepository().GetReleaseByIdAsync(result.Value.Release.Id); Assert.That(stored!.Description, Is.EqualTo(prose)); } diff --git a/DeepDrftTests/UploadDuplicateDetectionTests.cs b/DeepDrftTests/UploadDuplicateDetectionTests.cs index d8b5c9b..ab8fceb 100644 --- a/DeepDrftTests/UploadDuplicateDetectionTests.cs +++ b/DeepDrftTests/UploadDuplicateDetectionTests.cs @@ -179,11 +179,58 @@ public class UploadDuplicateDetectionTests Assert.That(message, Does.StartWith(UnifiedTrackService.CardinalityViolationMarker)); } + // ATTACH anti-forgery guard: when the caller supplies a releaseId that does NOT match the release + // the natural key (title, artist) resolves to, the upload is rejected. Guards against a stale or + // forged releaseId pointing at a different (title, artist) than this row carries. + [Test] + public async Task UploadAsync_AttachWithMismatchedReleaseId_IsRejectedWithDuplicateMarker() + { + var manager = CreateManager(); + var service = await CreateUnifiedServiceAsync(manager); + + // Create two separate releases so we have two distinct ids. + var releaseA = await UploadAsync( + service, await WriteWavAsync(2.0), "Track One", "Artist A", "Release A", ReleaseMedium.Cut, releaseId: null); + Assert.That(releaseA.Success, Is.True, "release A must be created"); + var idA = releaseA.Value!.ReleaseId!.Value; + + var releaseB = await UploadAsync( + service, await WriteWavAsync(2.0), "Track One", "Artist B", "Release B", ReleaseMedium.Cut, releaseId: null); + Assert.That(releaseB.Success, Is.True, "release B must be created"); + + // Try to ATTACH to release A while carrying release B's (title, artist). The natural-key lookup + // resolves to B — id A ≠ B.Id → anti-forgery guard fires. + var forged = await UploadAsync( + service, await WriteWavAsync(2.0), "Track Two", "Artist B", "Release B", ReleaseMedium.Cut, releaseId: idA); + + Assert.That(forged.Success, Is.False); + var message = forged.Messages.FirstOrDefault()?.Message ?? string.Empty; + Assert.That(message, Does.StartWith(UnifiedTrackService.DuplicateReleaseMarker)); + } + + // Loose-track success: an upload with null/whitespace album stays release-less (ReleaseId null). + // Confirms the duplicate guard is correctly bypassed for tracks that carry no album. + [Test] + public async Task UploadAsync_NullAlbum_SucceedsAsLooseTrack() + { + var service = await CreateUnifiedServiceAsync(CreateManager()); + + var result = await UploadAsync( + service, await WriteWavAsync(2.0), "Standalone Cut", "DJ Solo", album: null, ReleaseMedium.Cut, releaseId: null); + + Assert.That(result.Success, Is.True, result.Messages.FirstOrDefault()?.Message); + Assert.That(result.Value!.ReleaseId, Is.Null, "a null-album track must stay a loose track with no release"); + } + + // Case-sensitivity caveat: the assertion below verifies ordinal == equality as implemented by the + // EF in-memory provider (which evaluates LINQ predicates in-process). The deployed PostgreSQL + // instance may use a different column collation (e.g. case-insensitive) — production case-sensitivity + // depends on the collation of the `release` table's `title` and `artist` columns, not on this test. // Matching semantics: GetReleaseByTitleAndArtist (the read both the pre-flight and the create-path // duplicate guard use) is exact — a case difference is NOT a match, so it does not trip the block. // This asserts the pre-flight and the create path agree by using the one shared read. [Test] - public async Task UploadAsync_CaseDifferentTitle_IsNotADuplicate() + public async Task UploadAsync_CaseDifferentTitle_IsNotADuplicateOnInMemoryProvider() { var service = await CreateUnifiedServiceAsync(CreateManager()); @@ -191,8 +238,8 @@ public class UploadDuplicateDetectionTests service, await WriteWavAsync(2.0), "Studio Album", "Artist C", "Studio Album", ReleaseMedium.Cut, releaseId: null); Assert.That(first.Success, Is.True); - // Different case → not the same natural key → admitted as a new release (matches the create - // path's exact == comparison; no normalization anywhere). + // Different case → not the same natural key under the in-memory provider's ordinal == → + // admitted as a new release. Production outcome depends on PostgreSQL column collation. var differentCase = await UploadAsync( service, await WriteWavAsync(2.0), "Studio Album", "Artist C", "STUDIO ALBUM", ReleaseMedium.Cut, releaseId: null);