Implement vendored dependency verification script - #8210
Implement vendored dependency verification script#8210Eddy Ashton (eddyashton) wants to merge 2 commits into
Conversation
| ("CLIUtils", "CLI11"): GitHubAsset("CLI11.hpp"), | ||
| ("bellard", "quickjs"): VersionedArchive( | ||
| "https://bellard.org/quickjs/quickjs-{tag}.tar.xz", | ||
| "VERSION", |
There was a problem hiding this comment.
Why does the vendored dependency script need to know about the VERSION file in quickjs?
There was a problem hiding this comment.
I'm not sure. There's discrepancies in several of our dependencies, some seem to be copy-paste (missing trailing newlines), others legitimate file moves so its easier for us to build/include (llhttp), but QuickJS is the weirdest. The GitHub mirror contains no tags or releases, and in fact the contained files differ from the commit that was claimed (in this case I think it's just a packaging script that was changed, presumably between "I look at the git commit where I created my tag" and "I've successfully run the script and produced a .zip", when this is non-automated, you sometimes fiddle with the packaging script). So we need to not just compare an artifact (rather than a git checkout), but an artifact from a basically-arbitrary URL. Exactly what's necessary/reasonable to then regain confidence that it comes from some git hash or tag, I don't know, but that's the backstory.
| environment.update({"GIT_TERMINAL_PROMPT": "0", "LC_ALL": "C"}) | ||
| try: | ||
| result = subprocess.run( | ||
| ["git", "-C", str(repository), *arguments], |
There was a problem hiding this comment.
Is it the case that shelling out to git rather than using GitPython was preferable here? We use it quite a bit for CCF version discovery etc in the LTS tests, and it's quite good I think.
| ) | ||
| headers = { | ||
| "Accept": "application/vnd.github+json", | ||
| "X-GitHub-Api-Version": "2022-11-28", |
There was a problem hiding this comment.
can we use 2026-03-10 instead?
|
I am nitpicking a little, but I do like this very much. |
There was a problem hiding this comment.
Pull request overview
Adds a Python verifier for vendored dependencies, supporting Git commits, archives, release assets, manifest mode, and filesystem comparisons.
Changes:
- Added dependency verification implementation and focused tests.
- Updated
cgmanifest.jsonwith QuickJS metadata. - Normalized endings in two vendored headers.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Summary | Review findings |
|---|---|---|
scripts/verify-vendored-dependency.py |
Verifier implementation | 8 findings: 6 critical security issues involving unsafe Git transports, mutable artifacts, symlink traversal, and HTTP redirects; 2 moderate issues involving malformed manifests and uppercase commit IDs. |
scripts/tests/verify_vendored_dependency_test.py |
Verifier tests | Nit (2 votes): Tests are not registered with automated CI or the project test runner. |
cgmanifest.json |
QuickJS release metadata | No findings. |
3rdparty/test/picobench/picobench.hpp |
File-ending normalization | No findings. |
3rdparty/internal/merklecpp/merklecpp.h |
File-ending normalization | No findings. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if __name__ == "__main__": | ||
| unittest.main() |
| try: | ||
| registrations = json.loads(path.read_text())["Registrations"] | ||
| except (OSError, KeyError, TypeError, json.JSONDecodeError) as exc: | ||
| raise VerificationError(f"Invalid manifest {path}") from exc |
| if component.repository_url.startswith("-"): | ||
| raise VerificationError("Repository URL must not begin with '-'") |
| artifact_url = override.url.format(tag=component.tag) | ||
| source = destination / "source" | ||
| source.mkdir() | ||
| extract_archive(download(artifact_url, MAX_ARTIFACT_SIZE), source) |
| if len(matches) != 1 or not isinstance( | ||
| asset_url := matches[0].get("browser_download_url"), str | ||
| ): | ||
| raise VerificationError( | ||
| f"GitHub release {component.tag} does not have one {asset_name} asset" | ||
| ) | ||
| return asset_url, download(asset_url, MAX_ARTIFACT_SIZE) |
| def files(directory: Path, *, vendored: bool = False) -> list[Path]: | ||
| if not directory.is_dir(): | ||
| raise VerificationError(f"Directory does not exist: {directory}") |
| def dependency_directory(manifest: Path, component: GitComponent) -> Path: | ||
| expected_name = DIRECTORY_ALIASES.get(component.identity, component.identity[1]) | ||
| matches = [ | ||
| path | ||
| for path in (manifest.parent / "3rdparty").glob("*/*") | ||
| if path.is_dir() and path.name == expected_name |
| if urlparse(url).scheme != "https": | ||
| raise VerificationError(f"Download URL must use HTTPS: {url}") | ||
| try: | ||
| with urlopen( | ||
| Request(url, headers=headers or {}), | ||
| timeout=NETWORK_TIMEOUT_SECONDS, | ||
| ) as response: |
| .decode() | ||
| .strip() | ||
| ) | ||
| if actual_commit != component.commit: |
This pull request introduces a new script to verify that vendored third-party dependencies match the claimed commit in their respective repositories. The implementation includes the following key changes:
New Script: Created
verify-vendored-dependency.pyto perform the verification process, which:Manifest Mode: Added a one-argument mode to check all dependencies listed in
cgmanifest.json, simplifying the command interface.Simplification: Refactored the script to reduce complexity by consolidating the handling of Git trees, archives, and raw assets into a single filesystem verifier, resulting in a 45% reduction in script size.
Testing: Introduced focused tests in
verify_vendored_dependency_test.pyto cover various scenarios, including nested upstream locations and duplicate basenames.Security Improvements: Removed unnecessary reading of
GITHUB_TOKENand path quoting, ensuring the script operates securely with public repositories.Validation has been completed successfully, with all tests passing and no correctness issues identified during independent review.