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.
This commit is contained in:
daniel-c-harvey
2026-06-19 15:55:08 -04:00
parent bd85507308
commit 558ff4b4c6
5 changed files with 90 additions and 27 deletions
+17 -5
View File
@@ -170,11 +170,13 @@ public class UnifiedTrackService
CreatedByUserId = createdByUserId, CreatedByUserId = createdByUserId,
}; };
// FindOrCreateRelease's find branch still backstops a concurrent insert of the same // FindOrCreateRelease either creates a fresh release (WasCreated = true) or returns the
// (title, artist) between the duplicate peek and this call — it returns the winning row // row the concurrent winner just inserted (WasCreated = false). In the CREATE path the
// rather than throwing. Medium and every other field apply only on the create it performs. // 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); 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"; var error = releaseResult.Messages.FirstOrDefault()?.Message ?? "Unknown error";
_logger.LogError( _logger.LogError(
@@ -183,7 +185,17 @@ public class UnifiedTrackService
return ResultContainer<TrackDto>.CreateFailResult($"Track was uploaded but could not be saved: {error}"); return ResultContainer<TrackDto>.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<TrackDto>.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); var trackDto = TrackConverter.Convert(unpersisted);
+5 -1
View File
@@ -59,8 +59,12 @@ public interface ITrackService
/// Resolve the release matching <paramref name="title"/> + <paramref name="artist"/>, creating /// Resolve the release matching <paramref name="title"/> + <paramref name="artist"/>, creating
/// one from <paramref name="releaseData"/> when none exists. Backs the upload flow's FK /// one from <paramref name="releaseData"/> when none exists. Backs the upload flow's FK
/// resolution so a track lands on a shared release rather than duplicating release-cardinal data. /// resolution so a track lands on a shared release rather than duplicating release-cardinal data.
/// The <c>WasCreated</c> flag in the result is <see langword="true"/> when a new row was inserted
/// and <see langword="false"/> when an existing row was found (including after a lost concurrent-insert
/// race). The CREATE path in <c>UnifiedTrackService.UploadAsync</c> uses this to turn a
/// "found existing" outcome into a duplicate rejection rather than a silent attach.
/// </summary> /// </summary>
Task<ResultContainer<ReleaseDto>> FindOrCreateRelease( Task<ResultContainer<(ReleaseDto Release, bool WasCreated)>> FindOrCreateRelease(
string title, string artist, ReleaseDto releaseData, CancellationToken cancellationToken = default); string title, string artist, ReleaseDto releaseData, CancellationToken cancellationToken = default);
/// <summary> /// <summary>
+9 -9
View File
@@ -164,14 +164,14 @@ public class TrackManager
} }
} }
public async Task<ResultContainer<ReleaseDto>> FindOrCreateRelease( public async Task<ResultContainer<(ReleaseDto Release, bool WasCreated)>> FindOrCreateRelease(
string title, string artist, ReleaseDto releaseData, CancellationToken cancellationToken = default) string title, string artist, ReleaseDto releaseData, CancellationToken cancellationToken = default)
{ {
try try
{ {
var existing = await Repository.GetReleaseByTitleAndArtistAsync(title, artist, cancellationToken); var existing = await Repository.GetReleaseByTitleAndArtistAsync(title, artist, cancellationToken);
if (existing is not null) if (existing is not null)
return ResultContainer<ReleaseDto>.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 // 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. // in releaseData so a typo upstream cannot create a release that won't be found again.
@@ -186,21 +186,21 @@ public class TrackManager
try try
{ {
var added = await Repository.AddReleaseAsync(entity, cancellationToken); var added = await Repository.AddReleaseAsync(entity, cancellationToken);
return ResultContainer<ReleaseDto>.CreatePassResult(TrackConverter.Convert(added)); return ResultContainer<(ReleaseDto, bool)>.CreatePassResult((TrackConverter.Convert(added), true));
} }
catch (ClassifiedDbException ex) when (ex.Error.Category == DbErrorCategory.UniqueViolation) catch (ClassifiedDbException ex) when (ex.Error.Category == DbErrorCategory.UniqueViolation)
{ {
// Concurrent upload inserted the same (title, artist) between our read and write. // 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 // Re-query and return the winning row as WasCreated=false so the caller (UploadAsync
// constraint just fired, but re-throw if it does so the caller sees an error. // CREATE path) treats the lost race as a duplicate rather than silently attaching.
var race = await Repository.GetReleaseByTitleAndArtistAsync(title, artist, cancellationToken); var race = await Repository.GetReleaseByTitleAndArtistAsync(title, artist, cancellationToken);
if (race is null) throw; if (race is null) throw;
return ResultContainer<ReleaseDto>.CreatePassResult(TrackConverter.Convert(race)); return ResultContainer<(ReleaseDto, bool)>.CreatePassResult((TrackConverter.Convert(race), false));
} }
} }
catch (Exception e) catch (Exception e)
{ {
return ResultContainer<ReleaseDto>.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)) if (newTrack.Release is { } release && !string.IsNullOrWhiteSpace(release.Title))
{ {
var resolved = await FindOrCreateRelease(release.Title, release.Artist, release); 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."; var error = resolved.Messages.FirstOrDefault()?.Message ?? "Failed to resolve release.";
return ResultContainer<TrackDto>.CreateFailResult(error); return ResultContainer<TrackDto>.CreateFailResult(error);
} }
newTrack.ReleaseId = resolved.Value.Id; newTrack.ReleaseId = resolved.Value.Release.Id;
} }
var added = await Repository.AddAsync(TrackConverter.Convert(newTrack)); var added = await Repository.AddAsync(TrackConverter.Convert(newTrack));
+9 -9
View File
@@ -60,9 +60,9 @@ public class MediumWritePathTests
"Live at the Vault", "Artist A", ReleaseData("Live at the Vault", "Artist A", ReleaseMedium.Session)); "Live at the Vault", "Artist A", ReleaseData("Live at the Vault", "Artist A", ReleaseMedium.Session));
Assert.That(result.Success, Is.True); 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)); Assert.That(stored!.Medium, Is.EqualTo(ReleaseMedium.Session));
} }
@@ -75,7 +75,7 @@ public class MediumWritePathTests
var result = await manager.FindOrCreateRelease( var result = await manager.FindOrCreateRelease(
"Sunset Set", "DJ B", ReleaseData("Sunset Set", "DJ B", ReleaseMedium.Mix)); "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. // 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( var result = await manager.FindOrCreateRelease(
"Studio Album", "Artist C", ReleaseData("Studio Album", "Artist C", ReleaseMedium.Cut)); "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 // 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( var found = await manager.FindOrCreateRelease(
"Live at the Vault", "Artist A", ReleaseData("Live at the Vault", "Artist A", ReleaseMedium.Cut)); "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.Release.Id, Is.EqualTo(created.Value.Release.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.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"); 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); var result = await manager.FindOrCreateRelease("Studio Album", "Artist C", data);
Assert.That(result.Success, Is.True); 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)); Assert.That(stored!.Description, Is.EqualTo(prose));
} }
+50 -3
View File
@@ -179,11 +179,58 @@ public class UploadDuplicateDetectionTests
Assert.That(message, Does.StartWith(UnifiedTrackService.CardinalityViolationMarker)); 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 // 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. // 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. // This asserts the pre-flight and the create path agree by using the one shared read.
[Test] [Test]
public async Task UploadAsync_CaseDifferentTitle_IsNotADuplicate() public async Task UploadAsync_CaseDifferentTitle_IsNotADuplicateOnInMemoryProvider()
{ {
var service = await CreateUnifiedServiceAsync(CreateManager()); 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); service, await WriteWavAsync(2.0), "Studio Album", "Artist C", "Studio Album", ReleaseMedium.Cut, releaseId: null);
Assert.That(first.Success, Is.True); Assert.That(first.Success, Is.True);
// Different case → not the same natural key → admitted as a new release (matches the create // Different case → not the same natural key under the in-memory provider's ordinal == →
// path's exact == comparison; no normalization anywhere). // admitted as a new release. Production outcome depends on PostgreSQL column collation.
var differentCase = await UploadAsync( var differentCase = await UploadAsync(
service, await WriteWavAsync(2.0), "Studio Album", "Artist C", "STUDIO ALBUM", ReleaseMedium.Cut, releaseId: null); service, await WriteWavAsync(2.0), "Studio Album", "Artist C", "STUDIO ALBUM", ReleaseMedium.Cut, releaseId: null);