From 84e3ba1482af48e8edd76178fbe976a6594e42e9 Mon Sep 17 00:00:00 2001 From: cloudhead Date: Wed, 31 Jul 2024 12:55:53 +0200 Subject: [PATCH] Validate project names properly in all places We were only validating names when passed as a flag to `rad init --name`. --- radicle-cli/src/commands/init.rs | 21 ++---- radicle-node/src/test/environment.rs | 2 +- radicle-node/src/test/peer.rs | 2 +- radicle-node/src/tests.rs | 2 +- radicle-tools/src/rad-init.rs | 2 +- radicle/src/identity/doc.rs | 4 +- radicle/src/identity/project.rs | 102 +++++++++++++++++++++++---- radicle/src/rad.rs | 10 +-- radicle/src/test/arbitrary.rs | 4 +- radicle/src/test/fixtures.rs | 4 +- 10 files changed, 111 insertions(+), 42 deletions(-) diff --git a/radicle-cli/src/commands/init.rs b/radicle-cli/src/commands/init.rs index e68f6d03..59113c19 100644 --- a/radicle-cli/src/commands/init.rs +++ b/radicle-cli/src/commands/init.rs @@ -13,6 +13,7 @@ use serde_json as json; use radicle::crypto::{ssh, Verified}; use radicle::explorer::ExplorerUrl; use radicle::git::RefString; +use radicle::identity::project::ProjectName; use radicle::identity::{RepoId, Visibility}; use radicle::node::events::UploadPack; use radicle::node::policy::Scope; @@ -57,7 +58,7 @@ Options #[derive(Default)] pub struct Options { pub path: Option, - pub name: Option, + pub name: Option, pub description: Option, pub branch: Option, pub interactive: Interactive, @@ -94,19 +95,8 @@ impl Args for Options { Long("name") if name.is_none() => { let value = parser.value()?; let value = term::args::string(&value); - let allowed = ['-', '_', '.']; + let value = ProjectName::try_from(value)?; - // Nb. We avoid characters that need to be quoted by shells, such as `$`, - // `!` etc., since repository names are used for naming folders during clone. - if !value - .chars() - .all(|c| c.is_alphanumeric() || allowed.contains(&c)) - { - anyhow::bail!( - "invalid repository name specified with `--name`, \ - only alphanumeric characters, '-', '_' and '.' are allowed" - ); - } name = Some(value); } Long("description") if description.is_none() => { @@ -243,7 +233,7 @@ pub fn init( term::format::dim(path.display()) )); - let name = match options.name { + let name: ProjectName = match options.name { Some(name) => name, None => { let default = path.file_name().map(|f| f.to_string_lossy().to_string()); @@ -252,6 +242,7 @@ pub fn init( default, Some("The name of your repository, eg. 'acme'"), )? + .try_into()? } }; let description = match options.description { @@ -287,7 +278,7 @@ pub fn init( match radicle::rad::init( &repo, - &name, + name, &description, branch.clone(), visibility, diff --git a/radicle-node/src/test/environment.rs b/radicle-node/src/test/environment.rs index aa16d2cd..fd3ab43f 100644 --- a/radicle-node/src/test/environment.rs +++ b/radicle-node/src/test/environment.rs @@ -548,7 +548,7 @@ impl + Signer + Clone> Node { let branch = refname!("master"); let id = rad::init( repo, - name, + name.try_into().unwrap(), description, branch.clone(), Visibility::default(), diff --git a/radicle-node/src/test/peer.rs b/radicle-node/src/test/peer.rs index a3ff1d79..88f372b2 100644 --- a/radicle-node/src/test/peer.rs +++ b/radicle-node/src/test/peer.rs @@ -132,7 +132,7 @@ impl Peer { let (repo, _) = fixtures::repository(self.tempdir.path().join(name)); let (rid, _, _) = rad::init( &repo, - name, + name.try_into().unwrap(), description, radicle::git::refname!("master"), Visibility::default(), diff --git a/radicle-node/src/tests.rs b/radicle-node/src/tests.rs index 6acaee4c..da243612 100644 --- a/radicle-node/src/tests.rs +++ b/radicle-node/src/tests.rs @@ -1648,7 +1648,7 @@ fn test_init_and_seed() { // Alice creates a new project. let (proj_id, _, _) = rad::init( &repo, - "alice", + "alice".try_into().unwrap(), "alice's repo", git::refname!("master"), Visibility::default(), diff --git a/radicle-tools/src/rad-init.rs b/radicle-tools/src/rad-init.rs index b1ab5410..3a846e55 100644 --- a/radicle-tools/src/rad-init.rs +++ b/radicle-tools/src/rad-init.rs @@ -10,7 +10,7 @@ fn main() -> anyhow::Result<()> { let signer = profile.signer()?; let (id, _, _) = radicle::rad::init( &repo, - &name, + name.try_into()?, "", git::refname!("master"), Visibility::default(), diff --git a/radicle/src/identity/doc.rs b/radicle/src/identity/doc.rs index e1c1670e..e0b2e854 100644 --- a/radicle/src/identity/doc.rs +++ b/radicle/src/identity/doc.rs @@ -462,7 +462,7 @@ mod test { let (repo, _) = fixtures::repository(tempdir.path().join("working")); let (id, _, _) = rad::init( &repo, - "heartwood", + "heartwood".try_into().unwrap(), "Radicle Heartwood Protocol & Stack", git::refname!("master"), Visibility::default(), @@ -509,7 +509,7 @@ mod test { let delegate = MockSigner::from_seed([0xff; 32]); let (rid, doc, _) = rad::init( &working, - "heartwood", + "heartwood".try_into().unwrap(), "Radicle Heartwood Protocol & Stack", git::refname!("master"), Visibility::default(), diff --git a/radicle/src/identity/project.rs b/radicle/src/identity/project.rs index 78ef7a60..97f1b4f1 100644 --- a/radicle/src/identity/project.rs +++ b/radicle/src/identity/project.rs @@ -1,4 +1,4 @@ -use std::fmt; +use std::{fmt, str::FromStr}; use serde::{ de::{self, MapAccess, Visitor}, @@ -24,12 +24,72 @@ pub enum ProjectError { DefaultBranch(&'static str), } +/// A valid project name. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(try_from = "String", into = "String")] +pub struct ProjectName(String); + +impl From for String { + fn from(value: ProjectName) -> Self { + value.0 + } +} + +impl ProjectName { + /// List of allowed special characters. + pub const ALLOWED_CHARS: &'static [char] = &['-', '_', '.']; + + /// Return a string reference to the name. + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl TryFrom<&str> for ProjectName { + type Error = ProjectError; + + fn try_from(s: &str) -> Result { + ProjectName::from_str(s) + } +} + +impl TryFrom for ProjectName { + type Error = ProjectError; + + fn try_from(s: String) -> Result { + if s.is_empty() { + return Err(ProjectError::Name("name cannot be empty")); + } else if s.len() > doc::MAX_STRING_LENGTH { + return Err(ProjectError::Name("name cannot exceed 255 bytes")); + } + // Nb. We avoid characters that need to be quoted by shells, such as `$`, + // `!` etc., since repository names are used for naming folders during clone. + if !s + .chars() + .all(|c| c.is_alphanumeric() || Self::ALLOWED_CHARS.contains(&c)) + { + return Err(ProjectError::Name( + "invalid repository name, only alphanumeric characters, '-', '_' and '.' are allowed", + )); + } + Ok(Self(s)) + } +} + +impl FromStr for ProjectName { + type Err = ProjectError; + + fn from_str(s: &str) -> Result { + Self::try_from(s.to_owned()) + } +} + /// A "project" payload in an identity document. #[derive(Debug, Clone, PartialEq, Eq, Serialize)] #[serde(rename_all = "camelCase")] pub struct Project { /// Project name. - name: String, + name: ProjectName, /// Project description. description: String, /// Project default branch. @@ -48,8 +108,7 @@ impl<'de> Deserialize<'de> for Project { Description, DefaultBranch, /// A catch-all variant to allow for unknown fields - #[allow(dead_code)] - Unknown(String), + Unknown(#[allow(dead_code)] String), } struct ProjectVisitor; @@ -123,18 +182,12 @@ impl Project { /// * `description`'s length must not exceed 255. /// * `default_branch`'s length must not be empty and must not exceed 255. pub fn new( - name: String, + name: ProjectName, description: String, default_branch: BranchName, ) -> Result> { let mut errs = Vec::new(); - if name.is_empty() { - errs.push(ProjectError::Name("name cannot be empty")); - } else if name.len() > doc::MAX_STRING_LENGTH { - errs.push(ProjectError::Name("name cannot exceed 255 bytes")); - } - if description.len() > doc::MAX_STRING_LENGTH { errs.push(ProjectError::Description( "description cannot exceed 255 bytes", @@ -171,7 +224,7 @@ impl Project { /// original validation rules (see [`Project::new`]). pub fn update( self, - name: impl Into>, + name: impl Into>, description: impl Into>, default_branch: impl Into>, ) -> Result> { @@ -183,7 +236,7 @@ impl Project { #[inline] pub fn name(&self) -> &str { - &self.name + self.name.as_str() } #[inline] @@ -205,3 +258,26 @@ impl From for Payload { Self::from(value) } } + +#[cfg(test)] +mod test { + use super::*; + use crate::assert_matches; + + #[test] + fn test_project_name() { + assert_matches!(serde_json::from_str::("\"\""), Err(_)); + assert_matches!( + serde_json::from_str::("\"invalid name\""), + Err(_) + ); + assert_matches!( + serde_json::from_str::("\"invalid%name\""), + Err(_) + ); + assert_eq!( + serde_json::from_str::("\"valid-name\"").unwrap(), + ProjectName("valid-name".to_owned()) + ); + } +} diff --git a/radicle/src/rad.rs b/radicle/src/rad.rs index d27e29d8..90731906 100644 --- a/radicle/src/rad.rs +++ b/radicle/src/rad.rs @@ -11,7 +11,7 @@ use crate::crypto::{Signer, Verified}; use crate::git; use crate::identity::doc; use crate::identity::doc::{DocError, RepoId, Visibility}; -use crate::identity::project::Project; +use crate::identity::project::{Project, ProjectName}; use crate::storage::git::transport; use crate::storage::git::Repository; use crate::storage::refs::SignedRefs; @@ -46,7 +46,7 @@ pub enum InitError { /// Initialize a new radicle project from a git repository. pub fn init( repo: &git2::Repository, - name: &str, + name: ProjectName, description: &str, default_branch: BranchName, visibility: Visibility, @@ -416,7 +416,7 @@ mod tests { let (repo, _) = fixtures::repository(tempdir.path().join("working")); let (proj, _, refs) = init( &repo, - "acme", + "acme".try_into().unwrap(), "Acme's repo", git::refname!("master"), Visibility::default(), @@ -471,7 +471,7 @@ mod tests { let (original, _) = fixtures::repository(tempdir.path().join("original")); let (id, _, alice_refs) = init( &original, - "acme", + "acme".try_into().unwrap(), "Acme's repo", git::refname!("master"), Visibility::default(), @@ -507,7 +507,7 @@ mod tests { let (original, _) = fixtures::repository(tempdir.path().join("original")); let (id, _, _) = init( &original, - "acme", + "acme".try_into().unwrap(), "Acme's repo", git::refname!("master"), Visibility::default(), diff --git a/radicle/src/test/arbitrary.rs b/radicle/src/test/arbitrary.rs index fea78c07..e90f3fa4 100644 --- a/radicle/src/test/arbitrary.rs +++ b/radicle/src/test/arbitrary.rs @@ -13,6 +13,7 @@ use qcheck::Arbitrary; use crate::collections::RandomMap; use crate::identity::doc::Visibility; +use crate::identity::project::ProjectName; use crate::identity::{ doc::{Doc, DocAt, RepoId}, project::Project, @@ -108,9 +109,10 @@ impl Arbitrary for Project { fn arbitrary(g: &mut qcheck::Gen) -> Self { let mut rng = fastrand::Rng::with_seed(u64::arbitrary(g)); let length = rng.usize(1..16); - let name = iter::repeat_with(|| rng.alphanumeric()) + let name: String = iter::repeat_with(|| rng.alphanumeric()) .take(length) .collect(); + let name = ProjectName::from_str(&name).unwrap(); let description = iter::repeat_with(|| rng.alphanumeric()) .take(length * 2) .collect(); diff --git a/radicle/src/test/fixtures.rs b/radicle/src/test/fixtures.rs index a8633034..7f41f0f3 100644 --- a/radicle/src/test/fixtures.rs +++ b/radicle/src/test/fixtures.rs @@ -44,7 +44,7 @@ pub fn storage, G: Signer>(path: P, signer: &G) -> Result, G: Signer>( let (working, head) = repository(path); let (id, _, refs) = rad::init( &working, - "acme", + "acme".try_into().unwrap(), "Acme's repository", git::refname!("master"), Visibility::default(),