fix: make onboarding installs prefer release artifacts with source fallback#323
Conversation
Summary of ChangesHello @serrrfirat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the installation mechanism for registry-managed components by implementing a download-first strategy for pre-built artifacts, coupled with a resilient fallback to source compilation. This change aims to enhance the reliability and efficiency of the onboarding process, ensuring that installations proceed smoothly even when preferred artifact downloads encounter issues. The accompanying documentation has been updated to reflect this improved behavior. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request implements a download-first installation strategy with a source fallback for onboarding, which aims to improve user experience by preferring pre-built artifacts. However, this introduces significant security risks due to insufficient validation of extension manifest content. Specifically, unsanitized manifest names could lead to Path Traversal, and unvalidated artifact URLs enable SSRF attacks. Sensitive information from failed network requests is also exposed in user-facing warning messages. Furthermore, error handling in the fallback logic needs improvement to ensure specific error variants like AlreadyInstalled are correctly propagated, aligning with the guideline for specific error variants.
| return self.install_from_source(manifest, force).await; | ||
| } | ||
|
|
||
| match self.install_from_artifact(manifest, force).await { |
There was a problem hiding this comment.
The install_with_source_fallback method (and the install_from_artifact method it calls) uses the manifest.name field to construct filesystem paths without sanitization. An attacker who can control the manifest content (e.g., through a hijacked local registry) can use path traversal sequences like .. to write files to arbitrary locations on the system. This risk is now exposed to the onboarding process through this new fallback mechanism.
| outcome.warnings.push(format!( | ||
| "Artifact install failed ({}); installed via source fallback.", | ||
| artifact_err | ||
| )); | ||
| Ok(outcome) | ||
| } | ||
| Err(source_err) => Err(RegistryError::ManifestRead { | ||
| path: self.repo_root.join(&manifest.source.dir), | ||
| reason: format!( | ||
| "artifact install failed: {}; source fallback failed: {}", | ||
| artifact_err, source_err | ||
| ), |
There was a problem hiding this comment.
Error details from failed artifact installation attempts are directly formatted into warning messages and returned error objects, which can expose sensitive information such as full URLs or internal filesystem paths to the user. Additionally, the current implementation wraps all source fallback errors into a generic RegistryError::ManifestRead, which swallows specific error variants like AlreadyInstalled that callers might need to handle specially. It's crucial to prevent sensitive data leakage and ensure specific error types are correctly propagated. This aligns with the guideline to create specific error variants for different failure modes to provide semantically correct and clear error messages.
References
- Create specific error variants for different failure modes (e.g.,
DownloadFailedwith a URL string vs.ManifestReadwith a file path) to provide semantically correct and clear error messages.
| return self.install_from_source(manifest, force).await; | ||
| } | ||
|
|
||
| match self.install_from_artifact(manifest, force).await { |
There was a problem hiding this comment.
The install_with_source_fallback method enables a download-first path that invokes install_from_artifact. This method downloads files from URLs specified in the manifest (url and capabilities_url) without validation or allow-listing. An attacker who can provide a malicious manifest can trigger the application to make GET requests to internal services or cloud metadata endpoints (SSRF).
zmanian
left a comment
There was a problem hiding this comment.
Good PR overall. The artifact-first / source-fallback pattern is the right approach for onboarding reliability, and the security controls look solid. A few things worth addressing before merging:
Medium — install_with_source_fallback should validate the manifest upfront
The function computes source_dir from the (potentially unvalidated) manifest before calling any validating method:
let source_dir = self.repo_root.join(&manifest.source.dir); // before validation
match self.install_from_artifact(manifest, force).await { // validation happens here
...
Err(artifact_err) => {
if !source_dir.is_dir() { ... } // unvalidated path used hereThis is safe in practice because:
install_from_artifactcallsvalidate_manifest_install_inputsand would returnInvalidManifestfor a badsource.dirshould_attempt_source_fallbackblocks fallback onInvalidManifest- So the bad
source_dir.is_dir()check is never reached
But it's a subtle invariant that's easy to break in future. A validate_manifest_install_inputs(manifest)?; call at the top of install_with_source_fallback would make the safety guarantee explicit, fail fast on the no-artifact path too, and remove the dependency on inner methods to do the validation.
Low — ALLOWED_ARTIFACT_HOSTS is silently GitHub-only
const ALLOWED_ARTIFACT_HOSTS: &[&str] = &[
"github.com",
"objects.githubusercontent.com",
...
];If a future manifest points to a trusted non-GitHub host (e.g., NEAR AI's CDN), the artifact download will silently fall back to source build with a warning, rather than surfacing a clear "host not allowed" error that tells the extension author what to fix. A short comment here noting that this is GitHub-only by design and that new trusted hosts must be explicitly added would save future confusion.
Low — DownloadFailed.url field is no longer in the error display
Before this PR: "Download failed for {url}: {reason}"
After: "Artifact download failed: {reason}" — but the url: String field is still in the struct.
The omission is correct from a security standpoint (don't expose internal URLs in user-facing messages). But the struct having a stored url that doesn't appear in Display is confusing — a maintainer could reasonably think the format string is missing a field. Add a comment to the variant explaining the intentional omission:
// `url` is stored for programmatic access but omitted from the display
// message to avoid leaking internal artifact URLs to users.
#[error("Artifact download failed: {reason}")]
DownloadFailed { url: String, reason: String },Low — Test helper only covers ManifestKind::Tool
test_manifest() always sets kind: ManifestKind::Tool and source.dir: "tools-src/...". The validation for ManifestKind::Channel (which requires channels-src/ prefix) has no test coverage. Worth adding test_install_from_source_rejects_wrong_prefix_for_channel alongside the existing path-traversal test.
Minor — double validation in fallback path
When artifact install fails and source fallback runs, validate_manifest_install_inputs is called twice: once inside install_from_artifact and again inside install_from_source. Not a bug, just worth noting so no one tries to "optimize" it out later without understanding the safety dependency.
The should_attempt_source_fallback policy (block on AlreadyInstalled, ChecksumMismatch, InvalidManifest) is correct. The download_failure_reason sanitization is a good improvement. The SSRF prevention via validate_artifact_url looks complete — IP addresses blocked, scheme enforcement, allowlist checked before is_allowed_artifact_host wildcard expansion.
|
Blocking issue missed in my earlier review: SHA256 is null for all current manifests Looking at the actual manifests in the repo (e.g. "artifacts": {
"wasm32-wasip2": {
"url": "https://github.com/nearai/ironclaw/releases/latest/download/slack-wasm32-wasip2.tar.gz",
"sha256": null
}
}Every manifest currently has if let Some(expected_sha) = &artifact.sha256 {
verify_sha256(&bytes, expected_sha, url)?;
} else {
println!("WARNING: No SHA256 checksum for '{}'; download is not cryptographically verified.", ...);
}So in practice this PR, as merged, would download WASM binaries during onboarding with no integrity verification -- a warning the user is unlikely to notice. The URLs also use Why this matters for the trust model: Before this PR, onboarding built tools from source code that's in the same repo as the rest of IronClaw. After this PR, onboarding downloads opaque pre-built binaries that are unverified in the current state of the manifests. I understand the PR is necessary to support the embedded-catalog / binary-distribution case (where there's no source tree to build from). That's the right direction. But I'd suggest one of:
Option 1 is safer because it makes the invariant enforceable in code rather than relying on a process discipline. |
- Add upfront validate_manifest_install_inputs() in install_with_source_fallback so bad manifests fail fast without relying on inner methods to catch them - Document ALLOWED_ARTIFACT_HOSTS as GitHub-only by design - Document intentional url omission from DownloadFailed Display - Add channel manifest validation tests (wrong prefix rejected, correct prefix accepted) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reject artifact installs when the manifest has sha256: null instead of warning and proceeding. This prevents installing unverified pre-built binaries during onboarding. The check runs before downloading to avoid wasting bandwidth. Since InvalidManifest blocks source fallback, manifests with URLs but no checksums will hard-fail rather than silently falling back to source build — forcing the manifest to be fixed. The release CI already computes SHA256 for each bundle; the manifests just need to be populated with the actual values. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix cargo fmt on SHA256 check code - Reorder release CI: build WASM extensions before binary so manifests can be patched with computed SHA256 before build.rs embeds them - Add "Patch manifests with WASM checksums" step in build-local-artifacts that reads checksums.txt and updates registry JSON files before building - Add update-registry-checksums job that commits patched manifests back to main after release, keeping the repo in sync with released artifacts This closes the integrity gap where all manifests had sha256: null and artifact downloads were unverified. The binary now embeds correct SHA256 values and the installer hard-rejects null checksums. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…llback (nearai#323) * fix: make onboarding installs prefer release artifacts with source fallback * fix: harden extension fallback errors and surface setup warnings * fix: validate registry artifacts and harden fallback errors * fix: address review feedback on installer fallback - Add upfront validate_manifest_install_inputs() in install_with_source_fallback so bad manifests fail fast without relying on inner methods to catch them - Document ALLOWED_ARTIFACT_HOSTS as GitHub-only by design - Document intentional url omission from DownloadFailed Display - Add channel manifest validation tests (wrong prefix rejected, correct prefix accepted) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: require SHA256 checksum for artifact downloads Reject artifact installs when the manifest has sha256: null instead of warning and proceeding. This prevents installing unverified pre-built binaries during onboarding. The check runs before downloading to avoid wasting bandwidth. Since InvalidManifest blocks source fallback, manifests with URLs but no checksums will hard-fail rather than silently falling back to source build — forcing the manifest to be fixed. The release CI already computes SHA256 for each bundle; the manifests just need to be populated with the actual values. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: enforce SHA256 checksums and auto-patch manifests in CI - Fix cargo fmt on SHA256 check code - Reorder release CI: build WASM extensions before binary so manifests can be patched with computed SHA256 before build.rs embeds them - Add "Patch manifests with WASM checksums" step in build-local-artifacts that reads checksums.txt and updates registry JSON files before building - Add update-registry-checksums job that commits patched manifests back to main after release, keeping the repo in sync with released artifacts This closes the integrity gap where all manifests had sha256: null and artifact downloads were unverified. The binary now embeds correct SHA256 values and the installer hard-rejects null checksums. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Bowen Wang <bowenwang1996@users.noreply.github.com>
…llback (nearai#323) * fix: make onboarding installs prefer release artifacts with source fallback * fix: harden extension fallback errors and surface setup warnings * fix: validate registry artifacts and harden fallback errors * fix: address review feedback on installer fallback - Add upfront validate_manifest_install_inputs() in install_with_source_fallback so bad manifests fail fast without relying on inner methods to catch them - Document ALLOWED_ARTIFACT_HOSTS as GitHub-only by design - Document intentional url omission from DownloadFailed Display - Add channel manifest validation tests (wrong prefix rejected, correct prefix accepted) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: require SHA256 checksum for artifact downloads Reject artifact installs when the manifest has sha256: null instead of warning and proceeding. This prevents installing unverified pre-built binaries during onboarding. The check runs before downloading to avoid wasting bandwidth. Since InvalidManifest blocks source fallback, manifests with URLs but no checksums will hard-fail rather than silently falling back to source build — forcing the manifest to be fixed. The release CI already computes SHA256 for each bundle; the manifests just need to be populated with the actual values. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: enforce SHA256 checksums and auto-patch manifests in CI - Fix cargo fmt on SHA256 check code - Reorder release CI: build WASM extensions before binary so manifests can be patched with computed SHA256 before build.rs embeds them - Add "Patch manifests with WASM checksums" step in build-local-artifacts that reads checksums.txt and updates registry JSON files before building - Add update-registry-checksums job that commits patched manifests back to main after release, keeping the repo in sync with released artifacts This closes the integrity gap where all manifests had sha256: null and artifact downloads were unverified. The binary now embeds correct SHA256 values and the installer hard-rejects null checksums. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Bowen Wang <bowenwang1996@users.noreply.github.com>
Summary
RegistryInstaller::install_with_source_fallback()for both tools and registry channelsAlreadyInstalledbehaviorValidation
cargo test --lib -- setupcargo test --lib registry::installercargo build