Security audit: MCP server secret management

[RafaelPo]

Summary

• Comprehensive defensive security audit of the MCP server's secret management
• Covers source code, deployment config (Helm, Docker, CI/CD), and git history
• Identifies 2 High, 5 Medium, and 4 Low severity findings with PoCs and recommended fixes
• Documents 18 positive security controls already in place

Key findings

• H-1: HKDF key derivation uses salt=None — deterministic across environments
• H-2: PendingAuth (PKCE verifier) stored unencrypted in Redis while auth codes/refresh tokens are encrypted
• M-1–M-5: Poll token in URL query params, no key rotation, silent encryption bypass, weak entropy validation, CSV parse error logging

Test plan

• Review audit report for accuracy against current codebase
• Validate severity ratings match risk appetite
• Create follow-up issues for accepted findings

🤖 Generated with Claude Code

[RafaelPo]

@claude code review

[github-actions]

Claude finished @RafaelPo's task in 3m 13s —— View job

---

Code Review: Security Audit Report

• Read the audit document and CLAUDE.md
• Verify H-1 (HKDF salt=None) against source code
• Verify H-2 (PendingAuth unencrypted) against source code
• Verify Medium findings (M-1 through M-5) against source code
• Verify Low findings (L-1 through L-4) against source code
• Check positive security controls claims
• Post review summary

---

Overall Assessment

Thorough audit — all 11 findings verified against the current codebase. Line numbers, code snippets, and severity context are accurate. One factual error in the positive findings table needs correction.

---

Factual Error: OIDC Claim is Wrong

The positive findings table states:

| GitHub Actions uses OIDC for GCP | PASS | deploy-mcp.yaml:89-91 — no long-lived service account keys in repo |

This is incorrect. The workflow at deploy-mcp.yaml:89-91 uses:

- uses: google-github-actions/auth@v2
  with:
    credentials_json: ${{ secrets.GCP_CREDENTIALS_GLOBAL }}

This is a long-lived JSON service account key stored in GitHub Secrets, not OIDC Workload Identity Federation. OIDC would use workload_identity_provider + service_account parameters instead. The claim should be removed or corrected to "GCP auth via static credentials in GitHub Secrets."

Fix this →

---

Finding Verification Summary

ID	Verified	Notes
H-1	✅	redis_store.py:64 — salt=None confirmed
H-2	✅	auth.py:360 — plaintext model_dump_json() confirmed. Also re-stored as plaintext at auth.py:373 (handle_start)
M-1	✅	result_store.py:162 — poll token in query string confirmed
M-2	✅	redis_store.py:52 — @lru_cache(maxsize=1), no MultiFernet
M-3	✅	redis_store.py:78 — silent plaintext return when Fernet is None
M-4	✅	http_config.py:75 — checks not settings.upload_secret (empty), no length validation
M-5	✅	uploads.py:293 — logger.warning("CSV parse failed for upload: %s", exc) logs full exception
L-1	✅	redis_store.py:133-138, 151 — INFO level confirmed
L-2	✅	app.py:38 — %r format confirmed
L-3	✅	auth.py:438, 507 and routes.py:93 — no InvalidToken catch
L-4	✅	deploy-mcp.yaml:150, 154 — sops decrypts to plaintext file

Positive findings table: 17 of 18 verified correct. The OIDC claim (row 18) is wrong as described above.

---

Severity Calibration Notes

H-1 (HKDF salt=None) — The severity is defensible but worth nuancing. Per RFC 5869, salt=None in HKDF is treated as a zero-filled byte string of hash length, which is explicitly allowed. The derivation is deterministic but not cryptographically broken if UPLOAD_SECRET has sufficient entropy. The real risk is cross-environment key reuse when the same UPLOAD_SECRET is deployed to multiple environments — the "Impact" section correctly identifies this. However, the sentence "HKDF without salt also removes the second layer of defense against brute-force" slightly overstates it; salt primarily provides domain separation, not brute-force resistance when the input keying material is already high-entropy. Consider softening to "reduces defense-in-depth."

H-1 recommended fix — The os.urandom(16) suggestion has a significant operational caveat that should be more prominent: deploying a random salt will immediately break decryption of all existing encrypted data in Redis (auth codes, refresh tokens, task tokens, poll tokens). The migration path needs to be spelled out more explicitly — e.g., dual-read with fallback to old key, or a coordinated flush of short-lived tokens.

---

Minor Nits in the Document

1. H-2 second occurrence: The audit mentions auth.py:357-361 but the same plaintext storage happens again at auth.py:370-373 in handle_start (re-stores after consuming). Worth adding this as a second location.

2. M-3 line reference: The audit says config.py:150-162 for the Redis SSL validator. Actual lines are config.py:149-163 — off by one on both ends, minor.

3. Docker-compose line reference: The positive findings table says docker-compose.yaml:49-55 for container hardening. The actual security_opt, cap_drop, and read_only are at lines 49-53. Minor.

---

Good Catch: Something the Audit Got Right That's Easy to Miss

The H-2 finding correctly noting that PendingAuth contains the PKCE supabase_code_verifier while auth codes and refresh tokens are encrypted is a sharp observation. The inconsistency is clearly unintentional, and the fix (wrapping with encrypt_value()) is minimal and safe.

---

Summary

The audit is high quality and actionable. The one required fix is correcting the OIDC positive finding. The H-1 recommended fix should include a migration note. All vulnerability findings are real and accurately described.

[sentry]

Bug: The code attempts to decrypt PendingAuth data from Redis without handling cases where the data might be unencrypted from a previous version, causing an unhandled exception.
_{Severity: HIGH}

Suggested Fix

Wrap the decryption call in a try...except InvalidToken block. In the except block, attempt to use the data as plaintext to maintain backward compatibility for entries created before the deployment. This ensures a smooth transition for in-flight authentication flows.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: everyrow-mcp/src/everyrow_mcp/auth.py#L284

Potential issue: The introduction of encryption for `PendingAuth` data lacks a backward
compatibility or migration strategy. During a deployment, if an authentication flow was
initiated before the update, an unencrypted `PendingAuth` entry will exist in Redis.
When the new code in `_validate_auth_request()` tries to process this entry, the call to
`decrypt_value(pending_data_encrypted)` will fail. The underlying `Fernet.decrypt()`
function will raise an `InvalidToken` exception on the plaintext data. Since this
exception is not caught, it will result in a 500 error, breaking the authentication flow
for any user caught in this transition period. The issue is temporary, as the affected
Redis keys have a 10-minute TTL.

_{Did we get this right? 👍 / 👎 to inform future reviews.}