Skip to content

reject symlinks to directories during serialization - #673

Merged
mihaimaruseac merged 1 commit into
sigstore:mainfrom
Support-itecz:symlink-dir-reject
Oct 2, 2026
Merged

mihaimaruseac merged 1 commit into
sigstore:mainfrom
Support-itecz:symlink-dir-reject

Conversation

@Support-itecz

Copy link
Copy Markdown
Contributor

Summary

file.Serializer.serialize and file_shard.Serializer.serialize walk the model with glob("**/*"), which never descends into a symlink that points to a directory. With allow_symlinks enabled the link passes check_file_or_directory, is not a file, and is skipped, so every file beneath it is left out of the manifest with no error. A model signed with a symlinked weights directory carries no digests for anything under that link, and changing or adding files there afterwards still verifies clean. Related to the symlink discussion in #666, though this gap is not in that list.

Raise on a symlink to a directory in both serializers, unless it is covered by the ignore paths, and keep accepting a model root that is itself a symlink since the walk does follow that one. The check sits in the walk loop, next to the place that fails to descend, so both signing and verification refuse the model instead of silently producing or accepting a partial manifest. Regression tests cover the rejection, the ignored-link case and the symlinked root for both serializers.

To reproduce before the fix: put weights -> /some/dir (containing a file) inside a model directory and hash it with set_allow_symlinks(True). The manifest lists only the files outside the link, and rewriting or adding files under /some/dir leaves the manifest unchanged.

Checklist
  • All commits are signed-off, using DCO
  • All new code has docstrings and type annotations
  • All new code is covered by tests. Aim for at least 90% coverage. CI is configured to highlight lines not covered by tests.
  • Public facing changes are paired with documentation changes
  • Release note has been added to CHANGELOG.md if needed

file.Serializer.serialize and file_shard.Serializer.serialize walk the
model with glob("**/*"), which never descends into a symlink that points
to a directory. With allow_symlinks enabled the link passes
check_file_or_directory, is not a file, and is skipped, so every file
beneath it is left out of the manifest with no error. A model signed with
a symlinked weights directory carries no digests for anything under that
link, and changing or adding files there afterwards still verifies clean.

Raise on a symlink to a directory in both serializers, unless it is
covered by the ignore paths, and keep accepting a model root that is
itself a symlink since the walk does follow that one. The check sits in
the walk loop, next to the place that fails to descend, so both signing
and verification refuse the model instead of silently producing or
accepting a partial manifest.

Signed-off-by: Itecz Solution <support@itecz.au>
@Support-itecz
Support-itecz requested review from a team as code owners September 30, 2026 16:00

@mihaimaruseac mihaimaruseac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

This makes sense and probably we would reach a state where symlinks will always be an error.

@mihaimaruseac
mihaimaruseac merged commit c2b9470 into sigstore:main Oct 2, 2026
51 of 52 checks passed
@Support-itecz

Copy link
Copy Markdown
Contributor Author

Thanks for the review and the merge on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants