Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement lockfiles checksums verification #937

Merged
merged 4 commits into from
Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 106 additions & 1 deletion scarb/src/core/resolver.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
use std::collections::HashMap;

use anyhow::{bail, Result};
use indoc::formatdoc;
use itertools::Itertools;
use petgraph::graphmap::DiGraphMap;
use petgraph::visit::{Dfs, EdgeFiltered, Walker};
use smallvec::SmallVec;
use std::collections::HashMap;

use crate::core::lockfile::Lockfile;
use crate::core::{PackageId, Summary, TargetKind};

/// Represents a fully-resolved package dependency graph.
Expand Down Expand Up @@ -96,3 +100,104 @@ impl From<TargetKind> for DependencyEdge {
Self(vec![target_kind].into())
}
}

/// Lockfiles handling.
impl Resolve {
/// Check that the newly generated resolve is compliant with the previous one generated
/// from a lock file.
///
/// Given an existing lock file, it should be forbidden to ever have a checksums which
/// *differ*. If the same package ids' summaries have differing checksums, then something
/// has gone wrong such as:
///
/// * something got seriously corrupted,
/// * a "mirror" is not actually a mirror as some changes were made,
/// * a replacement source was not actually a replacement, some changes were made.
///
/// In all of these cases, we want to report an error to indicate that something is awry.
/// Normal execution (esp. just using the default registry) should never run into this.
pub fn check_checksums(&self, lockfile: &Lockfile) -> Result<()> {
for package_lock in &lockfile.packages {
let (locked, source_id) = match (package_lock.checksum.as_ref(), package_lock.source) {
(None, None) => continue,
(Some(_), None) => {
unreachable!(
"Package lock entry `{n} v{v}` has `checksum` but no `source` field.",
n = package_lock.name,
v = package_lock.version
);
}
(locked, Some(source_id)) => (locked, source_id),
};

let id = PackageId::new(
package_lock.name.clone(),
package_lock.version.clone(),
source_id,
);

let Some(actual) = self.summaries.get(&id).map(|s| s.checksum.as_ref()) else {
continue;
};

match (actual, locked) {
// If the checksums are the same, or both are not present, then we are good.
(Some(actual), Some(locked)) if actual == locked => {}
(None, None) => {}

// If the locked checksum was not calculated, and the current checksum is `Some`,
// it may indicate that a source was erroneously replaced or was replaced with
// something that desires stronger checksum guarantees than can be afforded
// elsewhere.
(Some(_), None) => {
bail!(formatdoc! {"
checksum for `{id}` was not previously calculated, but now it could be

this could be indicative of a few possible situations:

* the source `{source_id}` did not previously support checksums, \
but was replaced with one that does
* newer Scarb implementations know how to checksum this source, \
but this older implementation does not
* the lock file is corrupt
"});
}

// If our checksum has not been calculated, then it could mean that future Scarb
// figured out how to do it or the source has been shadowed by with
// a different one thanks to some unknown future logic.
(None, Some(_)) => {
bail!(formatdoc! {"
checksum for `{id}` could not be calculated, but a checksum is listed in \
the existing lock file

this could be indicative of a few possible situations:

* the source `{source_id}` supports checksums, \
but was replaced with one that does not
* the lock file is corrupt

unable to verify that `{id}` is the same as when the lockfile was generated
"});
}

// Both checksums are known, but they differ.
(Some(_), Some(_)) => {
bail!(formatdoc! {"
checksum for `{id}` changed between lock files

this could be indicative of a few possible errors:

* the lock file is corrupt
* a replacement source in use (e.g. a mirror) returned a different \
checksum
* the source itself may be corrupt in one way or another

unable to verify that `{id}` is the same as when the lockfile was generated
"});
}
}
}
Ok(())
}
}
4 changes: 3 additions & 1 deletion scarb/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,9 @@ pub async fn resolve(
"});
}

Ok(Resolve { graph, summaries })
let resolve = Resolve { graph, summaries };
resolve.check_checksums(&lockfile)?;
Ok(resolve)
}

fn rewrite_locked_dependency(
Expand Down
182 changes: 182 additions & 0 deletions scarb/tests/lockfile_checksums.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
use assert_fs::prelude::*;
use assert_fs::TempDir;
use indoc::indoc;
use toml_edit::value;

use scarb_test_support::command::Scarb;
use scarb_test_support::fsx::ChildPathEx;
use scarb_test_support::gitx;
use scarb_test_support::project_builder::{Dep, DepBuilder, ProjectBuilder};
use scarb_test_support::registry::local::LocalRegistry;

#[test]
fn checksum_changed_upstream() {
let mut registry = LocalRegistry::create();
registry.publish(|t| {
ProjectBuilder::start()
.name("bar")
.version("1.0.0")
.lib_cairo(r#"fn f() -> felt252 { 0 }"#)
.build(t);
});

let t = TempDir::new().unwrap();
ProjectBuilder::start()
.name("foo")
.version("0.1.0")
.dep("bar", Dep.version("1.0.0").registry(&registry))
.lib_cairo(r#"fn f() -> felt252 { bar::f() }"#)
.build(&t);

Scarb::quick_snapbox()
.arg("fetch")
.current_dir(&t)
.assert()
.success();

let expected_lockfile = t.child("Scarb.lock").read_to_string();

// Now, let's redeploy the same package, but with a different source → different checksum.
registry.publish(|t| {
ProjectBuilder::start()
.name("bar")
.version("1.0.0")
.lib_cairo(r#"fn f() -> felt252 { 1234 }"#)
.build(t);
});

Scarb::quick_snapbox()
.arg("fetch")
.current_dir(&t)
.assert()
.failure()
.stdout_matches(indoc! {r#"
error: checksum for `bar v1.0.0 (registry+file://[..])` changed between lock files

this could be indicative of a few possible errors:

* the lock file is corrupt
* a replacement source in use (e.g. a mirror) returned a different checksum
* the source itself may be corrupt in one way or another

unable to verify that `bar v1.0.0 (registry+file://[..])` is the same as when the lockfile was generated
"#});

// Let's verify that the lockfile was not modified.
let actual_lockfile = t.child("Scarb.lock").read_to_string();
assert_eq!(expected_lockfile, actual_lockfile);
}

#[test]
fn checksum_locked_for_unexpected_source() {
let bar = gitx::new("bar", |t| {
ProjectBuilder::start()
.name("bar")
.version("1.0.0")
.build(&t);
});

let t = TempDir::new().unwrap();
ProjectBuilder::start()
.name("foo")
.version("0.1.0")
.dep("bar", &bar)
.build(&t);

// Let's generate a valid Scarb.lock and then corrupt it, by adding a `checksum` where it is
// not supposed to be. Git dependencies are not expected to ever have checksums.
Scarb::quick_snapbox()
.arg("fetch")
.current_dir(&t)
.assert()
.success();

let mut lockfile = t.child("Scarb.lock").assert_is_toml_document();
lockfile
.get_mut("package")
.unwrap()
.as_array_of_tables_mut()
.unwrap()
.iter_mut()
.find(|t| t["name"].as_str().unwrap() == "bar")
.unwrap()
.insert(
"checksum",
value("sha256:b62fc4b9bfbd9310a47d2e595d2c8f468354266be0827aeea9b465d9984908de"),
);
t.child("Scarb.lock")
.write_str(&lockfile.to_string())
.unwrap();

Scarb::quick_snapbox()
.arg("fetch")
.current_dir(&t)
.assert()
.failure()
.stdout_matches(indoc! {r#"
[..] Updating git repository [..]
error: checksum for `bar v1.0.0 ([..])` could not be calculated, but a checksum is listed in the existing lock file

this could be indicative of a few possible situations:

* the source `[..]` supports checksums, but was replaced with one that does not
* the lock file is corrupt

unable to verify that `bar v1.0.0 ([..])` is the same as when the lockfile was generated
"#});
}

#[test]
fn unlisted_checksum_for_source_supporting_it() {
let mut registry = LocalRegistry::create();
registry.publish(|t| {
ProjectBuilder::start()
.name("bar")
.version("1.0.0")
.build(t);
});

let t = TempDir::new().unwrap();
ProjectBuilder::start()
.name("foo")
.version("0.1.0")
.dep("bar", Dep.version("1.0.0").registry(&registry))
.build(&t);

// Let's generate a valid Scarb.lock and then corrupt it, by removing `checksum` fields
// everywhere applicable.
Scarb::quick_snapbox()
.arg("fetch")
.current_dir(&t)
.assert()
.success();

let mut lockfile = t.child("Scarb.lock").assert_is_toml_document();
lockfile
.get_mut("package")
.unwrap()
.as_array_of_tables_mut()
.unwrap()
.iter_mut()
.for_each(|t| {
t.remove("checksum");
});
t.child("Scarb.lock")
.write_str(&lockfile.to_string())
.unwrap();

Scarb::quick_snapbox()
.arg("fetch")
.current_dir(&t)
.assert()
.failure()
.stdout_matches(indoc! {r#"
error: checksum for `bar v1.0.0 ([..])` was not previously calculated, but now it could be

this could be indicative of a few possible situations:

* the source `[..]` did not previously support checksums, but was replaced with one that does
* newer Scarb implementations know how to checksum this source, but this older implementation does not
* the lock file is corrupt
"#});
}
5 changes: 5 additions & 0 deletions utils/scarb-test-support/src/fsx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ pub trait ChildPathEx {
fn files(&self) -> Vec<String>;
fn tree(&self) -> String;
fn assert_is_json<T: DeserializeOwned>(&self) -> T;
fn assert_is_toml_document(&self) -> toml_edit::Document;
}

impl ChildPathEx for ChildPath {
Expand Down Expand Up @@ -104,6 +105,10 @@ impl ChildPathEx for ChildPath {
let reader = BufReader::new(file);
serde_json::from_reader(reader).unwrap()
}

fn assert_is_toml_document(&self) -> toml_edit::Document {
self.read_to_string().parse().unwrap()
}
}

/// Convert all UNIX-style paths in a string to platform native.
Expand Down