Repository navigation
reject broken symlinks instead of skipping them as cycles - #681
Merged
Merged
Conversation
check_symlink() treats every failure of resolve(strict=True) as a symlink cycle and skips the entry with a warning. A dangling symlink also fails strict resolution, so with allow_symlinks enabled it is silently left out of the manifest when signing, and one added to a signed model afterwards passes verification unreported, while an added regular file fails with "Extra files found". The recorded allow_symlinks policy is applied automatically when verifying, so this holds for the plain CLI verify flow too. Narrow the cycle handling to actual loops (RuntimeError on Python 3.12 and earlier, OSError with ELOOP on 3.13 and later) and raise ValueError for a symlink that cannot be resolved, restoring the fail-closed behavior that existed before check_symlink was introduced. Signed-off-by: Itecz Solution <support@itecz.au>
mihaimaruseac
approved these changes
Oct 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
check_symlink()(added in #680) treats every failure ofpath.resolve(strict=True)as a symlink cycle: it logs "Symlink cycle detected" and both serializers skip the entry. A dangling symlink fails strict resolution too, so withallow_symlinksenabled it is silently left out of the manifest when signing, and one added to a signed model afterwards passes verification unreported, while an added regular file fails with "Extra files found in model". Since #667 the verifier applies theallow_symlinkspolicy recorded in the signature, so any model signed with symlinks allowed verifies this way regardless of the caller's flag. The link can name a target that only appears after verification (under/tmp, a cache path), at which point a loader walking the verified directory reads content nobody signed. Before #680 a broken symlink failed closed with aValueErrorfromcheck_file_or_directory.Narrow the cycle handling to actual loops (
RuntimeErroron Python 3.12 and earlier,OSErrorwithELOOPon 3.13 and later) and raiseValueErrorfor a symlink that cannot be resolved, at both signing and verification. To reproduce: sign a directory withset_allow_symlinks(True), add(model / "x").symlink_to(model / "missing"), verify. Before this change verification succeeds; with it, it fails the same way an added regular file does.Checklist