[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.