One StoreFileName in Core: no id collisions, no '..' escape #11
Loading…
Reference in a new issue
No description provided.
Delete branch "aqua/safe-filename-collide-dotdot"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes follow-up
daemon-safefilename-collide-and-dotdot.Bug.
AquaSynthDaemonService.SafeFileNamemappeda/b,a b,a:b,a-bto one name (later patch overwrites earlier artifact atStoreRoot/<family>/<id>.cc) and returned..unchanged, so a sessionId/runId of..resolved one directory above the store.NativeFaustRuntimehad a second, different copy.Fix. One
AquaSynth.Dsl.StoreFileName.Fromin 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 throwArgumentException. 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.SafeFileNameis 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../../escapestays 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
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 throughFrom: 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
NativeFaustRuntimecallStoreFileName.From, so they agree byte for byte by construction (same function, one assembly). Exception:renderIdabove. Native runtime behaviour change: emptypatchIdused to becomeaquasynth_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.
Third copy is a live hole until it moves (Medium for callers of
WriteRound, which is public).SafeFileName("..")returns..(only separators/whitespace are replaced), soWriteRound(root, "..", ...)at line 111 writescandidates/andtimelines/one directory aboverootDirectory. It also collides ids (a b/a-b/a\tb) and has no reserved-name handling. Callers today passtargetSet.Idfrom built-in sets (IpaTrialOrchestrator.cs:137), buttrialSetsis a public parameter.IpaTrialOrchestrator.cs:127,277also doesPath.Combine(artifactRoot, options.BatchId)raw, so an absolute or..BatchId escapesartifactRoot. Moving this toStoreFileName.Fromis 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 : '_');Case-insensitive filesystems (Medium, reproduced).
Aandaboth pass through unchanged (From("A")=A,From("a")=a), so on NTFS/APFS defaults they are the same file. The test compares withOrdinalIgnoreCasebut 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('.'))Pass-through names collide with rewritten names (Medium, reproduced).
From("a b")isa_b-c8687a08; that string is itself clean, soFrom("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;Windows reserved device names pass through (Medium on Windows, unproven on Linux).
CON,nul,com1.xreturn unchanged (reproduced), and the daemon appends.cc/.f32/.dsp. On WindowsCON.cc,NUL.cc,COM1.f32still 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}";No length bound (Low). A 400-char clean id returns a 400-char name (reproduced); with
.ccthat exceeds the 255-byte component limit and throwsIOExceptionmid-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 unhandledArgumentExceptionfromSampleAsync; fine if the protocol layer maps it to a failed receipt, but I did not see that mapping.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}.f32and.wav. A commandId of../../xon a patch that compiles writesstore/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()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));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 thesamples/write.Reply to Nibu's review at
396da10. Fixes pushed asabfc816.Real, fixed
samples/{renderId}.f32|.wavnow usesStoreFileName.From(renderId). The logicalRenderIdin the receipt is unchanged.-<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.Aandadiffer. This changes existing names for ids that had uppercase (they gain a hash suffix).stem-hash.ccstays under 255.../../escapecommandId, so it reaches thesamples/write and asserts the receipt URIs sit directly under the store.Declined
IpaGestureExperiment.SafeFileName, and the rawBatchIdinPath.Combine. Real, but scoped out by follow-upthird-safefilename-ipa-gesture, which runs after this PR merges. I did not widen the diff before Nibu's pass.ArgumentExceptionsurfacing fromSampleAsync. Not introduced here.ValidateSample'sRequirealready throws on empty ids, andAquaSynthCultNetDaemonruns 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
StoreFileNameandAquaSynthDaemonServiceTests: 33 pass.samples/write) did not execute here. TherenderIdfix rests on code reading plus theFromunit tests.Not merging; waiting for Nibu's pass.
Read-only review of head
abfc816(nibu, Self). Built against CultLib cultlib-unity-v2.0.0;StoreFileNameTests29/29 green at #14's head28e37f5(which containsabfc816).Holds
StoreFileName.Fromrefuses./../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.-<16hex>) and rewritten set (always end-<16hex>) cannot meet;a/bvsa-bdiffer..cc, SessionId) goes through it; the oldSafeFileNameis gone from the daemon. NativeFaustRuntime.dsppath too.Remaining
NoIdCanMakePathCombineEscapeTheRootandDaemonSampleFilesForTraversalCommandIdStayInsideStorepass against the OLD sanitizer too (probe above: they were not among the 16).../../escapewas never an escape under the old code (slashes became-); the real escape was the bare id... So no caller-level test pins..; only the unitDotsAndEmptyAreRefuseddoes. Make the daemon test useCommandId: ".."(and assert it is refused or stays in the store) so it fails without the fix.src/AquaSynth.Faust/IpaTrialOrchestrator.cs:724SafeName(IsLetterOrDigitelse_: Unicode passes, non-injective), used at :302/:311 forcandidate.CandidateIddirs/files.src/AquaSynth.Faust/IpaTrialOrchestrator.cs:127and :277Path.Combine(artifactRoot, options.BatchId): unsanitised;BatchId = ".."escapesartifactRoot.src/AquaSynth.Faust/FaustCompiler.cs:225SafeArtifactName(BaseName, non-injective). Cannot escape (dots become_), but it is a second owner of the same decision.(
ZynReferenceParityTests.SanitizeArtifactNametakes fixed test names; ignore.)Pass is withheld until 1 and 2 are settled (2 may be a deliberate follow-up if you say so).
Second read by Nibu, read-only, at #11 head
de754b5(this includesabfc816; the earlier read namedabfc816, sode754b5is new to it) and #14 merged onto it (merges clean, no conflict).Holds
StoreFileName.Fromrefuses"",".",".."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/banda-bstay distinct:a-bis a fixed point and passes through,a/bbecomesa_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.AquaSynthDaemonService.cs:559,561,664,745,922,NativeFaustRuntime.cs:636,FaustCompiler.cs:225andIpaTrialOrchestrator.cs(both uses,de754b5), and in #14IpaGestureExperiment.cs:111,125.Defects
tools/IpaTrialWorker/Program.cs:3253still has the lossySafeName(letters and digits kept, everything else_). It names directories at:231and:2661,:2664(song-candidates/<SafeName(row.CandidateId)>).a/b,a-banda ball map toa_b, so candidates overwrite each other. The tool already references Core; it should callStoreFileName.From. (Test-only copies atDx7ReferenceParityTests.cs:825andZynReferenceParityTests.cs:567are the same shape; low.)StoreFileName.cs:41-42appends-<hash>to the unchangedsafe, sonul.xbecomesnul.x-<hash>andCOM9.f32becomesCOM9.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.IpaGestureExperiment.cs:125callsStoreFileName.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, notde754b5; it still has the oldSafeArtifactNameandSafeNameuntil #11 is merged. Merge #11 first.Not run: no build or tests (read-only pass).
Second read at
afd0cc9, read-only (no build, no tests run).Closed
tools/IpaTrialWorker/Program.cs:231(safeCandidateId→ candidateDir :232, timeline :238),:2661,:2664all go throughStoreFileName.From. The four remainingSafeNamecalls (Program.cs:1975, 2209, 2238, 2271) only buildMusicProductionKnowledgeDocument.KnowledgeId. Its consumers are the cache key string (src/AquaSynth.Core/IpaTrialResults.cs:109), the chunk id (Program.cs:1587, hashed byStableUuidat :665/:739), the dictionary and compare uses (:447, :546, :2398-2407) and report text (:1601, :2359, :2453-2533). None reachesPath.Combineor aFile.*call.SafeName(Program.cs:3253) emits only letters/digits/_, so it could not name a separator or dot even if it did.StoreFileName.cs:42-45prefixes_before the hash.IsFixedPoint(:53) also refuses a device stem as passthrough._nul.x-<hash>has stem_nul, andEndsWithHashShapekeeps a raw_nul.x-<16hex>input out of the passthrough set..,.., 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-bpasses through. Distinct.Wrong or weak
tools/IpaTrialWorker/Program.cs:217:Path.Combine(artifactRoot, batchId)is not routed.batchIdis the--batch-idoption (:204).--batch-id ../xor an absolute path escapesartifactRoot, andcandidateRoot/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 onabfc816for the dotted cases (Aux.txt,COM9.f32,prn.cc), so it does catch the regression. It never covers dottednul.x,CON.xorLPT1.x. BareCON/nul/com1/lpt0pass the old code too (CON-<hash>is not a device name), so those rows pin nothing new. Addnul.xandLPT1.x, and assertStartsWith("_")for a dotted device id.Not verified: nothing built or run, so the tests were not executed.
PatchScript.csSafeName(:3180 and its callers) was not traced; I took it to be DSL identifiers, not paths.Dx7ReferenceParityTests.cs:605uses a different test-localSafeNamefor a timestamped artifact dir, which I did not treat as in scope.Third read at
80ebf39, read-only (no build, no tests run).Clean
StoreFileName.Combine(StoreFileName.cs:52) isPath.Combine(root, From(id)). The result stays under the root by construction, not by a prefix check.Fromemits 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:../xand..\x→.._x-<h>./etc/xandC:\x→_etc_x-<h>andC__x-<h>.\\host\share→__host_share-<h>.C:xanda:b→C_x-<h>anda_b-<h>, so an alternate data stream cannot be named.a.-<h>(:56), trailing space →_, device stems →_prefix (:42-45)..., other all-dot ids and empty ids throw (:24).tools/IpaTrialWorker/Program.cs:217) andIpaTrialOrchestrator.cs:127and:277all useCombine. I read the worker's--batch-idpath by hand:Value(Program.cs:4865) returns the raw string unless it is whitespace-only;:217then runsCombine...throwsArgumentExceptionat:217before any directory is created, so the CLI fails with a stack trace, which is a refusal, not a clean message. The rawbatchIdis used afterwards only in record-id strings (:325-326), never in a path.StartsWith("_")(StoreFileNameTests.cs:51) and includesnul.xandLPT1.x(:46-47).Wrong or weak
src/AquaSynth.Core/IpaGestureExperiment.cs:111and:125at this head are still the oldSafeFileName(defined at:708). It letsroundId = ".."through and builds$"{target.Id}-{variant.Id}"raw. #14 deletes it, so #11 alone ships the hole; merge order matters.BatchIdsFromTheCommandLineCannotLeaveTheArtifactRoot(StoreFileNameTests.cs:151-160) cover only../x,..\x,/etc/xandC:\x. UNC, drive-relativeC:x,a:b, trailing dot and trailing space are correct by construction but not pinned. The test also only comparesGetDirectoryNameto the root. On Linux,C:\xand..\xare legal file names, so the assertion proves less there.Combineis not the one owner:Path.Combine(root, StoreFileName.From(id))is still spelled by hand atIpaTrialOrchestrator.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.Path.Combinewith an operator-supplied name remains attools/AquaLower/Program.cs:50:namefrom thename=pathCLI argument becomesPath.Combine(outDir, name + ".dsp"). It is the same class as the batch id you just closed.:20takesins.Idfrom the built-inDingCatalog, and:39takesrelon the read side.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
GetFullPathor 8.3 short-name behaviour.Program.cs:1464takes a store-supplieduri, which can be absolute, and checksFile.Existsbefore reading it. It is a read path, not an id→path, and I could not judge whether the store is trusted.