feat(#2302): fold is_plain_ident into PlainIdent newtype

This commit is contained in:
damocles 2026-07-19 17:56:55 +02:00 committed by mara
commit d4e91bfeeb
3 changed files with 112 additions and 86 deletions

View file

@ -23,20 +23,9 @@ use axum::extract::{Form, Query};
use axum::response::{IntoResponse, Response};
use serde::{Deserialize, Serialize};
use super::error_response;
use super::{error_response, idents::PlainIdent};
use crate::coordinator::Coordinator;
/// Plain-identifier check matching hive-priv's `validate_name_chars`
/// (lowercase ascii + digits + hyphens) — same guard used by
/// `matrix_accounts::is_plain_ident`. Duplicated locally (private, not
/// worth a shared-util churn for one predicate) rather than exported from
/// that module, since both call sites are dashboard-only.
fn is_plain_ident(s: &str) -> bool {
!s.is_empty()
&& s.chars()
.all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-')
}
#[derive(Deserialize)]
struct ForgeSidecar {
base_url: String,
@ -75,10 +64,10 @@ pub(super) struct ExtraForgesQuery {
/// sidecar when present. Never returns a token.
pub(super) async fn get_extra_forges(Query(q): Query<ExtraForgesQuery>) -> Response {
let agent = q.agent.trim();
if !is_plain_ident(agent) {
let Ok(agent) = PlainIdent::parse(agent) else {
return error_response(&format!("extra-forges: invalid agent {agent:?}"));
}
let dir = Coordinator::agent_notes_dir(agent);
};
let dir = Coordinator::agent_notes_dir(agent.as_str());
let mut forges = Vec::new();
match std::fs::read_dir(&dir) {
Ok(entries) => {
@ -141,12 +130,12 @@ struct ExtraForgeAccountResult {
pub(super) async fn post_extra_forge_account(Form(f): Form<ExtraForgeAccountForm>) -> Response {
let agent = f.agent.trim();
let label = f.label.trim();
if !is_plain_ident(agent) {
let Ok(agent) = PlainIdent::parse(agent) else {
return error_response(&format!("extra-forge-account: invalid agent {agent:?}"));
}
if !is_plain_ident(label) {
};
let Ok(label) = PlainIdent::parse(label) else {
return error_response(&format!("extra-forge-account: invalid label {label:?}"));
}
};
match f.action.as_str() {
"add" => {
@ -160,9 +149,13 @@ pub(super) async fn post_extra_forge_account(Form(f): Form<ExtraForgeAccountForm
let Some(token) = f.token.as_deref().filter(|t| !t.is_empty()) else {
return error_response("extra-forge-account: token is required");
};
if let Err(e) =
crate::priv_client::write_agent_extra_forge_account(agent, label, base_url, token)
.await
if let Err(e) = crate::priv_client::write_agent_extra_forge_account(
agent.as_str(),
label.as_str(),
base_url,
token,
)
.await
{
return error_response(&format!(
"extra-forge-account: write account failed: {e:#}"
@ -171,7 +164,9 @@ pub(super) async fn post_extra_forge_account(Form(f): Form<ExtraForgeAccountForm
tracing::info!(%agent, %label, "extra-forge-account: provisioned");
}
"remove" => {
if let Err(e) = crate::priv_client::delete_agent_extra_forge_account(agent, label).await
if let Err(e) =
crate::priv_client::delete_agent_extra_forge_account(agent.as_str(), label.as_str())
.await
{
return error_response(&format!(
"extra-forge-account: delete account failed: {e:#}"
@ -187,19 +182,3 @@ pub(super) async fn post_extra_forge_account(Form(f): Form<ExtraForgeAccountForm
}
axum::Json(ExtraForgeAccountResult { ok: true }).into_response()
}
#[cfg(test)]
mod tests {
use super::is_plain_ident;
#[test]
fn is_plain_ident_matches_validate_name_chars() {
assert!(is_plain_ident("codeberg"));
assert!(is_plain_ident("my-forge-1"));
assert!(!is_plain_ident(""));
assert!(!is_plain_ident("MyForge"));
assert!(!is_plain_ident("my_forge"));
assert!(!is_plain_ident("../escape"));
assert!(!is_plain_ident("a/b"));
}
}

View file

@ -1,5 +1,6 @@
//! Validated identifier newtypes for dashboard path-params — "parse, don't
//! validate" (#2302).
//! Validated identifier newtypes for dashboard path-params — the
//! "parse, don't validate" discipline applied to agent-name / plain-ident
//! path parameters.
//!
//! [`AgentName`] can only be constructed through a validating parser, so
//! "this string passed the naming whitelist" becomes a compile-time fact the
@ -8,6 +9,11 @@
//! whether the name refers to a *live* agent is a separate, stateful runtime
//! concern kept at the lookup sites (see `guard_agent_name`), deliberately not
//! folded into the constructor.
//!
//! [`PlainIdent`] is the slightly stricter sibling (no underscore, no length
//! cap) used for dashboard-provisioned labels / account names — it folds the
//! two hand-synced `is_plain_ident` copies that used to live in
//! `extra_forges` + `matrix_accounts` into one parser.
use std::fmt;
@ -57,9 +63,52 @@ impl fmt::Display for AgentName {
}
}
/// A validated plain identifier: one or more chars of `[a-z0-9-]`.
///
/// Matches hive-priv's `validate_name_chars` (lowercase ascii + digits +
/// hyphens, no underscore, no length cap). Used for dashboard-provisioned
/// labels + account names (extra-forge labels, matrix account names) that
/// become filesystem path segments, so the same `../` / uppercase / slash
/// rejects as [`AgentName`] apply — the two differ only in the underscore
/// (allowed by `AgentName`, not here) and the 63-char cap.
#[derive(Debug, Clone, PartialEq, Eq)]
pub(crate) struct PlainIdent(String);
impl PlainIdent {
/// Parse + validate a plain identifier.
///
/// # Errors
/// Returns `Err(reason)` when `s` is empty or contains any byte outside
/// `[a-z0-9-]`.
pub(crate) fn parse(s: &str) -> Result<Self, &'static str> {
if s.is_empty() {
return Err("identifier must not be empty");
}
if !s
.bytes()
.all(|b| b.is_ascii_lowercase() || b.is_ascii_digit() || b == b'-')
{
return Err("identifier must contain only [a-z0-9-]");
}
Ok(Self(s.to_owned()))
}
/// The validated identifier as a string slice.
#[must_use]
pub(crate) fn as_str(&self) -> &str {
&self.0
}
}
impl fmt::Display for PlainIdent {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str(&self.0)
}
}
#[cfg(test)]
mod tests {
use super::AgentName;
use super::{AgentName, PlainIdent};
#[test]
fn agent_name_accepts_canonical_shapes() {
@ -100,4 +149,25 @@ mod tests {
fn agent_name_round_trips_as_str() {
assert_eq!(AgentName::parse("damocles").unwrap().as_str(), "damocles");
}
#[test]
fn plain_ident_accepts_labels_and_accounts() {
for ok in ["codeberg", "catgirl", "my-forge-1", "acct-1"] {
assert!(PlainIdent::parse(ok).is_ok(), "should accept {ok:?}");
}
}
#[test]
fn plain_ident_rejects_bad_input() {
// Underscore is allowed for AgentName but NOT here (matches
// hive-priv's stricter `validate_name_chars`).
for bad in ["", "MyForge", "my_forge", "../escape", "a/b", "a.b"] {
assert!(PlainIdent::parse(bad).is_err(), "should reject {bad:?}");
}
}
#[test]
fn plain_ident_round_trips_as_str() {
assert_eq!(PlainIdent::parse("codeberg").unwrap().as_str(), "codeberg");
}
}

View file

@ -23,7 +23,7 @@ use axum::extract::{Form, Query};
use axum::response::{IntoResponse, Response};
use serde::{Deserialize, Serialize};
use super::error_response;
use super::{error_response, idents::PlainIdent};
use crate::coordinator::Coordinator;
#[derive(Deserialize)]
@ -176,17 +176,6 @@ struct MatrixLoginResult {
user_id: String,
}
/// Plain-identifier check matching hive-priv's `validate_name_chars`
/// exactly (lowercase ascii + digits + hyphens) — the root-side guard
/// re-applies the same rule before building the token path. Keeping the
/// dashboard check identical means a name that passes here can't then be
/// rejected at the priv boundary with a confusing "write token failed".
fn is_plain_ident(s: &str) -> bool {
!s.is_empty()
&& s.chars()
.all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-')
}
/// Provision (or refresh) the token for an agent's extra matrix account.
/// password mode → `m.login.password`; token mode → validate via `whoami`.
/// On success writes the token to `matrix-token-<account>` via hive-priv and
@ -196,15 +185,15 @@ pub(super) async fn post_matrix_account_login(Form(f): Form<MatrixLoginForm>) ->
let agent = f.agent.trim();
let account = f.account.trim();
let homeserver = f.homeserver.trim().trim_end_matches('/');
if !is_plain_ident(agent) {
let Ok(agent) = PlainIdent::parse(agent) else {
return error_response(&format!("matrix-account-login: invalid agent {agent:?}"));
}
if !is_plain_ident(account) {
};
let Ok(account) = PlainIdent::parse(account) else {
return error_response(&format!(
"matrix-account-login: invalid account {account:?}"
));
}
if account == "main" {
};
if account.as_str() == "main" {
return error_response(
"matrix-account-login: 'main' is the hive-internal account; it is \
provisioned via the normal flow, not this form",
@ -244,15 +233,19 @@ pub(super) async fn post_matrix_account_login(Form(f): Form<MatrixLoginForm>) ->
}
};
if let Err(e) =
crate::priv_client::write_agent_matrix_token(agent, &token, Some(account), Some(homeserver))
.await
if let Err(e) = crate::priv_client::write_agent_matrix_token(
agent.as_str(),
&token,
Some(account.as_str()),
Some(homeserver),
)
.await
{
return error_response(&format!("matrix-account-login: write token failed: {e:#}"));
}
// Best-effort kick so the daemon picks up the new account without a full
// container restart; not fatal if the container isn't running.
if let Err(e) = crate::priv_client::restart_matrix_daemon(agent).await {
if let Err(e) = crate::priv_client::restart_matrix_daemon(agent.as_str()).await {
tracing::warn!(
%agent, %account, error = ?e,
"matrix-account-login: daemon restart failed (token written; loads on next start)"
@ -288,13 +281,13 @@ struct GithubAccountResult {
pub(super) async fn post_github_account(Form(f): Form<GithubAccountForm>) -> Response {
let agent = f.agent.trim();
let token = f.token.trim();
if !is_plain_ident(agent) {
let Ok(agent) = PlainIdent::parse(agent) else {
return error_response(&format!("github-account: invalid agent {agent:?}"));
}
};
if token.is_empty() {
return error_response("github-account: token is required");
}
if let Err(e) = crate::priv_client::write_agent_github_token(agent, token).await {
if let Err(e) = crate::priv_client::write_agent_github_token(agent.as_str(), token).await {
return error_response(&format!("github-account: write token failed: {e:#}"));
}
tracing::info!(%agent, "github-account: provisioned github PAT");
@ -320,10 +313,10 @@ struct GithubAccountStatus {
/// Never returns the token itself.
pub(super) async fn get_github_account(Query(q): Query<GithubAccountQuery>) -> Response {
let agent = q.agent.trim();
if !is_plain_ident(agent) {
let Ok(agent) = PlainIdent::parse(agent) else {
return error_response(&format!("github-account: invalid agent {agent:?}"));
}
let present = Coordinator::agent_notes_dir(agent)
};
let present = Coordinator::agent_notes_dir(agent.as_str())
.join("github-token")
.exists();
axum::Json(GithubAccountStatus { present }).into_response()
@ -402,7 +395,7 @@ async fn matrix_whoami(homeserver: &str, token: &str) -> Result<String, String>
#[cfg(test)]
mod tests {
use super::{account_name_from_filename, is_plain_ident, read_accounts_snapshot};
use super::{account_name_from_filename, read_accounts_snapshot};
fn unique_dir(tag: &str) -> std::path::PathBuf {
let d = std::env::temp_dir().join(format!(
@ -488,20 +481,4 @@ mod tests {
assert_eq!(account_name_from_filename("notes.md"), None);
assert_eq!(account_name_from_filename("matrix-avatar-icon-hash"), None);
}
#[test]
fn is_plain_ident_matches_validate_name_chars() {
// Accepts exactly what hive-priv's validate_name_chars does:
// lowercase ascii + digits + hyphens.
assert!(is_plain_ident("catgirl"));
assert!(is_plain_ident("acct-1"));
assert!(!is_plain_ident(""));
// Rejected: uppercase + underscore (would pass a looser check
// then fail at the priv boundary), and path chars.
assert!(!is_plain_ident("MyAccount"));
assert!(!is_plain_ident("my_account"));
assert!(!is_plain_ident("../escape"));
assert!(!is_plain_ident("a/b"));
assert!(!is_plain_ident("a.b"));
}
}