test: add validateMetadataValue coverage for GCP metadata injection protection#2467
test: add validateMetadataValue coverage for GCP metadata injection protection#2467
Conversation
…ection Agent: test-engineer Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
PR maintenance check: All CI checks pass (Unit Tests, Mock Tests, ShellCheck, Biome Lint, macOS Compatibility). PR is mergeable and ready for human review approval. -- refactor/pr-maintainer |
louisgv
left a comment
There was a problem hiding this comment.
Security Review
Verdict: APPROVED
Commit: a5d0039
Summary
This PR adds comprehensive test coverage for validateMetadataValue(), a security-critical function that prevents command injection via GCP metadata fields (zone, project, etc.) in tampered history files.
Security Analysis
No vulnerabilities found. The PR:
- Adds 25 new test cases covering all attack vectors (command substitution, pipe injection, path traversal, shell metacharacters)
- Tests the
/^[a-zA-Z0-9_.-]+$/allowlist pattern that blocks all shell metacharacters - Verifies error messages include field names for debuggability
- Tests both valid inputs (GCP zones, project IDs, alphanumeric values) and malicious inputs
- Follows project testing standards (no
asassertions, usesexpect().toThrow()patterns)
Findings
None. Clean implementation.
Tests
- bun test security-connection-validation.test.ts: PASS (59 tests, 120 assertions)
- TypeScript patterns: PASS (no banned
asassertions) - Code quality: PASS (follows existing test structure)
Validation
✅ Command injection protection: All injection vectors blocked ($(cmd), cmd, ;, |, &, ..)
✅ Length limit: 128 characters enforced
✅ Allowlist: Only alphanumeric + hyphens + underscores + dots
✅ Empty strings: Correctly allowed (caller provides defaults)
✅ Error messages: Include field names for debugging
-- security/pr-reviewer
Why:
validateMetadataValueinsecurity.tsis called incommands/delete.tsto validate GCP zone and project values from history before shell execution. It has zero test coverage — a regression here could silently allow command injection via tampered history files.Changes
describe("validateMetadataValue")block tosecurity-connection-validation.test.tsTest plan
bunx @biomejs/biome check src/— 0 errorsbun test— all 1513 tests pass-- refactor/test-engineer