diff --git a/crates/cli/skill/SKILL.md b/crates/cli/skill/SKILL.md index 2006280c..a21ed03b 100644 --- a/crates/cli/skill/SKILL.md +++ b/crates/cli/skill/SKILL.md @@ -95,7 +95,7 @@ Some commands act for the user. Run them only when the user asks, and pass `--wh | Replace a key that still works | `tokenstash rotate NAME` | | Delete a stored key | `tokenstash forget NAME` | | Always use another identity for a key in this project | `tokenstash bind NAME --identity work` | -| Ask again for a key they declined | `tokenstash need NAME --force` (once per key until the "no" expires) | +| Ask again for a key they declined | `tokenstash need NAME --force` (once per key and identity until the "no" expires) | | Stop agents loading this skill on their own, or let them again | `tokenstash init --mode explicit`, `tokenstash init --mode auto` | | Register or remove the MCP server | `tokenstash init --mcp`, `tokenstash init --no-mcp` | | Take tokenstash out of their agents | `tokenstash init --undo` | diff --git a/crates/cli/skill/reference.md b/crates/cli/skill/reference.md index 3c227076..403a84bc 100644 --- a/crates/cli/skill/reference.md +++ b/crates/cli/skill/reference.md @@ -30,7 +30,7 @@ Writes each key to the project's env file, or files a card for it. The project i | `--blocking` | Wait for the user instead of returning at once | | `--timeout SECONDS` | With `--blocking`, how long to wait (default 600) | | `--agent NAME` | Your name on the card and in the audit log (detected otherwise) | -| `--force` | Ask again after the user declined. Once per key and project until the "no" expires; the card says it is a second ask | +| `--force` | Ask again after the user declined. Once per key, identity and project until the "no" expires; the card says it is a second ask, and the same command run again while that card waits returns it | | `--json` | One JSON object instead of lines | Generated secrets (`AUTH_SECRET`, `JWT_SECRET`, `SESSION_SECRET`, `NEXTAUTH_SECRET`, `ENCRYPTION_KEY`) are never asked for: tokenstash generates one per project, or keeps the value the env file already holds. @@ -95,7 +95,9 @@ Runs COMMAND with the env file loaded into its environment. Its output reaches y ### `tokenstash doctor` -Checks the setup, one line each: config file, stash backend (and how long keys last in it), database, provider registry, inbox, agent mode, the skill and any MCP registration per agent, this directory (refused, paired, or not yet), and the binary path. Exit 0 when nothing is wrong, 1 otherwise. Safe to run at any time. +Checks the setup, one line each: config file, stash backend (and how long keys last in it), older copies (Linux kernel keyring only), database, provider registry, inbox, agent mode, the skill and any MCP registration per agent, this directory (refused, paired, or not yet), and the binary path. Exit 0 when nothing is wrong, 1 otherwise. Safe to run at any time. + +The older copies line looks at every key this home's index lists and every key the kernel keyring holds for this home, for copies a tokenstash older than 0.4 keeps. It fails when a key's user keyring and persistent keyring hold different values, because a read can then return the old key. It passes with a note when only another login session holds a copy, which matters only while an older tokenstash runs in that session. When `/proc/keys` cannot be read, it says so and looks only at the keys the index lists. ### `tokenstash remote [tailscale [--login LOGIN] | off]` diff --git a/crates/cli/skill/troubleshooting.md b/crates/cli/skill/troubleshooting.md index 222a1753..e16bb74c 100644 --- a/crates/cli/skill/troubleshooting.md +++ b/crates/cli/skill/troubleshooting.md @@ -47,7 +47,7 @@ Unless remote access is on, the inbox listens on `127.0.0.1` of the machine toke Follow "When a provider rejects a key" in `SKILL.md`: `tokenstash report-bad NAME --status 401`, then `tokenstash need NAME`. If the second `need` writes the key again, the provider accepted it on re-check: compare your request with the provider's documentation (auth header name and format, base URL, project or organization header). -If the user pasted a new key and the old, rejected one keeps coming back, run `tokenstash doctor` and look at the stash backend line. With `keyutils` (the Linux kernel keyring, used when no Secret Service runs), versions before 0.4 kept a separate copy per login session, so a key pasted from one session could be shadowed in another; `doctor`'s last line shows which binary runs. Tell the user what you found. +If the user pasted a new key and the old, rejected one keeps coming back, run `tokenstash doctor` and look at the stash backend line. With `keyutils` (the Linux kernel keyring, used when no Secret Service runs), versions before 0.4 kept a separate copy per login session, so a key pasted from one session could be shadowed in another; `doctor`'s last line shows which binary runs. An older tokenstash still running in another login session (an inbox or `tokenstash mcp` started before the upgrade) keeps doing this, and this version cannot fully prevent it. No other session can write the copy that process holds. When the process links its copy into the shared keyring, the kernel does not record whether it read an old value or wrote a new one, so this version keeps the value it had, and a key pasted through the old process can be passed over. The `older copies` line of `doctor` then names the keys. It fails when a key's user keyring and persistent keyring hold different values, which is when a read can return the old key. When another login session only holds a copy, the line passes, since the copy matters only while an older tokenstash runs in that session. Tell the user what you found. They stop the older process, and if the old key is still the one in use, replace it with `tokenstash rotate NAME`. ## `need` exits 1 diff --git a/crates/cli/src/cmd/actions.rs b/crates/cli/src/cmd/actions.rs index 74f49b8f..8a5fb53f 100644 --- a/crates/cli/src/cmd/actions.rs +++ b/crates/cli/src/cmd/actions.rs @@ -7,7 +7,7 @@ use crate::notify; use crate::util::{self, App}; use anyhow::{bail, Result}; use std::path::Path; -use tokenstash_core::actions::{self, Action}; +use tokenstash_core::actions::{self, Action, Forgot}; use tokenstash_core::config::AgentMode; use tokenstash_core::db::Task; @@ -32,9 +32,10 @@ pub fn print_pending(app: &App, project: &Path, agent: &str, t: &Task) -> Result /// Person side, from the inbox's full session or the person's terminal: claim the card, do /// what it says, then close it. The claim means a second confirm (another tab, a double click) -/// runs nothing: an old forget card confirmed again would delete a key stored since. The card -/// stays pending until the action has run, so if this process stops half way the claim runs -/// out and the person can confirm it again; if the action fails, the claim is given back. +/// runs nothing: an old forget card confirmed again would delete a key stored since. The claim +/// is renewed while the action runs, so a slow one keeps its card however long it takes. The +/// card stays pending until the action has run, so if this process stops half way the claim +/// runs out and the person can confirm it again; if the action fails, the claim is given back. pub fn confirm(app: &App, task: &Task, action: &Action) -> Result { use tokenstash_core::db::TaskStatus; let Some(claim) = app.db.claim_action(&task.id)? else { @@ -43,11 +44,16 @@ pub fn confirm(app: &App, task: &Task, action: &Action) -> Result { _ => bail!("this card was already answered"), } }; - match perform(app, task, action) { + // A keeper that cannot start gives the claim back before it returns the error. + let kept = app.db.keep_claim(&task.id, &claim)?; + let ran = perform(app, task, action, &claim); + drop(kept); + match ran { Ok(done) => { if !app.db.finish_action(&task.id, &claim, &done)? { - // Ran past the claim's time and another confirm took over: the change is made, - // and the card is the other confirm's to close. + // The claim went unrenewed for its whole time while this ran (the machine + // slept, or the index stayed locked) and another confirm took over: the change + // is made, and the card is the other confirm's to close. return Ok(format!("{done} (another confirm took this card over meanwhile)")); } app.db.audit(Some(&task.project), Some(&task.agent), "action.confirmed", None, None, Some(&task.expects))?; @@ -60,12 +66,14 @@ pub fn confirm(app: &App, task: &Task, action: &Action) -> Result { } } -fn perform(app: &App, task: &Task, action: &Action) -> Result { +fn perform(app: &App, task: &Task, action: &Action, claim: &str) -> Result { match action { - Action::Forget { name, identity } => Ok(if forget_key(app, name, identity)? { - format!("Forgot {name}@{identity}") - } else { - format!("Nothing was stored for {name}@{identity}") + Action::Forget { name, identity } => Ok(match actions::forget(&app.ctx(), name, identity, Some((&task.id, claim)))? { + Forgot::Deleted => format!("Forgot {name}@{identity}"), + Forgot::NothingStored => format!("Nothing was stored for {name}@{identity}"), + Forgot::Gone { kept: false } => format!("Forgot {name}@{identity} (an earlier confirm of this card had deleted it)"), + Forgot::Gone { kept: true } => format!("Kept {name}@{identity}: it was stored again after this card was first confirmed, so it is not the key this card was about"), + Forgot::KeptUnrecorded => format!("A value is still stored under {name}@{identity}. This card cannot tell it from one stored after the card was first confirmed, so it was kept. A new forget card removes it"), }), Action::Bind { name, identity } => { let project = Path::new(&task.project); @@ -99,10 +107,7 @@ fn perform(app: &App, task: &Task, action: &Action) -> Result { } } -/// Delete a stored key and its index row. True if either existed. +/// Delete a stored key and its index row, for a person at a terminal. True if either existed. pub fn forget_key(app: &App, name: &str, identity: &str) -> Result { - let had = app.stash.delete(&tokenstash_core::stash::stash_key(name, identity))?; - let meta = app.db.delete_secret(name, identity)?; - app.db.audit(None, None, "forget", Some(name), Some(identity), None)?; - Ok(had || meta) + Ok(actions::forget(&app.ctx(), name, identity, None)? == Forgot::Deleted) } diff --git a/crates/cli/src/cmd/admin.rs b/crates/cli/src/cmd/admin.rs index 798b40e5..8fc08eef 100644 --- a/crates/cli/src/cmd/admin.rs +++ b/crates/cli/src/cmd/admin.rs @@ -296,7 +296,7 @@ pub fn audit(a: AuditArgs) -> Result { } else { let project = tokenstash_core::project::current(); match app.db.find_workspace(&project)? { - Some(ws) => app.db.recent_audit_for(&project.to_string_lossy(), &ws.created, a.limit)?, + Some(ws) => app.db.recent_audit_for(&project.to_string_lossy(), &ws, a.limit)?, None => vec![], } }; diff --git a/crates/cli/src/cmd/doctor.rs b/crates/cli/src/cmd/doctor.rs index 5e3a08ac..174b5ad2 100644 --- a/crates/cli/src/cmd/doctor.rs +++ b/crates/cli/src/cmd/doctor.rs @@ -25,6 +25,40 @@ pub fn doctor() -> Result { (Ok(()), None) => String::new(), }; ok &= check("stash backend", probe.is_ok(), format!("{}{note}", s.backend())); + // With the kernel keyring, an older tokenstash keeps its own copy of a key in each + // login session it ran in, and while it runs it can put an old value back. + let keys: Vec = Db::open_default() + .and_then(|db| db.list_secrets()) + .map(|v| v.iter().map(|m| tokenstash_core::stash::stash_key(&m.name, &m.identity)).collect()) + .unwrap_or_default(); + // One line whenever the backend has copies to look for, so a missing line never + // stands for a check that found nothing. + if let Some(found) = s.stray_copies(&keys) { + let mut detail = vec![]; + // Two keyrings that disagree are a fault, because a read can return the old + // key now. + let differ: Vec<&str> = found.copies.iter().filter(|c| c.differs).map(|c| c.key.as_str()).collect(); + if !differ.is_empty() { + detail.push(format!( + "The user keyring and the persistent keyring hold different values for {}, so a read can return the old key. tokenstash 0.3.0 or earlier, still running in another login session (an inbox or `tokenstash mcp` started before the upgrade), put the other value there. Stop it, then replace the key with `tokenstash rotate NAME` if the old one is in use.", + differ.join(", ") + )); + } + // A copy only another session holds is a note. It does nothing until an older + // tokenstash runs there, and an upgrade leaves idle sessions holding them. + let held: Vec<&str> = found.copies.iter().filter(|c| c.elsewhere > 0).map(|c| c.key.as_str()).collect(); + if !held.is_empty() { + detail.push(format!( + "Other login sessions hold their own copy of {}, left by tokenstash 0.3.0 or earlier. It matters only while an older tokenstash still runs in one of them (an inbox or `tokenstash mcp` started before the upgrade), because it could put the old key back. Stop it, or end that session.", + held.join(", ") + )); + } + if detail.is_empty() { + detail.push(format!("None among the {} key(s) checked.", found.checked)); + } + detail.extend(found.limited); + ok &= check("older copies", differ.is_empty(), detail.join(" ")); + } } Err(e) => { ok &= check("stash backend", false, e.to_string()); } } diff --git a/crates/cli/src/cmd/inbox.rs b/crates/cli/src/cmd/inbox.rs index 77d46d7a..33f9e874 100644 --- a/crates/cli/src/cmd/inbox.rs +++ b/crates/cli/src/cmd/inbox.rs @@ -337,13 +337,8 @@ fn handle(app: &App, req: Request, tokens: &inbox_auth::Tokens) -> Result<()> { // send it to them the one way the agent cannot read, the desktop. Anyone with // the link can press this; all it does is notify the person again. (_, "notify") => send_full_link(app, &task), - (kind, "deny") => { - // An approval card is the human's decision either way: closing it from - // the agent's link would let the agent bury its own pairing card for a - // day, or another agent's in the same directory. - if kind == TaskKind::Approval && scope != Scope::Full { - anyhow::bail!("closing an approval card needs your own inbox link: use the one in the desktop notification (the button below sends it again) and decide on this card there"); - } + // An approval card's Deny is decided below, with Allow. + (kind, "deny") if kind != TaskKind::Approval => { tasks::deny(&ctx, &task, form.get("note").map(|s| s.as_str()))?; Ok(format!("Denied {}", task.title)) } @@ -371,19 +366,28 @@ fn handle(app: &App, req: Request, tokens: &inbox_auth::Tokens) -> Result<()> { (TaskKind::Approval, _) => { // Approving is the one thing a card session must not do: it is the // human's yes to "this project may use my key", and the agent's link - // must not be able to give it. Refuse with no state change. + // must not be able to give it. Refuse with no state change. Closing the card + // is the human's decision too: from the agent's link it would let the agent + // bury its own pairing card for a day, or another agent's in the same + // directory. if scope != Scope::Full { + if action == "deny" { + anyhow::bail!("closing an approval card needs your own inbox link: use the one in the desktop notification (the button below sends it again) and decide on this card there"); + } anyhow::bail!("approving needs your own inbox link: use the one in the desktop notification (the button below sends it again) and approve this card there"); } let decision = match action.as_str() { "allow" => tasks::Decision::Allow, "allow_broad" => tasks::Decision::AllowBroad, _ => tasks::Decision::Deny }; // What the page listed when it was rendered: a card that grew since - // (an agent asked for more) is refused and re-read. + // (an agent asked for more) is refused and re-read. That holds for Deny + // too, which closes the card and every key on it: a key added after the + // page loaded is not declined unseen. The comparison and the close happen + // under the index write lock in `answer_approval`. // The browser form always carries `seen`; a POST without it (a // person scripting `curl`) is judged on the card as it is now. let seen: Option> = form.get("seen").map(|s| s.split(',').filter(|x| !x.is_empty()).map(String::from).collect()); match tasks::answer_approval(&ctx, &task, decision, seen.as_deref())? { AnswerResult::Approved { injected, replaced } => Ok(format!("Approved; injected {}{}", if injected.is_empty() { "nothing new".into() } else { injected.join(", ") }, if replaced.is_empty() { String::new() } else { format!(". {} rejected by the provider at delivery — a Replace card is waiting", replaced.join(", ")) })), - _ => Ok("Denied".into()), + _ => Ok(format!("Denied {}", task.title)), } } (TaskKind::Human, _) => match tokenstash_core::actions::Action::of(&task) { diff --git a/crates/cli/src/cmd/init.rs b/crates/cli/src/cmd/init.rs index f8dece29..97f6477d 100644 --- a/crates/cli/src/cmd/init.rs +++ b/crates/cli/src/cmd/init.rs @@ -394,7 +394,7 @@ fn undo_with(m: Manifest, claude_cli: bool, home: &Path) -> Result { cur.save()?; } if cur.claude_mcp_registered { - let ok = claude_cli && claude_mcp(&["remove", "-s", "user", "tokenstash"]); + let ok = claude_cli && claude_mcp(&["remove", "-s", "user", "tokenstash"])?; if ok { println!("✓ claude mcp remove tokenstash"); cur.claude_mcp_registered = false; cur.save()?; } else { println!("! could not run `claude mcp remove -s user tokenstash` (kept in the manifest; run it by hand or re-run --undo with `claude` on PATH)"); } @@ -667,9 +667,33 @@ fn reinsert(r: &Removed) -> Result<()> { Ok(()) } -fn claude_mcp(args: &[&str]) -> bool { - std::process::Command::new("claude").arg("mcp").args(args) - .stdout(std::process::Stdio::null()).stderr(std::process::Stdio::null()).status().map(|s| s.success()).unwrap_or(false) +/// How long `claude mcp add` or `claude mcp remove` may run. An action card that runs one +/// holds its claim meanwhile, so a hung `claude` must not hold it for as long as the inbox runs. +const CLAUDE_MCP_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(60); + +/// Run `claude mcp ARGS`. True if it succeeded, false if it failed or could not start, and an +/// error if it ran past [`CLAUDE_MCP_TIMEOUT`]: the child is killed, and a confirm that ran it +/// gives its card back. +fn claude_mcp(args: &[&str]) -> Result { + claude_mcp_with(Path::new("claude"), args, CLAUDE_MCP_TIMEOUT) +} + +fn claude_mcp_with(claude: &Path, args: &[&str], limit: std::time::Duration) -> Result { + let Ok(mut child) = std::process::Command::new(claude).arg("mcp").args(args) + .stdout(std::process::Stdio::null()).stderr(std::process::Stdio::null()).spawn() else { return Ok(false) }; + // The output goes nowhere, so no pipe can fill and stall the child: polling is enough. + let deadline = std::time::Instant::now() + limit; + loop { + if let Some(s) = child.try_wait()? { + return Ok(s.success()); + } + if std::time::Instant::now() >= deadline { + let _ = child.kill(); + let _ = child.wait(); + anyhow::bail!("`claude mcp {}` did not finish within {limit:?} and was stopped", args.join(" ")); + } + std::thread::sleep(std::time::Duration::from_millis(20)); + } } @@ -822,7 +846,9 @@ fn register_mcp(manifest: &mut Manifest, w: &Wiring) -> Result> { let mut args: Vec = vec!["add".into(), "-s".into(), "user".into()]; if let Some(h) = &w.ts_home { args.push("-e".into()); args.push(format!("TOKENSTASH_HOME={h}")); } args.extend(["tokenstash".into(), "--".into(), w.exe.clone(), "mcp".into()]); - let ok = claude_mcp(&args.iter().map(String::as_str).collect::>()); + // A time-out is an error and keeps the record: the stopped command may have + // registered the server, and a record can be undone. + let ok = claude_mcp(&args.iter().map(String::as_str).collect::>())?; if !ok { manifest.claude_mcp_registered = false; manifest.save()?; } ok } else { @@ -2279,4 +2305,32 @@ mod tests { assert_eq!(undo_with(m, false, &w.home).unwrap(), 0); assert_eq!(read(&p), theirs, "their entry stays"); } + + /// A `claude mcp` that hangs is stopped at the time limit and returns an error, so an + /// action card that runs it is given back and the person can decline it, instead of + /// staying claimed for as long as the inbox runs. One that finishes reports how it exited. + #[cfg(unix)] + #[test] + fn a_hung_claude_mcp_is_stopped_at_the_time_limit() { + use std::os::unix::fs::PermissionsExt; + use std::time::{Duration, Instant}; + let dir = scratch("hung-claude"); + let fake = |name: &str, script: &str| { + let bin = dir.join(name).join("claude"); + fs::create_dir_all(bin.parent().unwrap()).unwrap(); + fs::write(&bin, script).unwrap(); + fs::set_permissions(&bin, fs::Permissions::from_mode(0o755)).unwrap(); + bin + }; + let hung = fake("hung", "#!/bin/sh\nexec sleep 30\n"); + let limit = Duration::from_millis(300); + let started = Instant::now(); + let err = claude_mcp_with(&hung, &["add", "-s", "user", "tokenstash"], limit).unwrap_err(); + assert!(started.elapsed() < limit + Duration::from_secs(2), "{:?}", started.elapsed()); + assert!(format!("{err:#}").contains("did not finish"), "{err:#}"); + assert!(claude_mcp_with(&fake("ok", "#!/bin/sh\nexit 0\n"), &["remove"], limit).unwrap()); + assert!(!claude_mcp_with(&fake("fails", "#!/bin/sh\nexit 1\n"), &["remove"], limit).unwrap()); + assert!(!claude_mcp_with(&dir.join("missing/claude"), &["remove"], limit).unwrap(), "one that cannot start is a failure, as before"); + let _ = fs::remove_dir_all(&dir); + } } diff --git a/crates/cli/src/cmd/mcp.rs b/crates/cli/src/cmd/mcp.rs index c7ba6555..1a8e917f 100644 --- a/crates/cli/src/cmd/mcp.rs +++ b/crates/cli/src/cmd/mcp.rs @@ -418,7 +418,7 @@ fn call(params: &Value, agent: &str, bound: &std::path::Path) -> Result<(Value, for s in &specs { let opts = NeedOpts { req: SecretRequest { why: s.why.clone(), url: s.url.clone(), steps: s.steps.clone(), pattern: s.pattern.clone() }, - identity: s.identity.clone(), blocking: false, timeout, force: false, require_approval: false, ask_again: false, + identity: s.identity.clone(), blocking: false, timeout, force: false, require_approval: false, ask_again: vec![], }; results.extend(need::need_with_budget(&app.ctx(), &project, agent, std::slice::from_ref(&s.name), &opts, &mut budget)?); } diff --git a/crates/cli/src/cmd/need.rs b/crates/cli/src/cmd/need.rs index be780713..c4a363a8 100644 --- a/crates/cli/src/cmd/need.rs +++ b/crates/cli/src/cmd/need.rs @@ -3,6 +3,7 @@ use crate::util::{self, App}; use anyhow::Result; use clap::Args; use std::time::Duration; +use tokenstash_core::db::TaskStatus; use tokenstash_core::exit; use tokenstash_core::need::{self, NeedOpts, Outcome}; use tokenstash_core::tasks::{self, HumanRequest, SecretRequest}; @@ -51,56 +52,69 @@ pub fn need(a: NeedArgs) -> Result { let project = util::project_from(&None); let agent = util::agent_from(&a.agent); // "Denied" is remembered for a day so a program failing in a loop cannot nag. An agent may - // ask again over it once per key and project in that window, when the user tells it to: - // the card says it is a second ask, and a second "no" stands for the rest of the window. - let mut why = a.why.clone(); + // ask again over it once per key, identity and project in that window, when the user tells + // it to. The card says it is a second ask, and a second "no" stands for the rest of the + // window. A person's own --force sets every "no" aside. + let human = util::looks_human(); let pid = project.to_string_lossy().to_string(); - // Only a key the person declined here is asked "again"; for any other, --force from an - // agent is an ordinary request and spends nothing. - let mut denied: Vec = vec![]; - if a.force && !util::looks_human() { + // Only a key the person declined here is asked "again", in the identity they declined; + // for any other, --force from an agent is an ordinary request and spends nothing. + let mut denied: Vec<(String, String)> = vec![]; + if a.force && !human { for name in &a.names { - if tokenstash_core::need::denied_here(&app.ctx(), &project, name, a.identity.as_deref())? { - denied.push(name.clone()); + if let Some(identity) = need::denied_here(&app.ctx(), &project, name, a.identity.as_deref())? { + denied.push((name.clone(), identity)); } } } - let ask_again = !denied.is_empty(); // The one extra ask is reserved before the card is filed, in one step, so two requests // at once cannot both take it; one that ends without a card for the key hands it back. - let mut reserved: Vec<(String, i64)> = vec![]; - if ask_again { - let since = app.cfg.ttl_since(); - for name in &denied { - match app.db.reserve_force(&pid, &agent, name, &since)? { - Some(row) => reserved.push((name.clone(), row)), - None => { - for (_, row) in &reserved { app.db.delete_audit_row(*row)?; } - anyhow::bail!("{name} was already asked for again once after the user declined it here; that answer stands for {} hours from the first no. Tell the user; do not ask again", app.cfg.task_ttl_hours); - } + // An ask already spent on a card that still waits returns that card, because running the + // same command again is how an agent checks on it. + let since = app.cfg.ttl_since(); + let mut reserved: Vec<(String, String, i64)> = vec![]; + let mut waiting: Vec = vec![]; + for (name, identity) in &denied { + if let Some(row) = app.db.reserve_force(&pid, &agent, name, identity, &since)? { + reserved.push((name.clone(), identity.clone(), row)); + continue; + } + app.db.expire_overdue()?; + let card = match app.db.force_card(&pid, name, identity, &since)? { + Some(id) => app.db.get_task(&id)?, + None => None, + }; + match card { + Some(t) if t.status == TaskStatus::Pending => { + waiting.push(Outcome::Pending { name: name.clone(), identity: identity.clone(), task_id: t.id, title: t.title, url: t.url }); + } + // The person said yes to the second ask, so this is an ordinary request now. + Some(t) if t.status == TaskStatus::Answered => {} + _ => { + for (_, _, row) in &reserved { app.db.delete_audit_row(*row)?; } + anyhow::bail!("{name} was already asked for again once after the user declined it here; that answer stands for {} hours from the first no. Tell the user; do not ask again", app.cfg.task_ttl_hours); } } - why = Some(match why { - Some(w) => format!("Asked again after you declined, because you asked {agent} to. {w}"), - None => format!("Asked again after you declined, because you asked {agent} to."), - }); } + let rest: Vec = a.names.iter().filter(|n| !waiting.iter().any(|o| o.name() == n.as_str())).cloned().collect(); let opts = NeedOpts { - req: SecretRequest { why, url: a.url.clone(), steps: a.steps.clone(), pattern: a.pattern.clone() }, + req: SecretRequest { why: a.why.clone(), url: a.url.clone(), steps: a.steps.clone(), pattern: a.pattern.clone() }, identity: a.identity.clone(), blocking: false, timeout: Duration::from_secs(a.timeout), - force: a.force, + // An agent's --force sets aside only the "no" to each key it reserved the extra ask + // for. A "no" to another key still stands, even one given after the check above. + force: a.force && human, require_approval: false, - ask_again, + ask_again: reserved.iter().map(|(name, identity, _)| format!("{name}@{identity}")).collect(), }; - let outcomes = need::need(&app.ctx(), &project, &agent, &a.names, &opts); - let mut outcomes = match outcomes { + let filed = if rest.is_empty() { Ok(vec![]) } else { need::need(&app.ctx(), &project, &agent, &rest, &opts) }; + let filed = match filed { Ok(o) => o, Err(e) => { // A request that failed part way may already have filed a card for some keys: // those asks are spent; the rest are handed back. - for (name, row) in &reserved { + for (name, _, row) in &reserved { match app.db.card_since_reservation(*row, &pid, name)? { Some(tid) => app.db.bind_force(*row, &tid)?, None => app.db.delete_audit_row(*row)?, @@ -111,12 +125,21 @@ pub fn need(a: NeedArgs) -> Result { }; // The one extra ask is spent only by a request that filed a card for the key: a request // that failed, or found the key already allowed, leaves it for later. - for (name, row) in &reserved { - match outcomes.iter().find_map(|o| match o { Outcome::Pending { name: n, task_id, .. } if n == name => Some(task_id.clone()), _ => None }) { + for (name, identity, row) in &reserved { + match filed.iter().find_map(|o| match o { Outcome::Pending { name: n, identity: i, task_id, .. } if n == name && i == identity => Some(task_id.clone()), _ => None }) { Some(tid) => app.db.bind_force(*row, &tid)?, None => app.db.delete_audit_row(*row)?, } } + // One result per name, in the order asked. + let mut filed = filed.into_iter(); + let mut outcomes: Vec = Vec::with_capacity(a.names.len()); + for name in &a.names { + match waiting.iter().position(|o| o.name() == name.as_str()) { + Some(i) => outcomes.push(waiting.remove(i)), + None => outcomes.extend(filed.next()), + } + } if outcomes.iter().any(|o| o.is_pending()) { notify_pending(&app, &project, &agent, &outcomes); diff --git a/crates/cli/tests/human_only.rs b/crates/cli/tests/human_only.rs index c4729838..ece15be8 100644 --- a/crates/cli/tests/human_only.rs +++ b/crates/cli/tests/human_only.rs @@ -135,6 +135,82 @@ fn an_agent_asks_on_a_card_and_nothing_changes_until_the_person_answers() { assert!(!out.status.success() && String::from_utf8_lossy(&out.stderr).contains("already asked for again once"), "{}", String::from_utf8_lossy(&out.stderr)); } +/// Have the person decline this directory's open card for `name`. +fn decline(home: &PathBuf, cwd: &PathBuf, name: &str, identity: &str) { + let card = tasks_json(home, cwd).into_iter().find(|t| t["name"] == name && t["identity"] == identity && t["status"] == "pending").unwrap(); + assert!(run(home, cwd, &["answer", card["id"].as_str().unwrap(), "--deny"]).status.success()); +} + +/// Greptile on #63: in one `need A B --force`, only the key the person declined says on its +/// card that it is a second ask. The other key was never declined, so its card is an ordinary +/// first ask with the agent's own reason. +#[test] +fn only_the_declined_key_is_called_a_second_ask() { + let home = home("again-label"); + let proj = tmp("again-label-proj"); + assert_eq!(run(&home, &proj, &["need", "RESEND_API_KEY"]).status.code(), Some(10)); + decline(&home, &proj, "RESEND_API_KEY", "default"); + let out = run(&home, &proj, &["need", "RESEND_API_KEY", "GROQ_API_KEY", "--force", "--why", "the signup emails"]); + assert_eq!(out.status.code(), Some(10), "{}", String::from_utf8_lossy(&out.stderr)); + let cards = tasks_json(&home, &proj); + let again = cards.iter().find(|t| t["name"] == "RESEND_API_KEY" && t["status"] == "pending").unwrap(); + assert!(again["why"].as_str().unwrap().starts_with("Asked again after you declined") && again["why"].as_str().unwrap().ends_with("the signup emails"), "{again}"); + let groq = cards.iter().find(|t| t["name"] == "GROQ_API_KEY").unwrap(); + assert_eq!(groq["why"], "the signup emails", "{groq}"); +} + +/// Greptile on #65: running the same `need NAME --force` again while its second-ask card waits +/// returns that card, with its `next`, as an ordinary pending result. Once the person answers +/// it, the same command delivers the key. +#[test] +fn asking_again_twice_returns_the_waiting_card() { + let home = home("again-repeat"); + let proj = tmp("again-repeat-proj"); + assert_eq!(run(&home, &proj, &["need", "RESEND_API_KEY"]).status.code(), Some(10)); + decline(&home, &proj, "RESEND_API_KEY", "default"); + let ask = |extra: &[&str]| { + let mut args = vec!["need", "RESEND_API_KEY", "--force", "--json"]; + args.extend(extra); + let out = run(&home, &proj, &args); + let v: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap_or_else(|_| panic!("{}", String::from_utf8_lossy(&out.stderr))); + (out.status.code(), v) + }; + let (code, first) = ask(&[]); + assert_eq!(code, Some(10), "{first}"); + let id = first["results"][0]["task_id"].as_str().unwrap().to_string(); + let (code, repeat) = ask(&[]); + assert_eq!(code, Some(10), "{repeat}"); + let r = &repeat["results"][0]; + assert_eq!((r["status"].as_str(), r["task_id"].as_str()), (Some("pending"), Some(id.as_str())), "{repeat}"); + assert!(r["next"].as_str().unwrap().contains(&id) && r["inbox"].is_string(), "{repeat}"); + // With another key in the same call, each result keeps its place and the other key is asked. + let out = run(&home, &proj, &["need", "GROQ_API_KEY", "RESEND_API_KEY", "--force", "--json"]); + let v: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap(); + assert_eq!((v["results"][0]["name"].as_str(), v["results"][1]["task_id"].as_str()), (Some("GROQ_API_KEY"), Some(id.as_str())), "{v}"); + assert_eq!(tasks_json(&home, &proj).iter().filter(|t| t["name"] == "RESEND_API_KEY").count(), 2, "no third card"); + paste(&home, &proj, &id, "re_askedagain_0123456789abcdef"); + let (code, done) = ask(&[]); + assert_eq!((code, done["results"][0]["status"].as_str()), (Some(0), Some("injected")), "{done}"); +} + +/// Greptile on #67: the person declines each identity of a key on its own card, and an agent +/// may ask again once for each. +#[test] +fn each_declined_identity_gets_its_own_ask_again() { + let home = home("again-identity"); + let proj = tmp("again-identity-proj"); + for identity in ["work", "personal"] { + assert_eq!(run(&home, &proj, &["need", "GROQ_API_KEY", "--identity", identity]).status.code(), Some(10)); + decline(&home, &proj, "GROQ_API_KEY", identity); + } + for identity in ["work", "personal"] { + let out = run(&home, &proj, &["need", "GROQ_API_KEY", "--identity", identity, "--force"]); + assert_eq!(out.status.code(), Some(10), "{identity}: {}", String::from_utf8_lossy(&out.stderr)); + let card = tasks_json(&home, &proj).into_iter().find(|t| t["name"] == "GROQ_API_KEY" && t["identity"] == identity && t["status"] == "pending").unwrap(); + assert!(card["why"].as_str().unwrap().starts_with("Asked again after you declined"), "{card}"); + } +} + /// `list` and `audit` show an agent its own directory and nothing else: the rest of the stash /// and the log are the person's inventory. #[test] diff --git a/crates/cli/tests/inbox_scope.rs b/crates/cli/tests/inbox_scope.rs index a1a212f3..fbbf01f6 100644 --- a/crates/cli/tests/inbox_scope.rs +++ b/crates/cli/tests/inbox_scope.rs @@ -458,3 +458,39 @@ fn an_action_card_runs_only_on_the_persons_confirm() { assert!(std::fs::read_to_string(home.join("user-home/.codex/config.toml")).unwrap().contains("[mcp_servers.tokenstash]")); inbox.stop(); } + +/// Greptile on #65: Deny on an approval card goes through the same locked comparison with +/// what the page showed as Allow does. A card that grew after the page loaded is refused and +/// stays open, so a key the agent added since is not declined unseen. +#[test] +fn denying_an_approval_card_that_grew_since_the_page_loaded_is_refused() { + let port = free_port(); + let home = home("deny-grown-home", port); + let proj_a = tmp("deny-grown-a"); + let proj_b = tmp("deny-grown-b"); + let inbox = Inbox::start(&home, port); + for (name, value) in [("OPENAI_API_KEY", "sk-proj-denygrown0123456789abcdef"), ("GROQ_API_KEY", "gsk_denygrown0123456789abcdefghij")] { + let need = run(&home, &proj_a, &["need", name, "--agent", "ci"]); + let (path, cred) = card_link(&need, port); + assert_eq!(http(port, "POST", &path, &[("tokenstash_card", cred.as_str())], Some(&format!("value={value}&skip_check=1&t={cred}"))).0, 303, "{name}"); + } + // B asks for one of them: a pairing card, which the person opens. + assert_eq!(run(&home, &proj_b, &["need", "OPENAI_API_KEY", "--agent", "ci"]).status.code(), Some(10)); + let approval = task_id_kind(&home, &proj_b, "approval"); + let session = std::fs::read_to_string(home.join("inbox.session")).unwrap(); + let full = [("tokenstash_inbox", session.as_str())]; + let (st, _, _, page) = http(port, "GET", &format!("/t/{approval}"), &full, None); + let seen = "OPENAI_API_KEY@default"; + assert!(st == 200 && page.contains(&format!("name=seen value='{seen}'")), "{page}"); + // The agent asks for the other key before the person answers: the same card grows. + assert_eq!(run(&home, &proj_b, &["need", "GROQ_API_KEY", "--agent", "ci"]).status.code(), Some(10)); + assert_eq!(task_id_kind(&home, &proj_b, "approval"), approval, "the card grew"); + let (st, _, _, page) = http(port, "POST", &format!("/t/{approval}"), &full, Some(&format!("action=deny&seen={seen}&t={session}"))); + assert!(st == 200 && page.contains("changed since you read it"), "{page}"); + assert_eq!(status_of(&home, &proj_b, &approval), "pending", "a Deny on what the page showed does not close the grown card"); + // Decided on the card as it is now, the Deny lands. + let (st, _, _, page) = http(port, "POST", &format!("/t/{approval}"), &full, Some(&format!("action=deny&seen={seen},GROQ_API_KEY@default&t={session}"))); + assert_eq!(st, 303, "{page}"); + assert_eq!(status_of(&home, &proj_b, &approval), "denied"); + inbox.stop(); +} diff --git a/crates/cli/tests/run_redact.rs b/crates/cli/tests/run_redact.rs index 6470f253..b1ed3b48 100644 --- a/crates/cli/tests/run_redact.rs +++ b/crates/cli/tests/run_redact.rs @@ -112,6 +112,32 @@ fn run_masks_lines_of_an_inherited_pem_block_whatever_its_name() { } } +/// Greptile on #64: a compact service-account JSON holds its private key on one line, with +/// the key's line breaks written as the two characters `\n`. A child that decodes it prints +/// the key one line at a time. None of those lines reaches the agent, whether the JSON came +/// from the env file or was inherited. +#[test] +fn run_masks_the_lines_of_a_key_a_child_decodes_from_a_compact_json() { + let home = home("home-json"); + let proj = tmp("proj-json"); + let body = fake_pem_body(); + let json = format!(r#"{{"type":"service_account","private_key":"-----BEGIN PRIVATE KEY-----\n{}\n-----END PRIVATE KEY-----\n","client_email":"ci@example.iam.gserviceaccount.com"}}"#, body.join("\\n")); + // Single quotes keep the backslashes as written. + std::fs::write(proj.join(".env.local"), format!("GCP_SA_JSON='{json}'\n")).unwrap(); + // `printf %b` decodes `\n` the way a JSON parser does. + let out = run_sh(&home, &proj, r#"printf '%b\n' "$GCP_SA_JSON"; printf '%b\n' "$GCP_SA_INHERITED" >&2"#, &[("GCP_SA_INHERITED", json.as_str())]); + let stdout = String::from_utf8_lossy(&out.stdout); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(out.status.success(), "run failed: {stderr}"); + // The child did print the decoded key, and its END line shows where. + assert!(stdout.contains("-----END PRIVATE KEY-----"), "stdout: {stdout}"); + assert!(stderr.contains("-----END PRIVATE KEY-----"), "stderr: {stderr}"); + for line in &body { + assert!(!stdout.contains(line.as_str()), "a line of the env file's key reached stdout: {stdout}"); + assert!(!stderr.contains(line.as_str()), "a line of the inherited key reached stderr: {stderr}"); + } +} + /// An exported shell function is a multi-line inherited value made of code. Its lines are not /// secrets, even when the function's name has TOKEN in it, and `return 0;` in a compiler error /// must reach the agent as written. diff --git a/crates/core/src/actions.rs b/crates/core/src/actions.rs index a552451b..85feb867 100644 --- a/crates/core/src/actions.rs +++ b/crates/core/src/actions.rs @@ -153,6 +153,64 @@ pub fn request(ctx: &Ctx, project: &Path, agent: &str, action: &Action, why: Opt Ok(t) } +/// What [`forget`] did. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Forgot { + /// The value, its index row, or both were deleted. + Deleted, + /// Nothing was stored under that name. + NothingStored, + /// The card's value is gone already: an earlier confirm of the card deleted it, or it was + /// replaced since the card was first confirmed. `kept` is true when the name holds a newer + /// value, which stays. + Gone { kept: bool }, + /// A later confirm of a card whose first confirm found no index row found a value in the + /// stash with no index row again. It may be a replacement whose store stopped before its + /// index row, and nothing tells the two apart, so it was kept. A new forget card removes it. + KeptUnrecorded, +} + +/// What a forget card records when nothing was stored under its name. +const NOTHING_STORED: &str = "-"; + +/// Delete a stored key and its index row. Both deletions run under the index write lock, +/// which a store holds around its stash write and its index row: a store lands wholly before +/// or wholly after, never between them, so no value is left without its row. +/// +/// `card` is the forget card being confirmed and the confirm's claim on it. Before deleting +/// anything, the first confirm records on the card which stored value it is about, and that +/// record is committed on its own. A confirm of the card after one that stopped half way +/// (the key deleted, the card still open) deletes only that value: a key stored since stays. +/// A value with no index row has no id to record, so a later confirm deletes no such value. +pub fn forget(ctx: &Ctx, name: &str, identity: &str, card: Option<(&str, &str)>) -> Result { + let key = crate::stash::stash_key(name, identity); + let about = match card { + Some((id, claim)) => { + let pinned = ctx.db.locked(|| { + let now = ctx.db.stored_value_id(name, identity)?; + ctx.db.pin_action_target(id, claim, now.as_deref().unwrap_or(NOTHING_STORED)) + })?; + Some(pinned.context("another confirm took this card over; reload it")?) + } + None => None, + }; + ctx.db.locked(|| { + let now = ctx.db.stored_value_id(name, identity)?; + if let Some((about, first)) = &about { + if now.as_deref().unwrap_or(NOTHING_STORED) != about { + return Ok(Forgot::Gone { kept: now.is_some() }); + } + if !first && about == NOTHING_STORED { + return Ok(if ctx.stash.get(&key)?.is_some() { Forgot::KeptUnrecorded } else { Forgot::NothingStored }); + } + } + let had = ctx.stash.delete(&key)?; + let meta = ctx.db.delete_secret(name, identity)?; + ctx.db.audit(None, None, "forget", Some(name), Some(identity), None)?; + Ok(if had || meta { Forgot::Deleted } else { Forgot::NothingStored }) + }) +} + /// An agent asks to replace a key, usually because the user told it to. Unlike a person's /// `rotate`, the stored key is not marked stale: it keeps working until the person pastes the /// new one on the Replace card, and declining the card changes nothing. diff --git a/crates/core/src/db.rs b/crates/core/src/db.rs index 635f99a5..1280148c 100644 --- a/crates/core/src/db.rs +++ b/crates/core/src/db.rs @@ -3,18 +3,52 @@ use rusqlite::{params, Connection, OptionalExtension}; use serde::{Deserialize, Serialize}; use std::path::Path; -/// (ts, project, agent, action, name, identity, detail) — never a value. -/// (ts, project, agent, action, name, identity, detail, grant_source) -/// The note an action card carries while it is being carried out, before the time. +/// The note an action card carries while it is being carried out: this, the time the claim +/// was last renewed, a space, and the claim's own 16-character token. pub const CONFIRMING: &str = "confirming since "; -/// How long a claim on an action card holds before it counts as left behind by a process that -/// stopped mid-action. Every action finishes in seconds. +/// How long a claim on an action card holds without being renewed. The process carrying the +/// action out renews it every [`CLAIM_RENEW_SECS`] while the action runs ([`Db::keep_claim`]), +/// so a claim runs out only once that process has stopped. pub const CLAIM_HOLDS_SECS: i64 = 120; +/// How often a running action renews its claim. +pub const CLAIM_RENEW_SECS: u64 = 20; fn claim_cutoff() -> String { (chrono::Utc::now() - chrono::Duration::seconds(CLAIM_HOLDS_SECS)).to_rfc3339_opts(chrono::SecondsFormat::Secs, true) } +/// The token at the end of a claim: what stays the same when the claim is renewed. +fn claim_token(claim: &str) -> &str { + claim.rsplit(' ').next().unwrap_or(claim) +} + +/// SQL: the card holds no live claim (none, or one not renewed since the cutoff bound to `?p`). +fn no_live_claim(p: u8) -> String { + format!("(note IS NULL OR note NOT LIKE 'confirming since %' OR substr(note, 18, 20) < ?{p})") +} + +/// SQL: the card's claim is the one whose token is bound to `?p`. +fn held_by(p: u8) -> String { + format!("(note LIKE 'confirming since %' AND substr(note, -16) = ?{p})") +} + +/// Renews a claim on an action card from a thread of its own until dropped. See +/// [`Db::keep_claim`]. +pub struct ClaimKeeper { + stop: Option>, + worker: Option>, +} + +impl Drop for ClaimKeeper { + fn drop(&mut self) { + drop(self.stop.take()); + if let Some(w) = self.worker.take() { + let _ = w.join(); + } + } +} + +/// (ts, project, agent, action, name, identity, detail, grant_source). Never a value. pub type AuditRow = (String, Option, Option, String, Option, Option, Option, Option); pub struct Db { @@ -306,6 +340,8 @@ impl Db { ("stale_source", "ALTER TABLE secrets ADD COLUMN stale_source TEXT"), ("next_probe", "ALTER TABLE secrets ADD COLUMN next_probe TEXT"), ("verify_off", "ALTER TABLE secrets ADD COLUMN verify_off INTEGER NOT NULL DEFAULT 0"), + // A random id each store gives the value it writes (`stored_value_id`). + ("value_id", "ALTER TABLE secrets ADD COLUMN value_id TEXT"), ] { let has: bool = conn.prepare("SELECT 1 FROM pragma_table_info('secrets') WHERE name=?1")?.exists([col])?; if !has { @@ -318,17 +354,32 @@ impl Db { } } } - // Cards remember whether their one desktop notification went out (0.2.1+). - { - let has: bool = conn.prepare("SELECT 1 FROM pragma_table_info('tasks') WHERE name=?1")?.exists(["notified"])?; + for (table, col, ddl) in [ + // Cards remember whether their one desktop notification went out (0.2.1+). + ("tasks", "notified", "ALTER TABLE tasks ADD COLUMN notified TEXT"), + // What a confirmed action card acts on, recorded by its first confirm + // (`pin_action_target`). + ("tasks", "acts_on", "ALTER TABLE tasks ADD COLUMN acts_on TEXT"), + // The last audit row from before the workspace was recorded: an agent's view of + // the log starts after it (`recent_audit_for`). + ("workspaces", "audit_from", "ALTER TABLE workspaces ADD COLUMN audit_from INTEGER"), + ] { + let has: bool = conn.prepare(&format!("SELECT 1 FROM pragma_table_info('{table}') WHERE name=?1"))?.exists([col])?; if !has { - if let Err(e) = conn.execute_batch("ALTER TABLE tasks ADD COLUMN notified TEXT") { + if let Err(e) = conn.execute_batch(ddl) { if !e.to_string().contains("duplicate column") { return Err(e.into()); } } } } + // Workspaces recorded before `audit_from` existed, or by an older version since: start + // after every row up to and including the second the workspace was recorded in. Times + // are whole seconds, so a row from that second may be the old directory's. + conn.execute( + "UPDATE workspaces SET audit_from=(SELECT COALESCE(MAX(id), 0) FROM audit WHERE ts <= workspaces.created) WHERE audit_from IS NULL", + [], + )?; // Rows marked stale before `stale_source` existed: the human's rotation is the only // one whose display text is a fixed constant, so it is the only one recoverable. conn.execute( @@ -442,14 +493,16 @@ impl Db { // ---------- secrets index (metadata only; values live in the stash) ---------- + /// Record a value just put in the stash. Every call is a new value (a store, an adoption, + /// an import), so it gets a new value id. pub fn upsert_secret(&self, m: &SecretMeta) -> Result<()> { self.conn.execute( - "INSERT INTO secrets (name, identity, provider, sensitive, source_url, created, last_used, stale, last_verified, stale_reason, stale_source, next_probe, verify_off) - VALUES (?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,?13) + "INSERT INTO secrets (name, identity, provider, sensitive, source_url, created, last_used, stale, last_verified, stale_reason, stale_source, next_probe, verify_off, value_id) + VALUES (?1,?2,?3,?4,?5,?6,?7,?8,?9,?10,?11,?12,?13, lower(hex(randomblob(8)))) ON CONFLICT(name, identity) DO UPDATE SET provider=excluded.provider, sensitive=excluded.sensitive, source_url=excluded.source_url, stale=excluded.stale, last_verified=excluded.last_verified, stale_reason=excluded.stale_reason, stale_source=excluded.stale_source, next_probe=excluded.next_probe, verify_off=excluded.verify_off, - created=excluded.created, last_used=COALESCE(excluded.last_used, secrets.last_used)", + created=excluded.created, last_used=COALESCE(excluded.last_used, secrets.last_used), value_id=excluded.value_id", params![m.name, m.identity, m.provider, m.sensitive as i32, m.source_url, m.created, m.last_used, m.stale as i32, m.last_verified, m.stale_reason, m.stale_source, m.next_probe, m.verify_off as i32], )?; Ok(()) @@ -621,6 +674,17 @@ impl Db { Ok(self.conn.execute("DELETE FROM secrets WHERE name=?1 AND identity=?2", params![name, identity])? > 0) } + /// What tells the value stored under (name, identity) apart from any stored before or + /// after it: its value id and when it was stored. A row from before value ids existed, + /// or one an older version rewrote, still differs by its time. `None` when the index + /// holds nothing under that name. + pub fn stored_value_id(&self, name: &str, identity: &str) -> Result> { + Ok(self + .conn + .query_row("SELECT COALESCE(value_id, '') || ' ' || created FROM secrets WHERE name=?1 AND identity=?2", params![name, identity], |r| r.get(0)) + .optional()?) + } + // ---------- tasks ---------- pub fn insert_task(&self, t: &Task) -> Result<()> { @@ -790,52 +854,122 @@ impl Db { /// Mark a pending action card as being carried out, so a second confirm does not run it /// again. Returns the claim (its note, unique to this caller) when this caller holds it now. - /// A claim older than [`CLAIM_HOLDS_SECS`] was left by a process that stopped mid-action - /// and is taken over: the card stays pending until the action has run, so an interrupted - /// one can be confirmed again. + /// The caller keeps it renewed while the action runs ([`Self::keep_claim`]). A claim not + /// renewed for [`CLAIM_HOLDS_SECS`] was left by a process that stopped mid-action and is + /// taken over: the card stays pending until the action has run, so an interrupted one can + /// be confirmed again. pub fn claim_action(&self, id: &str) -> Result> { let claim = format!("{CONFIRMING}{} {:016x}", crate::now(), rand::random::()); let n = self.conn.execute( - "UPDATE tasks SET note=?2 WHERE id=?1 AND status='pending' AND (note IS NULL OR note NOT LIKE 'confirming since %' OR substr(note, 18, 20) < ?3)", + &format!("UPDATE tasks SET note=?2 WHERE id=?1 AND status='pending' AND {}", no_live_claim(3)), params![id, claim, claim_cutoff()], )?; Ok((n == 1).then_some(claim)) } + /// Move `claim`'s time to now, if it still holds the card. False once the card is closed, + /// the claim given back, or taken over. + pub fn renew_action_claim(&self, id: &str, claim: &str) -> Result { + let token = claim_token(claim); + Ok(self.conn.execute( + &format!("UPDATE tasks SET note=?3 WHERE id=?1 AND status='pending' AND {}", held_by(2)), + params![id, token, format!("{CONFIRMING}{} {token}", crate::now())], + )? == 1) + } + + /// Keep `claim` on card `id` renewed from a thread of its own, every [`CLAIM_RENEW_SECS`], + /// until the returned keeper is dropped. An action that runs longer than + /// [`CLAIM_HOLDS_SECS`] (`claude mcp add` on a slow machine) keeps its card: no other + /// confirm takes it over, no decline lands, and it does not expire. A process that stops + /// stops renewing, and the claim runs out as before. If the keeper cannot start, it gives + /// the claim back before returning the error, so the card can be confirmed again or + /// declined at once and does not wait for the claim to run out. + pub fn keep_claim(&self, id: &str, claim: &str) -> Result { + self.keep_claim_every(id, claim, std::time::Duration::from_secs(CLAIM_RENEW_SECS)) + } + + pub(crate) fn keep_claim_every(&self, id: &str, claim: &str, every: std::time::Duration) -> Result { + // A connection of its own: this one stays with the caller, which runs the action. + let opened = self.conn.path().filter(|p| !p.is_empty()).context("the index has no file to open a second connection to").and_then(|p| Db::open(Path::new(p))); + let db = match opened { + Ok(db) => db, + Err(e) => { + self.release_action_claim(id, claim)?; + return Err(e.context("starting the renewal of this card's claim; nothing was done")); + } + }; + let (stop, stopped) = std::sync::mpsc::channel::<()>(); + let (id, claim) = (id.to_string(), claim.to_string()); + // Wakes every `every` until the keeper drops its sender. A renewal that fails (the + // index busy past its timeout) is tried again next time; one that finds the claim gone + // ends the loop. + let worker = std::thread::spawn(move || { + while let Err(std::sync::mpsc::RecvTimeoutError::Timeout) = stopped.recv_timeout(every) { + if let Ok(false) = db.renew_action_claim(&id, &claim) { + break; + } + } + }); + Ok(ClaimKeeper { stop: Some(stop), worker: Some(worker) }) + } + /// Close a card whose action ran, if `claim` still holds it: a worker that was taken over /// after its claim ran out must not close the card under the one that took it. pub fn finish_action(&self, id: &str, claim: &str, done: &str) -> Result { Ok(self.conn.execute( - "UPDATE tasks SET status='answered', answered_at=?3, note=?4 WHERE id=?1 AND status='pending' AND note=?2", - params![id, claim, crate::now(), done], + &format!("UPDATE tasks SET status='answered', answered_at=?3, note=?4 WHERE id=?1 AND status='pending' AND {}", held_by(2)), + params![id, claim_token(claim), crate::now(), done], )? == 1) } /// Give `claim` back after the action failed, so the person can try again or decline. pub fn release_action_claim(&self, id: &str, claim: &str) -> Result<()> { - self.conn.execute("UPDATE tasks SET note=NULL WHERE id=?1 AND status='pending' AND note=?2", params![id, claim])?; + self.conn.execute(&format!("UPDATE tasks SET note=NULL WHERE id=?1 AND status='pending' AND {}", held_by(2)), params![id, claim_token(claim)])?; Ok(()) } - /// Decline a card unless an action on it is being carried out right now (a fresh claim): + /// Record on a card that `claim` holds what its action acts on (`target`), unless an + /// earlier confirm of the card already did, and return what the card holds then (this + /// call's target or the earlier one) and whether this call recorded it. `None` when + /// `claim` no longer holds the card. The record outlives the claim, so a confirm after one + /// that stopped half way acts on the same thing, not on whatever is there by then. + pub fn pin_action_target(&self, id: &str, claim: &str, target: &str) -> Result> { + let token = claim_token(claim); + let first = self.conn.execute( + &format!("UPDATE tasks SET acts_on=?3 WHERE id=?1 AND status='pending' AND acts_on IS NULL AND {}", held_by(2)), + params![id, token, target], + )? == 1; + Ok(self + .conn + .query_row(&format!("SELECT acts_on FROM tasks WHERE id=?1 AND status='pending' AND {}", held_by(2)), params![id, token], |r| r.get::<_, Option>(0)) + .optional()? + .flatten() + .map(|pinned| (pinned, first))) + } + + /// Decline a card unless an action on it is being carried out right now (a live claim): /// a decline must not land while the change it declines is half made. pub fn deny_unless_claimed(&self, id: &str, note: Option<&str>) -> Result { Ok(self.conn.execute( - "UPDATE tasks SET status='denied', answered_at=?2, note=COALESCE(?3, note) WHERE id=?1 AND status='pending' AND (note IS NULL OR note NOT LIKE 'confirming since %' OR substr(note, 18, 20) < ?4)", + &format!("UPDATE tasks SET status='denied', answered_at=?2, note=COALESCE(?3, note) WHERE id=?1 AND status='pending' AND {}", no_live_claim(4)), params![id, crate::now(), note, claim_cutoff()], )? == 1) } - /// Reserve the one extra ask after a no (`need --force` from an agent) for (project, name), - /// in one step. A reservation is spent once it names the card it filed ([`bind_force`]). - /// One that names none is an ask still being filed for a minute, then one a stopped process - /// left behind, which is taken back here. The row id when this caller got the ask. - pub fn reserve_force(&self, project: &str, agent: &str, name: &str, since: &str) -> Result> { + /// Reserve the one extra ask after a no (`need --force` from an agent) for (project, name, + /// identity), in one step. The person declines each identity of a key on its own card, so + /// each identity has its own ask. A reservation is spent once it names the card it filed ([`bind_force`]). One that names + /// none is an ask still being filed for a minute, then one a stopped process left behind, + /// which is taken back here. The row id when this caller got the ask. + /// + /// A row with no identity was written before reservations named one, and counts for every + /// identity of the key, as it did then. + pub fn reserve_force(&self, project: &str, agent: &str, name: &str, identity: &str, since: &str) -> Result> { let in_flight = (chrono::Utc::now() - chrono::Duration::seconds(60)).to_rfc3339_opts(chrono::SecondsFormat::Secs, true); self.conn.execute_batch("BEGIN IMMEDIATE").context("locking the index")?; let r = (|| -> Result> { - let mut st = self.conn.prepare("SELECT id, ts, detail FROM audit WHERE project=?1 AND name=?2 AND action='need.force' AND ts >= ?3")?; - let rows: Vec<(i64, String, Option)> = st.query_map(params![project, name, since], |r| Ok((r.get(0)?, r.get(1)?, r.get(2)?)))?.collect::>()?; + let mut st = self.conn.prepare("SELECT id, ts, detail FROM audit WHERE project=?1 AND name=?2 AND (identity=?3 OR identity IS NULL) AND action='need.force' AND ts >= ?4")?; + let rows: Vec<(i64, String, Option)> = st.query_map(params![project, name, identity, since], |r| Ok((r.get(0)?, r.get(1)?, r.get(2)?)))?.collect::>()?; for (id, ts, card) in rows { if card.is_some() || ts >= in_flight { return Ok(None); @@ -848,7 +982,7 @@ impl Db { } self.conn.execute("DELETE FROM audit WHERE id=?1", params![id])?; } - self.audit(Some(project), Some(agent), "need.force", Some(name), None, None)?; + self.audit(Some(project), Some(agent), "need.force", Some(name), Some(identity), None)?; Ok(Some(self.conn.last_insert_rowid())) })(); match r { @@ -869,12 +1003,27 @@ impl Db { Ok(()) } + /// The card the extra ask for (project, name, identity) was spent on within the window, + /// if it was spent ([`reserve_force`] then returns `None`) on a card for that identity. + /// A row with no identity still spends the ask for every identity of the key, but the + /// card it names is returned only for the identity the card asks for. + pub fn force_card(&self, project: &str, name: &str, identity: &str, since: &str) -> Result> { + Ok(self.conn.query_row( + "SELECT t.id FROM audit a JOIN tasks t ON t.id = a.detail WHERE a.project=?1 AND a.name=?2 AND (a.identity=?3 OR a.identity IS NULL) AND a.action='need.force' AND a.ts >= ?4 \ + AND ((t.name=?2 AND t.identity=?3) OR instr(t.names, '\"' || ?2 || '@' || ?3 || '\"') > 0) ORDER BY a.id DESC LIMIT 1", + params![project, name, identity, since], + |r| r.get(0), + ).optional()?) + } + /// The card the request behind reservation `row` filed for `name`, if any: a card for that - /// key in `project`, from the same agent, filed within the minute after the reservation - /// (a request files its cards at once; anything later is another request's). + /// key and the reservation's identity in `project`, from the same agent, filed within the + /// minute after the reservation (a request files its cards at once; anything later is + /// another request's). A reservation with no identity matches the key in any identity. pub fn card_since_reservation(&self, row: i64, project: &str, name: &str) -> Result> { Ok(self.conn.query_row( - "SELECT t.id FROM tasks t, audit a WHERE a.id=?1 AND t.project=?2 AND t.agent=a.agent AND t.created >= a.ts AND t.created <= strftime('%Y-%m-%dT%H:%M:%SZ', a.ts, '+60 seconds') AND (t.name=?3 OR instr(t.names, '\"' || ?3 || '@') > 0) ORDER BY t.created LIMIT 1", + "SELECT t.id FROM tasks t, audit a WHERE a.id=?1 AND t.project=?2 AND t.agent=a.agent AND t.created >= a.ts AND t.created <= strftime('%Y-%m-%dT%H:%M:%SZ', a.ts, '+60 seconds') \ + AND ((t.name=?3 AND (a.identity IS NULL OR t.identity=a.identity)) OR instr(t.names, '\"' || ?3 || '@' || COALESCE(a.identity || '\"', '')) > 0) ORDER BY t.created LIMIT 1", params![row, project, name], |r| r.get(0), ).optional()?) @@ -914,11 +1063,19 @@ impl Db { Ok(n == 1) } - /// Mark pending tasks past their deadline as expired. Returns count. + /// Replace an open card's `why`. Returns false when the card is no longer pending. + pub fn set_task_why(&self, id: &str, why: &str) -> Result { + Ok(self.conn.execute("UPDATE tasks SET why=?2 WHERE id=?1 AND status='pending'", params![id, why])? == 1) + } + + /// Mark pending tasks past their deadline as expired. Returns count. A card whose action + /// is being carried out (a live claim) is left to the confirm that runs it: the change is + /// being made, and that confirm closes the card with what it did. Once the claim runs out + /// (the process stopped) or is given back, the card expires on the next call. pub fn expire_overdue(&self) -> Result { Ok(self.conn.execute( - "UPDATE tasks SET status='expired' WHERE status='pending' AND deadline < ?1", - params![crate::now()], + &format!("UPDATE tasks SET status='expired' WHERE status='pending' AND deadline < ?1 AND {}", no_live_claim(2)), + params![crate::now(), claim_cutoff()], )?) } @@ -948,11 +1105,20 @@ impl Db { } let id = new_workspace_id(); let created = crate::now(); + self.insert_workspace(&id, &key, &fp, &created)?; + Ok(Some(Workspace { id, root: key, ino: fp.ino, fingerprint_weak: fp.btime.is_none(), btime: fp.btime, dev: fp.dev, created, fingerprint_ok: true })) + } + + /// Record a workspace. Its agent's view of the audit log starts after the last row written + /// before it ([`Self::recent_audit_for`]): row ids only grow, where times are whole + /// seconds, so a directory paired again within the second of an old row still never + /// sees that row. + fn insert_workspace(&self, id: &str, root: &str, fp: &Fingerprint, created: &str) -> Result<()> { self.conn.execute( - "INSERT INTO workspaces (id, root, ino, btime, dev, created) VALUES (?1,?2,?3,?4,?5,?6)", - params![id, key, fp.ino as i64, fp.btime, fp.dev as i64, created], + "INSERT INTO workspaces (id, root, ino, btime, dev, created, audit_from) VALUES (?1,?2,?3,?4,?5,?6,(SELECT COALESCE(MAX(id), 0) FROM audit))", + params![id, root, fp.ino as i64, fp.btime, fp.dev as i64, created], )?; - Ok(Some(Workspace { id, root: key, ino: fp.ino, fingerprint_weak: fp.btime.is_none(), btime: fp.btime, dev: fp.dev, created, fingerprint_ok: true })) + Ok(()) } /// The workspace for a root, created if this is its first contact. Only the delivery @@ -1017,7 +1183,7 @@ impl Db { } let id = new_workspace_id(); let created = crate::now(); - self.conn.execute("INSERT INTO workspaces (id, root, ino, btime, dev, created) VALUES (?1,?2,?3,?4,?5,?6)", params![id, key, fp.ino as i64, fp.btime, fp.dev as i64, created])?; + self.insert_workspace(&id, &key, &fp, &created)?; Ok(Workspace { id, root: key, ino: fp.ino, fingerprint_weak: fp.btime.is_none(), btime: fp.btime, dev: fp.dev, created, fingerprint_ok: true }) } @@ -1172,19 +1338,42 @@ impl Db { Ok(rows.collect::, _>>()?) } - /// The most recent audit rows for one project since `since`, newest first: what an agent - /// may see of the log. `since` is when the directory now at that path was paired, so a - /// checkout re-created at the same path does not read the old one's history. - pub fn recent_audit_for(&self, project: &str, since: &str, limit: usize) -> Result> { + /// The most recent audit rows for one project written after workspace `ws` was recorded, + /// newest first: what an agent may see of the log. A checkout re-created at the same path + /// does not read the old one's history, even when it was paired again within the same + /// second as the old one's last event: the bound is the workspace's audit row id, not a + /// time. A record an older version wrote has no such id until the next open fills it in, + /// and until then its view starts after its creation second. + pub fn recent_audit_for(&self, project: &str, ws: &Workspace, limit: usize) -> Result> { let mut st = self.conn.prepare( - "SELECT ts, project, agent, action, name, identity, detail, grant_source FROM audit WHERE project=?1 AND ts >= ?2 ORDER BY id DESC LIMIT ?3", + "SELECT a.ts, a.project, a.agent, a.action, a.name, a.identity, a.detail, a.grant_source FROM audit a JOIN workspaces w ON w.id=?2 + WHERE a.project=?1 AND CASE WHEN w.audit_from IS NULL THEN a.ts > w.created ELSE a.id > w.audit_from END + ORDER BY a.id DESC LIMIT ?3", )?; - let rows = st.query_map(params![project, since, limit as i64], |r| { + let rows = st.query_map(params![project, ws.id, limit as i64], |r| { Ok((r.get(0)?, r.get(1)?, r.get(2)?, r.get(3)?, r.get(4)?, r.get(5)?, r.get(6)?, r.get(7)?)) })?; Ok(rows.collect::, _>>()?) } + /// The newest audit row's id, 0 when there is none. Writers take turns and an id is never + /// reused (AUTOINCREMENT), so a row with a larger id was committed after this was read. + pub fn audit_mark(&self) -> Result { + Ok(self.conn.query_row("SELECT COALESCE(MAX(id), 0) FROM audit", [], |r| r.get(0))?) + } + + /// Did a store (`store`, by `tasks::record_stored`), an import (`import`, by `tokenstash + /// import`) or an adoption (`adopt`, by `need`) record `name@identity` after `mark`, a + /// value of [`Db::audit_mark`]? Each writes the key's index row and this audit row in one + /// transaction, so a match means a finished store whatever `created` time it recorded. + /// An import keeps the time from its bundle. + pub fn recorded_since(&self, name: &str, identity: &str, mark: i64) -> Result { + Ok(self + .conn + .prepare("SELECT 1 FROM audit WHERE id > ?1 AND name = ?2 AND identity = ?3 AND action IN ('store', 'import', 'adopt')")? + .exists(params![mark, name, identity])?) + } + /// Is there an audit row `action` for (project, name) since `since`? pub fn audited_since(&self, project: &str, name: &str, action: &str, since: &str) -> Result { let n: i64 = self.conn.query_row( diff --git a/crates/core/src/need.rs b/crates/core/src/need.rs index 249612c9..d489839a 100644 --- a/crates/core/src/need.rs +++ b/crates/core/src/need.rs @@ -19,16 +19,30 @@ pub struct NeedOpts { pub identity: Option, pub blocking: bool, pub timeout: Duration, - /// Ask again even if the user recently denied this key for this project. + /// Ask again even if the user recently denied this key for this project. This is the + /// person's own `need --force`, and it sets every recent "no" aside. pub force: bool, /// Never inject silently: route every hit through a fresh approval task, even if this /// project was approved before. Used when the request was derived from untrusted input /// (a program's output in `run`) — each invocation needs its own human yes. pub require_approval: bool, - /// An agent asking again after the person said no (`need --force` from an agent). With - /// `force`, a denied key gets a fresh card, and never arrives on the strength of a broad - /// grant or an on-disk match: those decided about the directory before the no. - pub ask_again: bool, + /// Keys an agent asks for again after the person said no (`need --force` from an agent), + /// as `NAME@identity`, each holding the one extra ask reserved for it. Only the "no" to a + /// listed key is set aside. Any other key keeps its denials, including one that lands + /// while this request runs. A listed key gets a fresh card that says it is a second ask, + /// and never arrives on the strength of a grant, because the grant came before the no. + pub ask_again: Vec, +} + +impl NeedOpts { + /// Is this `NAME@identity` asked for again after a no? + fn asks_again(&self, entry: &str) -> bool { + self.ask_again.iter().any(|e| e == entry) + } + /// Is a recent "no" to this `NAME@identity` set aside for this request? + fn forced(&self, entry: &str) -> bool { + self.force || self.asks_again(entry) + } } #[derive(Debug, Clone, Serialize)] @@ -122,19 +136,15 @@ pub fn need_with_budget(ctx: &Ctx, project: &Path, agent: &str, names: &[String] anyhow::bail!("{i:?} is not an identity (letters, digits, dot, dash and underscore, up to 64 characters)"); } } - // A generated secret is this directory's own signing key and nothing else's. The - // caller does not get to choose its identity: `identity: "default"` from project B - // would hit the value an older tokenstash stored for project A under the shared - // label, and a broad grant would deliver A's key to B without a card. The binding - // is ignored for the same reason. - let identity = if provider.and_then(|p| p.generate.as_deref()).is_some() { - project_identity(project) - } else { - opts.identity.clone().or(ctx.db.binding(&ws.id, name)?).unwrap_or_else(|| "default".into()) - }; + let identity = identity_for(ctx, project, Some(&ws), name, opts.identity.as_deref())?; + // The person's own --force sets any recent "no" aside; an agent's sets aside only the + // "no" to a key it holds the extra ask for. + let entry = format!("{name}@{identity}"); + let again = opts.asks_again(&entry); + let force = opts.forced(&entry); // When this read finds a generated key missing or stale, the store that follows - // compares the key's index row with this moment (`tasks::store_generated`). - let read_at = crate::now(); + // looks for a store recorded after this mark (`tasks::store_generated`). + let read_mark = ctx.db.audit_mark()?; let hit = ctx.stash.get(&stash_key(name, &identity))?; if let Some(value) = hit { @@ -162,8 +172,12 @@ pub fn need_with_budget(ctx: &Ctx, project: &Path, agent: &str, names: &[String] next_probe: None, verify_off: false, }; - ctx.db.upsert_secret(&m)?; - ctx.db.audit(Some(&pid), Some(agent), "adopt", Some(name), Some(&identity), Some("found in the stash but not in this home's index"))?; + // The row and its audit line commit together, because a generated key's store + // reads them as one record (`Db::recorded_since`). + ctx.db.locked(|| { + ctx.db.upsert_secret(&m)?; + ctx.db.audit(Some(&pid), Some(agent), "adopt", Some(name), Some(&identity), Some("found in the stash but not in this home's index")) + })?; meta = Some(m); } let sensitive = meta.as_ref().map(|m| m.sensitive).unwrap_or(false) || provider.map(|p| p.sensitive).unwrap_or(false); @@ -206,7 +220,7 @@ pub fn need_with_budget(ctx: &Ctx, project: &Path, agent: &str, names: &[String] // Asked again after a no: the person answers a card again, whatever grant // opened the gate before the no, an exact one included (a grant outlives // `forget`, and the key may since have been stored from another directory). - Gate::Open { .. } if opts.ask_again && denied_now(ctx)? => Gate::NeedsApproval { reason: if sensitive { GateReason::Sensitive } else { GateReason::Pairing } }, + Gate::Open { .. } if again && denied_now(ctx)? => Gate::NeedsApproval { reason: if sensitive { GateReason::Sensitive } else { GateReason::Pairing } }, Gate::Open { source } if source == crate::db::GRANT_BROAD || source == crate::db::GRANT_ON_DISK => { let since = ctx.cfg.ttl_since(); // Both kinds of "no" count: a refused paste card and a refused pairing or @@ -218,7 +232,7 @@ pub fn need_with_budget(ctx: &Ctx, project: &Path, agent: &str, names: &[String] None => denied_card_for(ctx, &pid, &format!("{name}@{identity}"), &since)?, }; match denied { - Some(tid) if !opts.force => { + Some(tid) if !force => { ctx.db.audit(Some(&pid), Some(agent), "deny.honored", Some(name), Some(&identity), Some("a broad grant does not overrule a denial for this key here"))?; outcomes.push(Outcome::Denied { name: name.clone(), task_id: tid }); continue; @@ -233,7 +247,7 @@ pub fn need_with_budget(ctx: &Ctx, project: &Path, agent: &str, names: &[String] Delivery::Injected { path, unverified } => { outcomes.push(Outcome::Injected { name: name.clone(), identity, written_to: path.display().to_string(), generated: false, unverified }); } - Delivery::Rejected { reason } => outcomes.push(replacement(ctx, project, agent, name, &identity, opts, &reason, &read_at)?), + Delivery::Rejected { reason } => outcomes.push(replacement(ctx, project, agent, name, &identity, opts, &reason, read_mark)?), // The value changed under an on-disk delivery: the directory never held the // new one, so it pairs like any other first delivery. Delivery::NotDelivered => { @@ -278,21 +292,24 @@ pub fn need_with_budget(ctx: &Ctx, project: &Path, agent: &str, names: &[String] continue; } if let Some(v) = generate(spec) { - let (p, generated) = tasks::store_generated(ctx, name, &identity, &v, project, agent, &read_at)?; + let (p, generated) = tasks::store_generated(ctx, name, &identity, &v, project, agent, read_mark)?; outcomes.push(Outcome::Injected { name: name.clone(), identity, written_to: p.map(|p| p.display().to_string()).unwrap_or_default(), generated, unverified: false }); continue; } } // Honor a recent refusal: "denied — do not ask again" must actually mean that. - if !opts.force { + if !force { let since = ctx.cfg.ttl_since(); if let Some(d) = ctx.db.recent_denial(&pid, name, &identity, &since)? { outcomes.push(Outcome::Denied { name: name.clone(), task_id: d.id }); continue; } } - let t = tasks::create_secret_task(ctx, project, agent, name, &identity, &opts.req)?; + // Only the key asked for again says so on its card; the others in the same request + // were never declined. + let req = if again { asked_again(&opts.req, agent) } else { opts.req.clone() }; + let t = tasks::create_secret_task(ctx, project, agent, name, &identity, &req)?; outcomes.push(Outcome::Pending { name: name.clone(), identity: identity.clone(), task_id: t.id, title: t.title, url: t.url }); } @@ -314,7 +331,7 @@ pub fn need_with_budget(ctx: &Ctx, project: &Path, agent: &str, names: &[String] continue; } for g in entries { - if denied_entries.iter().any(|(e, _)| e == g) { + if opts.asks_again(g) || denied_entries.iter().any(|(e, _)| e == g) { continue; } let name_only = tasks::split_identity(g).0.to_string(); @@ -338,7 +355,8 @@ pub fn need_with_budget(ctx: &Ctx, project: &Path, agent: &str, names: &[String] if still.is_empty() { continue; } - let t = tasks::create_approval_task(ctx, project, agent, &still, kind)?; + let again: Vec = still.iter().filter(|g| opts.asks_again(g)).cloned().collect(); + let t = tasks::create_approval_task(ctx, project, agent, &still, kind, &again)?; for o in outcomes.iter_mut() { if let Outcome::Pending { name, identity, task_id, title, .. } = o { if task_id.is_empty() && still.contains(&format!("{name}@{identity}")) { @@ -392,7 +410,7 @@ pub fn wait(ctx: &Ctx, project: &Path, outcomes: &mut [Outcome], timeout: Durati if !authorised { // ask again, on a card of its own let kind = if t.expects == tasks::APPROVAL_SENSITIVE { tasks::ApprovalKind::Sensitive } else { tasks::ApprovalKind::Pairing }; - let nt = tasks::create_approval_task(ctx, project, &t.agent, &[entry], kind)?; + let nt = tasks::create_approval_task(ctx, project, &t.agent, &[entry], kind, &[])?; *o = Outcome::Pending { name: name.clone(), identity: identity.clone(), task_id: nt.id, title: nt.title, url: nt.url }; any_pending = true; continue; @@ -483,17 +501,48 @@ pub(crate) fn adoptable(project: &Path, env_file: &str, name: &str) -> Option, name: &str, asked: Option<&str>) -> Result { + if registry::lookup(name).and_then(|p| p.generate.as_deref()).is_some() { + return Ok(project_identity(project)); + } + if let Some(i) = asked { + return Ok(i.to_string()); + } + let bound = match ws { Some(ws) => ctx.db.binding(&ws.id, name)?, None => None }; + Ok(bound.unwrap_or_else(|| "default".into())) +} + /// Has the person said no to `name` for this project within the TTL: a declined paste card, -/// or a declined pairing or sensitive card that named it? The identity is resolved the way -/// `need` resolves it (the one given, else the project's binding, else `default`). An agent -/// asking again (`need --force`) counts as a second ask only after a no. -pub fn denied_here(ctx: &Ctx, project: &Path, name: &str, identity: Option<&str>) -> Result { - let Ok(project) = project.canonicalize() else { return Ok(false) }; +/// or a declined pairing or sensitive card that named it? Returns the identity that was +/// declined, resolved the way `need` resolves it, or `None` when there was no "no". An agent +/// asking again (`need --force`) counts as a second ask only after a no, and only for that +/// identity. +pub fn denied_here(ctx: &Ctx, project: &Path, name: &str, identity: Option<&str>) -> Result> { + let Ok(project) = project.canonicalize() else { return Ok(None) }; let pid = project.to_string_lossy().to_string(); - let bound = match ctx.db.find_workspace(&project)? { Some(ws) => ctx.db.binding(&ws.id, name)?, None => None }; - let identity = identity.map(String::from).or(bound).unwrap_or_else(|| "default".into()); + let ws = ctx.db.find_workspace(&project)?; + let identity = identity_for(ctx, &project, ws.as_ref(), name, identity)?; let since = ctx.cfg.ttl_since(); - Ok(ctx.db.recent_denial(&pid, name, &identity, &since)?.is_some() || denied_card_for(ctx, &pid, &format!("{name}@{identity}"), &since)?.is_some()) + let denied = ctx.db.recent_denial(&pid, name, &identity, &since)?.is_some() || denied_card_for(ctx, &pid, &format!("{name}@{identity}"), &since)?.is_some(); + Ok(denied.then_some(identity)) +} + +/// The request for a key asked for again after a no. Its card says so first. +fn asked_again(req: &SecretRequest, agent: &str) -> SecretRequest { + let note = format!("Asked again after you declined, because you asked {agent} to."); + let why = match &req.why { + Some(w) => format!("{note} {w}"), + None => note, + }; + SecretRequest { why: Some(why), ..req.clone() } } /// A denied pairing or sensitive card within the TTL that named this entry. A denied @@ -690,19 +739,20 @@ fn verify_at_use(ctx: &Ctx, project: &Path, agent: &str, name: &str, identity: & /// The stash holds a value the project may have, but it is stale: regenerate a generated /// secret, honour a recent "do not ask again", else file the replacement card. -/// `read_at` is when the caller read the stale value from the stash. +/// `read_mark` is [`crate::db::Db::audit_mark`] taken just before the caller read the stale +/// value from the stash. #[allow(clippy::too_many_arguments)] -fn replacement(ctx: &Ctx, project: &Path, agent: &str, name: &str, identity: &str, opts: &NeedOpts, reason: &str, read_at: &str) -> Result { +fn replacement(ctx: &Ctx, project: &Path, agent: &str, name: &str, identity: &str, opts: &NeedOpts, reason: &str, read_mark: i64) -> Result { let pid = project.to_string_lossy().to_string(); let provider = registry::lookup(name); // Generated secrets are never pasted: regenerate. if let Some(spec) = provider.and_then(|p| p.generate.as_deref()) { if let Some(v) = generate(spec) { - let (p, generated) = tasks::store_generated(ctx, name, identity, &v, project, agent, read_at)?; + let (p, generated) = tasks::store_generated(ctx, name, identity, &v, project, agent, read_mark)?; return Ok(Outcome::Injected { name: name.into(), identity: identity.into(), written_to: p.map(|p| p.display().to_string()).unwrap_or_default(), generated, unverified: false }); } } - if !opts.force { + if !opts.forced(&format!("{name}@{identity}")) { let since = ctx.cfg.ttl_since(); if let Some(d) = ctx.db.recent_denial(&pid, name, identity, &since)? { return Ok(Outcome::Denied { name: name.into(), task_id: d.id }); diff --git a/crates/core/src/redact.rs b/crates/core/src/redact.rs index d9d594b5..a2d0a561 100644 --- a/crates/core/src/redact.rs +++ b/crates/core/src/redact.rs @@ -32,22 +32,35 @@ impl Redactor { /// shows the whole value to a single `redact` call. Lines are trimmed, and PEM armor is /// skipped. A line shorter than the shortest secret the stash accepts is masked only where /// it stands alone, so the `apple` of `apple\nberry` does not mask `pineapple`. + /// + /// Line breaks written as the two characters `\n` or `\r` split lines too. A compact + /// service-account JSON holds its private key that way, on one line, and a child that + /// decodes the JSON and prints the key prints it one line at a time. pub fn add(&mut self, v: &SecretString) { self.add_whole_value(v); let s = v.expose_secret(); - if !s.contains(['\n', '\r']) { - return; - } - for line in s.split(['\n', '\r']).map(str::trim) { - let short = line.chars().count() < crate::tasks::MIN_SECRET_CHARS; - // A short line with no letter or digit (`{`, `},`) is structure, and as a whole - // token it would match wherever it appears. - if is_pem_armor(line) || (short && !line.chars().any(char::is_alphanumeric)) { - continue; + for line in s.split(['\n', '\r']) { + // A value with no line break is one line, registered whole above. + if line.len() < s.len() { + self.add_line(line); + } + if line.contains("\\n") || line.contains("\\r") { + for part in line.split("\\n").flat_map(|p| p.split("\\r")) { + self.add_line(part); + } } - self.patterns.push(Pattern { text: line.to_string(), whole_token: short }); } } + fn add_line(&mut self, line: &str) { + let line = line.trim(); + let short = line.chars().count() < crate::tasks::MIN_SECRET_CHARS; + // A short line with no letter or digit (`{`, `},`) is structure, and as a whole + // token it would match wherever it appears. + if is_pem_armor(line) || (short && !line.chars().any(char::is_alphanumeric)) { + return; + } + self.patterns.push(Pattern { text: line.to_string(), whole_token: short }); + } /// Register `v` as one string only. For a value that is only suspected to be a secret, /// whose lines on their own may be ordinary text. pub fn add_whole_value(&mut self, v: &SecretString) { diff --git a/crates/core/src/stash.rs b/crates/core/src/stash.rs index 199eb8e8..8f5a788b 100644 --- a/crates/core/src/stash.rs +++ b/crates/core/src/stash.rs @@ -62,6 +62,33 @@ pub trait Stash { fn get(&self, key: &str) -> Result>; fn set(&self, key: &str, value: &SecretString) -> Result<()>; fn delete(&self, key: &str) -> Result; + /// Keys with a copy this backend cannot keep up to date, among `keys` and any other key it + /// holds for this home. `None` when the backend keeps no such copies. `doctor` shows it. + fn stray_copies(&self, _keys: &[String]) -> Option { + None + } +} + +/// What [`Stash::stray_copies`] found. +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub struct StrayCheck { + /// How many keys it looked at. + pub checked: usize, + pub copies: Vec, + /// What it could not look at, and why, when part of the check could not run. + pub limited: Option, +} + +/// A key with a copy its backend cannot keep up to date (see [`Stash::stray_copies`]). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct StrayCopy { + /// `NAME@identity`. + pub key: String, + /// The two places a read looks hold different values, so a read can return the old one. + pub differs: bool, + /// Live copies that only other login sessions hold. They matter only while an older + /// tokenstash runs in one of those sessions. + pub elsewhere: usize, } /// Stash key format: `NAME@identity`. Decided day one so identities never need a migration. @@ -189,6 +216,21 @@ impl Stash for KeyringStash { /// keep keyring-rs's description, `keyring-rs:@`, so keys stored by /// older versions are found where they are. /// +/// A read that finds the key linked into both rings changes nothing. Any other read moves +/// links (it adopts a key older code left in one ring or in the session keyring, or puts the +/// user keyring's key back into the persistent keyring), and it does so under the writers' +/// lock from a fresh look, so it never links an older object over one a writer in another +/// session stored meanwhile. +/// +/// One case this cannot fix. An older tokenstash still running in another login session +/// reads and writes its own session copy, which no other session may write. Its read links +/// that copy into the persistent keyring, and so does its write, after giving the copy the +/// new value. Both leave the user keyring holding one value and the persistent keyring +/// another, and the kernel keeps no time of write to tell which is newer. A read here keeps +/// the user keyring's value, which is right after the old process read and wrong after it +/// wrote. Taking the persistent keyring's value instead would let any old process's read +/// bring an old key back. [`Stash::stray_copies`] finds both states for `doctor`. +/// /// The user keyring lives while any process of this user runs; the persistent keyring /// survives a logout but is dropped after `/proc/sys/kernel/keys/persistent_keyring_expiry` /// seconds (three days by default) without use. Neither survives a reboot. @@ -239,6 +281,16 @@ mod kernel { } } + /// The user keyring's key, when the persistent keyring links the same object or the + /// kernel has no persistent keyring. Reading that key changes no links. + pub(super) fn settled(user: &KeyRing, persistent: Option<&KeyRing>, desc: &str) -> Result> { + let Some(k) = find(user, desc)? else { return Ok(None) }; + match persistent { + None => Ok(Some(k)), + Some(p) => Ok((find(p, desc)? == Some(k)).then_some(k)), + } + } + /// The key to read: the user keyring's, else the persistent keyring's, else (keys only /// older code stored) whatever the session keyring holds. pub(super) fn current(user: &KeyRing, persistent: Option<&KeyRing>, desc: &str) -> Result> { @@ -265,6 +317,87 @@ mod kernel { Ok(out) } + /// [`super::Stash::stray_copies`] for the kernel keyring, given what reading `/proc/keys` + /// returned. That file lists every key this user may view, in any session. It adds the + /// keys stored under this home's service name that this home's index does not list (a + /// lost index, another home with the same service name), which `need` can still read + /// and adopt. A copy another session holds is visible there but, made by older code, + /// not writable from here. + pub(super) fn stray_check(keys: &[String], listing: std::io::Result) -> super::StrayCheck { + let mut check = super::StrayCheck::default(); + let user = match user() { + Ok(u) => u, + Err(e) => { + check.limited = Some(format!("Nothing was checked ({e:#}).")); + return check; + } + }; + let persistent = persistent(); + let mut names = keys.to_vec(); + match &listing { + Ok(l) => names.extend(stored_keys(l)), + Err(e) => { + check.limited = Some(format!("Could not read /proc/keys ({e}), so only the keys this home's index lists were checked, and not for copies other login sessions hold.")); + } + } + names.sort(); + names.dedup(); + check.checked = names.len(); + for key in names { + if let Ok((differs, elsewhere)) = strays(&user, persistent.as_ref(), &description(&key), listing.as_deref().ok()) { + if differs || elsewhere > 0 { + check.copies.push(super::StrayCopy { key, differs, elsewhere }); + } + } + } + check + } + + /// Whether the user keyring and the persistent keyring link different objects under + /// `desc` that hold different values, and how many live copies in `listing` (the text + /// of `/proc/keys`) only other login sessions link. + fn strays(user: &KeyRing, persistent: Option<&KeyRing>, desc: &str, listing: Option<&str>) -> Result<(bool, usize)> { + let u = find(user, desc)?; + let p = persistent.map(|p| find(p, desc)).transpose()?.flatten(); + let differs = match (u, p) { + (Some(u), Some(p)) if u != p => { + let (a, b) = (read(u).ok().flatten().map(zeroize::Zeroizing::new), read(p).ok().flatten().map(zeroize::Zeroizing::new)); + matches!((a, b), (Some(a), Some(b)) if a != b) + } + _ => false, + }; + let own = session().map(|s| find(&s, desc)).transpose()?.flatten(); + let elsewhere = listing.map_or(0, |l| listed(l).filter(|(k, d)| *d == desc && ![u, p, own].contains(&Some(*k))).count()); + Ok((differs, elsewhere)) + } + + /// The `NAME@identity` of every key in `listing` stored under this home's service name, + /// leaving out the backend probe's. + fn stored_keys(listing: &str) -> Vec { + let suffix = format!("@{}", super::service()); + listed(listing) + .filter_map(|(_, d)| d.strip_prefix("keyring-rs:")?.strip_suffix(suffix.as_str()).map(str::to_string)) + .filter(|k| k != "__tokenstash_probe__") + .collect() + } + + /// The live `user` keys in `listing`, the text of `/proc/keys`, with their descriptions. + /// A line reads `serial flags usage timeout perm uid gid type description: length`. + fn listed(listing: &str) -> impl Iterator { + listing.lines().filter_map(|line| { + let f: Vec<&str> = line.split_whitespace().collect(); + let [serial, flags, usage, timeout, _perm, _uid, _gid, kind, name, ..] = f.as_slice() else { return None }; + // Instantiated, and not revoked, dead, negative or invalidated; still referenced + // (a key waiting for the collector has a usage of 0); not expired. + let live = flags.starts_with('I') && !flags.contains(['R', 'D', 'N', 'i']) && usage.parse::().is_ok_and(|n| n > 0) && *timeout != "expd"; + if !live || *kind != "user" { + return None; + } + let serial = u32::from_str_radix(serial, 16).ok()?; + Some((Key::from_id(linux_keyutils::KeySerialId(serial as i32)), name.strip_suffix(':').unwrap_or(name))) + }) + } + /// This user may use the key from any session. The kernel checks possession again on /// every call that names a key by id, and a session keyring that does not link the user /// keyring (a systemd service's private one, `keyctl session`) does not possess what is @@ -351,11 +484,33 @@ impl Stash for KernelKeyring { fn get(&self, key: &str) -> Result> { let desc = kernel::description(key); let (user, persistent) = (kernel::user()?, kernel::persistent()); - let Some(k) = kernel::current(&user, persistent.as_ref(), &desc)? else { return Ok(None) }; - // Adopt a key older code left only in the persistent or the session keyring, and - // re-pin it so the persistent link outlives the next logout. The read does not - // depend on it. - let _ = kernel::pin(&user, persistent.as_ref(), k); + let k = match kernel::settled(&user, persistent.as_ref(), &desc)? { + Some(k) => k, + None => { + if kernel::current(&user, persistent.as_ref(), &desc)?.is_none() { + return Ok(None); + } + #[cfg(test)] + if let Some(hook) = BEFORE_RELINK.with(|h| h.borrow_mut().take()) { + hook(); + } + // Adopt a key older code left only in the persistent or the session keyring, + // or pin the user keyring's key into the persistent keyring again, so it + // outlives the next logout. A writer in another session may store the key + // after the look above. Under its lock, a fresh look finds what it stored, + // and the old object is not linked over it. The read does not depend on + // the links. + let found = Self::locked(|| { + let k = kernel::current(&user, persistent.as_ref(), &desc)?; + if let Some(k) = k { + let _ = kernel::pin(&user, persistent.as_ref(), k); + } + Ok(k) + })?; + let Some(k) = found else { return Ok(None) }; + k + } + }; let Some(bytes) = kernel::read(k)? else { return Ok(None) }; let v = String::from_utf8(bytes).map_err(|_| anyhow!("the kernel keyring entry for {key} is not UTF-8"))?; Ok(Some(SecretString::from(v))) @@ -394,6 +549,17 @@ impl Stash for KernelKeyring { Ok(!copies.is_empty()) }) } + fn stray_copies(&self, keys: &[String]) -> Option { + Some(kernel::stray_check(keys, std::fs::read_to_string("/proc/keys"))) + } +} + +// Test-only: runs once, on this thread, in a kernel keyring read that is about to move +// links, after its first look and before it takes the lock. Tests use it to make another +// session store the key in that gap. +#[cfg(all(test, target_os = "linux"))] +thread_local! { + static BEFORE_RELINK: std::cell::RefCell>> = const { std::cell::RefCell::new(None) }; } // ---------------- insecure file (tests/CI only) ---------------- @@ -472,15 +638,14 @@ mod kernel_tests { false } - /// Run the test `name` in a new process with a session keyring of its own and `env` - /// added; assert it ran its body to the end. False if keyctl is refused here. - fn spawn_session(name: &str, env: &[(&str, &std::ffi::OsStr)]) -> bool { + /// What the second process in a test does, for tests that start one. + const ROLE: &str = "TOKENSTASH_KERNEL_TEST_ROLE"; + + /// The test `name` again, in a process that will join a session keyring of its own. + fn session_command(name: &str) -> std::process::Command { use std::os::unix::process::CommandExt; let mut cmd = std::process::Command::new(std::env::current_exe().unwrap()); cmd.args([name, "--exact", "--nocapture", "--test-threads=1"]).env(CHILD, "1"); - for (k, v) in env { - cmd.env(k, v); - } // SAFETY: only an async-signal-safe syscall runs between fork and exec. unsafe { cmd.pre_exec(|| { @@ -491,6 +656,16 @@ mod kernel_tests { Ok(()) }); } + cmd + } + + /// Run the test `name` in a new process with a session keyring of its own and `env` + /// added; assert it ran its body to the end. False if keyctl is refused here. + fn spawn_session(name: &str, env: &[(&str, &std::ffi::OsStr)]) -> bool { + let mut cmd = session_command(name); + for (k, v) in env { + cmd.env(k, v); + } let out = match cmd.output() { Ok(o) => o, Err(e) => { @@ -508,6 +683,27 @@ mod kernel_tests { /// pass as a pass. const RAN: &str = "kernel keyring test body completed"; + /// Printed by a holder (see [`while_another_session_holds`]) once its copy is in place. + const HELD: &str = "kernel keyring test copy held"; + + /// Run the test `name` as a holder (`ROLE=holder`) in a new process with a session + /// keyring of its own, and call `f` while that process, and so its session keyring and + /// whatever it linked there, is still alive. The holder prints [`HELD`], then waits for + /// its stdin to close. + fn while_another_session_holds(name: &str, f: impl FnOnce()) { + use std::io::{BufRead, Read}; + let mut child = session_command(name).env(ROLE, "holder").stdin(std::process::Stdio::piped()).stdout(std::process::Stdio::piped()).spawn().unwrap(); + let mut out = std::io::BufReader::new(child.stdout.take().unwrap()); + let mut text = String::new(); + while !text.contains(HELD) { + assert_ne!(out.read_line(&mut text).unwrap(), 0, "the holder exited before it held a copy:\n{text}"); + } + f(); + drop(child.stdin.take()); + out.read_to_string(&mut text).unwrap(); + assert!(child.wait().unwrap().success() && text.contains(RAN), "the holder did not run its body to the end:\n{text}"); + } + fn value(s: &dyn Stash, key: &str) -> Option { s.get(key).unwrap().map(|v| v.expose_secret().to_string()) } @@ -576,7 +772,7 @@ mod kernel_tests { return; } let key = "OPENAI_API_KEY@default"; - if std::env::var_os("TOKENSTASH_KERNEL_TEST_ROLE").is_some() { + if std::env::var_os(ROLE).is_some() { // The second session: a paste, nothing else. KernelKeyring.set(key, &SecretString::from("sk-pasted-in-the-other-session")).unwrap(); println!("{RAN}"); @@ -590,11 +786,151 @@ mod kernel_tests { let session = KeyRing::from_special_id(KeyRingIdentifier::Session, false).unwrap(); let shadow = session.add_key(&kernel::description(key), b"sk-old-session-copy").unwrap(); assert_eq!(session.search(&kernel::description(key)).unwrap(), shadow, "the setup must reproduce the shadowing"); - assert!(spawn_session(NAME, &[("TOKENSTASH_KERNEL_TEST_ROLE", std::ffi::OsStr::new("writer"))]), "the second session must run"); + assert!(spawn_session(NAME, &[(ROLE, std::ffi::OsStr::new("writer"))]), "the second session must run"); assert_eq!(value(&KernelKeyring, key).as_deref(), Some("sk-pasted-in-the-other-session")); println!("{RAN}"); } + /// Greptile on #61, #63 and #64: a read that finds a key only in this session's keyring (a + /// copy older code left) links it into the user and persistent keyrings. Another session + /// can store the key between that read's search and its links. The read must then return + /// the stored key and leave the old copy out of both keyrings, not link it over the new one. + #[test] + fn a_read_does_not_link_a_session_copy_over_a_key_stored_meanwhile() { + const NAME: &str = "stash::kernel_tests::a_read_does_not_link_a_session_copy_over_a_key_stored_meanwhile"; + if !in_own_session(NAME) { + return; + } + let key = "OPENAI_API_KEY@default"; + if std::env::var_os(ROLE).is_some() { + // The second session: a paste, nothing else. + KernelKeyring.set(key, &SecretString::from("sk-stored-from-another-session")).unwrap(); + println!("{RAN}"); + return; + } + probe(&KernelKeyring).expect("the kernel keyring works in a session that does not link the user keyring"); + let _cleanup = Cleanup(key); + let desc = kernel::description(key); + // What older tokenstash left in this session, and nowhere else. + let session = KeyRing::from_special_id(KeyRingIdentifier::Session, false).unwrap(); + let old = session.add_key(&desc, b"sk-old-copy-only-in-this-session").unwrap(); + // The read has found that copy; before it links it, the other session stores the key. + BEFORE_RELINK.with(|h| { + *h.borrow_mut() = Some(Box::new(|| assert!(spawn_session(NAME, &[(ROLE, std::ffi::OsStr::new("writer"))]), "the second session must run"))) + }); + assert_eq!(value(&KernelKeyring, key).as_deref(), Some("sk-stored-from-another-session")); + assert!(BEFORE_RELINK.with(|h| h.borrow().is_none()), "the read went through the gap"); + let user = KeyRing::from_special_id(KeyRingIdentifier::User, false).unwrap(); + let persistent = KeyRing::get_persistent(KeyRingIdentifier::Session).unwrap(); + let stored = user.search(&desc).unwrap(); + assert_ne!(stored, old, "the old copy is not linked into the user keyring"); + assert_eq!(persistent.search(&desc).unwrap(), stored, "nor into the persistent keyring"); + println!("{RAN}"); + } + + /// What `doctor`'s check finds for `keys` (the keys the index lists), with `/proc/keys` + /// read as usual. + fn found(keys: &[&str]) -> Vec { + let keys: Vec = keys.iter().map(|k| k.to_string()).collect(); + let check = KernelKeyring.stray_copies(&keys).expect("the kernel keyring checks for copies"); + assert_eq!(check.limited, None, "/proc/keys is readable here"); + check.copies + } + + /// `/proc/keys` could not be read. + fn unreadable() -> std::io::Result { + Err(std::io::Error::new(std::io::ErrorKind::NotFound, "no /proc here")) + } + + /// Greptile on #60 and #67: an older tokenstash still running in another login session + /// reads its own old copy of a key and links it into the persistent keyring, so the user + /// keyring and the persistent keyring hold different values. `doctor`'s check names the + /// key while they differ. A read here keeps the user keyring's value and links that key + /// into the persistent keyring again. A write from that old process leaves the same state + /// with the newer value in the persistent keyring; the kernel cannot tell the two apart, + /// so that case is reported, not repaired. + /// + /// Greptile on #77: the check also covers a key this home's index does not list, which + /// `need` can still read and adopt, and still compares the two keyrings when `/proc/keys` + /// cannot be read, saying what it could not check. + #[test] + fn an_old_copy_linked_into_the_persistent_keyring_is_reported_and_not_read() { + const NAME: &str = "stash::kernel_tests::an_old_copy_linked_into_the_persistent_keyring_is_reported_and_not_read"; + if !in_own_session(NAME) { + return; + } + let key = "OPENAI_API_KEY@default"; + let desc = kernel::description(key); + if std::env::var_os(ROLE).is_some() { + // What keyring-rs's keyutils store does when it reads in a session that holds + // its own copy. It links that copy into the persistent keyring. + let session = KeyRing::from_special_id(KeyRingIdentifier::Session, false).unwrap(); + let old = session.add_key(&desc, b"sk-old-copy-of-that-session").unwrap(); + KeyRing::get_persistent(KeyRingIdentifier::Session).unwrap().link_key(old).unwrap(); + println!("{RAN}"); + return; + } + probe(&KernelKeyring).expect("the kernel keyring works in a session that does not link the user keyring"); + let _cleanup = Cleanup(key); + KernelKeyring.set(key, &SecretString::from("sk-current-value")).unwrap(); + assert!(found(&[key]).is_empty(), "one object, linked into both keyrings"); + assert!(spawn_session(NAME, &[(ROLE, std::ffi::OsStr::new("old-reader"))]), "the second session must run"); + let user = KeyRing::from_special_id(KeyRingIdentifier::User, false).unwrap(); + let persistent = KeyRing::get_persistent(KeyRingIdentifier::Session).unwrap(); + assert_ne!(user.search(&desc).unwrap(), persistent.search(&desc).unwrap(), "the setup must reproduce the old process's link"); + + let differs = vec![StrayCopy { key: key.into(), differs: true, elsewhere: 0 }]; + assert_eq!(found(&[key]), differs); + assert_eq!(found(&[]), differs, "a key the index does not list is found in the keyrings"); + let blind = kernel::stray_check(&[key.to_string()], unreadable()); + assert_eq!(blind.copies, differs, "the two keyrings are compared without /proc/keys"); + assert!(blind.limited.as_deref().is_some_and(|w| w.starts_with("Could not read /proc/keys (no /proc here)")), "{blind:?}"); + let blind = kernel::stray_check(&[], unreadable()); + assert!(blind.copies.is_empty() && blind.limited.is_some(), "without /proc/keys only indexed keys are checked, and the check says so: {blind:?}"); + + assert_eq!(value(&KernelKeyring, key).as_deref(), Some("sk-current-value")); + assert_eq!(persistent.search(&desc).unwrap(), user.search(&desc).unwrap(), "the read linked the user keyring's key into the persistent keyring again"); + // The old copy may still count as held elsewhere until the kernel collects the dead + // session keyring that linked it, but the two keyrings agree again. + assert!(found(&[key]).iter().all(|c| !c.differs)); + println!("{RAN}"); + } + + /// Greptile on #67: a copy that only another login session links cannot be written from + /// here, so a replacement stored here does not reach it, and an older tokenstash in that + /// session keeps reading it. `doctor`'s check counts it. + #[test] + fn a_copy_only_another_live_session_holds_is_counted() { + const NAME: &str = "stash::kernel_tests::a_copy_only_another_live_session_holds_is_counted"; + if !in_own_session(NAME) { + return; + } + let key = "OPENAI_API_KEY@default"; + let desc = kernel::description(key); + if std::env::var_os(ROLE).is_some() { + // An older tokenstash's session copy, held while the other process checks. + let session = KeyRing::from_special_id(KeyRingIdentifier::Session, false).unwrap(); + session.add_key(&desc, b"sk-old-copy-of-that-session").unwrap(); + println!("{HELD}"); + let _ = std::io::stdin().read_line(&mut String::new()); + println!("{RAN}"); + return; + } + probe(&KernelKeyring).expect("the kernel keyring works in a session that does not link the user keyring"); + let _cleanup = Cleanup(key); + KernelKeyring.set(key, &SecretString::from("sk-current-value")).unwrap(); + assert!(found(&[key]).is_empty(), "one object, linked into both keyrings"); + while_another_session_holds(NAME, || { + assert_eq!(found(&[key]), vec![StrayCopy { key: key.into(), differs: false, elsewhere: 1 }]); + // Without /proc/keys the copy cannot be seen, and the check says so. + let blind = kernel::stray_check(&[key.to_string()], unreadable()); + assert!(blind.copies.is_empty() && blind.limited.is_some(), "{blind:?}"); + // It is not the key read here. + assert_eq!(value(&KernelKeyring, key).as_deref(), Some("sk-current-value")); + }); + println!("{RAN}"); + } + /// A value written in one session is the value every later session reads, and a /// replacement updates the same object instead of adding a second one. #[test] diff --git a/crates/core/src/tasks.rs b/crates/core/src/tasks.rs index 89b86c6c..3ba2c429 100644 --- a/crates/core/src/tasks.rs +++ b/crates/core/src/tasks.rs @@ -234,8 +234,11 @@ pub enum Decision { } /// One card per kind per workspace: a pairing card and a sensitive card merge with the -/// open one of their kind; a one-time card never merges. -pub fn create_approval_task(ctx: &Ctx, project: &Path, agent: &str, names: &[String], kind: ApprovalKind) -> Result { +/// open one of their kind; a one-time card never merges. `asked_again` lists the entries +/// an agent asks for again after the person declined them (`need --force`). The card says +/// so first, one sentence per key, whether it is new or merged into, so a key asked for the +/// first time on the same card is not called a second ask. +pub fn create_approval_task(ctx: &Ctx, project: &Path, agent: &str, names: &[String], kind: ApprovalKind, asked_again: &[String]) -> Result { let pid = project.to_string_lossy().to_string(); if kind != ApprovalKind::Once { // Read-merge-write under the write lock: two processes growing the same card must @@ -250,11 +253,13 @@ pub fn create_approval_task(ctx: &Ctx, project: &Path, agent: &str, names: &[Str merged.push(n.clone()); } } - if merged == t.names { + let why = with_asked_again(t.why.as_deref().unwrap_or_default(), asked_again, agent); + if merged == t.names && t.why.as_deref() == Some(why.as_str()) { return Ok(Some(t)); } - if ctx.db.update_task_names(&t.id, &merged)? { + if ctx.db.update_task_names(&t.id, &merged)? && ctx.db.set_task_why(&t.id, &why)? { t.names = merged; + t.why = Some(why); return Ok(Some(t)); } Ok(None) // answered between our read and this write: a new card, not a free ride @@ -266,12 +271,14 @@ pub fn create_approval_task(ctx: &Ctx, project: &Path, agent: &str, names: &[Str return Ok(t); } } - let shown: Vec = names.iter().map(|n| n.strip_suffix("@default").unwrap_or(n).to_string()).collect(); + let shown: Vec = names.iter().map(|n| shown_entry(n).to_string()).collect(); let short = crate::project::short(project); + // A card that only asks again does not call itself the directory's first ask. + let first = if names.iter().all(|n| asked_again.contains(n)) { "" } else { "First time this directory asks for stored keys. " }; let (title, why) = match kind { ApprovalKind::Pairing => ( format!("{short} wants {}", if shown.len() == 1 { shown[0].clone() } else { format!("{} keys", shown.len()) }), - format!("First time this directory asks for stored keys. \"Allow these\" writes exactly these into {}: {}. \"Allow these + any non-sensitive key here\" also lets this directory receive any registry-confirmed non-sensitive key for the same identity, without asking. Nothing applies to any other directory.", project.join(&ctx.cfg.env_file).display(), shown.join(", ")), + format!("{first}\"Allow these\" writes exactly these into {}: {}. \"Allow these + any non-sensitive key here\" also lets this directory receive any registry-confirmed non-sensitive key for the same identity, without asking. Nothing applies to any other directory.", project.join(&ctx.cfg.env_file).display(), shown.join(", ")), ), ApprovalKind::Sensitive => ( format!("{short} wants sensitive key(s): {}", shown.join(", ")), @@ -285,7 +292,7 @@ pub fn create_approval_task(ctx: &Ctx, project: &Path, agent: &str, names: &[Str // The directory name is the agent's to choose (`mkdir`, `cd`): a card title must not // carry its control, bidi or zero-width characters into `tasks` output or the inbox. let title = clean_text(&title, MAX_TITLE_CHARS); - let why = clean_text(&why, MAX_APPROVAL_WHY_CHARS); + let why = with_asked_again(&why, asked_again, agent); let t = Task { id: new_id("a"), kind: TaskKind::Approval, @@ -311,6 +318,24 @@ pub fn create_approval_task(ctx: &Ctx, project: &Path, agent: &str, names: &[Str Ok(t) } +/// An approval entry as a card shows it: `NAME`, or `NAME@identity` for any other identity. +fn shown_entry(entry: &str) -> &str { + entry.strip_suffix("@default").unwrap_or(entry) +} + +/// An approval card's `why` with a sentence in front for each entry of `asked_again` it does +/// not name yet, capped like any approval text. +fn with_asked_again(why: &str, asked_again: &[String], agent: &str) -> String { + let mut notes = String::new(); + for entry in asked_again { + let lead = format!("Asked again after you declined {},", shown_entry(entry)); + if !why.contains(&lead) && !notes.contains(&lead) { + notes.push_str(&format!("{lead} because you asked {agent} to. ")); + } + } + clean_text(&format!("{notes}{why}"), MAX_APPROVAL_WHY_CHARS) +} + pub struct HumanRequest { pub title: String, pub why: Option, @@ -532,25 +557,26 @@ pub fn store_and_inject( /// first to take the index write lock stores its own. The second finds that finished store, /// keeps its value, and writes that one to the env file. Storing the second value would /// replace the first in the stash after the first had reached the env file, and the -/// application's signing key would change on a later request. `read_at` is [`crate::now`] -/// taken by the caller just before the stash read that found the key missing or stale. -/// Returns the env file written and whether `value` is the one stored. -pub fn store_generated(ctx: &Ctx, name: &str, identity: &str, value: &SecretString, project: &Path, agent: &str, read_at: &str) -> Result<(Option, bool)> { +/// application's signing key would change on a later request. `read_mark` is +/// [`crate::db::Db::audit_mark`] taken by the caller just before the stash read that found +/// the key missing or stale. Returns the env file written and whether `value` is the one +/// stored. +pub fn store_generated(ctx: &Ctx, name: &str, identity: &str, value: &SecretString, project: &Path, agent: &str, read_mark: i64) -> Result<(Option, bool)> { let mut stored = true; let provider = registry::lookup(name).map(|p| p.provider.clone()); let written = store_and_inject_gated(ctx, name, identity, value, provider, None, false, project, agent, None, Verified::Unknown, crate::db::GRANT_GENERATED, |_| { // A finished store wrote its stash value after the caller's read, which found - // nothing usable, and its index row commits with that value. So the value counts as - // another process's finished store only if its row is fresh and was recorded at or - // after `read_at`. A value with no row, or beside a row recorded before that read, - // is left by a store whose index write or COMMIT failed after its stash write, the - // one step SQLite cannot roll back. That store returned before its env file write, - // so no application has the value, and this value is stored in its place with the - // row and the grant. Keeping it would deliver a value no row or grant describes. - // Timestamps have one-second resolution. A row from the same second as the read - // counts as later, which errs toward keeping a finished store. + // nothing usable, and its index row and audit row commit with that value. So the + // value counts as another process's finished store only if its row is fresh and a + // store, import or adoption of this key was recorded after `read_mark`. The row's + // `created` time cannot say that, because an import keeps the time from its bundle, + // older than the read. A value with no such record is left by a store whose index write or + // COMMIT failed after its stash write, the one step SQLite cannot roll back. That + // store returned before its env file write, so no application has the value, and + // this value is stored in its place with the row and the grant. Keeping it would + // deliver a value no row or grant describes. let present = ctx.stash.get(&stash_key(name, identity))?.is_some(); - let finished = present && ctx.db.get_secret(name, identity)?.is_some_and(|m| !m.stale && m.created.as_str() >= read_at); + let finished = present && ctx.db.get_secret(name, identity)?.is_some_and(|m| !m.stale) && ctx.db.recorded_since(name, identity, read_mark)?; stored = !finished; Ok(stored) })?; @@ -839,7 +865,7 @@ pub fn answer_approval(ctx: &Ctx, task: &Task, decision: Decision, seen: Option< // generatable name is approved *before* anything is generated, so the stash is // empty here. Approving is the human saying yes to the delivery — generate it now, // or the card would be answered and nothing would ever arrive. - let read_at = crate::now(); + let read_mark = ctx.db.audit_mark()?; let stashed = ctx.stash.get(&stash_key(n, identity))?; let stashed = match stashed { Some(v) => Some(v), @@ -852,7 +878,7 @@ pub fn answer_approval(ctx: &Ctx, task: &Task, decision: Decision, seen: Option< match adopted.or_else(|| crate::need::generate(&spec)) { Some(v) => { let stored = if source == crate::db::GRANT_GENERATED { - store_generated(ctx, n, identity, &v, project, &task.agent, &read_at).map(|_| ()) + store_generated(ctx, n, identity, &v, project, &task.agent, read_mark).map(|_| ()) } else { store_and_inject(ctx, n, identity, &v, registry::lookup(n).map(|p| p.provider.clone()), None, false, project, &task.agent, None, Verified::Unknown, source).map(|_| ()) }; diff --git a/crates/core/src/tests.rs b/crates/core/src/tests.rs index 49446038..269037fc 100644 --- a/crates/core/src/tests.rs +++ b/crates/core/src/tests.rs @@ -328,6 +328,31 @@ fn redactor_masks_each_line_of_a_multiline_value() { assert_eq!(r.redact("x runHook preBuild\nreturn 0; y"), "x [redacted] y"); } +/// Greptile on #64: a compact service-account JSON holds its private key on one line, with +/// the key's line breaks written as the two characters `\n` (or `\r\n`). A child that +/// decodes the JSON prints the key one line at a time, and each of those lines is masked, +/// by the same length rules as a real line. +#[test] +fn redactor_masks_the_lines_of_a_key_held_with_escaped_line_breaks() { + let body: Vec = (0..3).map(|i| format!("MIIfake{i}").repeat(8)).collect(); + for sep in ["\\n", "\\r\\n"] { + let json = format!(r#"{{"type":"service_account","private_key":"-----BEGIN PRIVATE KEY-----{sep}{}{sep}-----END PRIVATE KEY-----{sep}","client_email":"ci@example.iam.gserviceaccount.com"}}"#, body.join(sep)); + assert!(!json.contains(['\n', '\r']), "the value is one line"); + let r = redact::Redactor::new().with(&SecretString::from(json.clone())); + for line in &body { + assert_eq!(r.redact(line), "[redacted]", "{sep}"); + } + // the armor is not secret and stays readable + assert_eq!(r.redact("-----END PRIVATE KEY-----"), "-----END PRIVATE KEY-----", "{sep}"); + // the whole value is still one match + assert_eq!(r.redact(&format!("sa: {json}")), "sa: [redacted]", "{sep}"); + } + // A part shorter than a stored secret can be is masked only where it stands alone. + let r = redact::Redactor::new().with(&SecretString::from("apple\\nberry".to_string())); + assert_eq!(r.redact("apple"), "[redacted]"); + assert_eq!(r.redact("pineapple blueberry"), "pineapple blueberry"); +} + #[test] fn a_pem_block_is_found_raw_or_inside_other_text() { let body = "MIIfake0MIIfake0\nMIIfake1MIIfake1"; @@ -1158,14 +1183,14 @@ fn a_run_shim_approval_is_not_a_standing_grant() { let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: stash.as_ref(), probe: tasks::Probe::Off }; let ws = db.workspace_for(&proj).unwrap(); // a program-derived approval is one-time - let t = tasks::create_approval_task(&ctx, &proj, "test", &["STRIPE_SECRET_KEY@default".to_string()], tasks::ApprovalKind::Once).unwrap(); + let t = tasks::create_approval_task(&ctx, &proj, "test", &["STRIPE_SECRET_KEY@default".to_string()], tasks::ApprovalKind::Once, &[]).unwrap(); assert_eq!(t.expects, tasks::APPROVAL_ONCE); tasks::answer_approval(&ctx, &t, tasks::Decision::Allow, None).unwrap(); assert_eq!(db.get_task(&t.id).unwrap().unwrap().status, db::TaskStatus::Answered, "the answer is recorded"); assert!(db.grant_source(&ws.id, "STRIPE_SECRET_KEY", "default").unwrap().is_none(), "but no grant was written"); assert!(matches!(trust::gate(&db, &ws, "STRIPE_SECRET_KEY", "default", true, true).unwrap(), trust::Gate::NeedsApproval { .. }), "the next bare request asks again"); // an ordinary (human-facing) sensitive approval does persist - let t2 = tasks::create_approval_task(&ctx, &proj, "test", &["STRIPE_SECRET_KEY@default".to_string()], tasks::ApprovalKind::Sensitive).unwrap(); + let t2 = tasks::create_approval_task(&ctx, &proj, "test", &["STRIPE_SECRET_KEY@default".to_string()], tasks::ApprovalKind::Sensitive, &[]).unwrap(); tasks::answer_approval(&ctx, &t2, tasks::Decision::Allow, None).unwrap(); assert_eq!(db.grant_source(&ws.id, "STRIPE_SECRET_KEY", "default").unwrap().as_deref(), Some(db::GRANT_SENSITIVE)); std::env::set_var("TOKENSTASH_HOME", base_home()); @@ -1397,7 +1422,7 @@ fn rotation_never_rewrites_a_project_without_a_standing_grant_and_ordinary_paste let t = tasks::create_secret_task(&ctx, &proj_a, "test", "GROQ_API_KEY", "default", &Default::default()).unwrap(); tasks::answer_secret(&ctx, &t, SecretString::from("gsk_old_aaaaaaaaaaaaaaaa".to_string()), true).unwrap(); let ws_b = db.workspace_for(&proj_b).unwrap(); - let once = tasks::create_approval_task(&ctx, &proj_b, "run", &["GROQ_API_KEY@default".to_string()], tasks::ApprovalKind::Once).unwrap(); + let once = tasks::create_approval_task(&ctx, &proj_b, "run", &["GROQ_API_KEY@default".to_string()], tasks::ApprovalKind::Once, &[]).unwrap(); tasks::answer_approval(&ctx, &once, tasks::Decision::Allow, None).unwrap(); assert!(std::fs::read_to_string(proj_b.join(".env.local")).unwrap().contains("gsk_old_")); assert!(db.grant_source(&ws_b.id, "GROQ_API_KEY", "default").unwrap().is_none(), "one-time approval left no grant"); @@ -2356,7 +2381,7 @@ fn rotation_with_a_broad_grant_never_writes_a_sensitive_key() { tasks::answer_secret(&ctx, &t, SecretString::from("sk_live_oldoldoldold".to_string()), true).unwrap(); let ws_b = db.workspace_for(&proj_b).unwrap(); db.grant(&ws_b.id, "*", "default", db::GRANT_BROAD, db::GRANT_PAIRING).unwrap(); - let once = tasks::create_approval_task(&ctx, &proj_b, "run", &["STRIPE_SECRET_KEY@default".to_string()], tasks::ApprovalKind::Once).unwrap(); + let once = tasks::create_approval_task(&ctx, &proj_b, "run", &["STRIPE_SECRET_KEY@default".to_string()], tasks::ApprovalKind::Once, &[]).unwrap(); tasks::answer_approval(&ctx, &once, tasks::Decision::Allow, None).unwrap(); assert!(std::fs::read_to_string(proj_b.join(".env.local")).unwrap().contains("sk_live_old")); let card = tasks::rotate(&ctx, &proj_a, "human", "STRIPE_SECRET_KEY", "default").unwrap(); @@ -2742,14 +2767,18 @@ fn concurrent_first_requests_keep_the_first_generated_secret() { let identity = need::project_identity(&proj); let key = stash::stash_key("JWT_SECRET", &identity); let first = "first-generated-value-0123456789abcdef"; - // Right after this `need` finds the stash empty, the other one commits its value. Its - // env file write has not run yet. + // Right after this `need` finds the stash empty, the other one commits its value with + // its index row and audit line, as `record_stored` writes them. Its env file write has + // not run yet. let stash = GetHookStash { inner: stash::open(&cfg).unwrap(), armed: std::cell::Cell::new(false), hook: || { - other_stash.set(&key, &SecretString::from(first.to_string())).unwrap(); - other_db.upsert_secret(&db::SecretMeta { name: "JWT_SECRET".into(), identity: identity.clone(), provider: None, sensitive: false, source_url: None, created: now(), last_used: None, stale: false, last_verified: None, stale_reason: None, stale_source: None, next_probe: None, verify_off: false }).unwrap(); + other_db.locked(|| { + other_stash.set(&key, &SecretString::from(first.to_string()))?; + other_db.upsert_secret(&db::SecretMeta { name: "JWT_SECRET".into(), identity: identity.clone(), provider: None, sensitive: false, source_url: None, created: now(), last_used: None, stale: false, last_verified: None, stale_reason: None, stale_source: None, next_probe: None, verify_off: false })?; + other_db.audit(None, Some("other"), "store", Some("JWT_SECRET"), Some(&identity), None) + }).unwrap(); }, }; let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: &stash, probe: tasks::Probe::Off }; @@ -2763,6 +2792,48 @@ fn concurrent_first_requests_keep_the_first_generated_secret() { std::env::set_var("TOKENSTASH_HOME", base_home()); std::env::remove_var("TOKENSTASH_STASH"); } +/// Greptile on #60: `tokenstash import` can store a generated key after a `need` found it +/// missing and before that `need` stores the value it generated. The import keeps the key's +/// `created` time from the bundle, older than the `need`'s read, but it is a finished store +/// all the same. The `need` keeps the imported value and writes it to the env file, so the +/// application's signing key is the one the user imported. +#[test] +fn a_generated_key_imported_while_need_generates_one_is_kept() { + let _g = env_lock(); + let (home, proj) = v2_world("gen-import"); + let cfg = Config::default(); + let db = Db::open(&home.join("t.db")).unwrap(); + let other_db = Db::open(&home.join("t.db")).unwrap(); + let other_stash = stash::open(&cfg).unwrap(); + let identity = need::project_identity(&proj); + let key = stash::stash_key("JWT_SECRET", &identity); + let imported = "imported-signing-key-0123456789abcdef"; + // Right after this `need` finds the stash empty, an import stores the key the way + // `bundle::apply_entry` does: stash value, index row with the bundle's `created`, and + // audit line, under the index write lock. + let stash = GetHookStash { + inner: stash::open(&cfg).unwrap(), + armed: std::cell::Cell::new(false), + hook: || { + other_db.locked(|| { + other_stash.set(&key, &SecretString::from(imported.to_string()))?; + other_db.upsert_secret(&db::SecretMeta { name: "JWT_SECRET".into(), identity: identity.clone(), provider: None, sensitive: false, source_url: None, created: "2020-01-01T00:00:00Z".into(), last_used: None, stale: false, last_verified: None, stale_reason: None, stale_source: None, next_probe: None, verify_off: false })?; + other_db.audit(None, None, "import", Some("JWT_SECRET"), Some(&identity), Some("bundle")) + }).unwrap(); + }, + }; + let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: &stash, probe: tasks::Probe::Off }; + stash.armed.set(true); + let out = need::need(&ctx, &proj, "agent", &["JWT_SECRET".to_string()], &Default::default()).unwrap(); + let stored = stash.get(&key).unwrap().unwrap(); + assert_eq!(secrecy::ExposeSecret::expose_secret(&stored), imported, "the imported value stays in the stash"); + let text = std::fs::read_to_string(proj.join(".env.local")).unwrap(); + assert_eq!(envfile::parse_line(text.lines().next().unwrap()).unwrap().1, imported, "and is the one in the env file"); + assert!(matches!(&out[0], need::Outcome::Injected { generated: false, .. }), "this call kept a value it did not generate: {:?}", out[0]); + assert_eq!(db.get_secret("JWT_SECRET", &identity).unwrap().unwrap().created, "2020-01-01T00:00:00Z", "the import's row is untouched"); + std::env::set_var("TOKENSTASH_HOME", base_home()); std::env::remove_var("TOKENSTASH_STASH"); +} + /// A generated value in the stash with no index row is a store whose index write failed /// after its stash write. It is not a finished store, so a first request stores its own /// value with the index row and the grant, and the next request is a plain delivery. @@ -3345,7 +3416,7 @@ fn an_approval_card_title_is_cleaned_of_the_directory_names_control_characters() let db = Db::open(&home.join("t.db")).unwrap(); let st = stash::FileStash::new().unwrap(); let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: &st, probe: tasks::Probe::Off }; - let t = tasks::create_approval_task(&ctx, &proj, "agent", &["OPENAI_API_KEY@default".to_string()], tasks::ApprovalKind::Pairing).unwrap(); + let t = tasks::create_approval_task(&ctx, &proj, "agent", &["OPENAI_API_KEY@default".to_string()], tasks::ApprovalKind::Pairing, &[]).unwrap(); assert!(!t.title.contains('\u{202e}') && !t.title.contains('\u{1b}'), "{:?}", t.title); assert!(t.title.contains("dir") && t.title.contains("wants"), "{:?}", t.title); std::env::set_var("TOKENSTASH_HOME", base_home()); @@ -4019,7 +4090,7 @@ fn asking_again_after_a_no_files_a_card_even_under_a_broad_grant() { let name = ["GROQ_API_KEY".to_string()]; let plain = need::need(&ctx, &proj, "agent", &name, &need::NeedOpts::default()).unwrap(); assert!(matches!(plain[0], need::Outcome::Denied { .. }), "{plain:?}"); - let again = need::need(&ctx, &proj, "agent", &name, &need::NeedOpts { force: true, ask_again: true, ..Default::default() }).unwrap(); + let again = need::need(&ctx, &proj, "agent", &name, &need::NeedOpts { ask_again: vec!["GROQ_API_KEY@default".into()], ..Default::default() }).unwrap(); assert!(matches!(again[0], need::Outcome::Pending { .. }), "a card, not the key: {again:?}"); assert!(!crate::envfile::has(&proj, ".env.local", "GROQ_API_KEY"), "nothing written"); let person = need::need(&ctx, &proj, "human", &name, &need::NeedOpts { force: true, ..Default::default() }).unwrap(); @@ -4084,6 +4155,350 @@ fn an_action_claim_belongs_to_its_worker_and_runs_out() { assert!(tasks::deny(&ctx, &u, None).is_ok()); } +/// Set a claim's time back to 2000, as if it had gone unrenewed past its hold time. The +/// claim's token stays, so the claim is still the same one. +fn age_claim(db: &Db, id: &str) { + db.conn.execute("UPDATE tasks SET note=?2 || substr(note, -17) WHERE id=?1", rusqlite::params![id, format!("{}2000-01-01T00:00:00Z", db::CONFIRMING)]).unwrap(); +} + +/// Store `value` the way a store does: the stash, then a new index row. +fn store_default(db: &Db, stash: &dyn Stash, name: &str, value: &str) { + stash.set(&stash::stash_key(name, "default"), &SecretString::from(value.to_string())).unwrap(); + db.upsert_secret(&db::SecretMeta { name: name.into(), identity: "default".into(), provider: None, sensitive: false, source_url: None, created: now(), last_used: None, stale: false, last_verified: None, stale_reason: None, stale_source: None, next_probe: None, verify_off: false }).unwrap(); +} + +fn stashed(stash: &dyn Stash, name: &str) -> Option { + stash.get(&stash::stash_key(name, "default")).unwrap().map(|v| secrecy::ExposeSecret::expose_secret(&v).to_string()) +} + +/// Greptile on #65: an action that runs longer than a claim holds (`claude mcp add` on a slow +/// machine) keeps its card while it runs: its claim is renewed, so no other confirm takes it +/// over and no decline lands. Once nothing renews it (the process stopped), it runs out and +/// the card is free again. +#[test] +fn a_running_action_keeps_its_claim_past_the_hold_time() { + use crate::actions::Action; + use std::time::{Duration, Instant}; + let _env = env_lock(); + let home = tmp("keep-claim-home"); + std::env::set_var("TOKENSTASH_HOME", &home); + std::env::set_var("TOKENSTASH_STASH", "insecure-file"); + let proj = tmp("keep-claim-proj").canonicalize().unwrap(); + let cfg = Config::default(); + let db = Db::open(&home.join("t.db")).unwrap(); + let stash = stash::open(&cfg).unwrap(); + let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: stash.as_ref(), probe: tasks::Probe::Off }; + let t = crate::actions::request(&ctx, &proj, "agent", &Action::Mcp(true), None).unwrap(); + let claim = db.claim_action(&t.id).unwrap().unwrap(); + let kept = db.keep_claim_every(&t.id, &claim, Duration::from_millis(20)).unwrap(); + // The action has been running for longer than a claim holds; the next renewal brings its + // time back to now. + age_claim(&db, &t.id); + let started = Instant::now(); + while db.get_task(&t.id).unwrap().unwrap().note.unwrap_or_default().contains("2000-01-01") { + assert!(started.elapsed() < Duration::from_secs(10), "the claim was not renewed"); + std::thread::sleep(Duration::from_millis(10)); + } + assert!(db.claim_action(&t.id).unwrap().is_none(), "no other confirm takes a running action's card"); + let err = tasks::deny(&ctx, &t, None).unwrap_err(); + assert!(format!("{err:#}").contains("being carried out"), "{err:#}"); + // The process stops: nothing renews the claim, and once it runs out the card is free. + drop(kept); + age_claim(&db, &t.id); + std::thread::sleep(Duration::from_millis(100)); + assert!(db.get_task(&t.id).unwrap().unwrap().note.unwrap_or_default().contains("2000-01-01"), "a stopped keeper renews nothing"); + assert!(tasks::deny(&ctx, &t, None).is_ok()); + std::env::set_var("TOKENSTASH_HOME", base_home()); +} + +/// Greptile on #60: a card whose action is being carried out does not expire under it, even +/// past its deadline, so the confirm closes it with what it did. A claim left by a stopped +/// process, or given back after a failure, holds nothing up: the card expires. +#[test] +fn a_card_being_carried_out_does_not_expire() { + use crate::actions::Action; + let _env = env_lock(); + let home = tmp("claim-expiry-home"); + std::env::set_var("TOKENSTASH_HOME", &home); + std::env::set_var("TOKENSTASH_STASH", "insecure-file"); + let proj = tmp("claim-expiry-proj").canonicalize().unwrap(); + let cfg = Config::default(); + let db = Db::open(&home.join("t.db")).unwrap(); + let stash = stash::open(&cfg).unwrap(); + let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: stash.as_ref(), probe: tasks::Probe::Off }; + let running = crate::actions::request(&ctx, &proj, "agent", &Action::Undo, None).unwrap(); + let stopped = crate::actions::request(&ctx, &proj, "agent", &Action::Mode("explicit".into()), None).unwrap(); + let failed = crate::actions::request(&ctx, &proj, "agent", &Action::Mcp(false), None).unwrap(); + let claim = db.claim_action(&running.id).unwrap().unwrap(); + db.claim_action(&stopped.id).unwrap().unwrap(); + age_claim(&db, &stopped.id); + let gave_back = db.claim_action(&failed.id).unwrap().unwrap(); + db.release_action_claim(&failed.id, &gave_back).unwrap(); + // The person confirmed near the deadline, and it passes while the actions run. + db.conn.execute("UPDATE tasks SET deadline='2000-01-01T00:00:00Z'", []).unwrap(); + assert_eq!(db.expire_overdue().unwrap(), 2); + let status = |id: &str| db.get_task(id).unwrap().unwrap().status; + assert_eq!(status(&running.id), db::TaskStatus::Pending, "a card being carried out does not expire"); + assert_eq!(status(&stopped.id), db::TaskStatus::Expired); + assert_eq!(status(&failed.id), db::TaskStatus::Expired); + assert!(db.renew_action_claim(&running.id, &claim).unwrap()); + assert!(db.finish_action(&running.id, &claim, "Took tokenstash out of your agents").unwrap(), "a renewed claim still closes its card"); + let done = db.get_task(&running.id).unwrap().unwrap(); + assert_eq!((done.status, done.note.as_deref()), (db::TaskStatus::Answered, Some("Took tokenstash out of your agents"))); + std::env::set_var("TOKENSTASH_HOME", base_home()); +} + +/// The file stash, but `delete` stops right after removing the value: the process carrying out +/// a forget stopped between the stash and the index. +struct StopsAfterDelete(Box); + +impl Stash for StopsAfterDelete { + fn backend(&self) -> &'static str { "stops-after-delete" } + fn get(&self, key: &str) -> anyhow::Result> { self.0.get(key) } + fn set(&self, key: &str, value: &SecretString) -> anyhow::Result<()> { self.0.set(key, value) } + fn delete(&self, key: &str) -> anyhow::Result { + self.0.delete(key)?; + anyhow::bail!("stopped after deleting the value") + } +} + +/// Greptile on #66, #67 and #71: a forget card confirmed again after a confirm that stopped +/// half way (the key deleted, the card still open) does not delete a key the person stored +/// since. The first confirm records which value the card is about before deleting anything; +/// a later confirm deletes only that value. +#[test] +fn a_forget_confirmed_again_after_a_stop_keeps_a_key_stored_since() { + use crate::actions::{Action, Forgot}; + let _env = env_lock(); + let home = tmp("forget-retry-home"); + std::env::set_var("TOKENSTASH_HOME", &home); + std::env::set_var("TOKENSTASH_STASH", "insecure-file"); + let proj = tmp("forget-retry-proj").canonicalize().unwrap(); + let cfg = Config::default(); + let db = Db::open(&home.join("t.db")).unwrap(); + let stash = stash::open(&cfg).unwrap(); + let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: stash.as_ref(), probe: tasks::Probe::Off }; + let forget = Action::Forget { name: "OPENAI_API_KEY".into(), identity: "default".into() }; + + // The first confirm deletes the key, then stops before closing the card. + store_default(&db, stash.as_ref(), "OPENAI_API_KEY", "sk-proj-first0123456789abcdef"); + let t = crate::actions::request(&ctx, &proj, "agent", &forget, None).unwrap(); + let first = db.claim_action(&t.id).unwrap().unwrap(); + assert_eq!(crate::actions::forget(&ctx, "OPENAI_API_KEY", "default", Some((&t.id, &first))).unwrap(), Forgot::Deleted); + assert_eq!(stashed(stash.as_ref(), "OPENAI_API_KEY"), None); + age_claim(&db, &t.id); + // The person stores the key again, then confirms the card that is still open. + store_default(&db, stash.as_ref(), "OPENAI_API_KEY", "sk-proj-storedsince0123456789ab"); + let second = db.claim_action(&t.id).unwrap().expect("a claim that ran out is taken over"); + assert_eq!(crate::actions::forget(&ctx, "OPENAI_API_KEY", "default", Some((&t.id, &second))).unwrap(), Forgot::Gone { kept: true }); + assert_eq!(stashed(stash.as_ref(), "OPENAI_API_KEY").as_deref(), Some("sk-proj-storedsince0123456789ab"), "the key stored since stays"); + assert!(db.get_secret("OPENAI_API_KEY", "default").unwrap().is_some(), "with its index row"); + assert!(db.finish_action(&t.id, &second, "kept").unwrap()); + + // Stopped between the stash and the index: the value is gone, its index row rolled back. + // The claim is given back, as `confirm` does after a failure, and the key stored again. + let u = crate::actions::request(&ctx, &proj, "agent", &forget, None).unwrap(); + assert_ne!(u.id, t.id); + let stops = StopsAfterDelete(stash::open(&cfg).unwrap()); + let ctx_stops = tasks::Ctx { cfg: &cfg, db: &db, stash: &stops, probe: tasks::Probe::Off }; + let c = db.claim_action(&u.id).unwrap().unwrap(); + assert!(crate::actions::forget(&ctx_stops, "OPENAI_API_KEY", "default", Some((&u.id, &c))).is_err()); + db.release_action_claim(&u.id, &c).unwrap(); + assert_eq!(stashed(stash.as_ref(), "OPENAI_API_KEY"), None); + assert!(db.get_secret("OPENAI_API_KEY", "default").unwrap().is_some(), "the index deletion rolled back"); + store_default(&db, stash.as_ref(), "OPENAI_API_KEY", "sk-proj-storedagain0123456789ab"); + let c = db.claim_action(&u.id).unwrap().unwrap(); + assert_eq!(crate::actions::forget(&ctx, "OPENAI_API_KEY", "default", Some((&u.id, &c))).unwrap(), Forgot::Gone { kept: true }); + assert_eq!(stashed(stash.as_ref(), "OPENAI_API_KEY").as_deref(), Some("sk-proj-storedagain0123456789ab")); + db.finish_action(&u.id, &c, "kept").unwrap(); + + // Stopped the same way, nothing stored since: confirming again finishes the job. + let v = crate::actions::request(&ctx, &proj, "agent", &forget, None).unwrap(); + let c = db.claim_action(&v.id).unwrap().unwrap(); + assert!(crate::actions::forget(&ctx_stops, "OPENAI_API_KEY", "default", Some((&v.id, &c))).is_err()); + db.release_action_claim(&v.id, &c).unwrap(); + let c = db.claim_action(&v.id).unwrap().unwrap(); + assert_eq!(crate::actions::forget(&ctx, "OPENAI_API_KEY", "default", Some((&v.id, &c))).unwrap(), Forgot::Deleted); + assert!(db.get_secret("OPENAI_API_KEY", "default").unwrap().is_none()); + // ...and a confirm after one that finished deleting, nothing stored since, deletes nothing. + age_claim(&db, &v.id); + let c = db.claim_action(&v.id).unwrap().unwrap(); + assert_eq!(crate::actions::forget(&ctx, "OPENAI_API_KEY", "default", Some((&v.id, &c))).unwrap(), Forgot::Gone { kept: false }); + assert!(db.finish_action(&v.id, &c, "done").unwrap()); + // A confirm that lost its claim records nothing and deletes nothing. + store_default(&db, stash.as_ref(), "OPENAI_API_KEY", "sk-proj-lastone0123456789abcdef"); + let w = crate::actions::request(&ctx, &proj, "agent", &forget, None).unwrap(); + assert!(crate::actions::forget(&ctx, "OPENAI_API_KEY", "default", Some((&w.id, "confirming since 2026-01-01T00:00:00Z 0123456789abcdef"))).is_err()); + assert!(stashed(stash.as_ref(), "OPENAI_API_KEY").is_some()); + std::env::set_var("TOKENSTASH_HOME", base_home()); +} + +/// Greptile on #76: a value with no index row has no id for a forget card to record. A +/// confirm after one that found such a value and stopped keeps whatever value with no index +/// row is there by then (it may be a replacement whose store stopped before its index row), +/// and says so; a new forget card removes it. +#[test] +fn a_forget_retry_keeps_a_value_with_no_index_row() { + use crate::actions::{Action, Forgot}; + let _env = env_lock(); + let home = tmp("forget-unrecorded-home"); + std::env::set_var("TOKENSTASH_HOME", &home); + std::env::set_var("TOKENSTASH_STASH", "insecure-file"); + let proj = tmp("forget-unrecorded-proj").canonicalize().unwrap(); + let cfg = Config::default(); + let db = Db::open(&home.join("t.db")).unwrap(); + let stash = stash::open(&cfg).unwrap(); + let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: stash.as_ref(), probe: tasks::Probe::Off }; + let forget = Action::Forget { name: "OPENAI_API_KEY".into(), identity: "default".into() }; + let key = stash::stash_key("OPENAI_API_KEY", "default"); + // A value in the stash with no index row here, as a store from another home leaves it. + stash.set(&key, &SecretString::from("sk-proj-noindexrow0123456789abc".to_string())).unwrap(); + let t = crate::actions::request(&ctx, &proj, "agent", &forget, None).unwrap(); + let first = db.claim_action(&t.id).unwrap().unwrap(); + assert_eq!(crate::actions::forget(&ctx, "OPENAI_API_KEY", "default", Some((&t.id, &first))).unwrap(), Forgot::Deleted, "the first confirm deletes it"); + assert_eq!(stashed(stash.as_ref(), "OPENAI_API_KEY"), None); + // That confirm stopped before closing the card. A store writes a replacement and stops + // before its index row. + age_claim(&db, &t.id); + stash.set(&key, &SecretString::from("sk-proj-replacement0123456789ab".to_string())).unwrap(); + let second = db.claim_action(&t.id).unwrap().unwrap(); + assert_eq!(crate::actions::forget(&ctx, "OPENAI_API_KEY", "default", Some((&t.id, &second))).unwrap(), Forgot::KeptUnrecorded); + assert_eq!(stashed(stash.as_ref(), "OPENAI_API_KEY").as_deref(), Some("sk-proj-replacement0123456789ab"), "the replacement stays"); + assert!(db.finish_action(&t.id, &second, "kept").unwrap(), "the card closes"); + // With nothing there by then, a later confirm has nothing to keep or delete. + let u = crate::actions::request(&ctx, &proj, "agent", &forget, None).unwrap(); + let c = db.claim_action(&u.id).unwrap().unwrap(); + assert_eq!(crate::actions::forget(&ctx, "OPENAI_API_KEY", "default", Some((&u.id, &c))).unwrap(), Forgot::Deleted, "a new card removes it"); + age_claim(&db, &u.id); + let c = db.claim_action(&u.id).unwrap().unwrap(); + assert_eq!(crate::actions::forget(&ctx, "OPENAI_API_KEY", "default", Some((&u.id, &c))).unwrap(), Forgot::NothingStored); + std::env::set_var("TOKENSTASH_HOME", base_home()); +} + +/// Greptile on #76: a confirm whose claim keeper cannot open its own connection to the index +/// gets the claim back with the error. Nothing ran, so the person can confirm the card again +/// or decline it at once, not once the claim runs out. +#[test] +fn a_keeper_that_cannot_start_gives_the_claim_back() { + use crate::actions::Action; + let _env = env_lock(); + let home = tmp("keeper-fails-home"); + std::env::set_var("TOKENSTASH_HOME", &home); + std::env::set_var("TOKENSTASH_STASH", "insecure-file"); + let proj = tmp("keeper-fails-proj").canonicalize().unwrap(); + let cfg = Config::default(); + let db = Db::open(&home.join("t.db")).unwrap(); + let stash = stash::open(&cfg).unwrap(); + let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: stash.as_ref(), probe: tasks::Probe::Off }; + let t = crate::actions::request(&ctx, &proj, "agent", &Action::Undo, None).unwrap(); + let claim = db.claim_action(&t.id).unwrap().unwrap(); + // A schema newer than this build: a second connection refuses to open the index. + db.conn.execute_batch(&format!("PRAGMA user_version = {}", db::SCHEMA_VERSION + 1)).unwrap(); + let err = db.keep_claim(&t.id, &claim).err().expect("the keeper cannot start"); + assert!(format!("{err:#}").contains("newer version"), "{err:#}"); + db.conn.execute_batch(&format!("PRAGMA user_version = {}", db::SCHEMA_VERSION)).unwrap(); + assert_eq!(db.get_task(&t.id).unwrap().unwrap().note, None, "the claim was given back"); + assert!(tasks::deny(&ctx, &t, None).is_ok(), "the person can decline at once"); + std::env::set_var("TOKENSTASH_HOME", base_home()); +} + +/// The file stash, with a hook that runs inside `delete`, after the value is removed. +struct DeleteHookStash { + inner: Box, + hook: F, +} + +impl Stash for DeleteHookStash { + fn backend(&self) -> &'static str { "delete-hook" } + fn get(&self, key: &str) -> anyhow::Result> { self.inner.get(key) } + fn set(&self, key: &str, value: &SecretString) -> anyhow::Result<()> { self.inner.set(key, value) } + fn delete(&self, key: &str) -> anyhow::Result { + let had = self.inner.delete(key)?; + (self.hook)(); + Ok(had) + } +} + +/// Greptile on #65: forget holds the index write lock across both deletions. A store that +/// starts between the stash deletion and the index deletion waits for forget to finish, so +/// it never loses its index row to forget and leaves a stored value with no record. +#[test] +fn forget_and_a_concurrent_store_do_not_interleave() { + use std::time::Duration; + let _env = env_lock(); + let home = tmp("forget-lock-home"); + std::env::set_var("TOKENSTASH_HOME", &home); + std::env::set_var("TOKENSTASH_STASH", "insecure-file"); + let cfg = Config::default(); + let db_path = home.join("t.db"); + let db = Db::open(&db_path).unwrap(); + let plain = stash::open(&cfg).unwrap(); + store_default(&db, plain.as_ref(), "OPENAI_API_KEY", "sk-proj-old0123456789abcdefgh"); + let store = std::cell::RefCell::new(None); + let hooked = DeleteHookStash { + inner: stash::open(&cfg).unwrap(), + hook: || { + // Another process stores a new value while forget is between its two deletions, + // under the index write lock, as a store does. + let db_path = db_path.clone(); + *store.borrow_mut() = Some(std::thread::spawn(move || { + let other = Db::open(&db_path).unwrap(); + let stash = stash::open(&Config::default()).unwrap(); + other.locked(|| { + store_default(&other, stash.as_ref(), "OPENAI_API_KEY", "sk-proj-new0123456789abcdefgh"); + Ok(()) + }).unwrap(); + })); + // Without the lock the store commits in this time, and forget then deletes its row. + std::thread::sleep(Duration::from_millis(300)); + }, + }; + let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: &hooked, probe: tasks::Probe::Off }; + assert_eq!(crate::actions::forget(&ctx, "OPENAI_API_KEY", "default", None).unwrap(), crate::actions::Forgot::Deleted); + store.borrow_mut().take().unwrap().join().unwrap(); + assert_eq!(stashed(plain.as_ref(), "OPENAI_API_KEY").as_deref(), Some("sk-proj-new0123456789abcdefgh")); + assert!(db.get_secret("OPENAI_API_KEY", "default").unwrap().is_some(), "the value stored meanwhile keeps its index row"); + std::env::set_var("TOKENSTASH_HOME", base_home()); +} + +/// Greptile on #65, #61 and #71: an agent in a directory paired again within the same second +/// as an event of the directory it replaced does not see that event. Times are whole seconds; +/// the agent's view starts after the last audit row written before its workspace was recorded. +#[test] +fn a_directory_paired_again_in_the_same_second_does_not_see_the_old_ones_events() { + let _env = env_lock(); + let home = tmp("audit-bound-home"); + std::env::set_var("TOKENSTASH_HOME", &home); + let proj = tmp("audit-bound-proj").canonicalize().unwrap(); + let pid = proj.to_string_lossy().to_string(); + let db = Db::open(&home.join("t.db")).unwrap(); + let old = db.workspace_for(&proj).unwrap(); + db.audit(Some(&pid), Some("agent"), "store", Some("OPENAI_API_KEY"), Some("default"), None).unwrap(); + assert_eq!(db.recent_audit_for(&pid, &old, 10).unwrap().len(), 1); + std::fs::remove_dir_all(&proj).unwrap(); + std::thread::sleep(std::time::Duration::from_millis(20)); + std::fs::create_dir_all(&proj).unwrap(); + let fresh = db.repair_workspace(&proj).unwrap(); + // Every row in the second the new directory was paired. + db.conn.execute("UPDATE audit SET ts=?1", rusqlite::params![fresh.created]).unwrap(); + let rows = db.recent_audit_for(&pid, &fresh, 10).unwrap(); + assert!(rows.is_empty(), "the old directory's events: {rows:?}"); + db.audit(Some(&pid), Some("agent"), "inject", Some("GROQ_API_KEY"), Some("default"), None).unwrap(); + db.conn.execute("UPDATE audit SET ts=?1", rusqlite::params![fresh.created]).unwrap(); + let rows = db.recent_audit_for(&pid, &fresh, 10).unwrap(); + assert_eq!(rows.iter().map(|r| r.4.as_deref()).collect::>(), vec![Some("GROQ_API_KEY")], "its own event, from the same second"); + // A record from before the bound existed gets one when the index opens: everything up to + // and including its creation second stays out. + db.conn.execute("UPDATE workspaces SET audit_from=NULL", []).unwrap(); + let rows = db.recent_audit_for(&pid, &fresh, 10).unwrap(); + assert!(rows.is_empty(), "until then, rows after its creation second only: {rows:?}"); + let db = Db::open(&home.join("t.db")).unwrap(); + assert!(db.recent_audit_for(&pid, &fresh, 10).unwrap().is_empty()); + db.audit(Some(&pid), Some("agent"), "inject", Some("RESEND_API_KEY"), Some("default"), None).unwrap(); + assert_eq!(db.recent_audit_for(&pid, &fresh, 10).unwrap().len(), 1); + std::env::set_var("TOKENSTASH_HOME", base_home()); +} + /// Greptile on #68: an exact grant from before a no does not deliver the key to an agent that /// asks again; the person gets a card. #[test] @@ -4103,7 +4518,7 @@ fn asking_again_files_a_card_even_with_an_exact_grant() { // Stored since, from another directory. stash.set(&stash::stash_key("GROQ_API_KEY", "default"), &SecretString::from("gsk_storedelsewhere_0123456789".to_string())).unwrap(); db.upsert_secret(&db::SecretMeta { name: "GROQ_API_KEY".into(), identity: "default".into(), provider: Some("Groq".into()), sensitive: false, source_url: None, created: now(), last_used: None, stale: false, last_verified: None, stale_reason: None, stale_source: None, next_probe: None, verify_off: false }).unwrap(); - let again = need::need(&ctx, &proj, "agent", &["GROQ_API_KEY".to_string()], &need::NeedOpts { force: true, ask_again: true, ..Default::default() }).unwrap(); + let again = need::need(&ctx, &proj, "agent", &["GROQ_API_KEY".to_string()], &need::NeedOpts { ask_again: vec!["GROQ_API_KEY@default".into()], ..Default::default() }).unwrap(); assert!(matches!(again[0], need::Outcome::Pending { .. }), "{again:?}"); assert!(!crate::envfile::has(&proj, ".env.local", "GROQ_API_KEY")); } @@ -4124,33 +4539,187 @@ fn the_extra_ask_is_spent_on_its_card_and_recovered_when_none_was_filed() { let stash = stash::open(&cfg).unwrap(); let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: stash.as_ref(), probe: tasks::Probe::Off }; let since = cfg.ttl_since(); - let row = db.reserve_force(&pid, "agent", "GROQ_API_KEY", &since).unwrap().expect("the first ask is free"); - assert!(db.reserve_force(&pid, "agent", "GROQ_API_KEY", &since).unwrap().is_none(), "one in flight holds it"); + let row = db.reserve_force(&pid, "agent", "GROQ_API_KEY", "default", &since).unwrap().expect("the first ask is free"); + assert!(db.reserve_force(&pid, "agent", "GROQ_API_KEY", "default", &since).unwrap().is_none(), "one in flight holds it"); // The process stopped before filing anything: a minute on, the ask comes back. db.conn.execute("UPDATE audit SET ts='2000-01-01T00:00:00Z' WHERE id=?1", rusqlite::params![row]).unwrap(); let old = cfg.ttl_since().replace(&cfg.ttl_since()[..4], "1999"); - let row = db.reserve_force(&pid, "agent", "GROQ_API_KEY", &old).unwrap().expect("a reservation left behind is taken back"); + let row = db.reserve_force(&pid, "agent", "GROQ_API_KEY", "default", &old).unwrap().expect("a reservation left behind is taken back"); // A card filed under a reservation that a stopped process never named still spends it... let filed = tasks::create_secret_task(&ctx, &proj, "agent", "RESEND_API_KEY", "default", &tasks::SecretRequest::default()).unwrap(); - let r2 = db.reserve_force(&pid, "agent", "RESEND_API_KEY", &old).unwrap().unwrap(); + let r2 = db.reserve_force(&pid, "agent", "RESEND_API_KEY", "default", &old).unwrap().unwrap(); db.conn.execute("UPDATE audit SET ts='2000-06-01T00:00:00Z' WHERE id=?1", rusqlite::params![r2]).unwrap(); db.conn.execute("UPDATE tasks SET created='2000-06-01T00:00:20Z' WHERE id=?1", rusqlite::params![filed.id]).unwrap(); - assert!(db.reserve_force(&pid, "agent", "RESEND_API_KEY", &old).unwrap().is_none(), "the card filed under it proves the ask was used"); + assert!(db.reserve_force(&pid, "agent", "RESEND_API_KEY", "default", &old).unwrap().is_none(), "the card filed under it proves the ask was used"); // ...but a later card from another request, or another agent, does not. - let r3 = db.reserve_force(&pid, "agent", "STRIPE_SECRET_KEY", &old).unwrap().unwrap(); + let r3 = db.reserve_force(&pid, "agent", "STRIPE_SECRET_KEY", "default", &old).unwrap().unwrap(); db.conn.execute("UPDATE audit SET ts='2000-06-01T00:00:00Z' WHERE id=?1", rusqlite::params![r3]).unwrap(); let later = tasks::create_secret_task(&ctx, &proj, "agent", "STRIPE_SECRET_KEY", "default", &tasks::SecretRequest::default()).unwrap(); db.conn.execute("UPDATE tasks SET created='2000-06-01T00:05:00Z' WHERE id=?1", rusqlite::params![later.id]).unwrap(); let other = tasks::create_secret_task(&ctx, &proj, "someone-else", "STRIPE_SECRET_KEY", "work", &tasks::SecretRequest::default()).unwrap(); db.conn.execute("UPDATE tasks SET created='2000-06-01T00:00:10Z' WHERE id=?1", rusqlite::params![other.id]).unwrap(); assert_eq!(db.card_since_reservation(r3, &pid, "STRIPE_SECRET_KEY").unwrap(), None); - assert!(db.reserve_force(&pid, "agent", "STRIPE_SECRET_KEY", &old).unwrap().is_some(), "an ask that never reached the user comes back"); + assert!(db.reserve_force(&pid, "agent", "STRIPE_SECRET_KEY", "default", &old).unwrap().is_some(), "an ask that never reached the user comes back"); // Spent on the card it filed: no more asks in the window. let card = tasks::create_secret_task(&ctx, &proj, "agent", "GROQ_API_KEY", "default", &tasks::SecretRequest::default()).unwrap(); assert_eq!(db.card_since_reservation(row, &pid, "GROQ_API_KEY").unwrap(), Some(card.id.clone())); db.bind_force(row, &card.id).unwrap(); db.conn.execute("UPDATE audit SET ts='2000-01-01T00:00:01Z' WHERE id=?1", rusqlite::params![row]).unwrap(); - assert!(db.reserve_force(&pid, "agent", "GROQ_API_KEY", &old).unwrap().is_none(), "a spent ask stays spent however old"); + assert!(db.reserve_force(&pid, "agent", "GROQ_API_KEY", "default", &old).unwrap().is_none(), "a spent ask stays spent however old"); +} + +/// Greptile on #63: an agent's `need --force` sets aside only the "no" to a key it holds the +/// extra ask for. Every other key keeps its denial, including one the person gave after the +/// CLI looked. A broad grant from before that "no" does not deliver it, a missing key gets no +/// new card, and a declined sensitive card is not filed again. +#[test] +fn an_agents_force_sets_aside_only_the_no_it_holds_the_ask_for() { + let _env = env_lock(); + let home = tmp("force-scope-home"); + std::env::set_var("TOKENSTASH_HOME", &home); + std::env::set_var("TOKENSTASH_STASH", "insecure-file"); + let proj = tmp("force-scope-proj").canonicalize().unwrap(); + let cfg = Config::default(); + let db = Db::open(&home.join("t.db")).unwrap(); + let stash = stash::open(&cfg).unwrap(); + let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: stash.as_ref(), probe: tasks::Probe::Off }; + for (name, value, sensitive) in [("GROQ_API_KEY", "gsk_forcescope_0123456789", false), ("MISTRAL_API_KEY", "mistral_forcescope_0123456789", false), ("TWILIO_AUTH_TOKEN", "twilio_forcescope_0123456789", true)] { + stash.set(&stash::stash_key(name, "default"), &SecretString::from(value.to_string())).unwrap(); + db.upsert_secret(&db::SecretMeta { name: name.into(), identity: "default".into(), provider: None, sensitive, source_url: None, created: now(), last_used: None, stale: false, last_verified: None, stale_reason: None, stale_source: None, next_probe: None, verify_off: false }).unwrap(); + } + let ws = db.workspace_for(&proj).unwrap(); + db.grant(&ws.id, "*", "default", db::GRANT_BROAD, db::GRANT_PAIRING).unwrap(); + // The person declined each key here. GROQ_API_KEY was declined before the agent looked, + // so the agent holds the extra ask for it; the others were declined after. + for name in ["GROQ_API_KEY", "MISTRAL_API_KEY", "RESEND_API_KEY"] { + let t = tasks::create_secret_task(&ctx, &proj, "agent", name, "default", &tasks::SecretRequest::default()).unwrap(); + tasks::deny(&ctx, &t, None).unwrap(); + } + let sensitive = tasks::create_approval_task(&ctx, &proj, "agent", &["TWILIO_AUTH_TOKEN@default".to_string()], tasks::ApprovalKind::Sensitive, &[]).unwrap(); + tasks::deny(&ctx, &sensitive, None).unwrap(); + let names: Vec = ["GROQ_API_KEY", "MISTRAL_API_KEY", "RESEND_API_KEY", "TWILIO_AUTH_TOKEN"].iter().map(|s| s.to_string()).collect(); + let opts = need::NeedOpts { ask_again: vec!["GROQ_API_KEY@default".into()], ..Default::default() }; + let out = need::need(&ctx, &proj, "agent", &names, &opts).unwrap(); + let need::Outcome::Pending { task_id, .. } = &out[0] else { panic!("the key asked for again gets a card: {out:?}") }; + assert_eq!(db.get_task(task_id).unwrap().unwrap().names, vec!["GROQ_API_KEY@default".to_string()], "the card asks for that key alone"); + for o in &out[1..] { + assert!(matches!(o, need::Outcome::Denied { .. }), "the newer no stands: {out:?}"); + } + assert!(!crate::envfile::has(&proj, ".env.local", "MISTRAL_API_KEY"), "the broad grant delivered nothing"); + assert_eq!(db.list_tasks(Some(&proj.to_string_lossy()), true).unwrap().iter().filter(|t| t.status == db::TaskStatus::Pending).count(), 1, "one card, for the key asked again"); +} + +/// Greptile on #67: the extra ask after a no is reserved per identity. The person declines +/// each identity of a key on its own card, so asking again for one leaves the other's ask, and +/// only a card for the reservation's own identity spends it. +#[test] +fn the_extra_ask_is_reserved_per_identity() { + let _env = env_lock(); + let home = tmp("force-identity-home"); + std::env::set_var("TOKENSTASH_HOME", &home); + std::env::set_var("TOKENSTASH_STASH", "insecure-file"); + let proj = tmp("force-identity-proj").canonicalize().unwrap(); + let pid = proj.to_string_lossy().to_string(); + let cfg = Config::default(); + let db = Db::open(&home.join("t.db")).unwrap(); + let stash = stash::open(&cfg).unwrap(); + let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: stash.as_ref(), probe: tasks::Probe::Off }; + let since = cfg.ttl_since(); + let work = db.reserve_force(&pid, "agent", "GROQ_API_KEY", "work", &since).unwrap().expect("work's ask"); + let personal = db.reserve_force(&pid, "agent", "GROQ_API_KEY", "personal", &since).unwrap().expect("personal keeps its own ask"); + assert!(db.reserve_force(&pid, "agent", "GROQ_API_KEY", "work", &since).unwrap().is_none(), "work's is taken"); + // A card for one identity is not the card the other's reservation filed. + let paste = tasks::create_secret_task(&ctx, &proj, "agent", "GROQ_API_KEY", "personal", &tasks::SecretRequest::default()).unwrap(); + assert_eq!(db.card_since_reservation(work, &pid, "GROQ_API_KEY").unwrap(), None); + assert_eq!(db.card_since_reservation(personal, &pid, "GROQ_API_KEY").unwrap(), Some(paste.id.clone())); + let approval = tasks::create_approval_task(&ctx, &proj, "agent", &["GROQ_API_KEY@work".to_string()], tasks::ApprovalKind::Pairing, &[]).unwrap(); + assert_eq!(db.card_since_reservation(work, &pid, "GROQ_API_KEY").unwrap(), Some(approval.id.clone())); + db.bind_force(personal, &paste.id).unwrap(); + assert_eq!(db.force_card(&pid, "GROQ_API_KEY", "personal", &since).unwrap(), Some(paste.id.clone())); + assert_eq!(db.force_card(&pid, "GROQ_API_KEY", "work", &since).unwrap(), None, "work's ask is not spent yet"); +} + +/// Greptile on #73: a reservation written before reservations named an identity still spends +/// the extra ask for every identity of the key, but the card it names comes back only for the +/// identity that card asks for. A retry for another identity gets no card of someone else's, +/// so the CLI gives its "already asked" error. +#[test] +fn an_old_reservation_returns_only_a_card_for_the_identity_retried() { + let _env = env_lock(); + let home = tmp("force-legacy-home"); + std::env::set_var("TOKENSTASH_HOME", &home); + std::env::set_var("TOKENSTASH_STASH", "insecure-file"); + let proj = tmp("force-legacy-proj").canonicalize().unwrap(); + let pid = proj.to_string_lossy().to_string(); + let cfg = Config::default(); + let db = Db::open(&home.join("t.db")).unwrap(); + let stash = stash::open(&cfg).unwrap(); + let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: stash.as_ref(), probe: tasks::Probe::Off }; + let since = cfg.ttl_since(); + let paste = tasks::create_secret_task(&ctx, &proj, "agent", "MISTRAL_API_KEY", "work", &tasks::SecretRequest::default()).unwrap(); + let approval = tasks::create_approval_task(&ctx, &proj, "agent", &["GROQ_API_KEY@work".to_string()], tasks::ApprovalKind::Pairing, &[]).unwrap(); + for (name, card) in [("MISTRAL_API_KEY", &paste.id), ("GROQ_API_KEY", &approval.id)] { + db.conn.execute( + "INSERT INTO audit (ts, project, agent, action, name, identity, detail) VALUES (?1, ?2, 'agent', 'need.force', ?3, NULL, ?4)", + rusqlite::params![now(), pid, name, card], + ).unwrap(); + for identity in ["work", "personal", "default"] { + assert!(db.reserve_force(&pid, "agent", name, identity, &since).unwrap().is_none(), "{name}@{identity}: the old row still spends the ask"); + } + assert_eq!(db.force_card(&pid, name, "work", &since).unwrap().as_ref(), Some(card), "{name}"); + for identity in ["personal", "default"] { + assert_eq!(db.force_card(&pid, name, identity, &since).unwrap(), None, "{name}@{identity} gets no card for work"); + } + } +} + +/// Greptile on #73: an agent asking again for a declined key that is stored gets an approval +/// card, and that card says first that it is a second ask, naming the key. A key asked for the +/// first time on the same card is not called a second ask, whether it was on the card before +/// or joins it later. +#[test] +fn an_approval_card_says_which_keys_are_asked_again() { + let _env = env_lock(); + let home = tmp("again-approval-home"); + std::env::set_var("TOKENSTASH_HOME", &home); + std::env::set_var("TOKENSTASH_STASH", "insecure-file"); + let proj = tmp("again-approval-proj").canonicalize().unwrap(); + let cfg = Config::default(); + let db = Db::open(&home.join("t.db")).unwrap(); + let stash = stash::open(&cfg).unwrap(); + let ctx = tasks::Ctx { cfg: &cfg, db: &db, stash: stash.as_ref(), probe: tasks::Probe::Off }; + for (name, value) in [("GROQ_API_KEY", "gsk_approvalagain_0123456789"), ("MISTRAL_API_KEY", "mistral_approvalagain_0123456789"), ("OPENROUTER_API_KEY", "sk-or-approvalagain-0123456789")] { + stash.set(&stash::stash_key(name, "default"), &SecretString::from(value.to_string())).unwrap(); + db.upsert_secret(&db::SecretMeta { name: name.into(), identity: "default".into(), provider: None, sensitive: false, source_url: None, created: now(), last_used: None, stale: false, last_verified: None, stale_reason: None, stale_source: None, next_probe: None, verify_off: false }).unwrap(); + } + let one = |n: &str| vec![n.to_string()]; + // The person declined the pairing card for GROQ_API_KEY; the agent asks again. + let first = need::need(&ctx, &proj, "agent", &one("GROQ_API_KEY"), &need::NeedOpts::default()).unwrap(); + let need::Outcome::Pending { task_id, .. } = &first[0] else { panic!("{first:?}") }; + tasks::deny(&ctx, &db.get_task(task_id).unwrap().unwrap(), None).unwrap(); + let again = need::NeedOpts { ask_again: vec!["GROQ_API_KEY@default".into()], ..Default::default() }; + let out = need::need(&ctx, &proj, "claude-code", &one("GROQ_API_KEY"), &again).unwrap(); + let need::Outcome::Pending { task_id, .. } = &out[0] else { panic!("{out:?}") }; + let card = db.get_task(task_id).unwrap().unwrap(); + let why = card.why.clone().unwrap(); + assert!(why.starts_with("Asked again after you declined GROQ_API_KEY, because you asked claude-code to. \"Allow these\""), "{why}"); + // A first ask for another key joins the same open card, and the card names only the key asked again. + let out = need::need(&ctx, &proj, "agent", &one("MISTRAL_API_KEY"), &need::NeedOpts::default()).unwrap(); + assert!(matches!(&out[0], need::Outcome::Pending { task_id, .. } if task_id == &card.id), "{out:?}"); + let why = db.get_task(&card.id).unwrap().unwrap().why.unwrap(); + assert!(!why.contains("MISTRAL_API_KEY"), "{why}"); + // An open first-ask card that a second ask joins gets the note for that key alone. + tasks::deny(&ctx, &db.get_task(&card.id).unwrap().unwrap(), None).unwrap(); + let out = need::need(&ctx, &proj, "agent", &one("OPENROUTER_API_KEY"), &need::NeedOpts::default()).unwrap(); + let need::Outcome::Pending { task_id: open, .. } = &out[0] else { panic!("{out:?}") }; + let both = need::NeedOpts { ask_again: vec!["MISTRAL_API_KEY@default".into()], ..Default::default() }; + let out = need::need(&ctx, &proj, "codex", &one("MISTRAL_API_KEY"), &both).unwrap(); + assert!(matches!(&out[0], need::Outcome::Pending { task_id, .. } if task_id == open), "{out:?}"); + let merged = db.get_task(open).unwrap().unwrap(); + let why = merged.why.unwrap(); + assert!(why.starts_with("Asked again after you declined MISTRAL_API_KEY, because you asked codex to. First time this directory asks"), "{why}"); + assert!(!why.contains("declined OPENROUTER_API_KEY") && !why.contains("declined GROQ_API_KEY"), "{why}"); + assert_eq!(merged.names, vec!["OPENROUTER_API_KEY@default".to_string(), "MISTRAL_API_KEY@default".to_string()]); }