Case study: storage containment

CNA snapshot 3b43bd86  ·  Development › Human Takeover  ·  source links pinned to 3b43bd86

This exercise reconstructs the real path-containment fix in CNA commit abaed98b2 and audits what the storage code does at snapshot 3b43bd86. It teaches how to review a small-looking change whose failure mode is a recursive delete outside the application's own directory. The defect is historical and fixed; no source change is needed for the exercise, and the destructive storage fixture must not be run before its target directory has been checked.

The historical commit and what followed

Commit abaed98b2a5af1dfb4d5de69b76954c4a52cf153 (2026-07-20, “fix(Task REMED-CONTENT-002): fix fs::path containment at 3 sites, sweep repo for more”) is an ancestor of snapshot 3b43bd86 and predates the v0.1.0-alpha.1 tag. It fixed three sites that joined caller- or file-supplied strings onto a base with std::filesystem::path::operator/ and no containment check:

• StorageDevice::DeleteContainer(titleName), which recursively deleted root / titleName;
• ContentReader::ReadExternalReference<T>() via ResolveRelativeAssetPath, where an absolute reference in a crafted .xnb/.cnj passed through unchanged despite the method's own documentation promising it would be rejected;
• PlaylistParser::Parse, which accepted absolute and ..-escaping .m3u/.m3u8 entries.

At the time the code lived in the pre-modularization layout (src/Microsoft/Xna/Framework/Storage/StorageDevice.cpp, include/CNA/Internal/PathContainment.hpp). The same files now live in modules/storage/src/StorageDevice.cpp and modules/core/include/CNA/Internal/PathContainment.hpp, and several later commits (all ancestors of the snapshot) changed the code this exercise reviews:

1. Turn a suspicious expression into a concrete invariant

The historical StorageDevice::DeleteContainer(titleName) called fs::remove_all(fs::path(EnsureStorageRoot()) / titleName) after checking only that titleName was not empty. In C++, joining an absolute right-hand path discards the left-hand base, and .. segments can climb above it. The invariant to state is not “paths look tidy”: the exact recursive-delete target must be a non-root descendant of the selected application storage root. At the snapshot that target is narrower still, <title>/AllPlayers or <title>/Player{N} for the calling device, so the invariant now reads “below the root, and only this device's player folder”.

Ask the questions that turn the expression into a review: who controls titleName; how the root is resolved and when it is created; whether open and delete use the same root and the same player folder; whether a path component may be a symlink, including one whose target does not exist yet; and whether the string may have been authored on another operating system. The storage internals page gives the owner and the fake-async acquisition path; this case makes the review method concrete.

Caller titleName                                        [untrusted text]
  -> StorageDevice::DeleteContainer                      [player folder of this device:
                                                           AllPlayers or Player1..Player4]
  -> ResolveStorageRoot                                  [process-static root, resolved but not created:
       Android files dir / $XDG_DATA_HOME / %LOCALAPPDATA% / HOME chain / cwd]
  -> StorageContainer::DeleteContainer(root, title, player)
  -> ResolveBelow(root, title + "/" + playerFolder)      [an empty title names the root's player folder]
       -> CNA::Internal::ResolveContainedUtf8Path
            [separators normalized; rooted and drive-letter text rejected on the string;
             lexical join; weakly_canonical check for symlinks; root itself rejected]
       -> DanglingLinkStaysBelow                           [a final link to a missing target is followed]
       escape -> System::ArgumentException
  -> not a directory? return                             [missing container or a file: nothing touched]
  -> std::filesystem::remove_all(native path, error)     [irreversible side effect]
       failure -> DirectoryNotFound / UnauthorizedAccess / IOException

Read the historical commit and the snapshot's StorageDevice.cpp and StorageContainer.cpp side by side. The commit establishes that no DeleteContainer test existed before the fix. It also records that this behaviour was CNA-introduced, because FNA's DeleteContainer throws NotImplementedException; that was commit evidence for one method, not a blanket licence to change other storage APIs. The later reference is XNA 4.0 itself: since 162e07b8a CNA deletes exactly what XNA's DeleteContainer deletes, and the source comment names the refusal of absolute and .. titles as the one deliberate difference from XNA's Path.Combine.

What differs today: the historical root came from SDL_GetPrefPath with an XDG_DATA_HOME/HOME fallback. At the snapshot the root is independent of the windowing platform. A browser build uses /cna-storage/<app>, an IndexedDB-backed IDBFS mount that the storage module's pre-run hook (WebStoragePre.js) populates before main() and persists automatically, and throws StorageDeviceNotConnectedException when that mount is unavailable (a threaded build that keeps WasmFS does not link it). Elsewhere it resolves, in order, the Android package files directory, $XDG_DATA_HOME/<app>, %LOCALAPPDATA%\<app> (read through the wide Windows environment), ~/Library/Application Support/<app> on Apple or ~/.local/share/<app> elsewhere, and finally ./<app>; <app> is game unless SetAppNameEXT was called. DeleteContainer resolves that root without creating it; only EnsureStorageRoot (behind GetStorageRootEXT, the device properties and Sharp Runtime's isolated storage) creates and latches it, after the directory exists, so a failed resolution throws and is attempted again on the next call. The delete target is a native path built through PathFromUtf8; an earlier revision handed the UTF-8 string to the narrow overload, which on a non-ASCII Windows path would silently remove nothing. The user-facing chain is documented in the Storage guide.

2. Design an oracle that cannot hide collateral damage

The historical tests did more than assert an exception. They placed a sentinel file under the storage root, called DeleteContainer with an empty name, an absolute name ("/etc"), a parent-escaping name ("../../../../../../../../tmp") and ".", and checked that the sentinel survived; the positive case checked that one named title disappeared while a sibling survived. The sentinel sits outside the authorized delete target: it would vanish if a mis-resolved call deleted the whole root. Note what it cannot see: whether a directory outside the root was touched. For the absolute and escaping cases that part of the claim rests on the thrown exception (std::invalid_argument then, System::ArgumentException at the snapshot), which is why the rejection happens before any filesystem call. The empty and "." titles are no longer refused: as in XNA they address the player folder at the root, and the snapshot's tests keep the sentinel to prove that only that folder goes.

The snapshot's StorageDeviceTests.cpp holds 27 StorageDeviceDeleteContainerTest cases and four StorageDeviceNotConnectedExceptionSerializationTest cases; six newer files beside it bring the module to 70 definitions. The containment and deletion cases this review relies on:

The generalizable technique is an outside-the-authorized-target sentinel, not a test that merely returns without throwing. For a real fix, run a new destructive test for the first time only in a disposable root, with a target whose resolved path has been inspected independently.

3. Follow the helper beyond the original subsystem

The fix introduced the shared header that is now PathContainment.hpp. In July 2026 it had two functions; at the snapshot it has a small family, with one native core that every string variant wraps:

The returned form is architectural. PlaylistParser produces song paths that MediaLibrary::songByPath_ looks up. The original commit reports that its first version returned the canonicalized absolute path and broke those lookups, so canonicalization became check-only. A later Windows run found the second half of the same contract: lexically_normal() returned backslashes on Windows while every other producer of the key used /, so playlist lookups missed there; the string variants now return PathToGenericUtf8, and MediaLibraryIndex.cpp keys its index in the same generic form. A “safer” canonical return value would violate a data-flow invariant even if containment stayed correct.

The three original sites have different authority roots, and the snapshot adds more callers. A helper call is not a substitute for deciding what the root means:

The original commit recorded two further findings as out of scope at the time: a duplicated ResolveRelativeFilePath() in the video and song readers with no containment, and eight unchecked .cnj/JSON manifest path fields in what was then ContentManager.cpp (REMED-CONTENT-007/-008). Reading the snapshot shows both now go through the helper (commit c04faaedb; the manifest fields now resolve in BuiltinContentLoaders.cpp). The commit also judged StorageContainer's joins FNA-faithful and left them; since 1af12dac3 they are contained too, documented in the source as an intentional security deviation from FNA's Path.Combine. That is the answer to “are the recorded sites still open?” for these files only; an exhaustive audit of every other path join in the tree was not made for this page.

4. State what the proof does not establish

Lexical and weak-canonical checks, with the dangling-link follow-up, reject the tested absolute, traversal and symlink cases at validation time. They do not make the subsequent path-based remove_all atomic against another process changing filesystem entries between check and use. The repository tests prove the specified rejection behaviour in their fixture, not a race-resistant sandbox under a hostile concurrent process. Nor do they cover a concurrent change of the process-static root: storageRoot_, appName_ and storageRootInitialized_ are unsynchronized statics, and SetAppNameEXT changes them.

Host coverage is its own limit. CNA's records for 72d493c7f report that the containment guard did not hold on Windows until then (a path starting with a separator is not is_absolute() there, yet operator/ treats it as rooted), and that the defect was found by running the suite on real Windows, with nine PathContainmentTest failures. Non-ASCII paths need the same kind of native evidence; tools/platform/win32_unicode_paths.ps1 exists for that host. Nothing on this page was run on Windows. Write these as limits in the review memo instead of turning “unit tests pass” into a universal security claim.

A human patch review should answer, in writing:

• Which string is untrusted, and on which OS might it have been authored?
• What is the authority root, and is it the same as the join base?
• Does the check reject every rooted spelling on this host and cross-host text, before any path is built?
• Is canonicalization used for checking only, or also for returned keys?
• Which operations are destructive, and what exactly is their target?
• Which test fixture paths will be created or deleted, and on which environment variables do they depend?
• Which other modules consume this helper, and which output keys do they store?
• Has the code changed since the history being cited?

If those answers are written before editing, the patch can be reasoned about without an AI agent or the original author.

5. Repeat the investigation manually

1. Compare the historical diff with the snapshot: git show abaed98b2 -- src/Microsoft/Xna/Framework/Storage/StorageDevice.cpp include/CNA/Internal/PathContainment.hpp, then git log --oneline abaed98b2..3b43bd86 -- modules/storage modules/core/include/CNA/Internal/PathContainment.hpp. Identify which lines moved during modularization, the SDL removal, the Windows path work and the October 2026 XNA-parity repairs.
2. Read the snapshot's ResolveStorageRoot, EnsureStorageRoot, both DeleteContainer functions, the StorageContainer constructor, ResolveBelow and DanglingLinkStaysBelow, then the helper. Draw separate arrows for the authorized root, the join base and the returned UTF-8 or native target.
3. List every call site of the helper family at the snapshot (git grep -n -E 'ResolveContained|ValidateContained|IsDisallowedAbsolutePath' 3b43bd86 -- modules) and classify each as read, write or recursive delete. Do not assume one normalization rule suits them all.
4. Read PathContainmentTests.cpp (30 PathContainmentTest cases at the snapshot, against 14 in the original commit, including the Windows drive, drive-relative, rooted-backslash and UNC spellings rejected even on POSIX) and the storage fixtures. Before running either, inspect any fixture that writes or deletes real paths.
5. Run isolated, focused filters and record passes, skips and missing hosts, for example XDG_DATA_HOME=$(mktemp -d) ./cmake-build-unit/CnaStorageTests --gtest_filter='StorageDeviceDeleteContainerTest.*:StorageResidualPathsTest.*' and ./cmake-build-unit/CnaCoreTests --gtest_filter='PathContainmentTest.*' after building those EXCLUDE_FROM_ALL targets (cmake --build cmake-build-unit --target CnaStorageTests CnaCoreTests, or the unit-core build preset). Commands checked against the CMake sources; not executed here.
6. Write a one-page review note with the invariant, call path, owner, exact delete target, other consumers, test side effects and remaining uncertainty. If you cannot explain why playlist and content behaviour stay correct after a helper change, you are not ready to merge it.

This is the pattern for taking over CNA: history explains why an invariant was added; current source proves what executes now; tests provide bounded evidence; the human maintainer supplies the cross-module judgment. The same boundary appears as a blast-radius row in Blast radius and readiness and as a test route in What to test after changing X. The investigation method itself is in How to understand code you did not write.