diff --git a/apps/orchestrator/src/git/git-validation.util.ts b/apps/orchestrator/src/git/git-validation.util.ts index e11b75c..38516c8 100644 --- a/apps/orchestrator/src/git/git-validation.util.ts +++ b/apps/orchestrator/src/git/git-validation.util.ts @@ -7,7 +7,9 @@ * Security: Whitelist-based approach - only allow known-safe characters. */ -import { BadRequestException } from "@nestjs/common"; +import { BadRequestException, Logger } from "@nestjs/common"; + +const logger = new Logger("GitValidation"); /** * Validates a git branch name for safety @@ -36,6 +38,13 @@ export function validateBranchName(branchName: string): void { // This prevents all forms of command injection const safePattern = /^[a-zA-Z0-9/_.-]+$/; if (!safePattern.test(branchName)) { + logger.warn({ + event: "GIT_COMMAND_INJECTION_BLOCKED", + input: branchName, + reason: "Invalid characters detected", + securityEvent: true, + timestamp: new Date().toISOString(), + }); throw new BadRequestException( `Branch name contains invalid characters. Only alphanumeric, hyphens, underscores, slashes, and dots are allowed.` ); @@ -43,6 +52,13 @@ export function validateBranchName(branchName: string): void { // Prevent git option injection (branch names starting with -) if (branchName.startsWith("-")) { + logger.warn({ + event: "GIT_OPTION_INJECTION_BLOCKED", + input: branchName, + reason: "Branch name starts with hyphen (option injection attempt)", + securityEvent: true, + timestamp: new Date().toISOString(), + }); throw new BadRequestException( "Branch name cannot start with a hyphen (prevents option injection)" ); @@ -50,11 +66,25 @@ export function validateBranchName(branchName: string): void { // Prevent double dots (used for range specifications in git) if (branchName.includes("..")) { + logger.warn({ + event: "GIT_RANGE_INJECTION_BLOCKED", + input: branchName, + reason: "Double dots detected (git range specification)", + securityEvent: true, + timestamp: new Date().toISOString(), + }); throw new BadRequestException("Branch name cannot contain consecutive dots (..)"); } // Prevent path traversal patterns if (branchName.includes("/../") || branchName.startsWith("../") || branchName.endsWith("/..")) { + logger.warn({ + event: "GIT_PATH_TRAVERSAL_BLOCKED", + input: branchName, + reason: "Path traversal pattern detected", + securityEvent: true, + timestamp: new Date().toISOString(), + }); throw new BadRequestException("Branch name cannot contain path traversal patterns"); } @@ -124,6 +154,14 @@ export function validateRepositoryUrl(repositoryUrl: string): void { for (const dangerous of dangerousProtocols) { if (url.toLowerCase().startsWith(dangerous)) { + logger.warn({ + event: "GIT_DANGEROUS_PROTOCOL_BLOCKED", + input: url, + protocol: dangerous, + reason: `Dangerous protocol detected: ${dangerous}`, + securityEvent: true, + timestamp: new Date().toISOString(), + }); throw new BadRequestException( `Repository URL cannot use ${dangerous} protocol (security risk)` ); @@ -140,6 +178,13 @@ export function validateRepositoryUrl(repositoryUrl: string): void { for (const pattern of localhostPatterns) { if (pattern.test(url)) { + logger.warn({ + event: "GIT_SSRF_ATTEMPT_BLOCKED", + input: url, + reason: "Repository URL points to localhost or internal network", + securityEvent: true, + timestamp: new Date().toISOString(), + }); throw new BadRequestException( "Repository URL cannot point to localhost or internal networks (SSRF protection)" ); diff --git a/docs/scratchpads/277-comprehensive-audit-logging.md b/docs/scratchpads/277-comprehensive-audit-logging.md new file mode 100644 index 0000000..dac6311 --- /dev/null +++ b/docs/scratchpads/277-comprehensive-audit-logging.md @@ -0,0 +1,120 @@ +# Issue #277: Add comprehensive audit logging for security events + +## Objective + +Add comprehensive audit logging for critical security events to enable forensic analysis and attack detection. + +## Missing Logging Areas + +### 1. Failed signature verifications + +- **Current**: DEBUG level only +- **Location**: `signature.service.ts` +- **Required**: WARN level with full details + +### 2. Failed OIDC validations + +- **Current**: No details logged +- **Location**: `auth` module +- **Required**: Full validation failure details + +### 3. Capability bypass attempts + +- **Current**: Not logged +- **Location**: `capability.guard.ts` +- **Required**: Log all denied capabilities + +### 4. Rate limit violations + +- **Current**: Not logged +- **Location**: ThrottlerGuard +- **Required**: Log rate limit hits + +### 5. Command injection attempts + +- **Current**: Not logged +- **Location**: `git-validation.util.ts` (recently added) +- **Required**: Log validation rejections + +## Already Implemented + +From issue #276 (commit 744290a): + +- ✅ Incoming connection attempts +- ✅ Failed signature verifications for connections +- ✅ Connection created events + +From issue #274 (commit 7a84d96): + +- ✅ Git command validation (but not logged) + +## Implementation Plan + +### Priority 1: Add missing audit methods + +1. `logSignatureVerificationFailed()` - Failed signatures +2. `logRateLimitViolation()` - Rate limit hits +3. `logCommandInjectionAttempt()` - Malicious input attempts + +### Priority 2: Update existing code + +1. Add logging to signature.service.ts +2. Add logging to git-validation.util.ts (throw + log) +3. Document rate limit violations (if not already handled by NestJS) + +### Priority 3: Review capability guard + +1. Check if logCapabilityDenied is being called +2. Add calls if missing + +## Status Assessment + +After reviewing issue #276, we already have: + +- ✅ logCapabilityDenied() method +- ✅ logIncomingConnectionAttempt() +- ✅ logIncomingConnectionRejected() +- ✅ Signature verification failures for connections + +What's actually missing: + +1. General signature verification failures (outside connection context) +2. Rate limit violation logging +3. Command injection attempt logging + +## Implementation Approach + +Focus on what's truly missing and actionable: + +1. **Add command injection attempt logging** + - Update git-validation.util.ts to log before throwing + - Create logCommandInjectionAttempt() method + +2. **Add rate limit logging** + - Check if NestJS throttler already logs + - Add custom logging if needed + +3. **Verify capability logging** + - Check that capability.guard.ts calls logCapabilityDenied + +## Progress + +- [ ] Create scratchpad +- [ ] Add logCommandInjectionAttempt() to audit service +- [ ] Update git-validation.util.ts to log attempts +- [ ] Check capability guard logging +- [ ] Check rate limit logging +- [ ] Add tests +- [ ] Run quality gates +- [ ] Commit changes +- [ ] Push and close issue + +## Notes + +Some of the required logging may already be in place. Need to verify: + +1. Capability guard usage +2. Rate limiter behavior +3. OIDC validation (may be in auth module, not federation) + +Focus on concrete, implementable improvements rather than theoretical gaps.