Skip to content

Commit

Permalink
Add suppport for checking all git sources have the required minimum spec
Browse files Browse the repository at this point in the history 
Resolves: #235
  • Loading branch information
@Jake-Shadle
Jake-Shadle committed Aug 6, 2020
1 parent 1f4856c commit 37f9e17
Show file tree
Hide file tree
Showing 4 changed files with 189 additions and 34 deletions.
44 changes: 44 additions & 0 deletions src/sources/cfg.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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>> {
Expand All @@ -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,
}
}
}
Expand Down Expand Up @@ -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,
})
}
}
Expand All @@ -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)]
Expand Down Expand Up @@ -179,5 +221,7 @@ mod test {
(OrgType::Bitbucket, "atlassian".to_owned().fake()),
]
);

assert_eq!(validated.required_git_spec.unwrap().value, GitSpec::Tag);
}
}
105 changes: 73 additions & 32 deletions src/sources/mod.rs
View file
Original file line number Diff line number Diff line change
@@ -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},
Expand Down Expand Up @@ -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();
Expand All @@ -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
Expand All @@ -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,))
Expand All @@ -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",
Expand All @@ -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,
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions tests/cfg/sources.toml
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
[sources]
unknown-registry = "allow"
unknown-git = "deny"
required-git-spec = "tag"
allow-registry = [
"https://sekretz.com/registry/index",
]
Expand Down
73 changes: 71 additions & 2 deletions tests/sources.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Expand Down Expand Up @@ -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']
Expand Down Expand Up @@ -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_eq!(severities.len(), 0);
} else {
assert!(severities.len() > 0);
assert!(severities.into_iter().all(|s| s == "error"));
}
}
}
}

0 comments on commit 37f9e17

Please sign in to comment.