One StoreFileName in Core: no id collisions, no '..' escape #11

Merged
aqua merged 5 commits from aqua/safe-filename-collide-dotdot into main 2026-11-07 10:39:10 +00:00
Member

Fixes follow-up daemon-safefilename-collide-and-dotdot.

Bug. AquaSynthDaemonService.SafeFileName mapped a/b, a b, a:b, a-b to one name (later patch overwrites earlier artifact at StoreRoot/<family>/<id>.cc) and returned .. unchanged, so a sessionId/runId of .. resolved one directory above the store. NativeFaustRuntime had a second, different copy.

Fix. One AquaSynth.Dsl.StoreFileName.From in AquaSynth.Core; both copies deleted. Callers: ids in-repo are clean, but ids reach the daemon as free strings from protocol callers (the old code already accepted them), so I kept sanitising rather than rejecting: ids within [A-Za-z0-9._-] pass through; otherwise sanitise and append first 8 hex of SHA-256 of the original id. Empty, . and all-dot ids throw ArgumentException. A trailing dot also gets the hash (Windows drops it).

Behaviour change. Stored names for ids with odd chars gain a hash suffix; leading/trailing - is no longer trimmed. Clean ids unchanged.

Not touched. IpaGestureExperiment.SafeFileName is a third copy (experiment output dirs, not the daemon store); left alone, out of the brief's scope.

Tests (StoreFileNameTests): a/b, 'a b', a:b, a-b distinct; clean ids unchanged; hash independently computed; ./../empty refused; traversal ids cannot escape via Path.Combine; daemon receipt for commandId ../../escape stays inside the store.

Suite dotnet test tests/AquaSynth.Dsl.Tests: 246 passed, 2 failed, 2 skipped. The 2 failures (ZynReferenceParityTests.ZynAddSubFxReferenceSourceIsPinnedForTestOnlyParity, SpeechDistributedTrainingTests.CultMeshLeasedWorkersScoreButAuthorityTrainerAppliesCheckpoint) fail identically on a clean origin/main worktree.

Not merged; for review.

🤖 Generated with Claude Code

Fixes follow-up `daemon-safefilename-collide-and-dotdot`. **Bug.** `AquaSynthDaemonService.SafeFileName` mapped `a/b`, `a b`, `a:b`, `a-b` to one name (later patch overwrites earlier artifact at `StoreRoot/<family>/<id>.cc`) and returned `..` unchanged, so a sessionId/runId of `..` resolved one directory above the store. `NativeFaustRuntime` had a second, different copy. **Fix.** One `AquaSynth.Dsl.StoreFileName.From` in AquaSynth.Core; both copies deleted. Callers: ids in-repo are clean, but ids reach the daemon as free strings from protocol callers (the old code already accepted them), so I kept sanitising rather than rejecting: ids within `[A-Za-z0-9._-]` pass through; otherwise sanitise and append first 8 hex of SHA-256 of the original id. Empty, `.` and all-dot ids throw `ArgumentException`. A trailing dot also gets the hash (Windows drops it). **Behaviour change.** Stored names for ids with odd chars gain a hash suffix; leading/trailing `-` is no longer trimmed. Clean ids unchanged. **Not touched.** `IpaGestureExperiment.SafeFileName` is a third copy (experiment output dirs, not the daemon store); left alone, out of the brief's scope. **Tests** (`StoreFileNameTests`): a/b, 'a b', a:b, a-b distinct; clean ids unchanged; hash independently computed; `.`/`..`/empty refused; traversal ids cannot escape via Path.Combine; daemon receipt for commandId `../../escape` stays inside the store. **Suite** `dotnet test tests/AquaSynth.Dsl.Tests`: 246 passed, 2 failed, 2 skipped. The 2 failures (`ZynReferenceParityTests.ZynAddSubFxReferenceSourceIsPinnedForTestOnlyParity`, `SpeechDistributedTrainingTests.CultMeshLeasedWorkersScoreButAuthorityTrainerAppliesCheckpoint`) fail identically on a clean origin/main worktree. Not merged; for review. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
The daemon and native-runtime SafeFileName copies sanitised differently:
the daemon mapped a/b, a b, a:b and a-b to the same name (a later patch
overwrote an earlier artifact) and returned ".." unchanged, so a
sessionId or runId of ".." resolved one directory above the store.

Replace both with AquaSynth.Dsl.StoreFileName.From. Ids within
[A-Za-z0-9._-] pass through; any other id is sanitised and suffixed with
the first 8 hex of SHA-256 of the original. Empty, "." and all-dot ids
are refused (ArgumentException).

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
nibu left a comment

Read-only review of PR #11 at 396da10; nothing changed or merged. Verified by building the head against CultLib 42dce36 and running the PR's StoreFileNameTests (17 pass) plus a scratch probe (deleted).

Does it still escape the store? Yes, one route: samples/{renderId}.* (daemon:664-665), unsanitised. Separators of both kinds, drive letters, UNC, NUL, Unicode and lookalikes cannot escape through From: it whitelists ASCII [A-Za-z0-9._-], replaces the rest with _, and refuses all-dot ids. Reproduced: C:, \\srv\share, \0x, e\u0301, fullwidth dots, bidi override all come out as flat names.

One implementation? Yes for the store: the daemon's other sites and NativeFaustRuntime call StoreFileName.From, so they agree byte for byte by construction (same function, one assembly). Exception: renderId above. Native runtime behaviour change: empty patchId used to become aquasynth_patch; it now throws.

Hash suffix collisions: yes, see line 30. Tests: distinctness real but narrow; daemon test does not pin the fix. Third copy: live, see IpaGestureExperiment.

Not done: Windows run, success-path sample write (no libfaust here), the PR's 2 failing suite tests.

Read-only review of PR #11 at 396da10; nothing changed or merged. Verified by building the head against CultLib 42dce36 and running the PR's StoreFileNameTests (17 pass) plus a scratch probe (deleted). **Does it still escape the store?** Yes, one route: `samples/{renderId}.*` (daemon:664-665), unsanitised. Separators of both kinds, drive letters, UNC, NUL, Unicode and lookalikes cannot escape through `From`: it whitelists ASCII `[A-Za-z0-9._-]`, replaces the rest with `_`, and refuses all-dot ids. Reproduced: `C:`, `\\srv\share`, `\0x`, `e\u0301`, fullwidth dots, bidi override all come out as flat names. **One implementation?** Yes for the store: the daemon's other sites and `NativeFaustRuntime` call `StoreFileName.From`, so they agree byte for byte by construction (same function, one assembly). Exception: `renderId` above. Native runtime behaviour change: empty `patchId` used to become `aquasynth_patch`; it now throws. **Hash suffix collisions:** yes, see line 30. **Tests:** distinctness real but narrow; daemon test does not pin the fix. **Third copy:** live, see IpaGestureExperiment. Not done: Windows run, success-path sample write (no libfaust here), the PR's 2 failing suite tests.
Member

Third copy is a live hole until it moves (Medium for callers of WriteRound, which is public). SafeFileName("..") returns .. (only separators/whitespace are replaced), so WriteRound(root, "..", ...) at line 111 writes candidates/ and timelines/ one directory above rootDirectory. It also collides ids (a b/a-b/a\tb) and has no reserved-name handling. Callers today pass targetSet.Id from built-in sets (IpaTrialOrchestrator.cs:137), but trialSets is a public parameter. IpaTrialOrchestrator.cs:127,277 also does Path.Combine(artifactRoot, options.BatchId) raw, so an absolute or .. BatchId escapes artifactRoot. Moving this to StoreFileName.From is a two-line change and should ride this PR.

**Third copy is a live hole until it moves (Medium for callers of `WriteRound`, which is public).** `SafeFileName("..")` returns `..` (only separators/whitespace are replaced), so `WriteRound(root, "..", ...)` at line 111 writes `candidates/` and `timelines/` one directory above `rootDirectory`. It also collides ids (`a b`/`a-b`/`a\tb`) and has no reserved-name handling. Callers today pass `targetSet.Id` from built-in sets (`IpaTrialOrchestrator.cs:137`), but `trialSets` is a public parameter. `IpaTrialOrchestrator.cs:127,277` also does `Path.Combine(artifactRoot, options.BatchId)` raw, so an absolute or `..` BatchId escapes `artifactRoot`. Moving this to `StoreFileName.From` is a two-line change and should ride this PR.
@ -0,0 +22,4 @@
var builder = new StringBuilder(id.Length);
foreach (var c in id)
{
builder.Append(c is >= 'a' and <= 'z' or >= 'A' and <= 'Z' or >= '0' and <= '9' or '-' or '_' or '.' ? c : '_');
Member

Case-insensitive filesystems (Medium, reproduced). A and a both pass through unchanged (From("A")=A, From("a")=a), so on NTFS/APFS defaults they are the same file. The test compares with OrdinalIgnoreCase but never feeds two ids differing only by case, so it cannot see this. Fix: hash when the id has any uppercase, or lowercase-fold into the hash input.

**Case-insensitive filesystems (Medium, reproduced).** `A` and `a` both pass through unchanged (`From("A")=A`, `From("a")=a`), so on NTFS/APFS defaults they are the same file. The test compares with `OrdinalIgnoreCase` but never feeds two ids differing only by case, so it cannot see this. Fix: hash when the id has any uppercase, or lowercase-fold into the hash input.
@ -0,0 +27,4 @@
var safe = builder.ToString();
// A trailing dot is dropped by Windows, so "a." would alias "a"; hash it like any other rewrite.
if (safe == id && !id.EndsWith('.'))
Member

Pass-through names collide with rewritten names (Medium, reproduced). From("a b") is a_b-c8687a08; that string is itself clean, so From("a_b-c8687a08") returns the same name. Two ids, one file, and the later receipt overwrites the earlier. The suffix is deterministic and unkeyed, so anyone who can choose ids can craft this; the claim 'distinct ids stay distinct' does not hold. Rewritten names also only carry 32 bits (birthday ~65k ids among rewritten ones, preimage for a chosen target ~2^32). Options: put the hash in a form a clean id cannot spell (e.g. refuse or escape ids that already match -[0-9a-f]{8}$), or always hash when the id is not a fixed-point, and use more bits.

**Pass-through names collide with rewritten names (Medium, reproduced).** `From("a b")` is `a_b-c8687a08`; that string is itself clean, so `From("a_b-c8687a08")` returns the same name. Two ids, one file, and the later receipt overwrites the earlier. The suffix is deterministic and unkeyed, so anyone who can choose ids can craft this; the claim 'distinct ids stay distinct' does not hold. Rewritten names also only carry 32 bits (birthday ~65k ids among rewritten ones, preimage for a chosen target ~2^32). Options: put the hash in a form a clean id cannot spell (e.g. refuse or escape ids that already match `-[0-9a-f]{8}$`), or always hash when the id is not a fixed-point, and use more bits.
@ -0,0 +29,4 @@
// A trailing dot is dropped by Windows, so "a." would alias "a"; hash it like any other rewrite.
if (safe == id && !id.EndsWith('.'))
{
return safe;
Member

Windows reserved device names pass through (Medium on Windows, unproven on Linux). CON, nul, com1.x return unchanged (reproduced), and the daemon appends .cc/.f32/.dsp. On Windows CON.cc, NUL.cc, COM1.f32 still resolve to the device (extension is ignored), so a receipt is lost or errors. Treat the reserved stems (CON PRN AUX NUL COM0-9 LPT0-9, case-insensitive, any extension) as a rewrite. Not run on Windows here.

**Windows reserved device names pass through (Medium on Windows, unproven on Linux).** `CON`, `nul`, `com1.x` return unchanged (reproduced), and the daemon appends `.cc`/`.f32`/`.dsp`. On Windows `CON.cc`, `NUL.cc`, `COM1.f32` still resolve to the device (extension is ignored), so a receipt is lost or errors. Treat the reserved stems (CON PRN AUX NUL COM0-9 LPT0-9, case-insensitive, any extension) as a rewrite. Not run on Windows here.
@ -0,0 +33,4 @@
}
var hash = Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(id)))[..8].ToLowerInvariant();
return $"{safe}-{hash}";
Member

No length bound (Low). A 400-char clean id returns a 400-char name (reproduced); with .cc that exceeds the 255-byte component limit and throws IOException mid-command after earlier receipts were written. Not an escape. Cap the stem and put the hash in the tail. Also the refusal of ./../"" surfaces as an unhandled ArgumentException from SampleAsync; fine if the protocol layer maps it to a failed receipt, but I did not see that mapping.

**No length bound (Low).** A 400-char clean id returns a 400-char name (reproduced); with `.cc` that exceeds the 255-byte component limit and throws `IOException` mid-command after earlier receipts were written. Not an escape. Cap the stem and put the hash in the tail. Also the refusal of `.`/`..`/`""` surfaces as an unhandled `ArgumentException` from `SampleAsync`; fine if the protocol layer maps it to a failed receipt, but I did not see that mapping.
Member

Escape still open (High, confirmed by code; success path not executed here: no libfaust on the review host). renderId = RenderId(command.CommandId) is $"{commandId}-render" (line 1128) and is used raw at 664-665: samples/{renderId}.f32 and .wav. A commandId of ../../x on a patch that compiles writes store/samples/../../x-render.f32|wav, i.e. outside the store. The PR migrated the receipt and live/stream sites but not this one. Fix: StoreFileName.From(renderId) here. The PR's daemon test cannot see it: it uses invalid syntax, so the compile-failed branch returns before this code.

**Escape still open (High, confirmed by code; success path not executed here: no libfaust on the review host).** `renderId = RenderId(command.CommandId)` is `$"{commandId}-render"` (line 1128) and is used raw at 664-665: `samples/{renderId}.f32` and `.wav`. A commandId of `../../x` on a patch that compiles writes `store/samples/../../x-render.f32|wav`, i.e. outside the store. The PR migrated the receipt and live/stream sites but not this one. Fix: `StoreFileName.From(renderId)` here. The PR's daemon test cannot see it: it uses invalid syntax, so the compile-failed branch returns before this code.
@ -0,0 +5,4 @@
public sealed class StoreFileNameTests
{
[Fact]
public void DistinctIdsGetDistinctNames()
Member

Distinctness test is real but narrow. The seven names are genuinely distinct (hashes differ), so it would fail on the old collapse. It does not cover the collision above (a rewritten id's output as an id), case-only variants, or reserved names, which is where the implementation is weak. Add those as the failing cases.

**Distinctness test is real but narrow.** The seven names are genuinely distinct (hashes differ), so it would fail on the old collapse. It does not cover the collision above (a rewritten id's output as an id), case-only variants, or reserved names, which is where the implementation is weak. Add those as the failing cases.
@ -0,0 +59,4 @@
using var service = new AquaSynthDaemonService(new AquaSynthDaemonOptions(storeRoot));
await service.SampleAsync(new AquaSynthInstrumentSampleCommand(
"../../escape", "test.invalid", "test_invalid", "this is not aqua syntax", DurationSeconds: 0.1f));
Member

This daemon test does not pin the fix. The old SafeFileName("../../escape") mapped / to - and gave ..-..-escape, which does not escape, so I expect this test passes on the base commit too (reasoned from the old code, not rerun). The old hole was exactly .. as an id. Also the compile-failed branch is the only one exercised. Use commandId .. and a commandId with a compiling patch (when Faust is present) that reaches the samples/ write.

**This daemon test does not pin the fix.** The old `SafeFileName("../../escape")` mapped `/` to `-` and gave `..-..-escape`, which does not escape, so I expect this test passes on the base commit too (reasoned from the old code, not rerun). The old hole was exactly `..` as an id. Also the compile-failed branch is the only one exercised. Use commandId `..` and a commandId with a compiling patch (when Faust is present) that reaches the `samples/` write.
- Pass-through ids may not end in "-<16 hex>", so a rewritten name fed back as an id cannot alias it; hash widened 32 -> 64 bits.
- Ids with uppercase are rewritten (case-insensitive filesystems).
- Windows device stems (CON PRN AUX NUL COM0-9 LPT0-9, any extension) are rewritten.
- Stem capped at 100 chars; longer ids carry the hash.
- Daemon samples/ write now goes through StoreFileName.From(renderId).
- Tests: rewritten-as-id, case-only, device stems, overlong; daemon test uses a compiling patch so the samples/ write is reached when Faust is present.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Author
Member

Reply to Nibu's review at 396da10. Fixes pushed as abfc816.

Real, fixed

  • renderId escape (High). samples/{renderId}.f32|.wav now uses StoreFileName.From(renderId). The logical RenderId in the receipt is unchanged.
  • Pass-through vs rewritten collision. Pass-through ids can no longer end in -<16 hex>, which is the shape every rewritten name ends in, so the two sets are disjoint. The hash is widened from 32 to 64 bits.
  • Case-insensitive filesystems. Any id with an uppercase letter is rewritten, so A and a differ. This changes existing names for ids that had uppercase (they gain a hash suffix).
  • Windows device stems. CON, PRN, AUX, NUL, COM0-9 and LPT0-9 are rewritten, case-insensitive, with any extension.
  • Length. The stem is capped at 100 characters and longer ids carry the hash, so stem-hash.cc stays under 255.
  • Tests. New cases: a rewritten output fed back as an id, case-only variants, device stems, and overlong ids that stay distinct. The daemon test now sends a compiling patch with a ../../escape commandId, so it reaches the samples/ write and asserts the receipt URIs sit directly under the store.

Declined

  • Third copy, IpaGestureExperiment.SafeFileName, and the raw BatchId in Path.Combine. Real, but scoped out by follow-up third-safefilename-ipa-gesture, which runs after this PR merges. I did not widen the diff before Nibu's pass.
  • ArgumentException surfacing from SampleAsync. Not introduced here. ValidateSample's Require already throws on empty ids, and AquaSynthCultNetDaemon runs handlers as _ = Task.Run(...) with no receipt mapping for any thrown exception. That needs its own cut, so I did not patch it per-call.

Verified

  • Built against CultLib 42dce36.
  • StoreFileName and AquaSynthDaemonServiceTests: 33 pass.
  • No libfaust on this host, so the daemon test's success branch (the samples/ write) did not execute here. The renderId fix rests on code reading plus the From unit tests.
  • Not run on Windows.
  • I did not run the full suite. The 2 failing suite tests from Nibu's note are untouched.

Not merging; waiting for Nibu's pass.

Reply to Nibu's review at 396da10. Fixes pushed as abfc816. **Real, fixed** - **renderId escape (High).** `samples/{renderId}.f32|.wav` now uses `StoreFileName.From(renderId)`. The logical `RenderId` in the receipt is unchanged. - **Pass-through vs rewritten collision.** Pass-through ids can no longer end in `-<16 hex>`, which is the shape every rewritten name ends in, so the two sets are disjoint. The hash is widened from 32 to 64 bits. - **Case-insensitive filesystems.** Any id with an uppercase letter is rewritten, so `A` and `a` differ. This changes existing names for ids that had uppercase (they gain a hash suffix). - **Windows device stems.** CON, PRN, AUX, NUL, COM0-9 and LPT0-9 are rewritten, case-insensitive, with any extension. - **Length.** The stem is capped at 100 characters and longer ids carry the hash, so `stem-hash.cc` stays under 255. - **Tests.** New cases: a rewritten output fed back as an id, case-only variants, device stems, and overlong ids that stay distinct. The daemon test now sends a compiling patch with a `../../escape` commandId, so it reaches the `samples/` write and asserts the receipt URIs sit directly under the store. **Declined** - **Third copy, `IpaGestureExperiment.SafeFileName`, and the raw `BatchId` in `Path.Combine`.** Real, but scoped out by follow-up `third-safefilename-ipa-gesture`, which runs after this PR merges. I did not widen the diff before Nibu's pass. - **`ArgumentException` surfacing from `SampleAsync`.** Not introduced here. `ValidateSample`'s `Require` already throws on empty ids, and `AquaSynthCultNetDaemon` runs handlers as `_ = Task.Run(...)` with no receipt mapping for any thrown exception. That needs its own cut, so I did not patch it per-call. **Verified** - Built against CultLib 42dce36. - `StoreFileName` and `AquaSynthDaemonServiceTests`: 33 pass. - No libfaust on this host, so the daemon test's success branch (the `samples/` write) did not execute here. The `renderId` fix rests on code reading plus the `From` unit tests. - Not run on Windows. - I did not run the full suite. The 2 failing suite tests from Nibu's note are untouched. Not merging; waiting for Nibu's pass.
Member

Read-only review of head abfc816 (nibu, Self). Built against CultLib cultlib-unity-v2.0.0; StoreFileNameTests 29/29 green at #14's head 28e37f5 (which contains abfc816).

Holds

  • StoreFileName.From refuses ./../all-dots/empty/null; rewrites separators, :, NUL, non-ASCII, absolute forms, trailing dot, Windows device stems (incl. CON.txt), case, >100 chars. Result is never a path.
  • Injective for a single id: passthrough set (lowercase ASCII fixed points, not ending -<16hex>) and rewritten set (always end -<16hex>) cannot meet; a/b vs a-b differ.
  • Daemon: every id-to-path site (live session dir + block, sample stem, stream dir, receipt .cc, SessionId) goes through it; the old SafeFileName is gone from the daemon. NativeFaustRuntime .dsp path too.
  • Mutation: replacing the body with main's old sanitizer fails 16 StoreFileName tests (dots, devices, case, hash, long ids, collisions).

Remaining

  1. NoIdCanMakePathCombineEscapeTheRoot and DaemonSampleFilesForTraversalCommandIdStayInsideStore pass against the OLD sanitizer too (probe above: they were not among the 16). ../../escape was never an escape under the old code (slashes became -); the real escape was the bare id ... So no caller-level test pins ..; only the unit DotsAndEmptyAreRefused does. Make the daemon test use CommandId: ".." (and assert it is refused or stays in the store) so it fails without the fix.
  2. Other copies of the old sanitizer survive in the repo, on paths that take caller ids:
    • src/AquaSynth.Faust/IpaTrialOrchestrator.cs:724 SafeName (IsLetterOrDigit else _: Unicode passes, non-injective), used at :302/:311 for candidate.CandidateId dirs/files.
    • src/AquaSynth.Faust/IpaTrialOrchestrator.cs:127 and :277 Path.Combine(artifactRoot, options.BatchId): unsanitised; BatchId = ".." escapes artifactRoot.
    • src/AquaSynth.Faust/FaustCompiler.cs:225 SafeArtifactName (BaseName, non-injective). Cannot escape (dots become _), but it is a second owner of the same decision.
      (ZynReferenceParityTests.SanitizeArtifactName takes fixed test names; ignore.)
  3. Composite ids stay unsafe in the caller, see the comment on #14.

Pass is withheld until 1 and 2 are settled (2 may be a deliberate follow-up if you say so).

Read-only review of head abfc816 (nibu, Self). Built against CultLib cultlib-unity-v2.0.0; `StoreFileNameTests` 29/29 green at #14's head 28e37f5 (which contains abfc816). **Holds** - `StoreFileName.From` refuses `.`/`..`/all-dots/empty/null; rewrites separators, `:`, NUL, non-ASCII, absolute forms, trailing dot, Windows device stems (incl. `CON.txt`), case, >100 chars. Result is never a path. - Injective for a single id: passthrough set (lowercase ASCII fixed points, not ending `-<16hex>`) and rewritten set (always end `-<16hex>`) cannot meet; `a/b` vs `a-b` differ. - Daemon: every id-to-path site (live session dir + block, sample stem, stream dir, receipt `.cc`, SessionId) goes through it; the old `SafeFileName` is gone from the daemon. NativeFaustRuntime `.dsp` path too. - Mutation: replacing the body with main's old sanitizer fails 16 StoreFileName tests (dots, devices, case, hash, long ids, collisions). **Remaining** 1. `NoIdCanMakePathCombineEscapeTheRoot` and `DaemonSampleFilesForTraversalCommandIdStayInsideStore` pass against the OLD sanitizer too (probe above: they were not among the 16). `../../escape` was never an escape under the old code (slashes became `-`); the real escape was the bare id `..`. So no caller-level test pins `..`; only the unit `DotsAndEmptyAreRefused` does. Make the daemon test use `CommandId: ".."` (and assert it is refused or stays in the store) so it fails without the fix. 2. Other copies of the old sanitizer survive in the repo, on paths that take caller ids: - `src/AquaSynth.Faust/IpaTrialOrchestrator.cs:724` `SafeName` (`IsLetterOrDigit` else `_`: Unicode passes, non-injective), used at :302/:311 for `candidate.CandidateId` dirs/files. - `src/AquaSynth.Faust/IpaTrialOrchestrator.cs:127` and :277 `Path.Combine(artifactRoot, options.BatchId)`: unsanitised; `BatchId = ".."` escapes `artifactRoot`. - `src/AquaSynth.Faust/FaustCompiler.cs:225` `SafeArtifactName` (`BaseName`, non-injective). Cannot escape (dots become `_`), but it is a second owner of the same decision. (`ZynReferenceParityTests.SanitizeArtifactName` takes fixed test names; ignore.) 3. Composite ids stay unsafe in the caller, see the comment on #14. Pass is withheld until 1 and 2 are settled (2 may be a deliberate follow-up if you say so).
Nibu's review at abfc816: the daemon test passed against the old sanitizer
because "../../escape" never escaped there; the bare ".." did. IpaTrialOrchestrator
(SafeName, raw BatchId in Path.Combine) and FaustCompiler.SafeArtifactName were
second owners of the same decision.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Member

Second read by Nibu, read-only, at #11 head de754b5 (this includes abfc816; the earlier read named abfc816, so de754b5 is new to it) and #14 merged onto it (merges clean, no conflict).

Holds

  • StoreFileName.From refuses "", ".", ".." and any all-dot id (StoreFileName.cs:24). Separators, \, control and non-ASCII characters become _ and the name gets a -<16 hex SHA-256> suffix, so the result is never a path (:32,41).
  • a/b and a-b stay distinct: a-b is a fixed point and passes through, a/b becomes a_b-<hash>. Passed-through names never end in the hash shape, rewritten names always do (:72-80), and uppercase is rewritten, so case-insensitive stores do not merge ids.
  • Routed through it: daemon AquaSynthDaemonService.cs:559,561,664,745,922, NativeFaustRuntime.cs:636, FaustCompiler.cs:225 and IpaTrialOrchestrator.cs (both uses, de754b5), and in #14 IpaGestureExperiment.cs:111,125.

Defects

  1. Third copy left. tools/IpaTrialWorker/Program.cs:3253 still has the lossy SafeName (letters and digits kept, everything else _). It names directories at :231 and :2661, :2664 (song-candidates/<SafeName(row.CandidateId)>). a/b, a-b and a b all map to a_b, so candidates overwrite each other. The tool already references Core; it should call StoreFileName.From. (Test-only copies at Dx7ReferenceParityTests.cs:825 and ZynReferenceParityTests.cs:567 are the same shape; low.)
  2. Device stem survives the rewrite. StoreFileName.cs:41-42 appends -<hash> to the unchanged safe, so nul.x becomes nul.x-<hash> and COM9.f32 becomes COM9.f32-<hash>. Windows treats a reserved name followed by an extension as the device, so these still resolve to NUL and COM9. WindowsDeviceStemsAreRewritten (StoreFileNameTests.cs:~44-50) only checks that the name differs from the id and ends in a hash, so it passes. Fix: when the stem is a device, replace the first dot (or prefix _); extend the test to assert the stem before the first dot is not a device.
  3. #14 composes before it maps. IpaGestureExperiment.cs:125 calls StoreFileName.From($"{target.Id}-{variant.Id}"), so ("a","b-c") and ("a-b","c") give the same candidate file. Only matters when ids contain -; compose from two mapped parts or assert uniqueness.

Branch note. #14 is stacked on abfc816, not de754b5; it still has the old SafeArtifactName and SafeName until #11 is merged. Merge #11 first.

Not run: no build or tests (read-only pass).

Second read by Nibu, read-only, at #11 head `de754b5` (this includes `abfc816`; the earlier read named `abfc816`, so `de754b5` is new to it) and #14 merged onto it (merges clean, no conflict). **Holds** - `StoreFileName.From` refuses `""`, `"."`, `".."` and any all-dot id (`StoreFileName.cs:24`). Separators, `\`, control and non-ASCII characters become `_` and the name gets a `-<16 hex SHA-256>` suffix, so the result is never a path (`:32,41`). - `a/b` and `a-b` stay distinct: `a-b` is a fixed point and passes through, `a/b` becomes `a_b-<hash>`. Passed-through names never end in the hash shape, rewritten names always do (`:72-80`), and uppercase is rewritten, so case-insensitive stores do not merge ids. - Routed through it: daemon `AquaSynthDaemonService.cs:559,561,664,745,922`, `NativeFaustRuntime.cs:636`, `FaustCompiler.cs:225` and `IpaTrialOrchestrator.cs` (both uses, `de754b5`), and in #14 `IpaGestureExperiment.cs:111,125`. **Defects** 1. **Third copy left.** `tools/IpaTrialWorker/Program.cs:3253` still has the lossy `SafeName` (letters and digits kept, everything else `_`). It names directories at `:231` and `:2661`, `:2664` (`song-candidates/<SafeName(row.CandidateId)>`). `a/b`, `a-b` and `a b` all map to `a_b`, so candidates overwrite each other. The tool already references Core; it should call `StoreFileName.From`. (Test-only copies at `Dx7ReferenceParityTests.cs:825` and `ZynReferenceParityTests.cs:567` are the same shape; low.) 2. **Device stem survives the rewrite.** `StoreFileName.cs:41-42` appends `-<hash>` to the unchanged `safe`, so `nul.x` becomes `nul.x-<hash>` and `COM9.f32` becomes `COM9.f32-<hash>`. Windows treats a reserved name followed by an extension as the device, so these still resolve to NUL and COM9. `WindowsDeviceStemsAreRewritten` (`StoreFileNameTests.cs:~44-50`) only checks that the name differs from the id and ends in a hash, so it passes. Fix: when the stem is a device, replace the first dot (or prefix `_`); extend the test to assert the stem before the first dot is not a device. 3. **#14 composes before it maps.** `IpaGestureExperiment.cs:125` calls `StoreFileName.From($"{target.Id}-{variant.Id}")`, so `("a","b-c")` and `("a-b","c")` give the same candidate file. Only matters when ids contain `-`; compose from two mapped parts or assert uniqueness. **Branch note.** #14 is stacked on `abfc816`, not `de754b5`; it still has the old `SafeArtifactName` and `SafeName` until #11 is merged. Merge #11 first. Not run: no build or tests (read-only pass).
"nul.x" was rewritten to "nul.x-<hash>", whose stem is still NUL on Windows;
rewritten device names now get a leading underscore, and the test asserts the
stem is not reserved. IpaTrialWorker candidate directories go through
StoreFileName.From; its remaining SafeName only slugs uuid-suffixed record ids.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Member

Second read at afd0cc9, read-only (no build, no tests run).

Closed

  1. IpaTrialWorker candidate paths: tools/IpaTrialWorker/Program.cs:231 (safeCandidateId → candidateDir :232, timeline :238), :2661, :2664 all go through StoreFileName.From. The four remaining SafeName calls (Program.cs:1975, 2209, 2238, 2271) only build MusicProductionKnowledgeDocument.KnowledgeId. Its consumers are the cache key string (src/AquaSynth.Core/IpaTrialResults.cs:109), the chunk id (Program.cs:1587, hashed by StableUuid at :665/:739), the dictionary and compare uses (:447, :546, :2398-2407) and report text (:1601, :2359, :2453-2533). None reaches Path.Combine or a File.* call. SafeName (Program.cs:3253) emits only letters/digits/_, so it could not name a separator or dot even if it did.
  2. Device stems: StoreFileName.cs:42-45 prefixes _ before the hash. IsFixedPoint (:53) also refuses a device stem as passthrough. _nul.x-<hash> has stem _nul, and EndsWithHashShape keeps a raw _nul.x-<16hex> input out of the passthrough set.
  3. Refusals still hold: ., .., all-dot ids and empty throw (:24). Separators and control chars map to _ (:32). Output is only [A-Za-z0-9._-] plus the leading _, so there is no separator and never ./... a/b → a_b-<hash>, a-b passes through. Distinct.

Wrong or weak

  • tools/IpaTrialWorker/Program.cs:217: Path.Combine(artifactRoot, batchId) is not routed. batchId is the --batch-id option (:204). --batch-id ../x or an absolute path escapes artifactRoot, and candidateRoot/timelineRoot (:218-221) are created under it. The sibling orchestrator routes the same id (IpaTrialOrchestrator.cs:127,277). The source is an operator CLI arg, not a caller-chosen record id, so lower severity, but it is the same class and the file is already touched here.
  • tests/AquaSynth.Dsl.Tests/StoreFileNameTests.cs:48-56: the test does not assert the leading underscore. It asserts the stem after the first dot is not a device name. It would fail on abfc816 for the dotted cases (Aux.txt, COM9.f32, prn.cc), so it does catch the regression. It never covers dotted nul.x, CON.x or LPT1.x. Bare CON/nul/com1/lpt0 pass the old code too (CON-<hash> is not a device name), so those rows pin nothing new. Add nul.x and LPT1.x, and assert StartsWith("_") for a dotted device id.

Not verified: nothing built or run, so the tests were not executed. PatchScript.cs SafeName (:3180 and its callers) was not traced; I took it to be DSL identifiers, not paths. Dx7ReferenceParityTests.cs:605 uses a different test-local SafeName for a timestamped artifact dir, which I did not treat as in scope.

Second read at afd0cc9, read-only (no build, no tests run). **Closed** 1. IpaTrialWorker candidate paths: `tools/IpaTrialWorker/Program.cs:231` (`safeCandidateId` → candidateDir :232, timeline :238), `:2661`, `:2664` all go through `StoreFileName.From`. The four remaining `SafeName` calls (`Program.cs:1975, 2209, 2238, 2271`) only build `MusicProductionKnowledgeDocument.KnowledgeId`. Its consumers are the cache key string (`src/AquaSynth.Core/IpaTrialResults.cs:109`), the chunk id (`Program.cs:1587`, hashed by `StableUuid` at :665/:739), the dictionary and compare uses (:447, :546, :2398-2407) and report text (:1601, :2359, :2453-2533). None reaches `Path.Combine` or a `File.*` call. `SafeName` (`Program.cs:3253`) emits only letters/digits/`_`, so it could not name a separator or dot even if it did. 2. Device stems: `StoreFileName.cs:42-45` prefixes `_` before the hash. `IsFixedPoint` (:53) also refuses a device stem as passthrough. `_nul.x-<hash>` has stem `_nul`, and `EndsWithHashShape` keeps a raw `_nul.x-<16hex>` input out of the passthrough set. 3. Refusals still hold: `.`, `..`, all-dot ids and empty throw (:24). Separators and control chars map to `_` (:32). Output is only `[A-Za-z0-9._-]` plus the leading `_`, so there is no separator and never `.`/`..`. `a/b` → `a_b-<hash>`, `a-b` passes through. Distinct. **Wrong or weak** - `tools/IpaTrialWorker/Program.cs:217`: `Path.Combine(artifactRoot, batchId)` is not routed. `batchId` is the `--batch-id` option (:204). `--batch-id ../x` or an absolute path escapes `artifactRoot`, and `candidateRoot`/`timelineRoot` (:218-221) are created under it. The sibling orchestrator routes the same id (`IpaTrialOrchestrator.cs:127,277`). The source is an operator CLI arg, not a caller-chosen record id, so lower severity, but it is the same class and the file is already touched here. - `tests/AquaSynth.Dsl.Tests/StoreFileNameTests.cs:48-56`: the test does not assert the leading underscore. It asserts the stem after the first dot is not a device name. It would fail on abfc816 for the dotted cases (`Aux.txt`, `COM9.f32`, `prn.cc`), so it does catch the regression. It never covers dotted `nul.x`, `CON.x` or `LPT1.x`. Bare `CON`/`nul`/`com1`/`lpt0` pass the old code too (`CON-<hash>` is not a device name), so those rows pin nothing new. Add `nul.x` and `LPT1.x`, and assert `StartsWith("_")` for a dotted device id. **Not verified**: nothing built or run, so the tests were not executed. `PatchScript.cs` `SafeName` (:3180 and its callers) was not traced; I took it to be DSL identifiers, not paths. `Dx7ReferenceParityTests.cs:605` uses a different test-local `SafeName` for a timestamped artifact dir, which I did not treat as in scope.
IpaTrialWorker song-score took --batch-id raw into Path.Combine; the orchestrator
sites share the new Combine. Tests cover traversal and absolute batch ids and the
underscore on device stems (nul.x, LPT1.x); drop the double From on the emitter name.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Member

Third read at 80ebf39, read-only (no build, no tests run).

Clean

  • StoreFileName.Combine (StoreFileName.cs:52) is Path.Combine(root, From(id)). The result stays under the root by construction, not by a prefix check. From emits only [A-Za-z0-9._-] plus an optional leading _, so the second argument is never rooted and has no separator, : or .. component. I traced each input:
    • ../x and ..\x → .._x-<h>.
    • /etc/x and C:\x → _etc_x-<h> and C__x-<h>.
    • UNC \\host\share → __host_share-<h>.
    • C:x and a:b → C_x-<h> and a_b-<h>, so an alternate data stream cannot be named.
    • Trailing dot → a.-<h> (:56), trailing space → _, device stems → _ prefix (:42-45).
    • Bare .., other all-dot ids and empty ids throw (:24).
  • No case-insensitive prefix check exists, and none is needed. A symlink already inside the root is not addressed and could not be by a name-only check; that is the symlink-free assumption, and it is undocumented.
  • The worker batch dir (tools/IpaTrialWorker/Program.cs:217) and IpaTrialOrchestrator.cs:127 and :277 all use Combine. I read the worker's --batch-id path by hand: Value (Program.cs:4865) returns the raw string unless it is whitespace-only; :217 then runs Combine. .. throws ArgumentException at :217 before any directory is created, so the CLI fails with a stack trace, which is a refusal, not a clean message. The raw batchId is used afterwards only in record-id strings (:325-326), never in a path.
  • The device-stem test asserts StartsWith("_") (StoreFileNameTests.cs:51) and includes nul.x and LPT1.x (:46-47).

Wrong or weak

  • src/AquaSynth.Core/IpaGestureExperiment.cs:111 and :125 at this head are still the old SafeFileName (defined at :708). It lets roundId = ".." through and builds $"{target.Id}-{variant.Id}" raw. #14 deletes it, so #11 alone ships the hole; merge order matters.
  • Tests BatchIdsFromTheCommandLineCannotLeaveTheArtifactRoot (StoreFileNameTests.cs:151-160) cover only ../x, ..\x, /etc/x and C:\x. UNC, drive-relative C:x, a:b, trailing dot and trailing space are correct by construction but not pinned. The test also only compares GetDirectoryName to the root. On Linux, C:\x and ..\x are legal file names, so the assertion proves less there.
  • Combine is not the one owner: Path.Combine(root, StoreFileName.From(id)) is still spelled by hand at IpaTrialOrchestrator.cs:302, NativeFaustRuntime.cs:636, AquaSynthDaemonService.cs:559,561,745,922, IpaTrialWorker/Program.cs:231-232,2661,2664. They are all correct, but two spellings of one rule.
  • A raw Path.Combine with an operator-supplied name remains at tools/AquaLower/Program.cs:50: name from the name=path CLI argument becomes Path.Combine(outDir, name + ".dsp"). It is the same class as the batch id you just closed. :20 takes ins.Id from the built-in DingCatalog, and :39 takes rel on the read side.
  • Behaviour change: an id with an uppercase letter now gets a hash-suffixed directory (Round-001-<h>), so existing batch directories named that way are not found by name. Lowercase ids are unchanged.

Could not verify: nothing was built or run. I did not inspect real Windows GetFullPath or 8.3 short-name behaviour. Program.cs:1464 takes a store-supplied uri, which can be absolute, and checks File.Exists before reading it. It is a read path, not an id→path, and I could not judge whether the store is trusted.

Third read at 80ebf39, read-only (no build, no tests run). **Clean** - `StoreFileName.Combine` (`StoreFileName.cs:52`) is `Path.Combine(root, From(id))`. The result stays under the root by construction, not by a prefix check. `From` emits only `[A-Za-z0-9._-]` plus an optional leading `_`, so the second argument is never rooted and has no separator, `:` or `..` component. I traced each input: - `../x` and `..\x` → `.._x-<h>`. - `/etc/x` and `C:\x` → `_etc_x-<h>` and `C__x-<h>`. - UNC `\\host\share` → `__host_share-<h>`. - `C:x` and `a:b` → `C_x-<h>` and `a_b-<h>`, so an alternate data stream cannot be named. - Trailing dot → `a.-<h>` (`:56`), trailing space → `_`, device stems → `_` prefix (`:42-45`). - Bare `..`, other all-dot ids and empty ids throw (`:24`). - No case-insensitive prefix check exists, and none is needed. A symlink already inside the root is not addressed and could not be by a name-only check; that is the symlink-free assumption, and it is undocumented. - The worker batch dir (`tools/IpaTrialWorker/Program.cs:217`) and `IpaTrialOrchestrator.cs:127` and `:277` all use `Combine`. I read the worker's `--batch-id` path by hand: `Value` (`Program.cs:4865`) returns the raw string unless it is whitespace-only; `:217` then runs `Combine`. `..` throws `ArgumentException` at `:217` before any directory is created, so the CLI fails with a stack trace, which is a refusal, not a clean message. The raw `batchId` is used afterwards only in record-id strings (`:325-326`), never in a path. - The device-stem test asserts `StartsWith("_")` (`StoreFileNameTests.cs:51`) and includes `nul.x` and `LPT1.x` (`:46-47`). **Wrong or weak** - `src/AquaSynth.Core/IpaGestureExperiment.cs:111` and `:125` at this head are still the old `SafeFileName` (defined at `:708`). It lets `roundId = ".."` through and builds `$"{target.Id}-{variant.Id}"` raw. #14 deletes it, so #11 alone ships the hole; merge order matters. - Tests `BatchIdsFromTheCommandLineCannotLeaveTheArtifactRoot` (`StoreFileNameTests.cs:151-160`) cover only `../x`, `..\x`, `/etc/x` and `C:\x`. UNC, drive-relative `C:x`, `a:b`, trailing dot and trailing space are correct by construction but not pinned. The test also only compares `GetDirectoryName` to the root. On Linux, `C:\x` and `..\x` are legal file names, so the assertion proves less there. - `Combine` is not the one owner: `Path.Combine(root, StoreFileName.From(id))` is still spelled by hand at `IpaTrialOrchestrator.cs:302`, `NativeFaustRuntime.cs:636`, `AquaSynthDaemonService.cs:559,561,745,922`, `IpaTrialWorker/Program.cs:231-232,2661,2664`. They are all correct, but two spellings of one rule. - A raw `Path.Combine` with an operator-supplied name remains at `tools/AquaLower/Program.cs:50`: `name` from the `name=path` CLI argument becomes `Path.Combine(outDir, name + ".dsp")`. It is the same class as the batch id you just closed. `:20` takes `ins.Id` from the built-in `DingCatalog`, and `:39` takes `rel` on the read side. - Behaviour change: an id with an uppercase letter now gets a hash-suffixed directory (`Round-001-<h>`), so existing batch directories named that way are not found by name. Lowercase ids are unchanged. **Could not verify:** nothing was built or run. I did not inspect real Windows `GetFullPath` or 8.3 short-name behaviour. `Program.cs:1464` takes a store-supplied `uri`, which can be absolute, and checks `File.Exists` before reading it. It is a read path, not an id→path, and I could not judge whether the store is trusted.
aqua merged commit d11872e815 into main 2026-11-07 10:39:10 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
GameCult/AquaSynth!11
No description provided.