hive-c0re: refuse an account name for the disk path, not just the store one

`token_path`'s doc claimed the compiler was the check and that nothing had to
remember to perform one. True of `agent`, which is an `Ident`. Not true of
`account`, a bare `&str` concatenated into the filename — safe only because
`deliver` happened to validate it first, which is the caller-must-remember
pattern the comment denied.

Observably a no-op today: the one call site already rejects a bad account
before reaching here. What changes is that the signature now enforces what the
comment asserted, so a second caller cannot skip it.

The check is `path::checked_segment`, made public rather than reimplemented.
Two copies of a charset are two charsets: they agree until one is edited, and
the day they diverge a name is legal in the store and not on disk.

An account name cannot simply become an `Ident` the way an agent name is:
it is an attribute name in `hyperhive.matrixAccounts`, so uppercase and
underscore are already configurable, and narrowing that is a decision rather
than a refactor. The new test's controls pin both.

Found by argus reviewing the merged PR.

hive-c0re/src/workers/credential.rs

@@ -32,13 +32,20 @@ const TOKEN_PREFIX: &str = "matrix-token";
 
 /// Where `agent`'s credential for `account` is written.
 ///
-/// Takes an [`Ident`] rather than a `&str` because the name arrives off the
+/// Both names arrive off the queue and both become path components, so both
-/// queue: `agent_state_dir` addresses a directory, and an unvalidated name
+/// are refused here rather than trusted: `agent` by its [`Ident`] type, and
-/// there is a path-traversal argument. The compiler refusing the `&str` is the
+/// `account` by the same charset check the store path uses. They are checked
-/// check — nothing here has to remember to perform one.
+/// by different mechanisms because only one of them has a newtype — an account
-#[must_use]
+/// name is an attribute name in `hyperhive.matrixAccounts`, so it is not an
-pub fn token_path(agent: &Ident, account: &str) -> PathBuf {
+/// `Ident` and cannot become one without narrowing what an operator may
-    agent_state_dir(agent).join(format!("{TOKEN_PREFIX}-{account}"))
+/// configure.
+///
+/// # Errors
+/// [`swarm_secret_client::Error::PathSegment`] when `account` is empty or
+/// holds anything outside `[A-Za-z0-9_-]`.
+pub fn token_path(agent: &Ident, account: &str) -> Result<PathBuf, swarm_secret_client::Error> {
+    path::checked_segment("account", account)?;
+    Ok(agent_state_dir(agent).join(format!("{TOKEN_PREFIX}-{account}")))
 }
 
 /// Read the credential `notice` names and write it into the agent's state dir.
@@ -65,7 +72,7 @@ pub async fn deliver(notice: &CredentialNotice, cert_role: &str) -> Result<()> {
         .await
         .with_context(|| format!("reading {secret_path} from the store"))?;
 
-    write_token(&token_path(&agent, &notice.account), &value)
+    write_token(&token_path(&agent, &notice.account)?, &value)
 }
 
 /// Write `value` to `dest` at `0600`, atomically.
@@ -103,7 +110,8 @@ mod tests {
         // The other end of this agreement is an assertion in
         // `nix/agent-modules/matrix.nix` and a `systemd.paths` glob — neither
         // reachable from a Rust test, so the prefix is pinned here.
-        let p = token_path(&Ident::parse("dmatrix").expect("a legal agent name"), "ccc");
+        let p = token_path(&Ident::parse("dmatrix").expect("a legal agent name"), "ccc")
+            .expect("a legal account name");
         let name = p.file_name().unwrap().to_str().unwrap();
         assert!(name.starts_with("matrix-token"), "got {name}");
         assert_eq!(name, "matrix-token-ccc");
@@ -113,7 +121,8 @@ mod tests {
     fn the_partial_file_cannot_match_that_glob() {
         // A temp name starting with `matrix-token` would be picked up
         // mid-write; the dot prefix is what stops it.
-        let p = token_path(&Ident::parse("dmatrix").expect("a legal agent name"), "ccc");
+        let p = token_path(&Ident::parse("dmatrix").expect("a legal agent name"), "ccc")
+            .expect("a legal account name");
         let name = p.file_name().unwrap().to_str().unwrap();
         let tmp = format!(".{name}.partial");
         assert!(!tmp.starts_with("matrix-token"), "got {tmp}");
@@ -132,8 +141,7 @@ mod tests {
     fn an_agent_name_off_the_queue_must_pass_the_ident_parser_too() {
         // Two independent refusals, not one restated: `path::matrix_account`
         // guards the address in the *store*, `Ident` guards the address on
-        // *disk*. `token_path` cannot even be called without the second,
+        // *disk*, and `token_path` cannot be called without the second.
-        // which is why it takes an `Ident` rather than validating internally.
         for bad in ["../argus", "dmatrix/../argus", "Dmatrix", "d matrix", ""] {
             assert!(Ident::parse(bad).is_err(), "{bad:?} must be refused");
         }
@@ -141,4 +149,26 @@ mod tests {
         // satisfy the loop above.
         assert!(Ident::parse("dmatrix").is_ok());
     }
+
+    /// The account half of that same rule, which the agent half's type does
+    /// not cover: an account name is an attr name in `hyperhive.matrixAccounts`
+    /// and so has no newtype to lean on. Without this, a second caller reaching
+    /// `token_path` without going through `path::matrix_account` first would
+    /// build a filename out of an unchecked name.
+    #[test]
+    fn an_account_name_off_the_queue_is_refused_for_the_disk_path_too() {
+        let agent = Ident::parse("dmatrix").expect("a legal agent name");
+        for bad in ["../argus", "a/b", "a b", "a.b", ""] {
+            assert!(
+                token_path(&agent, bad).is_err(),
+                "account {bad:?} must be refused"
+            );
+        }
+        // Controls: the legal charset stays reachable, so the loop above is
+        // not passing because everything is refused. Uppercase and underscore
+        // are deliberate — `matrixAccounts` is an attrset, so both are names
+        // an operator can already write.
+        assert!(token_path(&agent, "ops-relay").is_ok());
+        assert!(token_path(&agent, "Ops_Relay9").is_ok());
+    }
 }

swarm-secret-client/src/path.rs

@@ -23,7 +23,17 @@ pub const AGENT_PREFIX: &str = "swarm/agents";
 /// out of the prefix entirely. Both are names this crate receives from
 /// elsewhere — an agent name from the topology, an account name from an
 /// agent's own config — so neither is trusted to be well-formed here.
-fn checked_segment(kind: &'static str, value: &str) -> Result<(), Error> {
+///
+/// Public because the same name is also used to build a path **on disk**, and
+/// that guard must accept exactly what this one does. Two copies of a charset
+/// are two charsets: they agree until one is edited, and the day they diverge
+/// is the day a name is legal in the store and not on the filesystem, or the
+/// reverse.
+///
+/// # Errors
+/// [`Error::PathSegment`] when `value` is empty or holds anything outside
+/// `[A-Za-z0-9_-]`.
+pub fn checked_segment(kind: &'static str, value: &str) -> Result<(), Error> {
     if value.is_empty() {
         return Err(Error::PathSegment {
             kind,