From f0db529bafe205f8ed2d7ac34a9a273919036cd2 Mon Sep 17 00:00:00 2001 From: willamhou Date: Wed, 24 Jun 2026 11:44:31 +0800 Subject: [PATCH] fix(security): CLI secrets via stdin/file; cap HN comment fan-out (M5/M7) PR-H of the security-hardening plan. - M5: secret-bearing CLI args (vault token, OAuth client secret, imported cookies) can now be read from stdin or a --*-file flag instead of a positional/flag value that leaks into shell history and process listings. The explicit arg still works but warns; an interactive TTY with no input errors instead of hanging. - M7: hackernews comments.list caps the comment fan-out (limit param, default 50, hard max 200) instead of fetching every kid, so one request can't spawn thousands of upstream fetches. The action declares `limit` and its pagination max matches. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/adapters/hackernews/src/lib.rs | 40 ++++++++++++++++++++------- crates/cli/src/commands/auth.rs | 12 +++++++- crates/cli/src/commands/exec.rs | 13 ++++----- crates/cli/src/commands/mod.rs | 37 +++++++++++++++++++++++++ crates/cli/src/commands/spec.rs | 5 ++-- crates/cli/src/commands/vault.rs | 9 ++++-- crates/cli/src/lib.rs | 21 ++++++++++---- crates/cli/tests/cli_test.rs | 17 +++++++++--- 8 files changed, 121 insertions(+), 33 deletions(-) diff --git a/crates/adapters/hackernews/src/lib.rs b/crates/adapters/hackernews/src/lib.rs index c424c5b..370a8c4 100644 --- a/crates/adapters/hackernews/src/lib.rs +++ b/crates/adapters/hackernews/src/lib.rs @@ -9,6 +9,10 @@ use tracing::debug; const BASE_URL: &str = "https://hacker-news.firebaseio.com/v0"; const DEFAULT_LIMIT: usize = 30; +/// Default / hard cap on comments fetched per `comments.list` call, to bound the +/// upstream fan-out (one request must not spawn thousands of fetches). +const DEFAULT_COMMENT_LIMIT: usize = 50; +const MAX_COMMENT_LIMIT: usize = 200; pub struct HackerNewsAdapter { client: Client, @@ -33,7 +37,11 @@ impl HackerNewsAdapter { Ok(body) } - async fn fetch_story_ids(&self, endpoint: &str, limit: usize) -> Result { + async fn fetch_story_ids( + &self, + endpoint: &str, + limit: usize, + ) -> Result { let url = format!("{}/{}.json", BASE_URL, endpoint); let ids = self.fetch_json(&url).await?; @@ -100,9 +108,15 @@ impl Adapter for HackerNewsAdapter { id: "list".into(), method: Method::Read, description: "List comments for a story".into(), - params: json!({"story_id": "integer"}), + params: json!({ + "story_id": "integer", + "limit": format!("integer (optional, default {DEFAULT_COMMENT_LIMIT}, max {MAX_COMMENT_LIMIT})"), + }), returns: json!({"type": "array", "items": "comment"}), - pagination: offset_pagination.clone(), + // comments are capped tighter than stories to bound fan-out. + pagination: Some(PaginationStyle::Offset { + max_limit: MAX_COMMENT_LIMIT as u32, + }), }], children: vec![], }; @@ -186,13 +200,19 @@ impl Adapter for HackerNewsAdapter { let story = self.fetch_item(story_id).await?; - let kid_ids = story["kids"] - .as_array() - .cloned() - .unwrap_or_default(); + let all_kids = story["kids"].as_array().cloned().unwrap_or_default(); + let total_kids = all_kids.len(); + + // Cap the fan-out: one request must not spawn thousands of upstream + // fetches. `limit` (default 50, hard max 200) bounds it. + let limit = ctx.params["limit"] + .as_u64() + .map(|n| n as usize) + .unwrap_or(DEFAULT_COMMENT_LIMIT) + .clamp(1, MAX_COMMENT_LIMIT); - let mut comments = Vec::with_capacity(kid_ids.len()); - for kid_value in &kid_ids { + let mut comments = Vec::with_capacity(limit.min(total_kids)); + for kid_value in all_kids.into_iter().take(limit) { if let Some(kid_id) = kid_value.as_u64() { match self.fetch_item(kid_id).await { Ok(comment) => comments.push(comment), @@ -209,7 +229,7 @@ impl Adapter for HackerNewsAdapter { data: Value::Array(comments), meta: ResponseMeta { pagination: Some(relais_core::PaginationInfo { - has_next: false, + has_next: total_kids > limit, cursor: None, total: Some(total), }), diff --git a/crates/cli/src/commands/auth.rs b/crates/cli/src/commands/auth.rs index 7d3088b..df47c8f 100644 --- a/crates/cli/src/commands/auth.rs +++ b/crates/cli/src/commands/auth.rs @@ -23,9 +23,15 @@ pub async fn run(action: AuthAction) -> Result<()> { token_url, client_id, client_secret, + client_secret_file, site, scopes, } => { + let client_secret = super::read_secret( + client_secret, + client_secret_file.as_deref(), + "OAuth client secret", + )?; let scope_list: Vec = scopes .split(',') .map(|s| s.trim().to_string()) @@ -45,7 +51,11 @@ pub async fn run(action: AuthAction) -> Result<()> { site, domain, cookies, - } => import_cookies(&site, &domain, &cookies), + cookies_file, + } => { + let cookies = super::read_secret(cookies, cookies_file.as_deref(), "cookies")?; + import_cookies(&site, &domain, &cookies) + } } } diff --git a/crates/cli/src/commands/exec.rs b/crates/cli/src/commands/exec.rs index cfac1ff..cc60760 100644 --- a/crates/cli/src/commands/exec.rs +++ b/crates/cli/src/commands/exec.rs @@ -22,14 +22,11 @@ pub async fn run(path: &str, data: Option<&str>) -> Result<()> { // If the vault is unavailable or no credential is found, proceed without credentials. let vault = open_vault().ok(); let credentials = vault.as_ref().and_then(|v| { - v.retrieve(site_id) - .ok() - .flatten() - .and_then(|json_str| { - serde_json::from_str::(&json_str) - .ok() - .or_else(|| Some(Credentials::api_key(&json_str))) - }) + v.retrieve(site_id).ok().flatten().and_then(|json_str| { + serde_json::from_str::(&json_str) + .ok() + .or_else(|| Some(Credentials::api_key(&json_str))) + }) }); // If the token is expired, try to refresh it automatically. diff --git a/crates/cli/src/commands/mod.rs b/crates/cli/src/commands/mod.rs index b2cadf7..8f17bba 100644 --- a/crates/cli/src/commands/mod.rs +++ b/crates/cli/src/commands/mod.rs @@ -9,6 +9,43 @@ pub mod vault; use relais_core::router::Router; +/// Resolve a secret value from an explicit (deprecated) CLI arg, a file, or stdin — +/// so secrets need not appear in shell history / process listings (M5). +pub fn read_secret( + value: Option, + file: Option<&str>, + what: &str, +) -> anyhow::Result { + use std::io::{IsTerminal, Read}; + if let Some(v) = value { + tracing::warn!( + "{what} was passed on the command line — it is exposed in shell history and process \ + listings; prefer the matching --*-file flag or piping it on stdin" + ); + return Ok(v); + } + if let Some(path) = file { + let s = std::fs::read_to_string(path) + .map_err(|e| anyhow::anyhow!("reading {what} from {path}: {e}"))?; + return Ok(s.trim_end_matches(['\n', '\r']).to_string()); + } + // Avoid hanging on an interactive terminal with no piped input. + if std::io::stdin().is_terminal() { + anyhow::bail!( + "no {what} provided: pass it as an argument, a --*-file flag, or pipe it on stdin" + ); + } + let mut s = String::new(); + std::io::stdin() + .read_to_string(&mut s) + .map_err(|e| anyhow::anyhow!("reading {what} from stdin: {e}"))?; + let s = s.trim_end_matches(['\n', '\r']).to_string(); + if s.is_empty() { + anyhow::bail!("no {what} provided (pass it as an argument, a --*-file flag, or via stdin)"); + } + Ok(s) +} + /// Resolve the audit signing-key passphrase (M6). Encrypted at rest by default via /// `RELAIS_AUDIT_PASSPHRASE`; an unencrypted key is allowed only under /// `RELAIS_AUDIT_ALLOW_UNENCRYPTED_KEY=1|true` (dev). diff --git a/crates/cli/src/commands/spec.rs b/crates/cli/src/commands/spec.rs index b27aba9..70ad365 100644 --- a/crates/cli/src/commands/spec.rs +++ b/crates/cli/src/commands/spec.rs @@ -17,8 +17,9 @@ pub fn run(path: &str) -> Result<()> { .ok_or_else(|| anyhow::anyhow!("site '{site_id}' not found"))?; let resources = adapter.resources(); - let action = find_action(&resources, resource_id, action_id) - .ok_or_else(|| anyhow::anyhow!("action '{resource_id}.{action_id}' not found in site '{site_id}'"))?; + let action = find_action(&resources, resource_id, action_id).ok_or_else(|| { + anyhow::anyhow!("action '{resource_id}.{action_id}' not found in site '{site_id}'") + })?; let json = serde_json::to_string_pretty(&action)?; println!("{json}"); diff --git a/crates/cli/src/commands/vault.rs b/crates/cli/src/commands/vault.rs index aaee4ba..f847f0a 100644 --- a/crates/cli/src/commands/vault.rs +++ b/crates/cli/src/commands/vault.rs @@ -7,8 +7,13 @@ pub fn run(action: &VaultAction) -> Result<()> { let vault = open_vault()?; match action { - VaultAction::Store { site, token } => { - vault.store(site, token)?; + VaultAction::Store { + site, + token, + token_file, + } => { + let token = super::read_secret(token.clone(), token_file.as_deref(), "vault token")?; + vault.store(site, &token)?; println!("Stored credential for '{site}'"); } VaultAction::List => { diff --git a/crates/cli/src/lib.rs b/crates/cli/src/lib.rs index c1278a1..1f47bff 100644 --- a/crates/cli/src/lib.rs +++ b/crates/cli/src/lib.rs @@ -96,8 +96,11 @@ pub enum VaultAction { Store { /// Site ID (e.g., "github") site: String, - /// The credential token to store - token: String, + /// The token (DEPRECATED: exposed in shell history; prefer --token-file or stdin) + token: Option, + /// Read the token from a file + #[arg(long)] + token_file: Option, }, /// List stored credentials List, @@ -128,9 +131,12 @@ pub enum AuthAction { /// Client ID #[arg(long)] client_id: String, - /// Client secret + /// Client secret (DEPRECATED on CLI; prefer --client-secret-file or stdin) + #[arg(long)] + client_secret: Option, + /// Read the client secret from a file #[arg(long)] - client_secret: String, + client_secret_file: Option, /// Site ID to store credential under #[arg(long)] site: String, @@ -145,8 +151,11 @@ pub enum AuthAction { /// Domain the cookies belong to #[arg(long)] domain: String, - /// Cookie string (name=value pairs, semicolon-separated) + /// Cookie string (DEPRECATED on CLI; prefer --cookies-file or stdin) + #[arg(long)] + cookies: Option, + /// Read the cookie string from a file #[arg(long)] - cookies: String, + cookies_file: Option, }, } diff --git a/crates/cli/tests/cli_test.rs b/crates/cli/tests/cli_test.rs index 0156da0..127418f 100644 --- a/crates/cli/tests/cli_test.rs +++ b/crates/cli/tests/cli_test.rs @@ -105,9 +105,14 @@ fn cli_parses_vault_store() { let cli = Cli::parse_from(["relais", "vault", "store", "github", "ghp_abc123"]); match cli.command { Commands::Vault { action } => match action { - VaultAction::Store { site, token } => { + VaultAction::Store { + site, + token, + token_file, + } => { assert_eq!(site, "github"); - assert_eq!(token, "ghp_abc123"); + assert_eq!(token.as_deref(), Some("ghp_abc123")); + assert_eq!(token_file, None); } _ => panic!("expected Store action"), }, @@ -189,13 +194,15 @@ fn cli_parses_auth_custom() { token_url, client_id, client_secret, + client_secret_file, site, scopes, } => { assert_eq!(auth_url, "https://example.com/auth"); assert_eq!(token_url, "https://example.com/token"); assert_eq!(client_id, "myid"); - assert_eq!(client_secret, "mysecret"); + assert_eq!(client_secret.as_deref(), Some("mysecret")); + assert_eq!(client_secret_file, None); assert_eq!(site, "example"); assert_eq!(scopes, ""); } @@ -253,10 +260,12 @@ fn cli_parses_auth_import_cookies() { site, domain, cookies, + cookies_file, } => { assert_eq!(site, "example"); assert_eq!(domain, "example.com"); - assert_eq!(cookies, "session=abc; token=xyz"); + assert_eq!(cookies.as_deref(), Some("session=abc; token=xyz")); + assert_eq!(cookies_file, None); } _ => panic!("expected ImportCookies action"), },