Skip to content

fix(security): SSRF protection, async crypto, shell injection, and credential masking#140

Open
joshlemos wants to merge 2 commits intogrcengineering:mainfrom
joshlemos:security/code-audit-fixes
Open

fix(security): SSRF protection, async crypto, shell injection, and credential masking#140
joshlemos wants to merge 2 commits intogrcengineering:mainfrom
joshlemos:security/code-audit-fixes

Conversation

@joshlemos
Copy link
Contributor

Summary

  • Add SSRF URL validation in BaseConnector blocking private IPs, loopback, link-local, and cloud metadata endpoints
  • Add createClient() for request-scoped HTTP clients to prevent credential leakage between concurrent tenant syncs
  • Replace blocking crypto.scryptSync with async crypto.scrypt to avoid event loop starvation under load, with cached legacy key derivation
  • Fix shell injection in seed-infisical.sh by using jq for safe JSON construction instead of string interpolation
  • Mask infisical:// references in API responses alongside encrypted values
  • Sanitize delete audit log to exclude encrypted config data

Dependencies

This PR depends on #139 (Infisical integration) — it modifies seed-infisical.sh and integrations.service.ts which were introduced/changed in that PR. Merge #139 first.

Test plan

  • SSRF: connector rejects requests to 169.254.169.254, 127.0.0.1, 10.x.x.x, ::1
  • Async crypto: encrypt/decrypt operations don't block event loop (no scryptSync calls remain)
  • Shell injection: seed-infisical.sh handles special characters in secrets safely via jq
  • Credential masking: API responses show [ENCRYPTED] for encrypted values and [INFISICAL] for Infisical references
  • Delete audit logs don't contain raw encrypted config

@joshlemos
Copy link
Contributor Author

joshlemos commented Feb 13, 2026

Security Review: PR #140 Test Results

Performed static analysis and behavioral verification of all 6 security fixes in this PR. Findings below.

Update: All 4 issues below have been fixed in PR #142.


1. SSRF URL Validation (validateUrlSafe) — Bypasses Found → Fixed in #142

The function correctly blocks direct IP-based vectors. Verified BLOCKED:

Vector Status
169.254.169.254 (cloud metadata) Blocked
127.0.0.1, localhost, [::1] Blocked
10.x, 172.16-31.x, 192.168.x private ranges Blocked
Decimal IP (http://2130706433/ → 127.0.0.1) Blocked
Hex IP (http://0x7f000001/) Blocked
Octal IP (http://0177.0.0.1/) Blocked (WHATWG parser normalizes octal)
URL with credentials (user:pass@127.0.0.1) Blocked

However, several bypass vectors remained open:

Vector Status Severity Fix
DNS rebindinghttp://attacker.com with A record pointing to 127.0.0.1 BYPASS Critical #142 — SSRF-safe agents with custom dns.lookup validate resolved IPs at connection time
Redirect-based — external URL 302-redirects to internal IP BYPASS Critical #142maxRedirects: 0 on all axios instances
Dotless internal domainhttp://internal.corp/ resolving to 10.0.0.1 BYPASS Critical #142 — same DNS lookup validation at connection time
IPv6-mapped IPv4http://[::ffff:127.0.0.1]/ BYPASS High #142isBlockedIPv6 extracts and validates embedded IPv4
IPv6 private rangeshttp://[fd00::1]/, http://[fe80::1]/ BYPASS High #142 — blocks fc00::/7 (ULA), fe80::/10 (link-local)

2. Async Crypto (scryptSyncscrypt) — PASS

  • scryptAsync wrapper is correct — standard callback→Promise pattern
  • Legacy key caching has a benign race (two concurrent first-requests may both derive the key), but the deterministic inputs produce identical output. Not a security issue, minor perf concern on cold start
  • isEncryptedFormat() heuristic is acceptable for its use case (very low false-positive probability on real credential data)
  • Depth guard of 3 for double-encryption is sufficient — GCM auth tag provides an additional natural recursion breaker

No issues found.


3. Shell Injection Fix (seed-infisical.sh) — Bug Found → Fixed in #142

JSON construction via jq --arg is correct — this is the gold standard for building JSON in shell scripts. Properly neutralizes all injection vectors in the payload body.

Bug: AUTH_TOKEN env var pattern was broken. Lines ~150-153:

response=$(AUTH_TOKEN="$TOKEN" curl -sf -X POST ... \
    -H "Authorization: Bearer $AUTH_TOKEN" \
    ...

The inline AUTH_TOKEN="$TOKEN" sets the variable in curl's child process environment, but $AUTH_TOKEN in the -H argument is expanded by the parent shell before exec. Since AUTH_TOKEN is never set in the parent shell, the Authorization header sends an empty bearer token, and all API requests fail with 401.

Fixed in #142 — replaced with curl --config <(printf 'header = "Authorization: Bearer %s"\n' "$TOKEN") which correctly passes the token and keeps it out of /proc/pid/cmdline.


4. Credential Masking — PASS (minor note)

  • Recursive maskObject correctly handles nested objects like config.credentials.apiKey
  • SENSITIVE_FIELDS list is comprehensive (23 entries) and uses substring matching — consistent between encryption and masking logic
  • Password-type fields show last 4 chars (UX hint); encrypted/Infisical values are fully masked
  • Arrays of objects skip recursion (!Array.isArray guard) — low practical risk since config schemas don't typically store secrets in arrays, but worth noting for future-proofing

5. Delete Audit Log Sanitization — PASS

Verified all three audit paths in the file:

  • Create (line ~630): uses metadata: { type, syncFrequency } — no config
  • Update (line ~696): uses allowlisted { name, status, syncFrequency } — no config
  • Delete (line ~759): uses allowlisted { name, type, status, syncFrequency } — no config

All paths properly exclude encrypted config data. The allowlist approach is correct (fails closed for new fields).


6. Request-Scoped HTTP Client (createClient) — PASS

  • Returns fresh axios.create() instance per call — eliminates credential leakage between concurrent tenant syncs
  • SSRF validation applied via validateUrlSafe(baseURL)
  • getClient() properly routes between shared and request-scoped instances
  • setHeaders/setBaseURL correctly marked as deprecated

Summary

Finding Severity Status
SSRF: No DNS resolution validation (rebinding, dotless domains) Critical ✅ Fixed in #142
SSRF: Redirect targets bypass validation (axios follows 5 by default) Critical ✅ Fixed in #142
SSRF: No IPv6 range checks / IPv6-mapped IPv4 bypass High ✅ Fixed in #142
Shell: AUTH_TOKEN env var sends empty bearer token Medium ✅ Fixed in #142
Async crypto migration None ✅ Correct
jq JSON construction None ✅ Correct
Credential masking None ✅ Correct
Audit log sanitization (all 3 paths) None ✅ Correct
Request-scoped HTTP clients None ✅ Correct

@chadfryer
Copy link
Collaborator

This PR depends on #139 which has merge conflicts. Please wait for #139 to be rebased and merged first.

joshlemos added 2 commits February 13, 2026 10:49
Phase 1 & 2 implementation of centralized secrets management:
- Add Infisical service to dev and prod Docker Compose configs
- Add SecretsModule/SecretsService to shared library
- Integrate SecretsModule into controls service for credential storage
- Add per-service auth modules for frameworks, policies, audit, trust
- Add database init scripts for Keycloak and Infisical databases
- Fix Keycloak DB URL to use dedicated database instead of app DB
- Update startup scripts with Infisical bootstrap secret generation
- Add Makefile targets for secrets-ui and secrets-seed
- Backward compatible: falls back to env vars when Infisical unavailable
…edential masking

- Add SSRF URL validation blocking private IPs, loopback, link-local,
  and cloud metadata endpoints in BaseConnector
- Add createClient() for request-scoped HTTP clients to prevent
  credential leakage between concurrent tenant syncs (singleton safety)
- Replace blocking crypto.scryptSync with async crypto.scrypt to avoid
  event loop starvation under load, with cached legacy key derivation
- Fix shell injection in seed-infisical.sh by using jq for safe JSON
  construction instead of string interpolation
- Mask Infisical references (infisical://) in API responses alongside
  encrypted values
- Sanitize delete audit log to exclude encrypted config data
@joshlemos joshlemos force-pushed the security/code-audit-fixes branch from aad9917 to 47f76f1 Compare February 13, 2026 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants