Stop granting an unscoped host-file read, and make a refused credential scrub recoverable
`core:default` was an alias for nine core plugins' default sets, and one of them — `core:image:default` — carries `allow-from-path`, whose handler is a bare `std::fs::read(path)` with no scope mechanism at all. Nothing imports `@tauri-apps/api/image`, so the plugin is dropped rather than scoped; there is nothing to scope it with. The capability file now enumerates what `app/src` actually invokes, which is `core:event`'s listen/unlisten and nothing else from core — every emit in this app originates in Rust. `core:menu`, `core:tray`, `core:window`, `core:path`, `core:resources` and the three dead `dialog:` grants go with it. `core:webview:allow-internal-toggle-devtools` stays because Tauri's own injected debug script calls it; both it and the command behind it are `cfg(any(debug_assertions, feature = "devtools"))`, so it is absent from a release bundle. Verified empirically: an unknown identifier fails the build, so every identifier kept is real and the regenerated `gen/schemas/capabilities.json` carries the opener scope verbatim rather than silently dropping it. `opener:allow-open-url` cannot be host-narrowed — the terminal opens links Claude printed inside the container — so what it does and does not buy is recorded instead, including the verified fact that each scope entry's `app` defaults to `Application::Default`, which matches only `with == None` and therefore refuses `openUrl(url, "/bin/sh")`. `clear_claude_token` deleted the keychain entry first and swept the snapshot images second. The sweep runs once and skips a project another operation holds, the deleted entry made `has_claude_token` false, and Revoke rendered only while a token was stored — so a project that happened to be starting during a revoke kept a live ~1-year OAuth token in its snapshot's `Config.Env` permanently, with Reset (which destroys both volumes) as the only remaining remedy. The sweep now runs first, so a crash mid-revoke leaves the app still saying "authenticated" with the same button still able to finish; a busy project is reported as `snapshots_skipped` rather than folded in with images that genuinely cannot be rewritten; and the panel keeps a retry visible independent of token status, plus offers the sweep outright when nothing is stored, because a snapshot committed by an older build carries the token either way. The retry is the same command — it is idempotent, and the images are the durable record. Also: `openai-compatible-api-key` was written but never deleted, so it outlived its project. The key list is now the single definition and an unlisted key is refused outright, so the writer cannot get ahead of the deleter again. `store_or_clear_project_secret` lands here unused on purpose: the editors send a blanked field as `null` and `store_secrets_for_project` skips `None`, so clearing a secret through the UI is impossible today. Its one call site is in `commands/project_commands.rs`, which belongs to another change in this round. No `devCsp` was added. `tauri dev` loads the main document straight from Vite, and Tauri only attaches a CSP to documents it serves itself — the dev server is proxied through `tauri://` only when `PROXY_DEV_SERVER`, which is `cfg!(all(dev, mobile))`. A `devCsp` here would be inert config that reads as protection. The reasoning, and the one place that could set one, are recorded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc
This commit is contained in:
Claude Opus 5
@@ -26,47 +26,122 @@ const CLAUDE_TOKEN_VERSION_SERVICE: &str = "triple-c-claude-oauth-token-version"
|
||||
/// Fixed account name used for every triple-c keychain entry.
|
||||
const KEYCHAIN_ACCOUNT: &str = "secret";
|
||||
|
||||
/// Every per-project secret this app stores, and therefore every one it has to
|
||||
/// be able to delete.
|
||||
///
|
||||
/// This list is the **only** definition. It used to exist twice — once
|
||||
/// implicitly, as whatever `store_secrets_for_project` happened to write, and
|
||||
/// once explicitly, as a literal array inside `delete_project_secrets` — and
|
||||
/// the two drifted: `openai-compatible-api-key` was added to the writer and
|
||||
/// never to the deleter, so removing a project left a live provider API key in
|
||||
/// the user's login keychain with nothing left in the app that referenced it,
|
||||
/// or would ever offer to clean it up.
|
||||
///
|
||||
/// Drift is now a compile-time-shaped error rather than a review-time one:
|
||||
/// [`project_secret_entry`] refuses a key that is not in this list, so a new
|
||||
/// secret cannot be stored until it has been added here, and adding it here is
|
||||
/// what makes [`delete_project_secrets`] cover it.
|
||||
pub const PROJECT_SECRET_KEYS: &[&str] = &[
|
||||
"git-token",
|
||||
"aws-access-key-id",
|
||||
"aws-secret-access-key",
|
||||
"aws-session-token",
|
||||
"aws-bearer-token",
|
||||
"openai-compatible-api-key",
|
||||
];
|
||||
|
||||
/// The keychain entry for one per-project secret, rejecting any key name not in
|
||||
/// [`PROJECT_SECRET_KEYS`]. See that constant for why the rejection matters.
|
||||
fn project_secret_entry(project_id: &str, key_name: &str) -> Result<keyring::Entry, String> {
|
||||
if !PROJECT_SECRET_KEYS.contains(&key_name) {
|
||||
return Err(format!(
|
||||
"Unknown project secret '{}'. Add it to PROJECT_SECRET_KEYS so project deletion \
|
||||
clears it too.",
|
||||
key_name
|
||||
));
|
||||
}
|
||||
let service = format!("triple-c-project-{}-{}", project_id, key_name);
|
||||
keyring::Entry::new(&service, KEYCHAIN_ACCOUNT).map_err(|e| format!("Keyring error: {}", e))
|
||||
}
|
||||
|
||||
/// Store a per-project secret in the OS keychain.
|
||||
pub fn store_project_secret(project_id: &str, key_name: &str, value: &str) -> Result<(), String> {
|
||||
let service = format!("triple-c-project-{}-{}", project_id, key_name);
|
||||
let entry = keyring::Entry::new(&service, "secret")
|
||||
.map_err(|e| format!("Keyring error: {}", e))?;
|
||||
entry
|
||||
project_secret_entry(project_id, key_name)?
|
||||
.set_password(value)
|
||||
.map_err(|e| format!("Failed to store project secret '{}': {}", key_name, e))
|
||||
}
|
||||
|
||||
/// Retrieve a per-project secret from the OS keychain.
|
||||
pub fn get_project_secret(project_id: &str, key_name: &str) -> Result<Option<String>, String> {
|
||||
let service = format!("triple-c-project-{}-{}", project_id, key_name);
|
||||
let entry = keyring::Entry::new(&service, "secret")
|
||||
.map_err(|e| format!("Keyring error: {}", e))?;
|
||||
match entry.get_password() {
|
||||
match project_secret_entry(project_id, key_name)?.get_password() {
|
||||
Ok(value) => Ok(Some(value)),
|
||||
Err(keyring::Error::NoEntry) => Ok(None),
|
||||
Err(e) => Err(format!("Failed to retrieve project secret '{}': {}", key_name, e)),
|
||||
}
|
||||
}
|
||||
|
||||
/// Delete all known secrets for a project from the OS keychain.
|
||||
/// Delete one per-project secret, treating "wasn't there" as success.
|
||||
pub fn delete_project_secret(project_id: &str, key_name: &str) -> Result<(), String> {
|
||||
match project_secret_entry(project_id, key_name)?.delete_credential() {
|
||||
Ok(()) | Err(keyring::Error::NoEntry) => Ok(()),
|
||||
Err(e) => Err(format!("Failed to delete project secret '{}': {}", key_name, e)),
|
||||
}
|
||||
}
|
||||
|
||||
/// Write a per-project secret, or **clear** it when there is nothing to write.
|
||||
///
|
||||
/// This is the function every save path should call, and the reason it exists
|
||||
/// is that the obvious `if let Some(v) = … { store(v) }` is wrong. The editors
|
||||
/// in `components/projects/home/config/` send a blanked field as `null`
|
||||
/// (`AccessSection.tsx`: `save({ git_token: gitToken || null })`), so a `None`
|
||||
/// is a user asking for the secret to be *removed* — and skipping it left the
|
||||
/// old value in the keychain, where `load_secrets_for_project` read it straight
|
||||
/// back out and put it back on the project. Clearing a credential through the
|
||||
/// UI was therefore impossible: the field looked empty and the container kept
|
||||
/// getting the old token.
|
||||
///
|
||||
/// `Some("")` and `Some(" ")` are treated the same as `None` — a field the
|
||||
/// user emptied, whichever shape it arrives in — because a stored empty secret
|
||||
/// is not a secret, and `container_config` would inject it as an env var that
|
||||
/// overrides the unset case with a blank.
|
||||
// TODO(handoff): `commands/project_commands.rs::store_secrets_for_project` is
|
||||
// the one caller this is for, and it still uses the `if let Some(v) = … ` shape
|
||||
// that cannot clear anything. That file belongs to another change in this round,
|
||||
// so the switch is deliberately left to it; the six call sites there become
|
||||
// `store_or_clear_project_secret(&project.id, "<key>", field.as_deref())?`.
|
||||
#[allow(dead_code)]
|
||||
pub fn store_or_clear_project_secret(
|
||||
project_id: &str,
|
||||
key_name: &str,
|
||||
value: Option<&str>,
|
||||
) -> Result<(), String> {
|
||||
match secret_to_store(value) {
|
||||
Some(v) => store_project_secret(project_id, key_name, v),
|
||||
None => delete_project_secret(project_id, key_name),
|
||||
}
|
||||
}
|
||||
|
||||
/// The store-or-clear decision, split out so it can be tested without a
|
||||
/// keychain backend: `Some` means "write this", `None` means "remove whatever
|
||||
/// is there".
|
||||
#[allow(dead_code)]
|
||||
fn secret_to_store(value: Option<&str>) -> Option<&str> {
|
||||
match value.map(str::trim) {
|
||||
Some(v) if !v.is_empty() => Some(v),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
/// Delete every known secret for a project from the OS keychain.
|
||||
///
|
||||
/// Called when a project is removed, so it must cover [`PROJECT_SECRET_KEYS`]
|
||||
/// exhaustively — a key missed here outlives the project that explained it.
|
||||
/// One key failing does not stop the rest: a partial cleanup that keeps going
|
||||
/// leaves strictly fewer credentials behind than one that gives up.
|
||||
pub fn delete_project_secrets(project_id: &str) -> Result<(), String> {
|
||||
let secret_keys = [
|
||||
"git-token",
|
||||
"aws-access-key-id",
|
||||
"aws-secret-access-key",
|
||||
"aws-session-token",
|
||||
"aws-bearer-token",
|
||||
];
|
||||
for key_name in &secret_keys {
|
||||
let service = format!("triple-c-project-{}-{}", project_id, key_name);
|
||||
let entry = keyring::Entry::new(&service, "secret")
|
||||
.map_err(|e| format!("Keyring error: {}", e))?;
|
||||
match entry.delete_credential() {
|
||||
Ok(()) => {}
|
||||
Err(keyring::Error::NoEntry) => {}
|
||||
Err(e) => {
|
||||
log::warn!("Failed to delete project secret '{}': {}", key_name, e);
|
||||
}
|
||||
for key_name in PROJECT_SECRET_KEYS {
|
||||
if let Err(e) = delete_project_secret(project_id, key_name) {
|
||||
log::warn!("Failed to delete project secret '{}': {}", key_name, e);
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
@@ -269,3 +344,78 @@ pub fn regenerate_gateway_master_key() -> Result<String, String> {
|
||||
bump_gateway_secret_version()?;
|
||||
Ok(key)
|
||||
}
|
||||
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// The regression this list exists for. `openai-compatible-api-key` was
|
||||
/// written by `store_secrets_for_project` and missing from the delete list,
|
||||
/// so it survived project deletion.
|
||||
#[test]
|
||||
fn every_secret_the_app_writes_is_one_it_can_delete() {
|
||||
for key in [
|
||||
"git-token",
|
||||
"aws-access-key-id",
|
||||
"aws-secret-access-key",
|
||||
"aws-session-token",
|
||||
"aws-bearer-token",
|
||||
"openai-compatible-api-key",
|
||||
] {
|
||||
assert!(
|
||||
PROJECT_SECRET_KEYS.contains(&key),
|
||||
"{} is written by commands/project_commands.rs but would outlive the project",
|
||||
key
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_key_list_has_no_duplicates() {
|
||||
let mut seen = std::collections::HashSet::new();
|
||||
for key in PROJECT_SECRET_KEYS {
|
||||
assert!(seen.insert(*key), "duplicate project secret key {}", key);
|
||||
}
|
||||
}
|
||||
|
||||
/// A key that is not in the list is refused *before* any keychain entry is
|
||||
/// constructed, which is what makes the list authoritative rather than
|
||||
/// advisory. Without this, a new secret can be stored under a name nothing
|
||||
/// ever deletes.
|
||||
#[test]
|
||||
fn an_unlisted_key_cannot_be_stored_at_all() {
|
||||
let err = store_project_secret("some-project", "brand-new-token", "value")
|
||||
.expect_err("an unlisted key must be refused");
|
||||
assert!(
|
||||
err.contains("PROJECT_SECRET_KEYS"),
|
||||
"the refusal should say how to fix it: {}",
|
||||
err
|
||||
);
|
||||
|
||||
let err = get_project_secret("some-project", "brand-new-token")
|
||||
.expect_err("an unlisted key must be refused on read too");
|
||||
assert!(err.contains("brand-new-token"), "{}", err);
|
||||
|
||||
let err = delete_project_secret("some-project", "brand-new-token")
|
||||
.expect_err("an unlisted key must be refused on delete too");
|
||||
assert!(err.contains("brand-new-token"), "{}", err);
|
||||
}
|
||||
|
||||
/// The blanked-field case. `AccessSection.tsx` sends `gitToken || null`, so
|
||||
/// a cleared field arrives as `None` — and before this existed, `None` was
|
||||
/// skipped and the old secret stayed in the keychain forever.
|
||||
#[test]
|
||||
fn a_blanked_field_clears_rather_than_being_skipped() {
|
||||
assert_eq!(secret_to_store(None), None);
|
||||
assert_eq!(secret_to_store(Some("")), None);
|
||||
assert_eq!(secret_to_store(Some(" \t\n")), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_real_value_is_stored_trimmed() {
|
||||
assert_eq!(secret_to_store(Some("ghp_abc123")), Some("ghp_abc123"));
|
||||
// Pasted credentials routinely carry a trailing newline.
|
||||
assert_eq!(secret_to_store(Some(" ghp_abc123\n")), Some("ghp_abc123"));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user