Validate project names properly in all places

We were only validating names when passed as a flag to `rad init
--name`.
This commit is contained in:
cloudhead 2024-07-31 12:55:53 +02:00
parent ee1c867800
commit 84e3ba1482
No known key found for this signature in database
10 changed files with 111 additions and 42 deletions

View File

@ -13,6 +13,7 @@ use serde_json as json;
use radicle::crypto::{ssh, Verified}; use radicle::crypto::{ssh, Verified};
use radicle::explorer::ExplorerUrl; use radicle::explorer::ExplorerUrl;
use radicle::git::RefString; use radicle::git::RefString;
use radicle::identity::project::ProjectName;
use radicle::identity::{RepoId, Visibility}; use radicle::identity::{RepoId, Visibility};
use radicle::node::events::UploadPack; use radicle::node::events::UploadPack;
use radicle::node::policy::Scope; use radicle::node::policy::Scope;
@ -57,7 +58,7 @@ Options
#[derive(Default)] #[derive(Default)]
pub struct Options { pub struct Options {
pub path: Option<PathBuf>, pub path: Option<PathBuf>,
pub name: Option<String>, pub name: Option<ProjectName>,
pub description: Option<String>, pub description: Option<String>,
pub branch: Option<String>, pub branch: Option<String>,
pub interactive: Interactive, pub interactive: Interactive,
@ -94,19 +95,8 @@ impl Args for Options {
Long("name") if name.is_none() => { Long("name") if name.is_none() => {
let value = parser.value()?; let value = parser.value()?;
let value = term::args::string(&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); name = Some(value);
} }
Long("description") if description.is_none() => { Long("description") if description.is_none() => {
@ -243,7 +233,7 @@ pub fn init(
term::format::dim(path.display()) term::format::dim(path.display())
)); ));
let name = match options.name { let name: ProjectName = match options.name {
Some(name) => name, Some(name) => name,
None => { None => {
let default = path.file_name().map(|f| f.to_string_lossy().to_string()); let default = path.file_name().map(|f| f.to_string_lossy().to_string());
@ -252,6 +242,7 @@ pub fn init(
default, default,
Some("The name of your repository, eg. 'acme'"), Some("The name of your repository, eg. 'acme'"),
)? )?
.try_into()?
} }
}; };
let description = match options.description { let description = match options.description {
@ -287,7 +278,7 @@ pub fn init(
match radicle::rad::init( match radicle::rad::init(
&repo, &repo,
&name, name,
&description, &description,
branch.clone(), branch.clone(),
visibility, visibility,

View File

@ -548,7 +548,7 @@ impl<G: cyphernet::Ecdh<Pk = NodeId> + Signer + Clone> Node<G> {
let branch = refname!("master"); let branch = refname!("master");
let id = rad::init( let id = rad::init(
repo, repo,
name, name.try_into().unwrap(),
description, description,
branch.clone(), branch.clone(),
Visibility::default(), Visibility::default(),

View File

@ -132,7 +132,7 @@ impl<G: Signer> Peer<Storage, G> {
let (repo, _) = fixtures::repository(self.tempdir.path().join(name)); let (repo, _) = fixtures::repository(self.tempdir.path().join(name));
let (rid, _, _) = rad::init( let (rid, _, _) = rad::init(
&repo, &repo,
name, name.try_into().unwrap(),
description, description,
radicle::git::refname!("master"), radicle::git::refname!("master"),
Visibility::default(), Visibility::default(),

View File

@ -1648,7 +1648,7 @@ fn test_init_and_seed() {
// Alice creates a new project. // Alice creates a new project.
let (proj_id, _, _) = rad::init( let (proj_id, _, _) = rad::init(
&repo, &repo,
"alice", "alice".try_into().unwrap(),
"alice's repo", "alice's repo",
git::refname!("master"), git::refname!("master"),
Visibility::default(), Visibility::default(),

View File

@ -10,7 +10,7 @@ fn main() -> anyhow::Result<()> {
let signer = profile.signer()?; let signer = profile.signer()?;
let (id, _, _) = radicle::rad::init( let (id, _, _) = radicle::rad::init(
&repo, &repo,
&name, name.try_into()?,
"", "",
git::refname!("master"), git::refname!("master"),
Visibility::default(), Visibility::default(),

View File

@ -462,7 +462,7 @@ mod test {
let (repo, _) = fixtures::repository(tempdir.path().join("working")); let (repo, _) = fixtures::repository(tempdir.path().join("working"));
let (id, _, _) = rad::init( let (id, _, _) = rad::init(
&repo, &repo,
"heartwood", "heartwood".try_into().unwrap(),
"Radicle Heartwood Protocol & Stack", "Radicle Heartwood Protocol & Stack",
git::refname!("master"), git::refname!("master"),
Visibility::default(), Visibility::default(),
@ -509,7 +509,7 @@ mod test {
let delegate = MockSigner::from_seed([0xff; 32]); let delegate = MockSigner::from_seed([0xff; 32]);
let (rid, doc, _) = rad::init( let (rid, doc, _) = rad::init(
&working, &working,
"heartwood", "heartwood".try_into().unwrap(),
"Radicle Heartwood Protocol & Stack", "Radicle Heartwood Protocol & Stack",
git::refname!("master"), git::refname!("master"),
Visibility::default(), Visibility::default(),

View File

@ -1,4 +1,4 @@
use std::fmt; use std::{fmt, str::FromStr};
use serde::{ use serde::{
de::{self, MapAccess, Visitor}, de::{self, MapAccess, Visitor},
@ -24,12 +24,72 @@ pub enum ProjectError {
DefaultBranch(&'static str), 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<ProjectName> 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<Self, Self::Error> {
ProjectName::from_str(s)
}
}
impl TryFrom<String> for ProjectName {
type Error = ProjectError;
fn try_from(s: String) -> Result<Self, Self::Error> {
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, Self::Err> {
Self::try_from(s.to_owned())
}
}
/// A "project" payload in an identity document. /// A "project" payload in an identity document.
#[derive(Debug, Clone, PartialEq, Eq, Serialize)] #[derive(Debug, Clone, PartialEq, Eq, Serialize)]
#[serde(rename_all = "camelCase")] #[serde(rename_all = "camelCase")]
pub struct Project { pub struct Project {
/// Project name. /// Project name.
name: String, name: ProjectName,
/// Project description. /// Project description.
description: String, description: String,
/// Project default branch. /// Project default branch.
@ -48,8 +108,7 @@ impl<'de> Deserialize<'de> for Project {
Description, Description,
DefaultBranch, DefaultBranch,
/// A catch-all variant to allow for unknown fields /// A catch-all variant to allow for unknown fields
#[allow(dead_code)] Unknown(#[allow(dead_code)] String),
Unknown(String),
} }
struct ProjectVisitor; struct ProjectVisitor;
@ -123,18 +182,12 @@ impl Project {
/// * `description`'s length must not exceed 255. /// * `description`'s length must not exceed 255.
/// * `default_branch`'s length must not be empty and must not exceed 255. /// * `default_branch`'s length must not be empty and must not exceed 255.
pub fn new( pub fn new(
name: String, name: ProjectName,
description: String, description: String,
default_branch: BranchName, default_branch: BranchName,
) -> Result<Self, Vec<ProjectError>> { ) -> Result<Self, Vec<ProjectError>> {
let mut errs = Vec::new(); 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 { if description.len() > doc::MAX_STRING_LENGTH {
errs.push(ProjectError::Description( errs.push(ProjectError::Description(
"description cannot exceed 255 bytes", "description cannot exceed 255 bytes",
@ -171,7 +224,7 @@ impl Project {
/// original validation rules (see [`Project::new`]). /// original validation rules (see [`Project::new`]).
pub fn update( pub fn update(
self, self,
name: impl Into<Option<String>>, name: impl Into<Option<ProjectName>>,
description: impl Into<Option<String>>, description: impl Into<Option<String>>,
default_branch: impl Into<Option<BranchName>>, default_branch: impl Into<Option<BranchName>>,
) -> Result<Self, Vec<ProjectError>> { ) -> Result<Self, Vec<ProjectError>> {
@ -183,7 +236,7 @@ impl Project {
#[inline] #[inline]
pub fn name(&self) -> &str { pub fn name(&self) -> &str {
&self.name self.name.as_str()
} }
#[inline] #[inline]
@ -205,3 +258,26 @@ impl From<Project> for Payload {
Self::from(value) Self::from(value)
} }
} }
#[cfg(test)]
mod test {
use super::*;
use crate::assert_matches;
#[test]
fn test_project_name() {
assert_matches!(serde_json::from_str::<ProjectName>("\"\""), Err(_));
assert_matches!(
serde_json::from_str::<ProjectName>("\"invalid name\""),
Err(_)
);
assert_matches!(
serde_json::from_str::<ProjectName>("\"invalid%name\""),
Err(_)
);
assert_eq!(
serde_json::from_str::<ProjectName>("\"valid-name\"").unwrap(),
ProjectName("valid-name".to_owned())
);
}
}

View File

@ -11,7 +11,7 @@ use crate::crypto::{Signer, Verified};
use crate::git; use crate::git;
use crate::identity::doc; use crate::identity::doc;
use crate::identity::doc::{DocError, RepoId, Visibility}; 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::transport;
use crate::storage::git::Repository; use crate::storage::git::Repository;
use crate::storage::refs::SignedRefs; use crate::storage::refs::SignedRefs;
@ -46,7 +46,7 @@ pub enum InitError {
/// Initialize a new radicle project from a git repository. /// Initialize a new radicle project from a git repository.
pub fn init<G: Signer, S: WriteStorage>( pub fn init<G: Signer, S: WriteStorage>(
repo: &git2::Repository, repo: &git2::Repository,
name: &str, name: ProjectName,
description: &str, description: &str,
default_branch: BranchName, default_branch: BranchName,
visibility: Visibility, visibility: Visibility,
@ -416,7 +416,7 @@ mod tests {
let (repo, _) = fixtures::repository(tempdir.path().join("working")); let (repo, _) = fixtures::repository(tempdir.path().join("working"));
let (proj, _, refs) = init( let (proj, _, refs) = init(
&repo, &repo,
"acme", "acme".try_into().unwrap(),
"Acme's repo", "Acme's repo",
git::refname!("master"), git::refname!("master"),
Visibility::default(), Visibility::default(),
@ -471,7 +471,7 @@ mod tests {
let (original, _) = fixtures::repository(tempdir.path().join("original")); let (original, _) = fixtures::repository(tempdir.path().join("original"));
let (id, _, alice_refs) = init( let (id, _, alice_refs) = init(
&original, &original,
"acme", "acme".try_into().unwrap(),
"Acme's repo", "Acme's repo",
git::refname!("master"), git::refname!("master"),
Visibility::default(), Visibility::default(),
@ -507,7 +507,7 @@ mod tests {
let (original, _) = fixtures::repository(tempdir.path().join("original")); let (original, _) = fixtures::repository(tempdir.path().join("original"));
let (id, _, _) = init( let (id, _, _) = init(
&original, &original,
"acme", "acme".try_into().unwrap(),
"Acme's repo", "Acme's repo",
git::refname!("master"), git::refname!("master"),
Visibility::default(), Visibility::default(),

View File

@ -13,6 +13,7 @@ use qcheck::Arbitrary;
use crate::collections::RandomMap; use crate::collections::RandomMap;
use crate::identity::doc::Visibility; use crate::identity::doc::Visibility;
use crate::identity::project::ProjectName;
use crate::identity::{ use crate::identity::{
doc::{Doc, DocAt, RepoId}, doc::{Doc, DocAt, RepoId},
project::Project, project::Project,
@ -108,9 +109,10 @@ impl Arbitrary for Project {
fn arbitrary(g: &mut qcheck::Gen) -> Self { fn arbitrary(g: &mut qcheck::Gen) -> Self {
let mut rng = fastrand::Rng::with_seed(u64::arbitrary(g)); let mut rng = fastrand::Rng::with_seed(u64::arbitrary(g));
let length = rng.usize(1..16); let length = rng.usize(1..16);
let name = iter::repeat_with(|| rng.alphanumeric()) let name: String = iter::repeat_with(|| rng.alphanumeric())
.take(length) .take(length)
.collect(); .collect();
let name = ProjectName::from_str(&name).unwrap();
let description = iter::repeat_with(|| rng.alphanumeric()) let description = iter::repeat_with(|| rng.alphanumeric())
.take(length * 2) .take(length * 2)
.collect(); .collect();

View File

@ -44,7 +44,7 @@ pub fn storage<P: AsRef<Path>, G: Signer>(path: P, signer: &G) -> Result<Storage
let (repo, _) = repository(path.join("workdir").join(name)); let (repo, _) = repository(path.join("workdir").join(name));
rad::init( rad::init(
&repo, &repo,
name, name.try_into().unwrap(),
desc, desc,
git::refname!("master"), git::refname!("master"),
Visibility::default(), Visibility::default(),
@ -67,7 +67,7 @@ pub fn project<P: AsRef<Path>, G: Signer>(
let (working, head) = repository(path); let (working, head) = repository(path);
let (id, _, refs) = rad::init( let (id, _, refs) = rad::init(
&working, &working,
"acme", "acme".try_into().unwrap(),
"Acme's repository", "Acme's repository",
git::refname!("master"), git::refname!("master"),
Visibility::default(), Visibility::default(),