GameCult/Fensalir
Watch
9
0
Fork
You've already forked Fensalir
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

ClientRuntimeLoader: client assemblies must resolve under a trusted root #1

Merged
aqua merged 8 commits from aqua/client-runtime-trusted-root into main 2026-11-07 11:50:05 +00:00
Conversation 6 Commits 8 Files changed 7 +771 -23
aqua commented 2026-11-07 10:45:24 +00:00
Member
Copy link

Closes the pointer-file load hole in the custody map (section 13.3). Proposal and reasoning: handoff note aqua-fensalir-pointer-load-fix.md.

The real boundary is the trusted directory's ACLs. This check decides which directory may hold client code; it does nothing about who can write there. scripts/dev-reload.ps1 creates artifacts\dev-reload with default ACLs, and this PR changes no ACL. Anyone who can write inside the root still owns the process. Everything under the root is client code: the host build copied into each slot and the shader sources next to it are loaded or consumed by the host as well, so the root must be writable only by the developer account, not just the client assembly.

Rule. Every client assembly, and every dependency or native library its load context resolves, must lie (links followed) under the trusted root, or the load is refused: Reload (initial --client-assembly load and pointer reload) and the load context's managed and unmanaged resolution all call one check. The pointer target gets the string rules (local, fully qualified) before any filesystem probe, and every logged path is escaped once, at the console write (exception messages carry the raw path), by code point: backslash, control, format including tag characters (U+E0001, U+E0020-E007F), astral format characters (e.g. U+1D173-1D17A), line/paragraph separators, and lone surrogates. Ordinary astral characters (emoji) are kept. A startup refusal in AquariumHost is printed the same way, with the escaped exception type name. Cost: on Windows the host now logs backslashes doubled (C:\\x). A path outside the root (as resolved or as given) is refused by string comparison before any filesystem access, so a link outside it (to a UNC share, a mapped drive) is never followed or probed. Relative and UNC paths (any / or \ mix at the start) are refused.

  • The root comes from --client-trusted-root / AQUARIUM_CLIENT_TRUSTED_ROOT, default the directory of the initial assembly. It is resolved once in the constructor and pinned; a drive or volume root is refused; the effective root is logged at startup. The environment variable is taken once by TrustedRootSetting (cleared on every path, flag or env, and cached) so processes this one starts do not inherit it, and an empty or missing flag value, or a blank environment value, is refused rather than falling back to the default root.
  • The loader loads and records the resolved final path. A pointer naming a link is compared by its resolved path, so it is not reloaded every poll.
  • A relative initial path is resolved against the working directory (operator-supplied); relative pointer contents are refused. No hash pin, no signatures.

dev-reload.ps1 passes $assemblyRoot (artifacts\dev-reload\assemblies) as the root, not the whole dev root (which also holds cache, state and logs): rebuilds land in assemblies\slots and dev-watch writes its reload pointer under assemblies\live-slots. dev-watch passes nothing itself. dev-reload also prunes live-slots (kept: -RetainSlots newest) and a -Reopen with state naming a path outside the root or a pruned file fails up front with a message; dev-watch matches the host ack by the unique slot leaf and assembly name, with backslashes doubled as the host logs them. Transition: a state.clixml written before this change names an old slots path outside the new root, so the first reopen after upgrading is refused until one fresh dev-reload build.

Tests (ClientRuntimeLoaderTrustTests, 30): in-root load and reload; sibling directory loads under an explicit root and not under the default; initial assembly outside the root; outside, .. and relative reloads; UNC in four separator mixes; volume root refused; dependency/native check (the static check the load context calls); symlinked file and directory escapes; a junction escape (a non-administrator junction refused by the OS as an untrusted mount point is reported as InvalidOperationException too); link stored by resolved path; root pinned at construction; pointer outside the root keeps the previous runtime; a UNC pointer is reported before any file probe; control characters in a pointer target are escaped in the log (skipped on Windows, where a file name cannot hold a newline); a literal backslash in a pointer target is shown as two characters, not four (escaped once); Printable escapes tag characters, musical format characters and lone surrogates by code point and keeps emoji; a startup refusal exits 1 and logs the escaped exception type; a loaded client's Assembly.Location equals its resolved file path; the env variable is cleared on flag and env paths and an empty flag is refused; the dev scripts keep slots and live-slots under one assemblies root that is not the dev root (this last one reads the script text, so it is brittle). Symlink tests report Skipped with a reason if the account cannot create links; the junction test is Skipped everywhere but Windows. Test cleanup retries, then prints a warning naming the directory if a locked assembly blocks the delete.

Results. On Linux: loader tests 29 pass, 1 skipped (junction), 30 total. Engine test project: 86 pass, 2 fail, 1 skipped, 89 total; the 2 failures are D3D12SharedTextureInteropTests (need d3d11.dll; identical on main 34aaebf). The new tests were run against the earlier loader and fail there: backslash-once, rune escaping and host startup (against 467a97f), Assembly.Location (against the in-memory loader of d2f280d). The mutation results from the earlier revision of this PR (lexical check, env handling, escaping, ack match, live-slots pruning, each killed by a named test) were not re-run after the later commits. Nothing in this PR has run on Windows.

A second reader should check

  1. Windows: the junction test has never run; LinkTarget on junctions, \?\ prefixes (refused as UNC-shaped), drive-relative C:x (not fully qualified).
  2. Gaps in the tests: the UNC-separator test cannot tell the explicit UNC check from the containment check on Linux (both refuse), so it has force only on Windows. The dependency test calls the shared check directly; no test drives a real client assembly whose .deps.json resolves outside the root, so removing the check inside the load context would still pass. The log line and the environment clearing are untested.
  3. TOCTOU between the check and the load: someone who can swap a file inside the root between them is already inside the boundary; not closed.
  4. Other clients (Aquarium repo, Epiphany) that reload from outside the initial directory must pass --client-trusted-root. Checked: Fensalir capture scripts, Perlines capture and Mimir stay inside their own directory.
  5. A refused pointer is retried every 500 ms and logs each time.
  6. Not run on Windows: dev-reload/dev-watch scripts, including the moved assemblies layout and the live-slots reload path; nothing parses the PowerShell, the script test only reads text.
  7. The PowerShell changes (ack matching, live-slots pruning, stale -Reopen check) are tested only by reading the script text and were never run; the ack regex and the doubled-backslash match are the likeliest place for a mistake.

Not merged.

Follow-ups (not in this PR)

  • 8.3 short names via GetFullPath: UNVERIFIED, nobody has tested it on Windows. The root check compares Path.GetFullPath results (ClientRuntimeLoader.cs RequireLexicallyUnderRoot and ResolveTrusted), which is believed not to expand 8.3 short names, so a short-name spelling of a path may not compare equal to its long form.
  • prune live-slots: pruning exists: Remove-OldSlots in scripts/dev-reload.ps1 keeps the newest -RetainSlots (default 12) directories per root, under both assemblies\slots and assemblies\live-slots, and dev-watch runs dev-reload on every reload. The residuals: directories from before this change sit at the old artifacts\dev-reload\slots and artifacts\dev-reload\live-slots and are never pruned (the new code only looks under assemblies\); dev-watch.ps1 itself never prunes, so between dev-reload runs live slots accumulate; a failed delete (locked slot) only logs. Unrun on Windows.
  • SynthPlayback alpha.3: SynthPlaybackTests.AquaSynthPatchCompilerCanRenderAudiblePatchForEnginePlayback fails with Method not found: AquaSynth.Dsl.FaustExportOptions..ctor(String, Boolean, Boolean); the checked-in AquaSynth.Core 0.1.0-alpha.3 lacks the three-argument constructor the Faust package calls (identical on main, PR does not touch AquaSynth).
  • SynthPlayback alpha.4: AquaSynth.Faust 0.1.0-alpha.4 (checked in, depends on Core alpha.3) was built against a newer Core; the pair in packages/ must be rebuilt or re-versioned together, and the test should skip or assert on a clear message until then.

Windows-only changes in this PR (junction refusal as InvalidOperationException, test cleanup tolerates locked files, newline test skipped) were not run on Windows.

Known limits

  • Check-to-open gap: the root check resolves a path, then the loader opens it by path; a writer with access inside the root can swap a link in between.
  • A managed dependency the resolver cannot resolve (ResolveAssemblyToPath null) returns null from Load (ClientRuntimeLoader.cs:339), so the runtime falls back to the default context, which probes the host directory with no root check; the shared host assembly is intentionally returned null the same way (:335).
  • A native library with no resolver path returns IntPtr.Zero (:345), so the default native probing runs, again without a root check.
  • Loaded native libraries stay locked and mapped for the process lifetime: a collectible context unloads managed assemblies, not natives (a .NET property, not tested here).
  • Native libraries are opened by path.
  • A handle-based check (open first, verify the opened file) is a later cut.

🤖 Generated with Claude Code

Closes the pointer-file load hole in the custody map (section 13.3). Proposal and reasoning: handoff note `aqua-fensalir-pointer-load-fix.md`. **The real boundary is the trusted directory's ACLs.** This check decides which directory may hold client code; it does nothing about who can write there. `scripts/dev-reload.ps1` creates `artifacts\dev-reload` with default ACLs, and this PR changes no ACL. Anyone who can write inside the root still owns the process. **Everything under the root is client code:** the host build copied into each slot and the shader sources next to it are loaded or consumed by the host as well, so the root must be writable only by the developer account, not just the client assembly. **Rule.** Every client assembly, and every dependency or native library its load context resolves, must lie (links followed) under the trusted root, or the load is refused: `Reload` (initial `--client-assembly` load and pointer reload) and the load context's managed and unmanaged resolution all call one check. The pointer target gets the string rules (local, fully qualified) before any filesystem probe, and every logged path is escaped once, at the console write (exception messages carry the raw path), by code point: backslash, control, format including tag characters (U+E0001, U+E0020-E007F), astral format characters (e.g. U+1D173-1D17A), line/paragraph separators, and lone surrogates. Ordinary astral characters (emoji) are kept. A startup refusal in `AquariumHost` is printed the same way, with the escaped exception type name. Cost: on Windows the host now logs backslashes doubled (`C:\\x`). A path outside the root (as resolved or as given) is refused by string comparison before any filesystem access, so a link outside it (to a UNC share, a mapped drive) is never followed or probed. Relative and UNC paths (any `/` or `\` mix at the start) are refused. - The root comes from `--client-trusted-root` / `AQUARIUM_CLIENT_TRUSTED_ROOT`, default the directory of the initial assembly. It is resolved once in the constructor and pinned; a drive or volume root is refused; the effective root is logged at startup. The environment variable is taken once by `TrustedRootSetting` (cleared on every path, flag or env, and cached) so processes this one starts do not inherit it, and an empty or missing flag value, or a blank environment value, is refused rather than falling back to the default root. - The loader loads and records the **resolved** final path. A pointer naming a link is compared by its resolved path, so it is not reloaded every poll. - A relative *initial* path is resolved against the working directory (operator-supplied); relative pointer contents are refused. No hash pin, no signatures. **dev-reload.ps1** passes `$assemblyRoot` (`artifacts\dev-reload\assemblies`) as the root, not the whole dev root (which also holds cache, state and logs): rebuilds land in `assemblies\slots` and dev-watch writes its reload pointer under `assemblies\live-slots`. dev-watch passes nothing itself. dev-reload also prunes `live-slots` (kept: `-RetainSlots` newest) and a -Reopen with state naming a path outside the root or a pruned file fails up front with a message; dev-watch matches the host ack by the unique slot leaf and assembly name, with backslashes doubled as the host logs them. Transition: a `state.clixml` written before this change names an old `slots` path outside the new root, so the first reopen after upgrading is refused until one fresh dev-reload build. **Tests** (`ClientRuntimeLoaderTrustTests`, 30): in-root load and reload; sibling directory loads under an explicit root and not under the default; initial assembly outside the root; outside, `..` and relative reloads; UNC in four separator mixes; volume root refused; dependency/native check (the static check the load context calls); symlinked file and directory escapes; a junction escape (a non-administrator junction refused by the OS as an untrusted mount point is reported as InvalidOperationException too); link stored by resolved path; root pinned at construction; pointer outside the root keeps the previous runtime; a UNC pointer is reported before any file probe; control characters in a pointer target are escaped in the log (skipped on Windows, where a file name cannot hold a newline); a literal backslash in a pointer target is shown as two characters, not four (escaped once); `Printable` escapes tag characters, musical format characters and lone surrogates by code point and keeps emoji; a startup refusal exits 1 and logs the escaped exception type; a loaded client's `Assembly.Location` equals its resolved file path; the env variable is cleared on flag and env paths and an empty flag is refused; the dev scripts keep slots and live-slots under one assemblies root that is not the dev root (this last one reads the script text, so it is brittle). Symlink tests report Skipped with a reason if the account cannot create links; the junction test is Skipped everywhere but Windows. Test cleanup retries, then prints a warning naming the directory if a locked assembly blocks the delete. **Results.** On Linux: loader tests 29 pass, 1 skipped (junction), 30 total. Engine test project: 86 pass, 2 fail, 1 skipped, 89 total; the 2 failures are `D3D12SharedTextureInteropTests` (need `d3d11.dll`; identical on main 34aaebf). The new tests were run against the earlier loader and fail there: backslash-once, rune escaping and host startup (against 467a97f), `Assembly.Location` (against the in-memory loader of d2f280d). The mutation results from the earlier revision of this PR (lexical check, env handling, escaping, ack match, live-slots pruning, each killed by a named test) were not re-run after the later commits. Nothing in this PR has run on Windows. **A second reader should check** 1. Windows: the junction test has never run; `LinkTarget` on junctions, `\?\` prefixes (refused as UNC-shaped), drive-relative `C:x` (not fully qualified). 2. **Gaps in the tests:** the UNC-separator test cannot tell the explicit UNC check from the containment check on Linux (both refuse), so it has force only on Windows. The dependency test calls the shared check directly; no test drives a real client assembly whose `.deps.json` resolves outside the root, so removing the check inside the load context would still pass. The log line and the environment clearing are untested. 3. TOCTOU between the check and the load: someone who can swap a file inside the root between them is already inside the boundary; not closed. 4. Other clients (Aquarium repo, Epiphany) that reload from outside the initial directory must pass `--client-trusted-root`. Checked: Fensalir capture scripts, Perlines capture and Mimir stay inside their own directory. 5. A refused pointer is retried every 500 ms and logs each time. 6. Not run on Windows: dev-reload/dev-watch scripts, including the moved assemblies layout and the live-slots reload path; nothing parses the PowerShell, the script test only reads text. 7. The PowerShell changes (ack matching, live-slots pruning, stale -Reopen check) are tested only by reading the script text and were never run; the ack regex and the doubled-backslash match are the likeliest place for a mistake. Not merged. ## Follow-ups (not in this PR) - `8.3 short names via GetFullPath`: UNVERIFIED, nobody has tested it on Windows. The root check compares `Path.GetFullPath` results (`ClientRuntimeLoader.cs` `RequireLexicallyUnderRoot` and `ResolveTrusted`), which is believed not to expand 8.3 short names, so a short-name spelling of a path may not compare equal to its long form. - `prune live-slots`: pruning exists: `Remove-OldSlots` in `scripts/dev-reload.ps1` keeps the newest `-RetainSlots` (default 12) directories per root, under both `assemblies\slots` and `assemblies\live-slots`, and `dev-watch` runs dev-reload on every reload. The residuals: directories from before this change sit at the old `artifacts\dev-reload\slots` and `artifacts\dev-reload\live-slots` and are never pruned (the new code only looks under `assemblies\`); `dev-watch.ps1` itself never prunes, so between dev-reload runs live slots accumulate; a failed delete (locked slot) only logs. Unrun on Windows. - `SynthPlayback alpha.3`: `SynthPlaybackTests.AquaSynthPatchCompilerCanRenderAudiblePatchForEnginePlayback` fails with `Method not found: AquaSynth.Dsl.FaustExportOptions..ctor(String, Boolean, Boolean)`; the checked-in AquaSynth.Core 0.1.0-alpha.3 lacks the three-argument constructor the Faust package calls (identical on main, PR does not touch AquaSynth). - `SynthPlayback alpha.4`: AquaSynth.Faust 0.1.0-alpha.4 (checked in, depends on Core alpha.3) was built against a newer Core; the pair in `packages/` must be rebuilt or re-versioned together, and the test should skip or assert on a clear message until then. Windows-only changes in this PR (junction refusal as InvalidOperationException, test cleanup tolerates locked files, newline test skipped) were not run on Windows. ## Known limits - Check-to-open gap: the root check resolves a path, then the loader opens it by path; a writer with access inside the root can swap a link in between. - A managed dependency the resolver cannot resolve (`ResolveAssemblyToPath` null) returns null from `Load` (`ClientRuntimeLoader.cs:339`), so the runtime falls back to the default context, which probes the host directory with no root check; the shared host assembly is intentionally returned null the same way (`:335`). - A native library with no resolver path returns `IntPtr.Zero` (`:345`), so the default native probing runs, again without a root check. - Loaded native libraries stay locked and mapped for the process lifetime: a collectible context unloads managed assemblies, not natives (a .NET property, not tested here). - Native libraries are opened by path. - A handle-based check (open first, verify the opened file) is a later cut. 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
The reload pointer, and the initial --client-assembly, named any path and ran
whatever it held in the engine process. Reload now refuses relative and UNC
paths and anything that, links followed, lies outside the trusted root
(--client-trusted-root or AQUARIUM_CLIENT_TRUSTED_ROOT; default: the directory
of the initial assembly). dev-reload passes its slot directory as the root.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
heimdall commented 2026-11-07 10:47:33 +00:00
Member
Copy link

Read-only second read at d44666e (no build, nothing run; findings are from reading the diff and the full loader).

Not clean.

  1. Load uses the unresolved path. ClientRuntimeLoader.cs:58 checks assemblyPath through ResolveLinks, but :60 (ClientAssemblyLoadContext, so also AssemblyDependencyResolver) and :65 (LoadFromAssemblyPath) take the original string, and :91 stores it. Check and load resolve the path separately, so a link swapped between them redirects the load. RequireTrusted should return the resolved final path and the load should use that. A race remains for anyone who can write inside the root, but that writer can already drop a dll there.
  2. Trust root is re-resolved on every check (:111, ResolveLinks(trustedRoot)), not pinned at construction. Replacing the root directory with a link silently moves the boundary. Needs write access to the root's parent, so same threat class as 1.
  3. Dependencies are not checked. Only the main dll goes through RequireTrusted. ClientAssemblyLoadContext.Load and LoadUnmanagedDll (:213-231) load whatever .deps.json resolves, with no root check. Inside-the-root writers only.
  4. Default root vs symlinked initial assembly. Default root is the directory of the unresolved initial path (:29-32). If the initial assembly is itself a link to elsewhere, ResolveLinks takes it outside the root and the initial load is refused. Fail-closed, but --client-trusted-root becomes mandatory there; untested.
  5. Windows paths. Junctions: DirectoryInfo.LinkTarget covers them, so OK in principle, but no junction test. \\?\ and \\.\ are refused by the \\ prefix test; \??\ is not fully qualified, refused. /\host\share\x.dll and \/host\share\x.dll pass the \\/// string test (:104-106, IsPathFullyQualified accepts mixed-separator UNC) and are caught only by the root comparison. If the root itself is UNC (via the hatch) they pass. Test the mixed forms or normalise first. Mapped drives pointing at shares are not resolved.
  6. Permissions are not tightened. The diff changes no ACL or mode, nor checks that the root is not writable by others; dev-reload.ps1 creates $slotRoot with New-Item default ACLs (:33). The rule is only as strong as who can write the root.
  7. Escape hatch widens trust silently. --client-trusted-root or AQUARIUM_CLIENT_TRUSTED_ROOT (AquariumHost.cs new ParseClientTrustedRoot) is accepted as any path, including a drive root or /, and nothing logs the effective root at startup. The env var is inherited by child processes. A flag with no value is silently ignored. Suggest: log the resolved root, and refuse or warn on a filesystem root.

Tests (ClientRuntimeLoaderTrustTests.cs), by reading, not mutation-run. Roughly 4 of 5 are real catches, with caveats:

  • :194 is a positive control only; it catches over-refusal, not loosening.
  • :205 real (initial-load check).
  • :212: the outside and .. assertions are real, but the relative (:219) and ///\\ UNC (:220-221) assertions are redundant with the root comparison. Deleting the explicit relative/UNC check leaves them green. Assert.Throws<InvalidOperationException> also accepts any reason, including "No trusted root".
  • :225 real when links can be made, but returns at :237 and reports a pass on hosts that cannot, such as Windows without symlink privilege. No junction case.
  • :247 real (removing RequireTrusted makes the runtime change), but has no positive control that an in-root pointer target does reload, so a dead pointer path would also pass.
Read-only second read at d44666e (no build, nothing run; findings are from reading the diff and the full loader). **Not clean.** 1. **Load uses the unresolved path.** `ClientRuntimeLoader.cs:58` checks `assemblyPath` through `ResolveLinks`, but `:60` (`ClientAssemblyLoadContext`, so also `AssemblyDependencyResolver`) and `:65` (`LoadFromAssemblyPath`) take the original string, and `:91` stores it. Check and load resolve the path separately, so a link swapped between them redirects the load. `RequireTrusted` should return the resolved final path and the load should use that. A race remains for anyone who can write inside the root, but that writer can already drop a dll there. 2. **Trust root is re-resolved on every check** (`:111`, `ResolveLinks(trustedRoot)`), not pinned at construction. Replacing the root directory with a link silently moves the boundary. Needs write access to the root's parent, so same threat class as 1. 3. **Dependencies are not checked.** Only the main dll goes through `RequireTrusted`. `ClientAssemblyLoadContext.Load` and `LoadUnmanagedDll` (`:213-231`) load whatever `.deps.json` resolves, with no root check. Inside-the-root writers only. 4. **Default root vs symlinked initial assembly.** Default root is the directory of the unresolved initial path (`:29-32`). If the initial assembly is itself a link to elsewhere, `ResolveLinks` takes it outside the root and the initial load is refused. Fail-closed, but `--client-trusted-root` becomes mandatory there; untested. 5. **Windows paths.** Junctions: `DirectoryInfo.LinkTarget` covers them, so OK in principle, but no junction test. `\\?\` and `\\.\` are refused by the `\\` prefix test; `\??\` is not fully qualified, refused. `/\host\share\x.dll` and `\/host\share\x.dll` pass the `\\`/`//` string test (`:104-106`, `IsPathFullyQualified` accepts mixed-separator UNC) and are caught only by the root comparison. If the root itself is UNC (via the hatch) they pass. Test the mixed forms or normalise first. Mapped drives pointing at shares are not resolved. 6. **Permissions are not tightened.** The diff changes no ACL or mode, nor checks that the root is not writable by others; `dev-reload.ps1` creates `$slotRoot` with `New-Item` default ACLs (`:33`). The rule is only as strong as who can write the root. 7. **Escape hatch widens trust silently.** `--client-trusted-root` or `AQUARIUM_CLIENT_TRUSTED_ROOT` (`AquariumHost.cs` new `ParseClientTrustedRoot`) is accepted as any path, including a drive root or `/`, and nothing logs the effective root at startup. The env var is inherited by child processes. A flag with no value is silently ignored. Suggest: log the resolved root, and refuse or warn on a filesystem root. **Tests (`ClientRuntimeLoaderTrustTests.cs`), by reading, not mutation-run.** Roughly 4 of 5 are real catches, with caveats: - `:194` is a positive control only; it catches over-refusal, not loosening. - `:205` real (initial-load check). - `:212`: the outside and `..` assertions are real, but the relative (`:219`) and `//`/`\\` UNC (`:220-221`) assertions are redundant with the root comparison. Deleting the explicit relative/UNC check leaves them green. `Assert.Throws<InvalidOperationException>` also accepts any reason, including "No trusted root". - `:225` real when links can be made, but returns at `:237` and reports a pass on hosts that cannot, such as Windows without symlink privilege. No junction case. - `:247` real (removing `RequireTrusted` makes the runtime change), but has no positive control that an in-root pointer target does reload, so a dead pointer path would also pass. 
dev-watch writes the reload pointer under artifacts/dev-reload/live-slots, a
sibling of slots; a slot-directory root refused every watch reload and the
reopen path. Test: a sibling directory loads inside an explicit root, not under
the default root, and an outside assembly is still refused.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Review (Heimdall): the root is resolved once at construction and a volume root
is refused and the effective root logged; the loader loads and records the
resolved final path (so a pointer naming a link does not reload every poll);
dependency and native-library resolution go through the same check; the
AQUARIUM_CLIENT_TRUSTED_ROOT variable is cleared after reading. Tests: UNC in any
separator mix, volume root, pinned root, resolved-path storage, junction and
symlink tests that report an explicit skip instead of passing silently.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
heimdall commented 2026-11-07 10:53:44 +00:00
Member
Copy link

Read-only re-read at 7ec7a0f (no build, nothing run).

Landed: root resolved once and pinned (ClientRuntimeLoader.cs:39,51); load and record use the resolved path (:73-74,84,110,146-153); volume root refused after link resolution (:40-46); effective root logged (:48); managed and native dependency resolution go through the same check (:255,261); mixed-separator UNC refused at :173; the PR body states the ACL boundary.

Wrong:

  1. AquariumHost.cs:266-268 returns before the env clear at :274. With --client-trusted-root present, AQUARIUM_CLIENT_TRUSTED_ROOT is never read or cleared, so children still inherit it. An empty flag value (--client-trusted-root "") also returns early, and the loader then takes the default root rather than the env value. Clear the variable unconditionally, before the flag loop.
  2. ClientRuntimeLoader.cs:139: File.Exists(candidatePath) runs on the raw pointer string before ResolveTrusted at :146. A pointer holding \\attacker\share\x.dll makes Windows touch the share (SMB/NTLM authentication) every 500 ms, then the UNC refusal fires. The UNC check at :173 is pure string work; run ResolveTrusted first, then test existence.
  3. :158, :175, :182 interpolate the pointer-controlled string into stderr/stdout. Interior newlines forge log lines; the length is unbounded. Quote or escape it (e.g. :q-style JSON string or Path with control chars replaced). The :48 line prints the absolute root (includes the user profile name); that is fine for a local log, but note dev-reload.ps1:30-31 writes stdout/stderr into $devRoot, which is now the trusted root.
  4. dev-reload.ps1:199 passes $devRoot, wider than the slots/live-slots the loader needs. It also contains the cultcache, state.clixml and the logs, so a dll that any of those writers drops there loads. Two subdirectories cannot be one root, but consider passing the pointer's target directory only, or document that the dev root is the trust unit.
  5. Env var read twice: only one reader exists (AquariumHost.cs:21, :273), so nothing breaks today. A second call to ParseClientTrustedRoot in the same process (a test, a relaunch) gets null and silently falls to the default root.

Clean by reading, but untested: case (GetRelativePath is OrdinalIgnoreCase on Windows, Ordinal elsewhere; exotic gap only for per-directory case-sensitive folders); 8.3 short names and trailing dots/spaces (GetFullPath expands and strips them before the containment check, and the normalised path is what gets loaded); \\?\, \\.\, \??\; /\ and \/ UNC. No test covers 8.3, trailing dots or \\?\. Mapped drives and subst letters are not resolved (a root on one trusts that share).

Read-only re-read at 7ec7a0f (no build, nothing run). **Landed:** root resolved once and pinned (`ClientRuntimeLoader.cs:39,51`); load and record use the resolved path (`:73-74,84,110,146-153`); volume root refused after link resolution (`:40-46`); effective root logged (`:48`); managed and native dependency resolution go through the same check (`:255,261`); mixed-separator UNC refused at `:173`; the PR body states the ACL boundary. **Wrong:** 1. `AquariumHost.cs:266-268` returns before the env clear at `:274`. With `--client-trusted-root` present, `AQUARIUM_CLIENT_TRUSTED_ROOT` is never read or cleared, so children still inherit it. An empty flag value (`--client-trusted-root ""`) also returns early, and the loader then takes the default root rather than the env value. Clear the variable unconditionally, before the flag loop. 2. `ClientRuntimeLoader.cs:139`: `File.Exists(candidatePath)` runs on the raw pointer string before `ResolveTrusted` at `:146`. A pointer holding `\\attacker\share\x.dll` makes Windows touch the share (SMB/NTLM authentication) every 500 ms, then the UNC refusal fires. The UNC check at `:173` is pure string work; run `ResolveTrusted` first, then test existence. 3. `:158`, `:175`, `:182` interpolate the pointer-controlled string into stderr/stdout. Interior newlines forge log lines; the length is unbounded. Quote or escape it (e.g. `:q`-style JSON string or `Path` with control chars replaced). The `:48` line prints the absolute root (includes the user profile name); that is fine for a local log, but note `dev-reload.ps1:30-31` writes stdout/stderr into `$devRoot`, which is now the trusted root. 4. `dev-reload.ps1:199` passes `$devRoot`, wider than the `slots`/`live-slots` the loader needs. It also contains the cultcache, `state.clixml` and the logs, so a dll that any of those writers drops there loads. Two subdirectories cannot be one root, but consider passing the pointer's target directory only, or document that the dev root is the trust unit. 5. Env var read twice: only one reader exists (`AquariumHost.cs:21`, `:273`), so nothing breaks today. A second call to `ParseClientTrustedRoot` in the same process (a test, a relaunch) gets null and silently falls to the default root. **Clean by reading, but untested:** case (`GetRelativePath` is OrdinalIgnoreCase on Windows, Ordinal elsewhere; exotic gap only for per-directory case-sensitive folders); 8.3 short names and trailing dots/spaces (`GetFullPath` expands and strips them before the containment check, and the normalised path is what gets loaded); `\\?\`, `\\.\`, `\??\`; `/\` and `\/` UNC. No test covers 8.3, trailing dots or `\\?\`. Mapped drives and `subst` letters are not resolved (a root on one trusts that share). 
Review (Heimdall): dev scripts keep slots and live-slots under artifacts/dev-reload/assemblies
and pass that as the root, not the whole dev root; --client-trusted-root refuses an empty or
missing value and the environment variable is cleared on every path; the UNC/fully-qualified
string rule runs before File.Exists on a pointer target; control characters are escaped in
every logged path.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
heimdall commented 2026-11-07 10:59:10 +00:00
Member
Copy link

Read-only re-read at 45b0b38 (no build, nothing run).

Landed: env read and cleared first on every path (AquariumHost.cs:264-265); flag without a value or with an empty one throws (:275-278); UNC string rules in any separator mix run before File.Exists (ClientRuntimeLoader.cs:146,190-196); every path and exception message in the loader's logs goes through Printable (:45,48,160,164,184,194,224,236); dev-reload passes artifacts\dev-reload\assemblies (dev-reload.ps1:202), and slots, live-slots (dev-watch.ps1:23) sit under it while state, cache, logs and the pointer stay outside.

Wrong:

  1. dev-watch.ps1:458,463: the live-reload acknowledgement looks for the exact line Client runtime reload applied: $liveAssemblyPath, but the host now prints the resolved path (ClientRuntimeLoader.cs:160, Printable(resolved)). Whenever the script's path differs from the resolved one (repo under a junction/symlink, 8.3 short name, ..), the reload is applied and dev-watch still throws "did not acknowledge" at :479. String.Contains is also case-sensitive, so a case difference does the same. Match on a path-insensitive token, or have the script resolve its path the same way.
  2. AquariumHost.cs:261-284: "read once and cached" is not done. The env var is read per call; a second call in the same process (test, relaunch) returns null and silently falls to the default root. Also an empty or whitespace AQUARIUM_CLIENT_TRUSTED_ROOT with no flag returns "" (:283) and the ctor takes the default root (ClientRuntimeLoader.cs:34), while an empty flag throws. Treat an empty env value as the same error.
  3. ClientRuntimeLoader.cs:147 (your item 7) and :180: containment is still checked after a filesystem probe. File.Exists touches the local path first, and ResolveTrusted then calls ResolveLinks, whose Directory.Exists/LinkTarget also probe every parent of the unchecked path (:227-239). A symlink to a UNC share planted anywhere local, or a mapped network drive letter named in the pointer, still makes the machine connect before the root check refuses. Do the lexical containment check (GetFullPath + GetRelativePath against the pinned root, no FS access) before any probe, then resolve links.
  4. Printable (:199-215) escapes only char.IsControl (Cc). Not escaped: U+2028/2029, bidi controls U+202A-202E and U+2066-2069 (log text can be visually reordered), and the backslash itself, so a pointer containing a literal \n or \u000a looks the same as an escaped newline. Escape Cf/Zl/Zp as well and escape \.
  5. dev-reload.ps1:85,91: Remove-OldSlots only prunes the new assemblies\slots. The old artifacts\dev-reload\slots is never cleaned, so up to 12 full build outputs stay orphaned. The stale state.clixml refusal on first reopen is as the PR says: Reopen-VisibleProcess (:242-248) passes the old liveAssembly and the host dies at load with the "outside the trusted root" message; the script does not detect it up front.
  6. The assemblies root is not only assemblies: each slot is the whole host build (Aquarium.Engine.exe, host dlls) and dev-watch copies shader sources into the running slot (dev-watch.ps1:347-348,426). Not a bypass, but the "everything under the root is client code" wording is looser than the contents.

Mapped/subst drives: no bypass found (a different letter for the same directory fails closed, since GetRelativePath returns a rooted path), but see item 3 for the probe. A UNC-mapped root is accepted.

Read-only re-read at 45b0b38 (no build, nothing run). **Landed:** env read and cleared first on every path (`AquariumHost.cs:264-265`); flag without a value or with an empty one throws (`:275-278`); UNC string rules in any separator mix run before `File.Exists` (`ClientRuntimeLoader.cs:146,190-196`); every path and exception message in the loader's logs goes through `Printable` (`:45,48,160,164,184,194,224,236`); dev-reload passes `artifacts\dev-reload\assemblies` (`dev-reload.ps1:202`), and slots, live-slots (`dev-watch.ps1:23`) sit under it while state, cache, logs and the pointer stay outside. **Wrong:** 1. `dev-watch.ps1:458,463`: the live-reload acknowledgement looks for the exact line `Client runtime reload applied: $liveAssemblyPath`, but the host now prints the resolved path (`ClientRuntimeLoader.cs:160`, `Printable(resolved)`). Whenever the script's path differs from the resolved one (repo under a junction/symlink, 8.3 short name, `..`), the reload is applied and dev-watch still throws "did not acknowledge" at `:479`. `String.Contains` is also case-sensitive, so a case difference does the same. Match on a path-insensitive token, or have the script resolve its path the same way. 2. `AquariumHost.cs:261-284`: "read once and cached" is not done. The env var is read per call; a second call in the same process (test, relaunch) returns null and silently falls to the default root. Also an empty or whitespace `AQUARIUM_CLIENT_TRUSTED_ROOT` with no flag returns `""` (`:283`) and the ctor takes the default root (`ClientRuntimeLoader.cs:34`), while an empty flag throws. Treat an empty env value as the same error. 3. `ClientRuntimeLoader.cs:147` (your item 7) and `:180`: containment is still checked after a filesystem probe. `File.Exists` touches the local path first, and `ResolveTrusted` then calls `ResolveLinks`, whose `Directory.Exists`/`LinkTarget` also probe every parent of the unchecked path (`:227-239`). A symlink to a UNC share planted anywhere local, or a mapped network drive letter named in the pointer, still makes the machine connect before the root check refuses. Do the lexical containment check (`GetFullPath` + `GetRelativePath` against the pinned root, no FS access) before any probe, then resolve links. 4. `Printable` (`:199-215`) escapes only `char.IsControl` (Cc). Not escaped: U+2028/2029, bidi controls U+202A-202E and U+2066-2069 (log text can be visually reordered), and the backslash itself, so a pointer containing a literal `\n` or `\u000a` looks the same as an escaped newline. Escape Cf/Zl/Zp as well and escape `\`. 5. `dev-reload.ps1:85,91`: `Remove-OldSlots` only prunes the new `assemblies\slots`. The old `artifacts\dev-reload\slots` is never cleaned, so up to 12 full build outputs stay orphaned. The stale `state.clixml` refusal on first reopen is as the PR says: `Reopen-VisibleProcess` (`:242-248`) passes the old `liveAssembly` and the host dies at load with the "outside the trusted root" message; the script does not detect it up front. 6. The assemblies root is not only assemblies: each slot is the whole host build (`Aquarium.Engine.exe`, host dlls) and dev-watch copies shader sources into the running slot (`dev-watch.ps1:347-348,426`). Not a bypass, but the "everything under the root is client code" wording is looser than the contents. **Mapped/subst drives:** no bypass found (a different letter for the same directory fails closed, since `GetRelativePath` returns a rooted path), but see item 3 for the probe. A UNC-mapped root is accepted. 
Review (Heimdall): a path outside the pinned root (as resolved or as given) is refused by
string comparison before File.Exists or any link is followed; the trusted-root setting is
its own small type that takes the environment variable once, caches it and refuses blank
values; Printable also escapes backslash, format characters (bidi, zero-width) and the
line and paragraph separators; dev-watch matches the host ack by the unique slot leaf with
doubled backslashes; dev-reload prunes live-slots and rejects stale reopen state up front.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
heimdall commented 2026-11-07 11:06:17 +00:00
Member
Copy link

Read-only re-read at 467a97f (no build, nothing run).

Landed: (3) lexical containment against the root as resolved and as given runs before File.Exists (ClientRuntimeLoader.cs:148-150) and before ResolveLinks (:201-203). (1) The dev-watch regex is right for the line the host writes: appliedSuffix is <slotLeaf>\\Aquarium.Fensalir.dll with doubled backslashes, [regex]::Escape turns each into \\, which matches the two literal backslashes Printable emits at :163; "$slotLeaf\" is a valid PowerShell string; the suffix is ASCII so code-page differences in the redirected log do not matter. (2) TrustedRootSetting reads and clears once via Lazy and caches; blank env, blank flag and a flag without a value throw (TrustedRootSetting.cs:25-41). (4) Printable covers backslash, control, Cf and U+2028/2029 for BMP characters. (5) Remove-OldSlots covers both roots and Reopen-VisibleProcess checks the recorded paths up front. (6) The PR body says everything under the root is client code.

Wrong:

  1. Double escaping from the doubled-backslash change. The loader's own exception messages are built with Printable (ClientRuntimeLoader.cs:188,206,216,252,264) and then TryReloadFromPointer passes the whole message through Printable again (:167). On Windows every backslash in a refusal message is therefore quadrupled (C:\\\\x) while the applied line (:163) is doubled, and a literal backslash in an attacker's pointer text is no longer distinguishable. Escape once: either keep raw paths in exception messages and escape at the log site, or log error.Message raw for the loader's own exceptions. The control-character test (:664) does not catch this.
  2. Printable is char-based and misses astral Cf characters (:225-240). char.GetUnicodeCategory(char) returns Surrogate for each half of a surrogate pair, so tag characters U+E0001, U+E0020-E007F (invisible, spoofable) and the musical format controls U+1D173-1D17A pass through unescaped, as do lone surrogates. Iterate runes (EnumerateRunes, Rune.GetUnicodeCategory) and escape Cf/Zl/Zp/Cs/Cn/Co as \u{...}. The test at :759-766 only has BMP cases.
  3. "Pure string comparison" is not strictly true on Windows. Path.GetFullPath (:180,202) expands 8.3 short names when the path contains ~, which touches the filesystem before the containment check passes. A pointer naming C:\evil\link~1\x.dll can make it enumerate local directories (including through a link) before refusal. Local-only, low risk; either reject ~ segments in the unchecked string, or compare Path.GetFullPath of the root and path after a lexical ..-collapse that does no expansion.
  4. Pruning only runs inside dev-reload (dev-reload.ps1:303,337 and the normal path). During a long dev-watch session new live slots accumulate (dev-reload.ps1 comment says dev-watch never prunes), so live-slots still grows without bound until the next full dev-reload. The pre-change artifacts\dev-reload\slots and live-slots are never cleaned either (Remove-OldSlots touches only the new roots).

Not tested: the dev-watch ack regex and -Reopen check are tested only by reading the script text; nothing parses the PowerShell or runs it.

Read-only re-read at 467a97f (no build, nothing run). **Landed:** (3) lexical containment against the root as resolved and as given runs before `File.Exists` (`ClientRuntimeLoader.cs:148-150`) and before `ResolveLinks` (`:201-203`). (1) The dev-watch regex is right for the line the host writes: `appliedSuffix` is `<slotLeaf>\\Aquarium.Fensalir.dll` with doubled backslashes, `[regex]::Escape` turns each into `\\`, which matches the two literal backslashes `Printable` emits at `:163`; `"$slotLeaf\"` is a valid PowerShell string; the suffix is ASCII so code-page differences in the redirected log do not matter. (2) `TrustedRootSetting` reads and clears once via `Lazy` and caches; blank env, blank flag and a flag without a value throw (`TrustedRootSetting.cs:25-41`). (4) `Printable` covers backslash, control, Cf and U+2028/2029 for BMP characters. (5) `Remove-OldSlots` covers both roots and `Reopen-VisibleProcess` checks the recorded paths up front. (6) The PR body says everything under the root is client code. **Wrong:** 1. **Double escaping from the doubled-backslash change.** The loader's own exception messages are built with `Printable` (`ClientRuntimeLoader.cs:188,206,216,252,264`) and then `TryReloadFromPointer` passes the whole message through `Printable` again (`:167`). On Windows every backslash in a refusal message is therefore quadrupled (`C:\\\\x`) while the applied line (`:163`) is doubled, and a literal backslash in an attacker's pointer text is no longer distinguishable. Escape once: either keep raw paths in exception messages and escape at the log site, or log `error.Message` raw for the loader's own exceptions. The control-character test (`:664`) does not catch this. 2. **`Printable` is char-based and misses astral Cf characters** (`:225-240`). `char.GetUnicodeCategory(char)` returns `Surrogate` for each half of a surrogate pair, so tag characters U+E0001, U+E0020-E007F (invisible, spoofable) and the musical format controls U+1D173-1D17A pass through unescaped, as do lone surrogates. Iterate runes (`EnumerateRunes`, `Rune.GetUnicodeCategory`) and escape Cf/Zl/Zp/Cs/Cn/Co as `\u{...}`. The test at `:759-766` only has BMP cases. 3. **"Pure string comparison" is not strictly true on Windows.** `Path.GetFullPath` (`:180,202`) expands 8.3 short names when the path contains `~`, which touches the filesystem before the containment check passes. A pointer naming `C:\evil\link~1\x.dll` can make it enumerate local directories (including through a link) before refusal. Local-only, low risk; either reject `~` segments in the unchecked string, or compare `Path.GetFullPath` of the root and path after a lexical `..`-collapse that does no expansion. 4. **Pruning only runs inside dev-reload** (`dev-reload.ps1:303,337` and the normal path). During a long dev-watch session new live slots accumulate (`dev-reload.ps1` comment says dev-watch never prunes), so `live-slots` still grows without bound until the next full dev-reload. The pre-change `artifacts\dev-reload\slots` and `live-slots` are never cleaned either (`Remove-OldSlots` touches only the new roots). **Not tested:** the dev-watch ack regex and `-Reopen` check are tested only by reading the script text; nothing parses the PowerShell or runs it. 
Exception messages carry raw paths and every console write escapes them once (Printable iterates runes: astral format characters and lone surrogates). AquariumHost reports startup refusals escaped. IOException from link resolution becomes InvalidOperationException. Managed assemblies load from memory so the temp dir can be deleted on Windows. The newline-in-file-name test is skipped on Windows.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
heimdall commented 2026-11-07 11:34:00 +00:00
Member
Copy link

Read-only re-read at d2f280d (no build, nothing run). What the in-memory load weakens, and what landed.

Landed: Printable now works by code point (tag characters, astral Cf, lone surrogates) and exception messages carry raw paths with the console write escaping once (ClientRuntimeLoader.cs:167, AquariumHost.cs:31); startup failure is caught, logged escaped and returns 1 with the loader disposed (AquariumHost.cs:25-34).

Weakened or wrong:

  1. Assembly.Location is now empty and the client uses it. FensalirRenderPlan.cs:35-36 (ShaderPath) and FensalirFractalScene.cs:45-46 (AssemblyDirectory) take Path.GetDirectoryName(typeof(...).Assembly.Location) ?? AppContext.BaseDirectory. For an in-memory assembly the directory is null, so both silently fall back to the host's base directory, not the client's slot. In a plain slot they coincide; after a dev-watch live reload the client dll sits in live-slots\<new> but shaders and world files (Aquarium.Fensalir.csproj:18-20 copies them next to the dll) are read from the host's original slot. Likely a regression for live reload (shader hot-reload copying into the running slot masks part of it, not the worlds/fractal assets). Needs a Windows run; there is no test for the fallback.
  2. Check-to-read gap is not closed; it is moved. LoadFromFileWithoutLock (:345-356) reads the bytes once and loads exactly those bytes, so the read-to-load gap is closed. But File.ReadAllBytes(path) reopens the resolved path by name after ResolveTrusted, so a link swap inside the root between check and read still wins. To close it: open a FileStream/handle, verify its final path is under the root (GetFinalPathNameByHandle on Windows, /proc/self/fd or realpath of the fd elsewhere), then read from that handle. For the same reason, the bytes are now a natural place for the hash pin the PR says it lacks.
  3. Symbols bypass the containment check (:348-355). Path.ChangeExtension(path, ".pdb") is neither resolved nor checked: File.Exists and ReadAllBytes follow a symlink out of the root and feed an arbitrary file to the runtime as a PDB, with its own check-to-read gap. Run it through ResolveTrusted (or skip symbols when it fails).
  4. Dependency resolution by name. Inside the root it is sound: ResolveAssemblyToPath → ResolveTrusted (:338-339). Anything the resolver returns null for falls to the default context (:335, :339 returns null) and is loaded from the host's TPA/base directory, outside the root, with no check. That is as before, but it is the one route by which a dependency not in deps.json comes from outside; state it in the PR body, or have Load refuse non-framework names.
  5. Native libraries. Still path-loaded: ResolveTrusted then LoadUnmanagedDllFromPath (:361) reopens the path, so the check-to-load gap is open for them, and a loaded native library stays locked and loaded for the process (collectible contexts do not free them), so the Windows lock the change avoids for managed code remains for natives. Names not in deps.json fall to default probing, and with no Assembly.Location the "assembly directory" probe is gone for stream-loaded dependencies.
  6. Debugger, symbols, resources. Symbols come through the stream only when a .pdb sits beside the dll (embedded PDBs still work). Debugger module paths are empty; Source Link, Edit-and-Continue and .NET Hot Reload likely do not bind to in-memory modules (unverified). Any dependency that reads its own Assembly.Location/CodeBase (plugin-style resolvers, resource probing) gets an empty string. No strong-name or hash check is lost, because none exists in .NET Core and the PR pins no hash. AssemblyLoadContext unload is unchanged (collectible context, no file handles for managed code).
Read-only re-read at d2f280d (no build, nothing run). What the in-memory load weakens, and what landed. **Landed:** `Printable` now works by code point (tag characters, astral Cf, lone surrogates) and exception messages carry raw paths with the console write escaping once (`ClientRuntimeLoader.cs:167`, `AquariumHost.cs:31`); startup failure is caught, logged escaped and returns 1 with the loader disposed (`AquariumHost.cs:25-34`). **Weakened or wrong:** 1. **`Assembly.Location` is now empty and the client uses it.** `FensalirRenderPlan.cs:35-36` (`ShaderPath`) and `FensalirFractalScene.cs:45-46` (`AssemblyDirectory`) take `Path.GetDirectoryName(typeof(...).Assembly.Location) ?? AppContext.BaseDirectory`. For an in-memory assembly the directory is null, so both silently fall back to the host's base directory, not the client's slot. In a plain slot they coincide; after a dev-watch live reload the client dll sits in `live-slots\<new>` but shaders and world files (`Aquarium.Fensalir.csproj:18-20` copies them next to the dll) are read from the host's original slot. Likely a regression for live reload (shader hot-reload copying into the running slot masks part of it, not the worlds/fractal assets). Needs a Windows run; there is no test for the fallback. 2. **Check-to-read gap is not closed; it is moved.** `LoadFromFileWithoutLock` (`:345-356`) reads the bytes once and loads exactly those bytes, so the read-to-load gap is closed. But `File.ReadAllBytes(path)` reopens the resolved path by name after `ResolveTrusted`, so a link swap inside the root between check and read still wins. To close it: open a `FileStream`/handle, verify its final path is under the root (`GetFinalPathNameByHandle` on Windows, `/proc/self/fd` or `realpath` of the fd elsewhere), then read from that handle. For the same reason, the bytes are now a natural place for the hash pin the PR says it lacks. 3. **Symbols bypass the containment check** (`:348-355`). `Path.ChangeExtension(path, ".pdb")` is neither resolved nor checked: `File.Exists` and `ReadAllBytes` follow a symlink out of the root and feed an arbitrary file to the runtime as a PDB, with its own check-to-read gap. Run it through `ResolveTrusted` (or skip symbols when it fails). 4. **Dependency resolution by name.** Inside the root it is sound: `ResolveAssemblyToPath` → `ResolveTrusted` (`:338-339`). Anything the resolver returns null for falls to the default context (`:335`, `:339` returns null) and is loaded from the host's TPA/base directory, outside the root, with no check. That is as before, but it is the one route by which a dependency not in `deps.json` comes from outside; state it in the PR body, or have `Load` refuse non-framework names. 5. **Native libraries.** Still path-loaded: `ResolveTrusted` then `LoadUnmanagedDllFromPath` (`:361`) reopens the path, so the check-to-load gap is open for them, and a loaded native library stays locked and loaded for the process (collectible contexts do not free them), so the Windows lock the change avoids for managed code remains for natives. Names not in `deps.json` fall to default probing, and with no `Assembly.Location` the "assembly directory" probe is gone for stream-loaded dependencies. 6. **Debugger, symbols, resources.** Symbols come through the stream only when a `.pdb` sits beside the dll (embedded PDBs still work). Debugger module paths are empty; Source Link, Edit-and-Continue and .NET Hot Reload likely do not bind to in-memory modules (unverified). Any dependency that reads its own `Assembly.Location`/`CodeBase` (plugin-style resolvers, resource probing) gets an empty string. No strong-name or hash check is lost, because none exists in .NET Core and the PR pins no hash. `AssemblyLoadContext` unload is unchanged (collectible context, no file handles for managed code). 
Render plan and fractal scene derive directories from Assembly.Location; the in-memory load emptied it.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
heimdall commented 2026-11-07 11:37:06 +00:00
Member
Copy link

Read-only re-read at 739e2bd against 467a97f and d2f280d (no build, nothing run).

Clean: (1) the revert is complete: ClientRuntimeLoader.cs has LoadFromAssemblyPath (:86,339) and LoadUnmanagedDllFromPath (:345) only; no LoadFromStream, ReadAllBytes, MemoryStream or .pdb handling remains, and the diff against 467a97f in this file is only the Printable (code-point) and junction-refusal changes. (2) Containment is unchanged: RequireLocalFullyQualified and RequireLexicallyUnderRoot before File.Exists (:148-150), lexical check before ResolveLinks (:201-203), and the path loaded is the resolved, checked one (:155,162, :339, :345). (3) The test-cleanup swallow is in ClientRuntimeLoaderTrustTests.Dispose only; no product code catches or hides IO errors. (5) Scripts, TrustedRootSetting.cs and AquariumHost.cs env handling are byte-identical to 467a97f; the only new differences are the intended ones (startup catch and return 1, raw exception messages with one escape at the console, junction as InvalidOperationException).

Wrong or weak:

  1. PR body, "Follow-ups" bullet prune live-slots: says old live-slot directories "are never removed", but the body (dev-reload also prunes live-slots) and dev-reload.ps1 Remove-OldSlots now prune both roots; only the pre-change artifacts\dev-reload\slots and live-slots are orphaned and pruning only runs inside a dev-reload run. Reword to the real residual.
  2. PR body, 8.3 short names follow-up vs the Rule paragraph: the Rule says a path outside the root is refused "by string comparison before any filesystem access". Path.GetFullPath (:180,202) is the first step of that comparison; whether it expands 8.3 names (and so touches the filesystem) on Windows when the path contains ~ is the point the follow-up asserts the opposite about. Unverified on Windows either way; the claim should be stated as unverified or the call replaced by a filesystem-free normaliser.
  3. PR body, "Tests (26)" and "Results" paragraphs are stale: they list the 467a97f tests and counts and do not mention the new startup-refusal-returns-1 test, the exactly-once backslash test, the astral/lone-surrogate Printable test or the Assembly.Location test. The Rule paragraph says escaped characters are "control, format (bidi, zero-width) and line/paragraph-separator"; the code now also covers tag characters, other astral Cf and lone surrogates.
  4. Known limits wording: "falls back to the host directory" for a null dependency is narrower than the code: Load returns null (:335,339), so resolution falls to the default load context (framework and host TPA, and the host base directory). Native names the resolver returns null for (:345) also fall to default probing. Add the framework/default-context wording, and that natives stay loaded and file-locked for the process lifetime. The "check-to-open" wording is accurate; the pdb path no longer exists.
  5. AquariumHost.cs:28-33: the startup catch prints only error.Message; a client factory or Start() exception loses its type and stack, which the earlier unhandled-exception path printed. Print the type name at least (escaped), and the stack to a log if the message path stays escaped.
  6. Test cleanup (ClientRuntimeLoaderTrustTests, Dispose): silently leaves the work directory after 5 failed attempts. Not a product risk, but a loader that never releases the file (a lock regression) passes every test on Linux and is silent on Windows; log a warning (or assert after the retry) so a persistent leak is visible.
Read-only re-read at 739e2bd against 467a97f and d2f280d (no build, nothing run). **Clean:** (1) the revert is complete: `ClientRuntimeLoader.cs` has `LoadFromAssemblyPath` (`:86,339`) and `LoadUnmanagedDllFromPath` (`:345`) only; no `LoadFromStream`, `ReadAllBytes`, `MemoryStream` or `.pdb` handling remains, and the diff against 467a97f in this file is only the Printable (code-point) and junction-refusal changes. (2) Containment is unchanged: `RequireLocalFullyQualified` and `RequireLexicallyUnderRoot` before `File.Exists` (`:148-150`), lexical check before `ResolveLinks` (`:201-203`), and the path loaded is the resolved, checked one (`:155,162`, `:339`, `:345`). (3) The test-cleanup swallow is in `ClientRuntimeLoaderTrustTests.Dispose` only; no product code catches or hides IO errors. (5) Scripts, `TrustedRootSetting.cs` and `AquariumHost.cs` env handling are byte-identical to 467a97f; the only new differences are the intended ones (startup catch and return 1, raw exception messages with one escape at the console, junction as `InvalidOperationException`). **Wrong or weak:** 1. **PR body, "Follow-ups" bullet `prune live-slots`:** says old live-slot directories "are never removed", but the body (`dev-reload also prunes live-slots`) and `dev-reload.ps1` `Remove-OldSlots` now prune both roots; only the pre-change `artifacts\dev-reload\slots` and `live-slots` are orphaned and pruning only runs inside a dev-reload run. Reword to the real residual. 2. **PR body, `8.3 short names` follow-up vs the Rule paragraph:** the Rule says a path outside the root is refused "by string comparison before any filesystem access". `Path.GetFullPath` (`:180,202`) is the first step of that comparison; whether it expands 8.3 names (and so touches the filesystem) on Windows when the path contains `~` is the point the follow-up asserts the opposite about. Unverified on Windows either way; the claim should be stated as unverified or the call replaced by a filesystem-free normaliser. 3. **PR body, "Tests (26)" and "Results" paragraphs are stale:** they list the 467a97f tests and counts and do not mention the new startup-refusal-returns-1 test, the exactly-once backslash test, the astral/lone-surrogate Printable test or the `Assembly.Location` test. The Rule paragraph says escaped characters are "control, format (bidi, zero-width) and line/paragraph-separator"; the code now also covers tag characters, other astral Cf and lone surrogates. 4. **Known limits wording:** "falls back to the host directory" for a null dependency is narrower than the code: `Load` returns null (`:335,339`), so resolution falls to the default load context (framework and host TPA, and the host base directory). Native names the resolver returns null for (`:345`) also fall to default probing. Add the framework/default-context wording, and that natives stay loaded and file-locked for the process lifetime. The "check-to-open" wording is accurate; the pdb path no longer exists. 5. **`AquariumHost.cs:28-33`:** the startup catch prints only `error.Message`; a client factory or `Start()` exception loses its type and stack, which the earlier unhandled-exception path printed. Print the type name at least (escaped), and the stack to a log if the message path stays escaped. 6. **Test cleanup (`ClientRuntimeLoaderTrustTests`, `Dispose`):** silently leaves the work directory after 5 failed attempts. Not a product risk, but a loader that never releases the file (a lock regression) passes every test on Linux and is silent on Windows; log a warning (or assert after the retry) so a persistent leak is visible. 
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
aqua merged commit 1a87c68fc7 into main 2026-11-07 11:50:05 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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/Fensalir!1
No description provided.