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

package namespace and name validation enforces lowercase only #265

Merged
merged 1 commit into from
Apr 24, 2024
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
2 changes: 1 addition & 1 deletion crates/client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ impl<R: RegistryStorage, C: ContentStorage, N: NamespaceMapStorage> Client<R, C,
let operator = self.registry().load_operator(&None).await?;
let operator_log_maps_namespace = if let Some(op) = operator {
let namespace_state = op.state.namespace_state(namespace);
if let Ok(Some(nm)) = namespace_state {
if let Some(nm) = namespace_state {
if let warg_protocol::operator::NamespaceState::Imported { registry } = nm {
self.api
.set_warg_registry(Some(RegistryDomain::from_str(registry)?));
Expand Down
60 changes: 14 additions & 46 deletions crates/protocol/src/operator/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,11 @@ pub enum ValidationError {
#[error("record has lower timestamp than previous")]
TimestampLowerThanPrevious,

#[error("the namespace `{namespace}` is invalid; namespace must be a kebab case string")]
#[error(
"the namespace `{namespace}` is invalid; namespace must be a lowercased kebab case string"
)]
InvalidNamespace { namespace: String },

#[error("the namespace `{namespace}` conflicts with the existing namespace `{existing}`; namespace must be unique in a case insensitive way")]
NamespaceConflict { namespace: String, existing: String },

#[error("the namespace `{namespace}` is already defined and cannot be redefined")]
NamespaceAlreadyDefined { namespace: String },
}
Expand All @@ -73,8 +72,6 @@ pub enum ValidationError {
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
#[serde(rename_all = "camelCase")]
struct NamespaceDefinition {
/// Case sensitive namespace name.
namespace: String,
/// Namespace state.
state: NamespaceState,
}
Expand Down Expand Up @@ -123,7 +120,7 @@ pub struct LogState {
/// The keys known to the state.
#[serde(skip_serializing_if = "IndexMap::is_empty")]
keys: IndexMap<signing::KeyID, signing::PublicKey>,
/// The namespaces known to the state. The key is the lowercased namespace.
/// The namespaces known to the state. The key is the namespace.
#[serde(skip_serializing_if = "IndexMap::is_empty")]
namespaces: IndexMap<String, NamespaceDefinition>,
}
Expand Down Expand Up @@ -164,20 +161,8 @@ impl LogState {
}

/// Gets the namespace state.
pub fn namespace_state(&self, namespace: &str) -> Result<Option<&NamespaceState>, &str> {
if let Some(def) = self.namespaces.get(&namespace.to_ascii_lowercase()) {
if def.namespace == namespace {
// namespace exact match, return namespace state
Ok(Some(&def.state))
} else {
// namespace matches a namespace but differ in a case sensitive way,
// so return error with existing namespace
Err(&def.namespace)
}
} else {
// namespace is not defined
Ok(None)
}
pub fn namespace_state(&self, namespace: &str) -> Option<&NamespaceState> {
self.namespaces.get(namespace).map(|def| &def.state)
}

/// Checks the key has permission to sign checkpoints.
Expand Down Expand Up @@ -411,30 +396,15 @@ impl LogState {
});
}

let namespace_lowercase = namespace.to_ascii_lowercase();

if let Some(def) = self.namespaces.get(&namespace_lowercase) {
if namespace == def.namespace {
// namespace matches exactly
Err(ValidationError::NamespaceAlreadyDefined {
namespace: namespace.to_string(),
})
} else {
// namespace matches an existing namespace but differs in a case sensitive way
Err(ValidationError::NamespaceConflict {
namespace: namespace.to_string(),
existing: def.namespace.to_string(),
})
}
if self.namespaces.contains_key(namespace) {
// namespace is already defined
Err(ValidationError::NamespaceAlreadyDefined {
namespace: namespace.to_string(),
})
} else {
// namespace is not defined
self.namespaces.insert(
namespace_lowercase,
NamespaceDefinition {
namespace: namespace.to_string(),
state,
},
);
self.namespaces
.insert(namespace.to_string(), NamespaceDefinition { state });

Ok(())
}
Expand Down Expand Up @@ -647,14 +617,12 @@ mod tests {
(
"my-namespace".to_string(),
NamespaceDefinition {
namespace: "my-namespace".to_string(),
state: NamespaceState::Defined,
},
),
(
"imported-namespace".to_string(),
NamespaceDefinition {
namespace: "imported-namespace".to_string(),
state: NamespaceState::Imported {
registry: "registry.example.com".to_string(),
},
Expand Down Expand Up @@ -716,7 +684,7 @@ mod tests {

// This validation should fail
match state.validate(&envelope).unwrap_err() {
ValidationError::NamespaceConflict { .. } => {}
ValidationError::InvalidNamespace { .. } => {}
_ => panic!("expected a different error"),
}
}
Expand Down
5 changes: 3 additions & 2 deletions crates/protocol/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ impl PackageName {
// Validate the namespace and name parts are valid kebab strings
if KebabStr::new(&name[colon + 1..]).is_some()
&& Self::is_valid_namespace(&name[..colon])
&& name[colon + 1..].chars().all(|c| !c.is_ascii_uppercase())
{
return Ok(Self {
package_name: name,
Expand All @@ -153,7 +154,7 @@ impl PackageName {
}
}

bail!("invalid package name `{name}`: expected format is `<namespace>:<name>`")
bail!("invalid package name `{name}`: expected format is `<namespace>:<name>` using lowercased characters")
}

/// Gets the namespace part of the package identifier.
Expand All @@ -168,7 +169,7 @@ impl PackageName {

/// Check if string is a valid namespace.
pub fn is_valid_namespace(namespace: &str) -> bool {
KebabStr::new(namespace).is_some()
KebabStr::new(namespace).is_some() && namespace.chars().all(|c| !c.is_ascii_uppercase())
}
}

Expand Down
10 changes: 2 additions & 8 deletions crates/server/src/datastore/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -734,25 +734,19 @@ impl DataStore for MemoryDataStore {
.state
.namespace_state(package_name.namespace())
{
Ok(Some(state)) => match state {
Some(state) => match state {
operator::NamespaceState::Defined => {}
operator::NamespaceState::Imported { .. } => {
return Err(DataStoreError::PackageNamespaceImported(
package_name.namespace().to_string(),
))
}
},
Ok(None) => {
None => {
return Err(DataStoreError::PackageNamespaceNotDefined(
package_name.namespace().to_string(),
))
}
Err(existing_namespace) => {
return Err(DataStoreError::PackageNamespaceConflict {
namespace: package_name.namespace().to_string(),
existing: existing_namespace.to_string(),
})
}
}

// verify package name is unique in a case insensitive way
Expand Down
10 changes: 2 additions & 8 deletions crates/server/src/datastore/postgres/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -976,25 +976,19 @@ impl DataStore for PostgresDataStore {

// verify namespace is defined and not imported
match validator.namespace_state(package_name.namespace()) {
Ok(Some(state)) => match state {
Some(state) => match state {
operator::NamespaceState::Defined => {}
operator::NamespaceState::Imported { .. } => {
return Err(DataStoreError::PackageNamespaceImported(
package_name.namespace().to_string(),
))
}
},
Ok(None) => {
None => {
return Err(DataStoreError::PackageNamespaceNotDefined(
package_name.namespace().to_string(),
))
}
Err(existing_namespace) => {
return Err(DataStoreError::PackageNamespaceConflict {
namespace: package_name.namespace().to_string(),
existing: existing_namespace.to_string(),
})
}
}

// verify package name is unique in a case insensitive way
Expand Down
6 changes: 0 additions & 6 deletions tests/memory/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,6 @@ async fn it_rejects_unknown_signing_key() -> Result<()> {
test_unknown_signing_key(&config).await
}

#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn it_rejects_package_name_conflict() -> Result<()> {
let (_server, config) = spawn_server(&root().await?, None, None, None).await?;
test_publishing_name_conflict(&config).await
}

#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn it_rejects_invalid_signature() -> Result<()> {
let (_server, config) = spawn_server(&root().await?, None, None, None).await?;
Expand Down
2 changes: 0 additions & 2 deletions tests/postgres/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ async fn it_works_with_postgres() -> TestResult {
test_wit_publishing(&config).await?;
test_wasm_content_policy(&config).await?;
test_unauthorized_signing_key(&config).await?;
test_publishing_name_conflict(&config).await?;
// This is tested below where a different server is used that
// allows any signing key
//test_unknown_signing_key(&config).await?;
Expand All @@ -62,7 +61,6 @@ async fn it_works_with_postgres() -> TestResult {
PackageName::new("test:yankee")?,
PackageName::new("test:wit-package")?,
PackageName::new("test:unauthorized-key")?,
PackageName::new("test:name")?,
];

// There should be two log entries in the registry
Expand Down
29 changes: 0 additions & 29 deletions tests/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,35 +336,6 @@ async fn test_unknown_signing_key(config: &Config) -> Result<()> {
Ok(())
}

async fn test_publishing_name_conflict(config: &Config) -> Result<()> {
let client = create_client(config)?;
let signing_key = test_signing_key();

publish_component(
&client,
&PackageName::new("test:name")?,
"0.1.0",
"(component)",
true,
&signing_key,
)
.await?;

// should be rejected
publish_component(
&client,
&PackageName::new("test:NAME")?,
"0.1.1",
"(component)",
true,
&signing_key,
)
.await
.expect_err("expected publish to fail");

Ok(())
}

async fn test_invalid_signature(config: &Config) -> Result<()> {
const PACKAGE_NAME: &str = "test:invalid-signature";

Expand Down
Loading