[Akshay Sonawane (apsonawane)]

This pull request introduces comprehensive path confinement and symlink validation for all operations involving shared_assets in portable model packages. The main goal is to ensure that all asset paths remain strictly within the package root, and that no symlinks are used to escape or replace package directories or assets, preventing potential security or integrity issues. Additionally, new tests have been added to verify these constraints.

Path confinement and symlink validation:

• Introduced ValidatePortablePath, a utility that checks that a path is strictly under the package root, does not traverse symlinks, and (optionally) exists, using both lexical and canonical path normalization. This is now called before any operation that reads, writes, or removes shared assets in portable packages. [1] [2] [3]
• Updated all relevant code paths (ModelPackage_RemoveSharedAsset, CommitSharedAssetsCopyIn, CommitToDestRoot, ModelPackage_Prune, and LoadSharedAssets) to invoke ValidatePortablePath before acting on shared asset directories or files, both for existence and non-existence checks as appropriate. [1] [2] [3] [4] [5] [6] [7] [8] [9]

Symlink and directory validation:

• Added explicit checks to reject symlinks as shared asset directories or sources in ComputeDirectoryAssetUri and CopyTreeNoFollow, returning schema or IO errors as appropriate. [1] [2]

Testing improvements:

• Added new tests to verify that opening a package, mutating shared assets, or committing to a destination package root all properly reject cases where symlinks are used to escape or replace shared asset directories. These ensure the new path and symlink validation logic is enforced. [1] [2]

Internal refactoring:

• Introduced the IsAncestorOrEqual helper for robust path ancestry checks and added proper namespace aliasing for std::filesystem. [1] [2]

These changes collectively harden the package system against path traversal and symlink attacks, ensuring portable packages are reliably confined to their intended directory structure.

[Copilot]

🟡 Changes recommended

Critical symlink-validation bypasses still allow linked assets to be accepted, hashed, or copied, while path-confinement checks incorrectly reject valid destination roots.

4 open issues

• Reject symlinked source roots with trailing separators or dot components · New
• Initialize package layout before loading shared assets · New
• Check original manifest override paths for symlinks · New
• Allow valid destination roots in path confinement checks · New

What changed in this PR

This PR hardens portable model packages by adding path-confinement and symlink checks around shared-asset operations.

Changes:

• Introduces a shared validator for lexical containment, symlink detection, and canonical containment.
• Applies validation to asset discovery, copying, removal, and pruning, with additional source-directory checks.
• Adds regression tests for linked asset paths and directories replaced after package creation.

File	Description
model_package/​tests/​test_commit.cc	Adds symlink-rejection tests for opening, mutation, and destination commits.
model_package/​src/​model_package_impl.h	Declares the portable-path validator.
model_package/​src/​model_package_impl.cc	Implements path containment and symlink validation.
model_package/​src/​manifest_parser.cc	Adds validation during shared-asset discovery and resolution.
model_package/​src/​commit_prune.cc	Adds checks around asset copying, staging, and pruning.
model_package/​src/​authoring.cc	Validates portable asset paths before removal.
model_package/​src/​asset_hasher.cc	Adds rejection of symlinked source directories before hashing.

_{Review effort: Balanced}

[Tianlei Wu (tianleiwu)]

Reviewed head fb2644bb2e. I built model_package standalone and ran the tests on Linux (ctest 4/4, test_commit 24/24 pass) plus a few extra probes through the public C API.

The four earlier automated threads (made against 3647791fee) look addressed on this head: PopulatePackageMetadata now runs before LoadSharedAssets, link/ and link/. source spellings are normalized and tested, override paths are validated before ResolvePath, and saved/ / . destination roots are covered by a test.

No blocking issues. Inline comments cover the remaining points:

• Windows junctions are not treated as links.
• The source-root symlink rejection also applies outside portable packages and is a public behavior change.
• RemoveSharedAsset mutates state before validating.
• One stale tmp symlink makes Prune abort.

Nits:

• The if (pkg->layout == "portable") { if (auto* s = ValidatePortablePath(...)) return s; } pattern repeats ~9 times; a small helper would cut the noise.
• CommitToDestRoot validates the destination shared_assets unconditionally while the other call sites are portable-gated; a short comment on why would help.
• The validation is check-then-use (TOCTOU). The staged re-hash covers the copy path, but not remove_all. A note in the ValidatePortablePath header doc would set expectations.

[Ti-Tai Wang (titaiwangms)]

Reviewed commit 5df3373. Requesting changes for four Major issues detailed inline.

ID	Finding	Raised by	Disposition
F1	Lexical normalization can select the wrong source directory	Critical and Code reviewers	Confirmed
F2	Failed rollback can lose existing asset records	Critical and Code reviewers	Confirmed
F3	Windows junction test calls unexported DLL helpers	Code and Integration reviewers	Confirmed
F4	New prune behavior contradicts an existing test expectation	Code reviewer	Confirmed
F5	Mandatory shared-asset checks override documented read-only options	Deep reviewer, with related Critical/Readability concerns	Confirmed documentation mismatch; Minor

Additional Minor feedback:

• Update the public option documentation and README to explain that portable shared-asset loading now enforces confinement/link rejection even when the advertised read-only allow_external_paths/follow_symlinks options would otherwise permit resolution. Deep review rated this Major; I treat it as Minor documentation/compatibility clarification because strict portable confinement is explicit PR intent. This is the only downgraded Major finding.
• Consolidate the differently implemented ancestry helpers in model_package_impl.cc:37 and commit_prune.cc:572 so confinement checks do not drift apart.

Nonblocking open questions: Deep review requested confirmation of Windows case handling, MinGW support for the MSVC-specific junction enum, and the intended installed-layout link protections. Readability review asked whether additional Windows reparse-point types belong in the link policy.

Validation and residual risk: all five review roles completed and the findings were checked against the pinned diff and source. No local builds or tests were run. Windows runtime behavior and race-free filesystem mutation were not verified; the check-then-use guards do not establish race-free mutation guarantees.

[Ti-Tai Wang (titaiwangms)]

Re-reviewed the revision delta from 5df3373 to 83355ea.

The previous four findings are addressed at the source level: source normalization preserves interior components, shared-asset refresh now builds temporary state before swapping, the Windows junction test uses public APIs, and the prune test expectation matches the new link-preservation policy. The documentation and duplicated ancestry-helper feedback are also addressed.

Requesting changes for one new Major build blocker: the two added regression-test functions are defined inside another function. There is also a Minor coverage issue: the unreadable-directory test skips because its target directory is never created. Details are inline.

Validation: static review of the revision delta and relevant controlling code only; no builds or tests were executed. The earlier fixes therefore have not been runtime-validated, and Windows behavior and filesystem replacement races remain outside this pass.

[Ti-Tai Wang (titaiwangms)]

Re-reviewed the revision delta from 83355ea to edfd586.

The previous nested-function compilation issue is fixed: both regression tests are now at namespace scope. The unreadable-directory test now creates its target directory and checks whether directory iteration is actually denied before deciding to skip, addressing the previous coverage issue.

One Major issue remains: the now-effective test exposes an existing ignored directory-open error in ModelPackage_Prune, so the new assertion fails for ordinary nonprivileged POSIX users. Please fix that error handling as detailed inline. The underlying runtime defect predates this revision; the newly exposed regression is the test failure.

Validation: static review of the revision delta, test setup, and controlling prune code. No local builds or tests were run. Windows runtime behavior and filesystem replacement races were not checked in this pass.

[Ti-Tai Wang (titaiwangms)]

Re-reviewed commit ef3c545 against the previously reviewed revision. The remaining directory-iterator construction error is now checked and returned as MODEL_PACKAGE_ERR_IO, matching the unreadable-directory regression test. All previously reported blocking findings are addressed at the source level, and no new actionable findings were identified in this revision delta.

Validation: static review of the revision delta, controlling prune logic, and relevant regression tests. No local builds or tests were run. Windows runtime behavior and race-free filesystem mutation were not verified.