Skip to content

Commit edfd0fa

Browse files
committed
fix(node): address review on icaptcha gate (validate config, bound key fetch)
Addresses CodeRabbit and @jatmn review feedback on #108: - parse_mode now returns Option and rejects unrecognized ICAPTCHA_MODE values; init() warns and falls back to off instead of silently disabling the gate on a typo like "enforced". [P2] - ICAPTCHA_REQUIRED_LEVEL: warn on a non-empty unparseable value instead of silently lowering the threshold to the default. [P2] - fetch_key uses a reqwest client with a 5s timeout so a hung /v1/pubkey can never block node startup; still fails inert on timeout/error. [P2] - .env.example: reorder ICAPTCHA_* keys (MODE, PUBKEY, REQUIRED_LEVEL, URL) to satisfy dotenv-linter. [P3] - add parse_mode unit test (rejects junk, accepts documented values). cargo test -p gitlawb-node icaptcha: 12 passed. clippy + fmt clean. Author: Kevin Codex <kevin@gitlawb.com>
1 parent 4f39baf commit edfd0fa

2 files changed

Lines changed: 51 additions & 13 deletions

File tree

‎.env.example‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,10 +96,10 @@ GITLAWB_AUTO_SYNC=false
9696
# shadow - verify + log would-be rejections, but always allow
9797
# enforce - reject requests without a valid, strong-enough proof
9898
ICAPTCHA_MODE=off
99-
# Base URL of the iCaptcha service.
100-
ICAPTCHA_URL=https://icaptcha.gitlawb.com
10199
# Optional base64url Ed25519 public key override; if unset it is fetched from
102100
# ICAPTCHA_URL/v1/pubkey at startup.
103101
ICAPTCHA_PUBKEY=
104102
# Minimum proof difficulty level required to pass.
105103
ICAPTCHA_REQUIRED_LEVEL=3
104+
# Base URL of the iCaptcha service.
105+
ICAPTCHA_URL=https://icaptcha.gitlawb.com

‎crates/gitlawb-node/src/icaptcha.rs‎

Lines changed: 49 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
//! ICAPTCHA_REQUIRED_LEVEL minimum proof level (default 3)
2020
2121
use std::sync::OnceLock;
22-
use std::time::{SystemTime, UNIX_EPOCH};
22+
use std::time::{Duration, SystemTime, UNIX_EPOCH};
2323

2424
use axum::http::HeaderMap;
2525
use base64::engine::general_purpose::URL_SAFE_NO_PAD;
@@ -48,11 +48,31 @@ impl Mode {
4848
}
4949
}
5050

51-
fn parse_mode(s: &str) -> Mode {
51+
/// Parse `ICAPTCHA_MODE`. Returns `None` for unrecognized values so the caller
52+
/// can surface the typo instead of silently disabling the gate.
53+
fn parse_mode(s: &str) -> Option<Mode> {
5254
match s.trim().to_ascii_lowercase().as_str() {
53-
"enforce" => Mode::Enforce,
54-
"shadow" => Mode::Shadow,
55-
_ => Mode::Off,
55+
"" | "off" => Some(Mode::Off),
56+
"shadow" => Some(Mode::Shadow),
57+
"enforce" => Some(Mode::Enforce),
58+
_ => None,
59+
}
60+
}
61+
62+
/// Parse `ICAPTCHA_REQUIRED_LEVEL`. Defaults to 3; warns (rather than silently
63+
/// lowering the threshold) when a non-empty value fails to parse.
64+
fn parse_required_level() -> u32 {
65+
const DEFAULT: u32 = 3;
66+
match std::env::var("ICAPTCHA_REQUIRED_LEVEL") {
67+
Ok(v) if !v.trim().is_empty() => v.trim().parse().unwrap_or_else(|_| {
68+
tracing::warn!(
69+
value = %v,
70+
default = DEFAULT,
71+
"invalid ICAPTCHA_REQUIRED_LEVEL; using default"
72+
);
73+
DEFAULT
74+
}),
75+
_ => DEFAULT,
5676
}
5777
}
5878

@@ -97,20 +117,27 @@ fn decode_key(b64url: &str) -> Option<VerifyingKey> {
97117

98118
async fn fetch_key(url: &str) -> Option<VerifyingKey> {
99119
let endpoint = format!("{}/v1/pubkey", url.trim_end_matches('/'));
100-
let jwks: Jwks = reqwest::get(&endpoint).await.ok()?.json().await.ok()?;
120+
// Bounded request: a hung /v1/pubkey must never block node startup. On
121+
// timeout/error we return None and the gate stays inert (fail safe).
122+
let client = reqwest::Client::builder()
123+
.timeout(Duration::from_secs(5))
124+
.build()
125+
.ok()?;
126+
let jwks: Jwks = client.get(&endpoint).send().await.ok()?.json().await.ok()?;
101127
decode_key(&jwks.keys.first()?.x)
102128
}
103129

104130
/// Initialize the gate from the environment. Call once at startup. Never panics;
105131
/// if the gate is active but no key can be loaded, it stays inert and warns.
106132
pub async fn init() {
107-
let mode = parse_mode(&std::env::var("ICAPTCHA_MODE").unwrap_or_default());
133+
let raw_mode = std::env::var("ICAPTCHA_MODE").unwrap_or_default();
134+
let mode = parse_mode(&raw_mode).unwrap_or_else(|| {
135+
tracing::warn!(value = %raw_mode, "invalid ICAPTCHA_MODE; disabling iCaptcha gate");
136+
Mode::Off
137+
});
108138
let url = std::env::var("ICAPTCHA_URL")
109139
.unwrap_or_else(|_| "https://icaptcha.gitlawb.com".to_string());
110-
let required_level = std::env::var("ICAPTCHA_REQUIRED_LEVEL")
111-
.ok()
112-
.and_then(|v| v.parse().ok())
113-
.unwrap_or(3);
140+
let required_level = parse_required_level();
114141

115142
let key = if mode == Mode::Off {
116143
None
@@ -287,6 +314,17 @@ mod tests {
287314
assert!(err.contains("missing"), "{err}");
288315
}
289316

317+
#[test]
318+
fn parse_mode_accepts_documented_values_and_rejects_junk() {
319+
assert_eq!(parse_mode(""), Some(Mode::Off));
320+
assert_eq!(parse_mode("off"), Some(Mode::Off));
321+
assert_eq!(parse_mode(" Shadow "), Some(Mode::Shadow));
322+
assert_eq!(parse_mode("ENFORCE"), Some(Mode::Enforce));
323+
// Typos must NOT silently disable the gate.
324+
assert_eq!(parse_mode("enforced"), None);
325+
assert_eq!(parse_mode("on"), None);
326+
}
327+
290328
#[test]
291329
fn off_mode_allows_everything() {
292330
let mut v = verifier(3);

0 commit comments

Comments
 (0)