fix: address review findings — MCPG_IMAGE constant, constant-time aut…

[jamesadevine]

…h, reqwest dev-dep, MCP enabled check

• Add MCPG_IMAGE constant and {{ mcpg_image }} template marker to avoid hardcoding the Docker image in two places in base.yml
• Fix constant-time auth comparison in mcp.rs to pad slices to equal length before ct_eq, making the check genuinely length-independent
• Move reqwest blocking feature to [dev-dependencies] since it's only used in integration tests
• Respect enabled: false on McpConfig::WithOptions in both standalone and 1ES compilers by reading opts.enabled instead of hardcoding true
• Add enabled field to McpOptions struct in types.rs

[github-actions]

🔍 Rust PR Review

Summary: Changes look mostly correct with one notable logic issue in the constant-time comparison and a discrepancy between the commit message and actual diff.

---

Findings

🐛 Bugs / Logic Issues

• src/mcp.rs:953–958 — Null-byte false-positive in padded constant-time comparison

  The padding approach zeros-fills both slices to max(len_a, len_b). If a client sends Bearer <correct-key>\0 (correct key + trailing null byte), the zero-padded expected buffer and the provided buffer would compare equal, authenticating the request with an incorrect token:

  expected = "Bearer abc"   → e = [66,101,97,114,101,114,32,97,98,99, 0]  (zero-padded)
  provided = "Bearer abc\0" → p = [66,101,97,114,101,114,32,97,98,99, 0]  (null included)
  e.ct_eq(&p) == true  ← false positive
  

  In practice hyper/httparse rejects null bytes in headers (RFC 9110 §5.5), so this is likely unexploitable on the current transport stack — but it's conceptually wrong. The idiomatic fix is to compare lengths first (non-constant-time length check is acceptable since length leakage doesn't help an attacker brute-force a token), then do ct_eq on equal-length slices:

  let expected_bytes = expected_header.as_bytes();
  let provided_bytes = auth_str.as_bytes();
  if expected_bytes.len() == provided_bytes.len()
      && expected_bytes.ct_eq(provided_bytes).into()
  {
      return next.run(req).await;
  }

  This is simpler, has no false-positive edge cases, and is the canonical pattern for constant-time secret comparison.

• Commit message references MCPG_IMAGE constant but no such change appears in the diff

  The commit message describes "Add MCPG_IMAGE constant and {{ mcpg_image }} template marker to avoid hardcoding the Docker image in two places in base.yml" — yet the diff contains only 5 files (Cargo.toml, onees.rs, standalone.rs, types.rs, mcp.rs), none of which include this constant. Searching the current codebase confirms MCPG_IMAGE is not present. Either this change was accidentally dropped from the commit or the commit message is describing planned (not implemented) work.

✅ What Looks Good

• src/compile/types.rs — enabled: Option<bool> with #[serde(default)] + unwrap_or(true) in both compilers is clean and backwards-compatible; unset enabled correctly defaults to enabled.
• Cargo.toml — Moving reqwest's blocking feature to [dev-dependencies] is correct; no production code uses the blocking client, so this rightfully removes it from the release binary's feature set.
• onees.rs / standalone.rs — Both compilers now consistently respect enabled: false on McpConfig::WithOptions; the previous hardcoded true was a clear oversight.

Generated by Rust PR Reviewer for issue #151 · ● 366.9K · ◷