Add suppport for checking all git sources have the required minimum spec

Resolves: #235

src/sources/cfg.rs

@@ -15,6 +15,41 @@ pub struct Orgs {
     bitbucket: Vec<Spanned<String>>,
 }
 
+/// The types of specifiers that can be used on git sources by cargo, in order
+/// of their specificity from least to greatest
+#[derive(Deserialize, PartialEq, Debug, PartialOrd)]
+#[serde(rename_all = "snake_case")]
+pub enum GitSpec {
+    /// Specifies the HEAD of the `master` branch, though eventually this might
+    /// change to the default branch
+    Any,
+    /// Specifies the HEAD of a particular branch
+    Branch,
+    /// Specifies the commit pointed to by a particular tag
+    Tag,
+    /// Specifies an exact commit
+    Rev,
+}
+
+use std::fmt;
+
+impl fmt::Display for GitSpec {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(match self {
+            Self::Any => "any",
+            Self::Branch => "branch",
+            Self::Tag => "tag",
+            Self::Rev => "rev",
+        })
+    }
+}
+
+impl Default for GitSpec {
+    fn default() -> Self {
+        GitSpec::Any
+    }
+}
+
 #[derive(Deserialize)]
 #[serde(rename_all = "kebab-case", deny_unknown_fields)]
 pub struct Config {
@@ -34,6 +69,10 @@ pub struct Config {
     /// The lists of source control organizations that crates can be sourced from.
     #[serde(default)]
     pub allow_org: Orgs,
+    /// The minimum specification required for git sources. Defaults to allowing
+    /// any.
+    #[serde(default)]
+    pub required_git_spec: Option<Spanned<GitSpec>>,
 }
 
 fn default_allow_registry() -> Vec<Spanned<String>> {
@@ -52,6 +91,7 @@ impl Default for Config {
             allow_registry: default_allow_registry(),
             allow_git: Vec::new(),
             allow_org: Orgs::default(),
+            required_git_spec: None,
         }
     }
 }
@@ -120,6 +160,7 @@ impl cfg::UnvalidatedConfig for Config {
             unknown_git: self.unknown_git,
             allowed_sources,
             allowed_orgs,
+            required_git_spec: self.required_git_spec,
         })
     }
 }
@@ -134,6 +175,7 @@ pub struct ValidConfig {
     pub unknown_git: LintLevel,
     pub allowed_sources: Vec<UrlSpan>,
     pub allowed_orgs: Vec<(OrgType, Spanned<String>)>,
+    pub required_git_spec: Option<Spanned<GitSpec>>,
 }
 
 #[cfg(test)]
@@ -179,5 +221,7 @@ mod test {
                 (OrgType::Bitbucket, "atlassian".to_owned().fake()),
             ]
         );
+
+        assert_eq!(validated.required_git_spec.unwrap().value, GitSpec::Tag);
     }
 }

src/sources/mod.rs

@@ -1,5 +1,5 @@
 mod cfg;
-pub use cfg::{Config, ValidConfig};
+pub use cfg::{Config, GitSpec, ValidConfig};
 
 use crate::{
     diag::{Check, Diagnostic, Label, Pack, Severity},
@@ -37,15 +37,7 @@ pub fn check(ctx: crate::CheckCtx<'_, ValidConfig>, sender: crossbeam::channel::
             url
         };
 
-        // get allowed list of sources to check
-
-        let (lint_level, type_name) = if source.is_registry() {
-            (ctx.cfg.unknown_registry, "registry")
-        } else if source.is_git() {
-            (ctx.cfg.unknown_git, "git")
-        } else {
-            continue;
-        };
+        let mut pack = Pack::with_kid(Check::Sources, krate.id.clone());
 
         let source_label = {
             let mut span = ctx.krate_spans[i].clone();
@@ -58,6 +50,53 @@ pub fn check(ctx: crate::CheckCtx<'_, ValidConfig>, sender: crossbeam::channel::
             Label::primary(ctx.spans_id, span).with_message("source")
         };
 
+        // get allowed list of sources to check
+        let (lint_level, type_name) = if source.is_registry() {
+            (ctx.cfg.unknown_registry, "registry")
+        } else if source.is_git() {
+            // Ensure the git source has at least the minimum specification
+            if let Some(cfg_min) = &ctx.cfg.required_git_spec {
+                pub use rustsec::package::source::GitReference;
+
+                let spec = source
+                    .git_reference()
+                    .map(|gr| match gr {
+                        GitReference::Branch(name) => {
+                            // TODO: Workaround logic hardcoded in the rustsec crate,
+                            // that crate can be changed to support the new v3 lock format
+                            // whenever it is stabilized https://github.com/rust-lang/cargo/pull/8522
+                            if name == "master" {
+                                GitSpec::Any
+                            } else {
+                                GitSpec::Branch
+                            }
+                        }
+                        GitReference::Tag(_) => GitSpec::Tag,
+                        GitReference::Rev(_) => GitSpec::Rev,
+                    })
+                    .unwrap_or_default();
+
+                if spec < cfg_min.value {
+                    pack.push(
+                        Diagnostic::new(Severity::Error)
+                            .with_message(format!(
+                                "'git' source is underspecified, expected '{}', found '{}'",
+                                cfg_min.value, spec,
+                            ))
+                            .with_labels(vec![
+                                source_label.clone(),
+                                Label::primary(ctx.cfg.file_id, cfg_min.span.clone())
+                                    .with_message("minimum spec configuration"),
+                            ]),
+                    );
+                }
+            }
+
+            (ctx.cfg.unknown_git, "git")
+        } else {
+            continue;
+        };
+
         // check if the source URL is list of allowed sources
         match ctx
             .cfg
@@ -70,7 +109,6 @@ pub fn check(ctx: crate::CheckCtx<'_, ValidConfig>, sender: crossbeam::channel::
                 // it's crates.io since that will be a vast majority of crates and
                 // is the default, so we might not have a real source location anyways
                 if source_url.as_str() != "https://github.com/rust-lang/crates.io-index" {
-                    let mut pack = Pack::with_kid(Check::Sources, krate.id.clone());
                     pack.push(
                         Diagnostic::new(Severity::Note)
                             .with_message(format!("'{}' source explicitly allowed", type_name,))
@@ -83,21 +121,18 @@ pub fn check(ctx: crate::CheckCtx<'_, ValidConfig>, sender: crossbeam::channel::
                                 .with_message("url allowance"),
                             ]),
                     );
-
-                    sender.send(pack).unwrap();
                 }
 
                 source_hits.as_mut_bitslice().set(ind, true)
             }
             None => {
-                // Check to see if the url is still allowed due to its org
-                if !ctx.cfg.allowed_orgs.is_empty() {
-                    if let Some((orgt, orgname)) = get_org(&source_url) {
-                        if let Some(ind) = ctx.cfg.allowed_orgs.iter().position(|(sorgt, sorgn)| {
+                let diag = match get_org(&source_url) {
+                    Some((orgt, orgname)) => {
+                        match ctx.cfg.allowed_orgs.iter().position(|(sorgt, sorgn)| {
                             orgt == *sorgt && sorgn.value.as_str() == orgname
                         }) {
-                            let mut pack = Pack::with_kid(Check::Sources, krate.id.clone());
-                            pack.push(
+                            Some(ind) => {
+                                org_hits.as_mut_bitslice().set(ind, true);
                                 Diagnostic::new(Severity::Note)
                                     .with_message(format!(
                                         "'{}' source explicitly allowed",
@@ -110,19 +145,21 @@ pub fn check(ctx: crate::CheckCtx<'_, ValidConfig>, sender: crossbeam::channel::
                                             ctx.cfg.allowed_orgs[ind].1.span.clone(),
                                         )
                                         .with_message("org allowance"),
-                                    ]),
-                            );
-
-                            sender.send(pack).unwrap();
-                            org_hits.as_mut_bitslice().set(ind, true);
-                            continue;
+                                    ])
+                            }
+                            None => Diagnostic::new(match lint_level {
+                                LintLevel::Warn => Severity::Warning,
+                                LintLevel::Deny => Severity::Error,
+                                LintLevel::Allow => Severity::Note,
+                            })
+                            .with_message(format!(
+                                "detected '{}' source not specifically allowed",
+                                type_name,
+                            ))
+                            .with_labels(vec![source_label]),
                         }
                     }
-                }
-
-                let mut pack = Pack::with_kid(Check::Sources, krate.id.clone());
-                pack.push(
-                    Diagnostic::new(match lint_level {
+                    None => Diagnostic::new(match lint_level {
                         LintLevel::Warn => Severity::Warning,
                         LintLevel::Deny => Severity::Error,
                         LintLevel::Allow => Severity::Note,
@@ -132,11 +169,15 @@ pub fn check(ctx: crate::CheckCtx<'_, ValidConfig>, sender: crossbeam::channel::
                         type_name,
                     ))
                     .with_labels(vec![source_label]),
-                );
+                };
 
-                sender.send(pack).unwrap();
+                pack.push(diag);
             }
         }
+
+        if !pack.is_empty() {
+            sender.send(pack).unwrap();
+        }
     }
 
     for src in source_hits

tests/cfg/sources.toml

@@ -1,6 +1,7 @@
 [sources]
 unknown-registry = "allow"
 unknown-git = "deny"
+required-git-spec = "tag"
 allow-registry = [
     "https://sekretz.com/registry/index",
 ]

tests/sources.rs

@@ -146,7 +146,6 @@ fn allows_github_org() {
 
 #[test]
 fn allows_gitlab_org() {
-    // We shouldn't have any errors for the embark urls now
     let cfg = "unknown-git = 'deny'
     [allow-org]
     gitlab = ['amethyst-engine']
@@ -193,7 +192,6 @@ fn allows_gitlab_org() {
 
 #[test]
 fn allows_bitbucket_org() {
-    // We shouldn't have any errors for the embark urls now
     let cfg = "unknown-git = 'deny'
     [allow-org]
     bitbucket = ['marshallpierce']
@@ -237,3 +235,74 @@ fn allows_bitbucket_org() {
         }
     }
 }
+
+#[test]
+fn validates_git_source_specs() {
+    use sources::GitSpec;
+
+    assert!(GitSpec::Rev > GitSpec::Tag);
+    assert!(GitSpec::Tag > GitSpec::Branch);
+    assert!(GitSpec::Branch > GitSpec::Any);
+
+    let levels = [
+        (GitSpec::Rev, "https://gitlab.com/amethyst-engine/amethyst"),
+        (GitSpec::Tag, "https://github.com/EmbarkStudios/spdx"),
+        (GitSpec::Branch, "https://github.com/EmbarkStudios/krates"),
+        (
+            GitSpec::Any,
+            "https://bitbucket.org/marshallpierce/line-wrap-rs",
+        ),
+    ];
+
+    for (i, (spec, _url)) in levels.iter().enumerate() {
+        let cfg = format!(
+            "unknown-git = 'allow'
+        required-git-spec = '{}'",
+            spec
+        );
+
+        let krates = utils::get_test_data_krates("sources").unwrap();
+        let mut diags = utils::gather_diagnostics::<sources::Config, _, _>(
+            krates,
+            "validates_git_source_specs",
+            Some(&cfg),
+            None,
+            |ctx, tx| {
+                sources::check(ctx, tx);
+            },
+        )
+        .unwrap();
+
+        diags.retain(|d| {
+            d.pointer("/fields/message")
+                .unwrap()
+                .as_str()
+                .unwrap()
+                .starts_with("'git' source is underspecified, expected")
+        });
+
+        for (j, (_, url)) in levels.iter().enumerate() {
+            let severities: Vec<_> = diags
+                .iter()
+                .filter_map(|d| {
+                    d.pointer("/fields/labels/0/span")
+                        .and_then(|u| u.as_str())
+                        .and_then(|u| {
+                            if u.contains(url) {
+                                d.pointer("/fields/severity").and_then(|s| s.as_str())
+                            } else {
+                                None
+                            }
+                        })
+                })
+                .collect();
+
+            if j <= i {
+                assert!(severities.is_empty());
+            } else {
+                assert!(!severities.is_empty());
+                assert!(severities.into_iter().all(|s| s == "error"));
+            }
+        }
+    }
+}