From 6f71edd14e1d17c7fc2cc0a4370659c8f9c8acdd Mon Sep 17 00:00:00 2001 From: Snider Date: Thu, 29 Jan 2026 13:29:26 +0000 Subject: [PATCH] fix(security): address P2 security items and migration mismatch P2-058: Migration Mismatch - Created new migration for vendor tracking tables (000004) - Added explicit $table property to all models with uptelligence_ prefix - Clarified dual-purpose nature (uptime monitoring + vendor tracking) - Added appropriate indexes for common query patterns P2-059: Webhook Signature Timing Attack Audit - Verified all signature verification uses hash_equals() - Added comprehensive tests in WebhookSignatureVerificationTest.php - Tests cover all providers, grace periods, edge cases P2-060: API Key Exposure in Logs - Added redactSensitiveData() to AIAnalyzerService - Added redactSensitiveData() to IssueGeneratorService - Added redactSensitiveData() to VendorUpdateCheckerService - Redacts API keys, tokens, bearer tokens, auth headers P2-061: Missing Webhook Payload Validation - Added MAX_PAYLOAD_SIZE (1MB) and MAX_JSON_DEPTH (32) limits - Added validatePayloadSize() for DoS protection - Added parseAndValidateJson() with depth limit - Added validatePayloadStructure() for provider-specific validation - Added hasExcessiveArraySize() to prevent memory exhaustion - Added tests in WebhookPayloadValidationTest.php Co-Authored-By: Claude Opus 4.5 --- Controllers/Api/WebhookController.php | 256 +++++++++++++- Models/AnalysisLog.php | 5 + Models/Asset.php | 5 + Models/AssetVersion.php | 5 + Models/DiffCache.php | 7 +- Models/UpstreamTodo.php | 5 + Models/Vendor.php | 7 + Models/VersionRelease.php | 5 + Services/AIAnalyzerService.php | 40 ++- Services/IssueGeneratorService.php | 35 +- Services/VendorUpdateCheckerService.php | 33 +- TODO.md | 131 +++---- ...0004_create_uptelligence_vendor_tables.php | 202 +++++++++++ tests/Unit/WebhookPayloadValidationTest.php | 332 ++++++++++++++++++ .../Unit/WebhookSignatureVerificationTest.php | 299 ++++++++++++++++ 15 files changed, 1293 insertions(+), 74 deletions(-) create mode 100644 database/migrations/0001_01_01_000004_create_uptelligence_vendor_tables.php create mode 100644 tests/Unit/WebhookPayloadValidationTest.php create mode 100644 tests/Unit/WebhookSignatureVerificationTest.php diff --git a/Controllers/Api/WebhookController.php b/Controllers/Api/WebhookController.php index 57bb97e..cb4901c 100644 --- a/Controllers/Api/WebhookController.php +++ b/Controllers/Api/WebhookController.php @@ -21,6 +21,16 @@ use Core\Mod\Uptelligence\Services\WebhookReceiverService; */ class WebhookController extends Controller { + /** + * Maximum allowed payload size in bytes (1 MB). + */ + protected const MAX_PAYLOAD_SIZE = 1048576; + + /** + * Maximum allowed JSON nesting depth. + */ + protected const MAX_JSON_DEPTH = 32; + public function __construct( protected WebhookReceiverService $service, ) {} @@ -55,6 +65,12 @@ class WebhookController extends Controller // Get raw payload $payload = $request->getContent(); + // Validate payload size (DoS protection) + $payloadValidation = $this->validatePayloadSize($payload, $webhook->id); + if ($payloadValidation !== null) { + return $payloadValidation; + } + // Verify signature $signature = $this->extractSignature($request, $webhook->provider); $signatureStatus = $this->service->verifySignature($webhook, $payload, $signature); @@ -69,17 +85,18 @@ class WebhookController extends Controller return response('Invalid signature', 401); } - // Parse JSON payload - $data = json_decode($payload, true); - if (json_last_error() !== JSON_ERROR_NONE) { - Log::warning('Uptelligence webhook invalid JSON payload', [ - 'webhook_id' => $webhook->id, - 'error' => json_last_error_msg(), - ]); - + // Parse and validate JSON payload + $data = $this->parseAndValidateJson($payload, $webhook->id); + if ($data === null) { return response('Invalid JSON payload', 400); } + // Validate payload structure + $structureValidation = $this->validatePayloadStructure($data, $webhook); + if ($structureValidation !== null) { + return $structureValidation; + } + // Determine event type $eventType = $this->determineEventType($request, $data, $webhook->provider); @@ -265,4 +282,227 @@ class WebhookController extends Controller 'has_secret' => ! empty($webhook->secret), ]); } + + // ------------------------------------------------------------------------- + // Payload Validation Methods + // ------------------------------------------------------------------------- + + /** + * Validate payload size to prevent DoS attacks. + */ + protected function validatePayloadSize(string $payload, int $webhookId): ?Response + { + $payloadSize = strlen($payload); + + if ($payloadSize > self::MAX_PAYLOAD_SIZE) { + Log::warning('Uptelligence webhook payload too large', [ + 'webhook_id' => $webhookId, + 'payload_size' => $payloadSize, + 'max_size' => self::MAX_PAYLOAD_SIZE, + ]); + + return response('Payload too large', 413); + } + + if ($payloadSize === 0) { + Log::warning('Uptelligence webhook empty payload', [ + 'webhook_id' => $webhookId, + ]); + + return response('Empty payload', 400); + } + + return null; + } + + /** + * Parse and validate JSON payload with depth limit. + * + * Returns the parsed data or null on failure. + */ + protected function parseAndValidateJson(string $payload, int $webhookId): ?array + { + // Parse with depth limit to prevent deeply nested JSON attacks + $data = json_decode($payload, true, self::MAX_JSON_DEPTH); + + if (json_last_error() !== JSON_ERROR_NONE) { + $errorMessage = json_last_error_msg(); + + // Check for depth-related errors + if (json_last_error() === JSON_ERROR_DEPTH) { + Log::warning('Uptelligence webhook JSON too deeply nested', [ + 'webhook_id' => $webhookId, + 'max_depth' => self::MAX_JSON_DEPTH, + ]); + } else { + Log::warning('Uptelligence webhook invalid JSON payload', [ + 'webhook_id' => $webhookId, + 'error' => $errorMessage, + ]); + } + + return null; + } + + // Ensure payload is an object/array (not a scalar) + if (! is_array($data)) { + Log::warning('Uptelligence webhook payload must be an object', [ + 'webhook_id' => $webhookId, + 'type' => gettype($data), + ]); + + return null; + } + + return $data; + } + + /** + * Validate payload structure based on provider. + * + * Performs basic schema validation to ensure expected fields exist. + */ + protected function validatePayloadStructure(array $data, UptelligenceWebhook $webhook): ?Response + { + $provider = $webhook->provider; + $webhookId = $webhook->id; + + // Validate based on provider + $validation = match ($provider) { + UptelligenceWebhook::PROVIDER_GITHUB => $this->validateGitHubPayload($data), + UptelligenceWebhook::PROVIDER_GITLAB => $this->validateGitLabPayload($data), + UptelligenceWebhook::PROVIDER_NPM => $this->validateNpmPayload($data), + UptelligenceWebhook::PROVIDER_PACKAGIST => $this->validatePackagistPayload($data), + default => $this->validateCustomPayload($data), + }; + + if ($validation !== true) { + Log::warning('Uptelligence webhook payload validation failed', [ + 'webhook_id' => $webhookId, + 'provider' => $provider, + 'error' => $validation, + ]); + + return response('Invalid payload structure: ' . $validation, 400); + } + + return null; + } + + /** + * Validate GitHub webhook payload structure. + */ + protected function validateGitHubPayload(array $data): string|bool + { + // GitHub webhooks should have an action field for most events + // Release events have release object + if (isset($data['release'])) { + if (! is_array($data['release'])) { + return 'release must be an object'; + } + } + + // Check for suspiciously large arrays (potential DoS) + if ($this->hasExcessiveArraySize($data)) { + return 'payload contains excessively large arrays'; + } + + return true; + } + + /** + * Validate GitLab webhook payload structure. + */ + protected function validateGitLabPayload(array $data): string|bool + { + // GitLab webhooks typically have object_kind + if (isset($data['object_kind']) && ! is_string($data['object_kind'])) { + return 'object_kind must be a string'; + } + + if ($this->hasExcessiveArraySize($data)) { + return 'payload contains excessively large arrays'; + } + + return true; + } + + /** + * Validate npm webhook payload structure. + */ + protected function validateNpmPayload(array $data): string|bool + { + // npm webhooks should have event field + if (isset($data['event']) && ! is_string($data['event'])) { + return 'event must be a string'; + } + + if ($this->hasExcessiveArraySize($data)) { + return 'payload contains excessively large arrays'; + } + + return true; + } + + /** + * Validate Packagist webhook payload structure. + */ + protected function validatePackagistPayload(array $data): string|bool + { + // Packagist should have package or repository info + if (isset($data['versions']) && ! is_array($data['versions'])) { + return 'versions must be an array'; + } + + if ($this->hasExcessiveArraySize($data)) { + return 'payload contains excessively large arrays'; + } + + return true; + } + + /** + * Validate custom webhook payload structure. + */ + protected function validateCustomPayload(array $data): string|bool + { + // Minimal validation for custom webhooks + if ($this->hasExcessiveArraySize($data)) { + return 'payload contains excessively large arrays'; + } + + return true; + } + + /** + * Check if payload contains excessively large arrays (DoS protection). + * + * Recursively checks array sizes to prevent memory exhaustion + * from payloads with many elements at any nesting level. + */ + protected function hasExcessiveArraySize(array $data, int $maxElements = 1000, int $depth = 0): bool + { + // Prevent infinite recursion + if ($depth > self::MAX_JSON_DEPTH) { + return true; + } + + $totalElements = 0; + + foreach ($data as $value) { + $totalElements++; + + if ($totalElements > $maxElements) { + return true; + } + + if (is_array($value)) { + if ($this->hasExcessiveArraySize($value, $maxElements - $totalElements, $depth + 1)) { + return true; + } + } + } + + return false; + } } diff --git a/Models/AnalysisLog.php b/Models/AnalysisLog.php index 4b426e8..df4a3ce 100644 --- a/Models/AnalysisLog.php +++ b/Models/AnalysisLog.php @@ -17,6 +17,11 @@ class AnalysisLog extends Model { use HasFactory; + /** + * The table associated with the model. + */ + protected $table = 'uptelligence_analysis_logs'; + // Actions public const ACTION_VERSION_DETECTED = 'version_detected'; diff --git a/Models/Asset.php b/Models/Asset.php index 0c93d78..4cb8543 100644 --- a/Models/Asset.php +++ b/Models/Asset.php @@ -17,6 +17,11 @@ class Asset extends Model { use HasFactory; + /** + * The table associated with the model. + */ + protected $table = 'uptelligence_assets'; + // Asset types public const TYPE_COMPOSER = 'composer'; diff --git a/Models/AssetVersion.php b/Models/AssetVersion.php index 923a81b..c647b6a 100644 --- a/Models/AssetVersion.php +++ b/Models/AssetVersion.php @@ -17,6 +17,11 @@ class AssetVersion extends Model { use HasFactory; + /** + * The table associated with the model. + */ + protected $table = 'uptelligence_asset_versions'; + protected $fillable = [ 'asset_id', 'version', diff --git a/Models/DiffCache.php b/Models/DiffCache.php index 0714e19..1c57240 100644 --- a/Models/DiffCache.php +++ b/Models/DiffCache.php @@ -17,7 +17,12 @@ class DiffCache extends Model { use HasFactory; - protected $table = 'diff_cache'; + /** + * The table associated with the model. + * + * Uses the uptelligence_ prefix for consistency with other module tables. + */ + protected $table = 'uptelligence_diff_cache'; // Change types public const CHANGE_ADDED = 'added'; diff --git a/Models/UpstreamTodo.php b/Models/UpstreamTodo.php index 2457e81..edc7024 100644 --- a/Models/UpstreamTodo.php +++ b/Models/UpstreamTodo.php @@ -19,6 +19,11 @@ class UpstreamTodo extends Model use HasFactory; use SoftDeletes; + /** + * The table associated with the model. + */ + protected $table = 'uptelligence_upstream_todos'; + // Types public const TYPE_FEATURE = 'feature'; diff --git a/Models/Vendor.php b/Models/Vendor.php index 3b21f85..c74221e 100644 --- a/Models/Vendor.php +++ b/Models/Vendor.php @@ -19,6 +19,13 @@ class Vendor extends Model use HasFactory; use SoftDeletes; + /** + * The table associated with the model. + * + * Explicitly set to match the uptelligence_ prefix convention. + */ + protected $table = 'uptelligence_vendors'; + // Source types public const SOURCE_LICENSED = 'licensed'; diff --git a/Models/VersionRelease.php b/Models/VersionRelease.php index 67ff3ac..f59a8e7 100644 --- a/Models/VersionRelease.php +++ b/Models/VersionRelease.php @@ -20,6 +20,11 @@ class VersionRelease extends Model use HasFactory; use SoftDeletes; + /** + * The table associated with the model. + */ + protected $table = 'uptelligence_version_releases'; + // Storage disk options public const DISK_LOCAL = 'local'; diff --git a/Services/AIAnalyzerService.php b/Services/AIAnalyzerService.php index 957af52..0b2110e 100644 --- a/Services/AIAnalyzerService.php +++ b/Services/AIAnalyzerService.php @@ -299,7 +299,7 @@ PROMPT; Log::error('Uptelligence: Anthropic API request failed', [ 'status' => $response->status(), - 'body' => substr($response->body(), 0, 500), + 'body' => $this->redactSensitiveData(substr($response->body(), 0, 500)), ]); return null; @@ -354,12 +354,48 @@ PROMPT; Log::error('Uptelligence: OpenAI API request failed', [ 'status' => $response->status(), - 'body' => substr($response->body(), 0, 500), + 'body' => $this->redactSensitiveData(substr($response->body(), 0, 500)), ]); return null; } + /** + * Redact sensitive data from log messages. + * + * Removes or masks API keys, tokens, and other credentials that + * might appear in error responses or debug output. + */ + protected function redactSensitiveData(string $content): string + { + // Redact common API key patterns + $patterns = [ + // Anthropic API keys (sk-ant-...) + '/sk-ant-[a-zA-Z0-9_-]+/' => '[REDACTED_ANTHROPIC_KEY]', + // OpenAI API keys (sk-...) + '/sk-[a-zA-Z0-9]{20,}/' => '[REDACTED_OPENAI_KEY]', + // Generic bearer tokens + '/Bearer\s+[a-zA-Z0-9._-]+/' => 'Bearer [REDACTED]', + // Authorization headers + '/["\']?[Aa]uthorization["\']?\s*:\s*["\']?[^"\'}\s]+/' => '"authorization": "[REDACTED]"', + // API key query parameters + '/api[_-]?key=([^&\s"\']+)/' => 'api_key=[REDACTED]', + // x-api-key header values + '/x-api-key["\']?\s*:\s*["\']?[^"\'}\s]+/' => '"x-api-key": "[REDACTED]"', + // Generic secret patterns + '/["\']?secret["\']?\s*:\s*["\']?[^"\'}\s]+/' => '"secret": "[REDACTED]"', + // Token patterns + '/["\']?token["\']?\s*:\s*["\']?[a-zA-Z0-9._-]{20,}["\']?/' => '"token": "[REDACTED]"', + ]; + + $redacted = $content; + foreach ($patterns as $pattern => $replacement) { + $redacted = preg_replace($pattern, $replacement, $redacted); + } + + return $redacted; + } + /** * Parse AI response into structured data. */ diff --git a/Services/IssueGeneratorService.php b/Services/IssueGeneratorService.php index 319727a..18649dd 100644 --- a/Services/IssueGeneratorService.php +++ b/Services/IssueGeneratorService.php @@ -188,7 +188,7 @@ class IssueGeneratorService Log::error('Uptelligence: GitHub issue creation failed', [ 'todo_id' => $todo->id, 'status' => $response->status(), - 'body' => substr($response->body(), 0, 500), + 'body' => $this->redactSensitiveData(substr($response->body(), 0, 500)), ]); return null; @@ -258,12 +258,43 @@ class IssueGeneratorService Log::error('Uptelligence: Gitea issue creation failed', [ 'todo_id' => $todo->id, 'status' => $response->status(), - 'body' => substr($response->body(), 0, 500), + 'body' => $this->redactSensitiveData(substr($response->body(), 0, 500)), ]); return null; } + /** + * Redact sensitive data from log messages. + * + * Removes or masks API tokens and credentials that might + * appear in error responses. + */ + protected function redactSensitiveData(string $content): string + { + $patterns = [ + // GitHub tokens (ghp_..., gho_..., github_pat_...) + '/ghp_[a-zA-Z0-9]+/' => '[REDACTED_GITHUB_TOKEN]', + '/gho_[a-zA-Z0-9]+/' => '[REDACTED_GITHUB_TOKEN]', + '/github_pat_[a-zA-Z0-9_]+/' => '[REDACTED_GITHUB_TOKEN]', + // Generic bearer tokens + '/Bearer\s+[a-zA-Z0-9._-]+/' => 'Bearer [REDACTED]', + // Gitea tokens + '/token\s+[a-zA-Z0-9]{20,}/' => 'token [REDACTED]', + // Authorization header values + '/["\']?[Aa]uthorization["\']?\s*:\s*["\']?[^"\'}\s]+/' => '"authorization": "[REDACTED]"', + // Generic token patterns in JSON + '/["\']?token["\']?\s*:\s*["\']?[a-zA-Z0-9._-]{20,}["\']?/' => '"token": "[REDACTED]"', + ]; + + $redacted = $content; + foreach ($patterns as $pattern => $replacement) { + $redacted = preg_replace($pattern, $replacement, $redacted); + } + + return $redacted; + } + /** * Build issue title. */ diff --git a/Services/VendorUpdateCheckerService.php b/Services/VendorUpdateCheckerService.php index 8cf5d76..2c0f146 100644 --- a/Services/VendorUpdateCheckerService.php +++ b/Services/VendorUpdateCheckerService.php @@ -119,7 +119,7 @@ class VendorUpdateCheckerService Log::warning('Uptelligence: GitHub API request failed', [ 'vendor' => $vendor->slug, 'status' => $response->status(), - 'body' => $response->body(), + 'body' => $this->redactSensitiveData(substr($response->body(), 0, 500)), ]); return $this->errorResult("GitHub API error: {$response->status()}"); @@ -464,4 +464,35 @@ class VendorUpdateCheckerService return $version ?: null; } + + /** + * Redact sensitive data from log messages. + * + * Removes or masks API tokens and credentials that might + * appear in error responses. + */ + protected function redactSensitiveData(string $content): string + { + $patterns = [ + // GitHub tokens (ghp_..., gho_..., github_pat_...) + '/ghp_[a-zA-Z0-9]+/' => '[REDACTED_GITHUB_TOKEN]', + '/gho_[a-zA-Z0-9]+/' => '[REDACTED_GITHUB_TOKEN]', + '/github_pat_[a-zA-Z0-9_]+/' => '[REDACTED_GITHUB_TOKEN]', + // Generic bearer tokens + '/Bearer\s+[a-zA-Z0-9._-]+/' => 'Bearer [REDACTED]', + // Authorization header values + '/["\']?[Aa]uthorization["\']?\s*:\s*["\']?[^"\'}\s]+/' => '"authorization": "[REDACTED]"', + // Generic token patterns in JSON + '/["\']?token["\']?\s*:\s*["\']?[a-zA-Z0-9._-]{20,}["\']?/' => '"token": "[REDACTED]"', + // API key patterns + '/api[_-]?key=([^&\s"\']+)/' => 'api_key=[REDACTED]', + ]; + + $redacted = $content; + foreach ($patterns as $pattern => $replacement) { + $redacted = preg_replace($pattern, $replacement, $redacted); + } + + return $redacted; + } } diff --git a/TODO.md b/TODO.md index 2bb4994..a64f98d 100644 --- a/TODO.md +++ b/TODO.md @@ -6,78 +6,81 @@ Upstream vendor tracking and dependency intelligence for Host UK. ## P1 - Critical / Security -### Migration Mismatch - Uptime Monitoring vs Vendor Tracking -The first migration (`0001_01_01_000001_create_uptelligence_tables.php`) creates uptime monitoring tables (`uptelligence_monitors`, `uptelligence_checks`, `uptelligence_incidents`, `uptelligence_daily_stats`) rather than vendor tracking tables. +### ~~Migration Mismatch - Uptime Monitoring vs Vendor Tracking~~ FIXED (P2-058) -**Note:** The code review from 2026-01-21 mentions migrations were created (`2026_01_21_100000_create_uptelligence_tables.php`), but this file is not present in the repository. The current migration appears to be for a different purpose (uptime monitoring). +**FIXED:** 2026-01-29 -**Files affected:** -- `database/migrations/0001_01_01_000001_create_uptelligence_tables.php` - Contains uptime monitoring tables -- `Models/Vendor.php` - References `vendors` table -- `Models/VersionRelease.php` - References `version_releases` table -- `Models/UpstreamTodo.php` - References `upstream_todos` table -- `Models/DiffCache.php` - References `diff_cache` table -- `Models/AnalysisLog.php` - References `analysis_logs` table -- `Models/Asset.php` - References `assets` table -- `Models/AssetVersion.php` - References `asset_versions` table +The package now clarifies that it serves dual purposes: +1. **Uptime monitoring** (existing migration 000001) - for server health tracking +2. **Vendor tracking** (new migration 000004) - for upstream dependency intelligence -**Acceptance criteria:** -- [ ] Clarify whether uptime monitoring is part of this package or a separate concern -- [ ] If vendor tracking is the focus, replace or supplement migration with vendor tables -- [ ] Ensure all model tables are created with appropriate columns -- [ ] Add indexes as noted in the prior code review +**Changes made:** +- [x] Created new migration `0001_01_01_000004_create_uptelligence_vendor_tables.php` with all vendor tracking tables +- [x] Added explicit `$table` property to all models with `uptelligence_` prefix: + - `Vendor` -> `uptelligence_vendors` + - `VersionRelease` -> `uptelligence_version_releases` + - `UpstreamTodo` -> `uptelligence_upstream_todos` + - `DiffCache` -> `uptelligence_diff_cache` + - `AnalysisLog` -> `uptelligence_analysis_logs` + - `Asset` -> `uptelligence_assets` + - `AssetVersion` -> `uptelligence_asset_versions` +- [x] Added appropriate indexes for common query patterns +- [x] Documented dual-purpose nature in migration comments -### Webhook Signature Timing Attack Vulnerability -The `verifyGitLabSignature` method uses direct string comparison which may be vulnerable to timing attacks. +### ~~Webhook Signature Timing Attack Vulnerability~~ FIXED (P2-059) -**File:** `Models/UptelligenceWebhook.php:250-253` -```php -protected function verifyGitLabSignature(string $signature, string $secret): bool -{ - return hash_equals($secret, $signature); // This is correct, but see below -} -``` +**FIXED:** 2026-01-29 -**Note:** The method itself uses `hash_equals`, but verify all callers pass correctly. Additionally, consider constant-time comparison for all providers. +**Audit result:** All signature verification methods already use `hash_equals()` for timing-safe comparison. The implementation is correct. -**Acceptance criteria:** -- [ ] Audit all signature verification paths for timing safety -- [ ] Add unit tests for signature verification edge cases +**Changes made:** +- [x] Audited all signature verification paths - all use `hash_equals()` +- [x] Added comprehensive unit tests in `tests/Unit/WebhookSignatureVerificationTest.php`: + - Tests for all providers (GitHub, GitLab, npm, Packagist, custom) + - Tests for grace period/secret rotation + - Tests for malformed signatures + - Tests for binary payloads + - Tests for edge cases (empty payloads, large payloads) -### API Key Exposure in Logs -The `AIAnalyzerService` and other services may log sensitive data in error scenarios. +### ~~API Key Exposure in Logs~~ FIXED (P2-060) -**Files affected:** -- `Services/AIAnalyzerService.php` - Logs API responses which could contain sensitive context -- `Services/IssueGeneratorService.php` - Logs response bodies on failure +**FIXED:** 2026-01-29 -**Acceptance criteria:** -- [ ] Audit all Log::error calls for sensitive data -- [ ] Truncate or redact sensitive information in logs -- [ ] Never log full API request/response bodies containing credentials +**Changes made:** +- [x] Added `redactSensitiveData()` method to `AIAnalyzerService` +- [x] Added `redactSensitiveData()` method to `IssueGeneratorService` +- [x] Added `redactSensitiveData()` method to `VendorUpdateCheckerService` +- [x] All Log::error calls now pass through redaction before logging +- [x] Redaction patterns cover: + - Anthropic API keys (sk-ant-...) + - OpenAI API keys (sk-...) + - GitHub tokens (ghp_..., gho_..., github_pat_...) + - Bearer tokens + - Authorization headers + - Generic API keys and secrets -### Missing Input Validation on Webhook Payloads -The `WebhookController` accepts JSON payloads without size limits or schema validation. +### ~~Missing Input Validation on Webhook Payloads~~ FIXED (P2-061) -**File:** `Controllers/Api/WebhookController.php` +**FIXED:** 2026-01-29 -**Acceptance criteria:** -- [ ] Add maximum payload size validation (e.g., 1MB limit) -- [ ] Add basic schema validation for expected payload structure -- [ ] Add protection against deeply nested JSON (DoS vector) +**Changes made:** +- [x] Added `MAX_PAYLOAD_SIZE` constant (1 MB limit) +- [x] Added `MAX_JSON_DEPTH` constant (32 levels) +- [x] Added `validatePayloadSize()` method - rejects payloads > 1MB +- [x] Added `parseAndValidateJson()` method - validates JSON with depth limit +- [x] Added `validatePayloadStructure()` method - provider-specific validation +- [x] Added `hasExcessiveArraySize()` method - prevents DoS via large arrays +- [x] Added comprehensive unit tests in `tests/Unit/WebhookPayloadValidationTest.php` --- ## P2 - High Priority -### Missing Table Name on Vendor Model -The `Vendor` model doesn't explicitly set `$table`, relying on Laravel's convention which would create `vendors` table. +### ~~Missing Table Name on Vendor Model~~ FIXED +**FIXED:** 2026-01-29 (as part of P2-058 migration fix) -**File:** `Models/Vendor.php` - -**Acceptance criteria:** -- [ ] Add explicit `protected $table = 'uptelligence_vendors';` for consistency with other models -- [ ] Update migration to match +- [x] Added explicit `protected $table = 'uptelligence_vendors';` to Vendor model +- [x] All models now have explicit table names with `uptelligence_` prefix ### DiffAnalyzerService Constructor Pattern Inconsistency `DiffAnalyzerService` requires a `Vendor` in constructor unlike other services (which use dependency injection). @@ -177,14 +180,12 @@ Some HTTP calls have retry logic, others don't. - [ ] Add retry logic to all external HTTP calls - [ ] Extract common HTTP client configuration to shared method -### DiffCache Table Name Hardcoded -Model sets table name to `diff_cache` but other tables use `uptelligence_` prefix. +### ~~DiffCache Table Name Hardcoded~~ FIXED +**FIXED:** 2026-01-29 (as part of P2-058 migration fix) -**File:** `Models/DiffCache.php:22` - -**Acceptance criteria:** -- [ ] Rename to `uptelligence_diff_cache` for consistency -- [ ] Update migration +- [x] Renamed table to `uptelligence_diff_cache` for consistency +- [x] Updated model `$table` property +- [x] Created migration with correct table name ### Missing Carbon Import in UptelligenceDigest Uses `\Carbon\Carbon` with full path instead of import. @@ -348,6 +349,16 @@ Extract structured changelog data from releases. ## Completed +### 2026-01-29 P2 Security & Infrastructure Fixes + +- [x] **P2-058: Migration Mismatch** - Created vendor tracking migration (`0001_01_01_000004_create_uptelligence_vendor_tables.php`), added explicit `$table` properties to all models with `uptelligence_` prefix, clarified dual-purpose nature (uptime + vendor tracking) +- [x] **P2-059: Webhook Signature Timing Attack Audit** - Verified all signature verification uses `hash_equals()`, added comprehensive tests in `tests/Unit/WebhookSignatureVerificationTest.php` +- [x] **P2-060: API Key Exposure in Logs** - Added `redactSensitiveData()` method to AIAnalyzerService, IssueGeneratorService, and VendorUpdateCheckerService to redact API keys, tokens, and credentials from log output +- [x] **P2-061: Missing Webhook Payload Validation** - Added payload size limit (1MB), JSON depth limit (32), provider-specific schema validation, array size limits, tests in `tests/Unit/WebhookPayloadValidationTest.php` +- [x] **P3-DiffCache Table Name** - Fixed table name from `diff_cache` to `uptelligence_diff_cache` for consistency + +### 2026-01-21 Code Review Wave + Items completed as part of the 2026-01-21 code review wave (per `changelog/2026/jan/code-review.md`): - [x] **Path traversal validation** - Added to `DiffAnalyzerService::validatePath()` to prevent directory traversal attacks diff --git a/database/migrations/0001_01_01_000004_create_uptelligence_vendor_tables.php b/database/migrations/0001_01_01_000004_create_uptelligence_vendor_tables.php new file mode 100644 index 0000000..892ccbe --- /dev/null +++ b/database/migrations/0001_01_01_000004_create_uptelligence_vendor_tables.php @@ -0,0 +1,202 @@ +id(); + $table->string('slug', 64)->unique(); + $table->string('name'); + $table->string('vendor_name')->nullable(); + $table->string('source_type', 32)->default('oss'); // licensed, oss, plugin + $table->string('plugin_platform', 32)->nullable(); // altum, wordpress, laravel, other + $table->string('git_repo_url', 512)->nullable(); + $table->string('current_version', 64)->nullable(); + $table->string('previous_version', 64)->nullable(); + $table->json('path_mapping')->nullable(); + $table->json('ignored_paths')->nullable(); + $table->json('priority_paths')->nullable(); + $table->string('target_repo', 256)->nullable(); // owner/repo for issue creation + $table->string('target_branch', 64)->default('main'); + $table->boolean('is_active')->default(true); + $table->timestamp('last_checked_at')->nullable(); + $table->timestamp('last_analyzed_at')->nullable(); + $table->timestamps(); + $table->softDeletes(); + + $table->index(['is_active', 'source_type']); + $table->index('last_checked_at'); + }); + } + + // 2. Version Releases - tracked releases from vendors + if (! Schema::hasTable('uptelligence_version_releases')) { + Schema::create('uptelligence_version_releases', function (Blueprint $table) { + $table->id(); + $table->foreignId('vendor_id')->constrained('uptelligence_vendors')->cascadeOnDelete(); + $table->string('version', 64); + $table->string('previous_version', 64)->nullable(); + $table->string('status', 32)->default('pending'); // pending, analyzed, skipped + $table->json('metadata_json')->nullable(); + $table->json('summary')->nullable(); + $table->unsignedInteger('files_changed')->default(0); + $table->unsignedInteger('todos_created')->default(0); + $table->timestamp('released_at')->nullable(); + $table->timestamp('analyzed_at')->nullable(); + $table->timestamps(); + $table->softDeletes(); + + $table->unique(['vendor_id', 'version']); + $table->index(['vendor_id', 'status']); + $table->index('released_at'); + }); + } + + // 3. Upstream Todos - porting tasks from vendor changes + if (! Schema::hasTable('uptelligence_upstream_todos')) { + Schema::create('uptelligence_upstream_todos', function (Blueprint $table) { + $table->id(); + $table->foreignId('vendor_id')->constrained('uptelligence_vendors')->cascadeOnDelete(); + $table->string('from_version', 64)->nullable(); + $table->string('to_version', 64)->nullable(); + $table->string('type', 32)->default('feature'); // feature, bugfix, security, ui, block, api, refactor, dependency + $table->string('status', 32)->default('pending'); // pending, in_progress, completed, skipped + $table->string('title'); + $table->text('description')->nullable(); + $table->text('port_notes')->nullable(); + $table->unsignedTinyInteger('priority')->default(5); // 1-10 + $table->string('effort', 16)->default('medium'); // low, medium, high + $table->boolean('has_conflicts')->default(false); + $table->text('conflict_reason')->nullable(); + $table->json('files')->nullable(); + $table->json('dependencies')->nullable(); + $table->json('tags')->nullable(); + $table->json('ai_analysis')->nullable(); + $table->decimal('ai_confidence', 3, 2)->nullable(); + $table->string('github_issue_number', 32)->nullable(); + $table->timestamp('completed_at')->nullable(); + $table->timestamps(); + $table->softDeletes(); + + $table->index(['vendor_id', 'status']); + $table->index(['status', 'priority']); + $table->index('type'); + }); + } + + // 4. Diff Cache - cached diffs between versions + if (! Schema::hasTable('uptelligence_diff_cache')) { + Schema::create('uptelligence_diff_cache', function (Blueprint $table) { + $table->id(); + $table->foreignId('version_release_id')->constrained('uptelligence_version_releases')->cascadeOnDelete(); + $table->string('file_path', 512); + $table->string('change_type', 16); // added, modified, deleted, renamed + $table->string('category', 32)->nullable(); // controller, model, view, migration, config, etc. + $table->mediumText('diff_content')->nullable(); + $table->unsignedInteger('lines_added')->default(0); + $table->unsignedInteger('lines_removed')->default(0); + $table->json('metadata')->nullable(); + $table->timestamps(); + + $table->index('version_release_id'); + $table->index('change_type'); + $table->index('category'); + }); + } + + // 5. Analysis Logs - audit trail for analysis operations + if (! Schema::hasTable('uptelligence_analysis_logs')) { + Schema::create('uptelligence_analysis_logs', function (Blueprint $table) { + $table->id(); + $table->foreignId('vendor_id')->nullable()->constrained('uptelligence_vendors')->nullOnDelete(); + $table->foreignId('todo_id')->nullable()->constrained('uptelligence_upstream_todos')->nullOnDelete(); + $table->string('action', 64); + $table->string('status', 32)->default('success'); + $table->json('context')->nullable(); + $table->text('message')->nullable(); + $table->timestamps(); + + $table->index(['vendor_id', 'created_at']); + $table->index('action'); + }); + } + + // 6. Assets - tracked software assets (Composer/NPM packages) + if (! Schema::hasTable('uptelligence_assets')) { + Schema::create('uptelligence_assets', function (Blueprint $table) { + $table->id(); + $table->string('package_name'); + $table->string('type', 16); // composer, npm + $table->string('current_version', 64)->nullable(); + $table->string('latest_version', 64)->nullable(); + $table->string('registry_url', 512)->nullable(); + $table->boolean('is_active')->default(true); + $table->timestamp('last_checked_at')->nullable(); + $table->json('metadata')->nullable(); + $table->timestamps(); + + $table->unique(['package_name', 'type']); + $table->index(['type', 'is_active']); + }); + } + + // 7. Asset Versions - version history for assets + if (! Schema::hasTable('uptelligence_asset_versions')) { + Schema::create('uptelligence_asset_versions', function (Blueprint $table) { + $table->id(); + $table->foreignId('asset_id')->constrained('uptelligence_assets')->cascadeOnDelete(); + $table->string('version', 64); + $table->timestamp('released_at')->nullable(); + $table->json('metadata')->nullable(); + $table->timestamps(); + + $table->unique(['asset_id', 'version']); + $table->index(['asset_id', 'released_at']); + }); + } + + Schema::enableForeignKeyConstraints(); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::disableForeignKeyConstraints(); + Schema::dropIfExists('uptelligence_asset_versions'); + Schema::dropIfExists('uptelligence_assets'); + Schema::dropIfExists('uptelligence_analysis_logs'); + Schema::dropIfExists('uptelligence_diff_cache'); + Schema::dropIfExists('uptelligence_upstream_todos'); + Schema::dropIfExists('uptelligence_version_releases'); + Schema::dropIfExists('uptelligence_vendors'); + Schema::enableForeignKeyConstraints(); + } +}; diff --git a/tests/Unit/WebhookPayloadValidationTest.php b/tests/Unit/WebhookPayloadValidationTest.php new file mode 100644 index 0000000..a88e1db --- /dev/null +++ b/tests/Unit/WebhookPayloadValidationTest.php @@ -0,0 +1,332 @@ +createMock(WebhookReceiverService::class); + $this->controller = new WebhookController($service); + } + + protected function getPackageProviders($app): array + { + return []; + } + + /** + * Helper to invoke protected methods. + */ + protected function invokeMethod(object $object, string $methodName, array $parameters = []): mixed + { + $reflection = new ReflectionClass($object); + $method = $reflection->getMethod($methodName); + $method->setAccessible(true); + + return $method->invokeArgs($object, $parameters); + } + + // ------------------------------------------------------------------------- + // Payload Size Validation Tests + // ------------------------------------------------------------------------- + + /** + * Test that normal-sized payloads are accepted. + */ + #[Test] + public function it_accepts_normal_sized_payloads(): void + { + $payload = json_encode(['event' => 'release', 'data' => str_repeat('x', 1000)]); + + $result = $this->invokeMethod($this->controller, 'validatePayloadSize', [$payload, 1]); + + $this->assertNull($result); + } + + /** + * Test that oversized payloads are rejected. + */ + #[Test] + public function it_rejects_oversized_payloads(): void + { + // Create a payload larger than 1MB + $payload = str_repeat('x', 1048577); + + $result = $this->invokeMethod($this->controller, 'validatePayloadSize', [$payload, 1]); + + $this->assertNotNull($result); + $this->assertEquals(413, $result->getStatusCode()); + } + + /** + * Test that empty payloads are rejected. + */ + #[Test] + public function it_rejects_empty_payloads(): void + { + $result = $this->invokeMethod($this->controller, 'validatePayloadSize', ['', 1]); + + $this->assertNotNull($result); + $this->assertEquals(400, $result->getStatusCode()); + } + + // ------------------------------------------------------------------------- + // JSON Parsing Tests + // ------------------------------------------------------------------------- + + /** + * Test that valid JSON is parsed correctly. + */ + #[Test] + public function it_parses_valid_json(): void + { + $payload = '{"event":"release","version":"1.0.0"}'; + + $result = $this->invokeMethod($this->controller, 'parseAndValidateJson', [$payload, 1]); + + $this->assertIsArray($result); + $this->assertEquals('release', $result['event']); + $this->assertEquals('1.0.0', $result['version']); + } + + /** + * Test that invalid JSON is rejected. + */ + #[Test] + public function it_rejects_invalid_json(): void + { + $payload = '{invalid json}'; + + $result = $this->invokeMethod($this->controller, 'parseAndValidateJson', [$payload, 1]); + + $this->assertNull($result); + } + + /** + * Test that deeply nested JSON is rejected. + */ + #[Test] + public function it_rejects_deeply_nested_json(): void + { + // Create JSON with 35 levels of nesting (exceeds max depth of 32) + $nested = '"value"'; + for ($i = 0; $i < 35; $i++) { + $nested = '{"level' . $i . '":' . $nested . '}'; + } + + $result = $this->invokeMethod($this->controller, 'parseAndValidateJson', [$nested, 1]); + + $this->assertNull($result); + } + + /** + * Test that scalar JSON values are rejected. + */ + #[Test] + #[DataProvider('scalarJsonValues')] + public function it_rejects_scalar_json_values(string $json): void + { + $result = $this->invokeMethod($this->controller, 'parseAndValidateJson', [$json, 1]); + + $this->assertNull($result); + } + + /** + * Data provider for scalar JSON values. + */ + public static function scalarJsonValues(): array + { + return [ + 'string' => ['"just a string"'], + 'number' => ['12345'], + 'boolean true' => ['true'], + 'boolean false' => ['false'], + 'null' => ['null'], + ]; + } + + // ------------------------------------------------------------------------- + // Payload Structure Validation Tests + // ------------------------------------------------------------------------- + + /** + * Test that valid GitHub payloads are accepted. + */ + #[Test] + public function it_accepts_valid_github_payload(): void + { + $data = [ + 'action' => 'published', + 'release' => [ + 'tag_name' => 'v1.0.0', + 'name' => 'Version 1.0.0', + ], + ]; + + $result = $this->invokeMethod($this->controller, 'validateGitHubPayload', [$data]); + + $this->assertTrue($result); + } + + /** + * Test that GitHub payloads with invalid release field are rejected. + */ + #[Test] + public function it_rejects_github_payload_with_invalid_release(): void + { + $data = [ + 'action' => 'published', + 'release' => 'not an array', + ]; + + $result = $this->invokeMethod($this->controller, 'validateGitHubPayload', [$data]); + + $this->assertIsString($result); + $this->assertStringContainsString('release must be an object', $result); + } + + /** + * Test that valid GitLab payloads are accepted. + */ + #[Test] + public function it_accepts_valid_gitlab_payload(): void + { + $data = [ + 'object_kind' => 'release', + 'action' => 'create', + 'tag' => 'v1.0.0', + ]; + + $result = $this->invokeMethod($this->controller, 'validateGitLabPayload', [$data]); + + $this->assertTrue($result); + } + + /** + * Test that valid npm payloads are accepted. + */ + #[Test] + public function it_accepts_valid_npm_payload(): void + { + $data = [ + 'event' => 'package:publish', + 'name' => 'my-package', + 'version' => '1.0.0', + ]; + + $result = $this->invokeMethod($this->controller, 'validateNpmPayload', [$data]); + + $this->assertTrue($result); + } + + /** + * Test that valid Packagist payloads are accepted. + */ + #[Test] + public function it_accepts_valid_packagist_payload(): void + { + $data = [ + 'repository' => [ + 'url' => 'https://packagist.org/packages/vendor/package', + ], + 'versions' => [ + '1.0.0' => ['version' => '1.0.0'], + ], + ]; + + $result = $this->invokeMethod($this->controller, 'validatePackagistPayload', [$data]); + + $this->assertTrue($result); + } + + // ------------------------------------------------------------------------- + // Excessive Array Size Tests + // ------------------------------------------------------------------------- + + /** + * Test that normal array sizes are accepted. + */ + #[Test] + public function it_accepts_normal_array_sizes(): void + { + $data = [ + 'items' => array_fill(0, 100, 'item'), + ]; + + $result = $this->invokeMethod($this->controller, 'hasExcessiveArraySize', [$data]); + + $this->assertFalse($result); + } + + /** + * Test that excessive array sizes are detected. + */ + #[Test] + public function it_detects_excessive_array_sizes(): void + { + $data = [ + 'items' => array_fill(0, 2000, 'item'), + ]; + + $result = $this->invokeMethod($this->controller, 'hasExcessiveArraySize', [$data]); + + $this->assertTrue($result); + } + + /** + * Test that deeply nested arrays with many elements are detected. + */ + #[Test] + public function it_detects_excessive_nested_array_sizes(): void + { + $data = [ + 'level1' => [ + 'level2' => [ + 'items' => array_fill(0, 1500, 'item'), + ], + ], + ]; + + $result = $this->invokeMethod($this->controller, 'hasExcessiveArraySize', [$data]); + + $this->assertTrue($result); + } + + /** + * Test that payloads with excessive arrays are rejected. + */ + #[Test] + public function it_rejects_payload_with_excessive_arrays(): void + { + $data = [ + 'commits' => array_fill(0, 2000, ['id' => 'abc']), + ]; + + $result = $this->invokeMethod($this->controller, 'validateGitHubPayload', [$data]); + + $this->assertIsString($result); + $this->assertStringContainsString('excessively large arrays', $result); + } +} diff --git a/tests/Unit/WebhookSignatureVerificationTest.php b/tests/Unit/WebhookSignatureVerificationTest.php new file mode 100644 index 0000000..a01967f --- /dev/null +++ b/tests/Unit/WebhookSignatureVerificationTest.php @@ -0,0 +1,299 @@ + UptelligenceWebhook::PROVIDER_GITHUB, + 'secret' => 'test-secret-key', + ]); + + $payload = '{"action":"published","release":{"tag_name":"v1.0.0"}}'; + $validSignature = 'sha256=' . hash_hmac('sha256', $payload, 'test-secret-key'); + $invalidSignature = 'sha256=' . hash_hmac('sha256', $payload, 'wrong-secret'); + + // Valid signature should pass + $this->assertTrue($webhook->verifySignature($payload, $validSignature)); + + // Invalid signature should fail + $this->assertFalse($webhook->verifySignature($payload, $invalidSignature)); + + // Signature without prefix should also work + $signatureWithoutPrefix = hash_hmac('sha256', $payload, 'test-secret-key'); + $this->assertTrue($webhook->verifySignature($payload, $signatureWithoutPrefix)); + } + + /** + * Test that GitLab signature verification uses hash_equals. + */ + #[Test] + public function it_verifies_gitlab_signature_with_timing_safe_comparison(): void + { + $webhook = new UptelligenceWebhook([ + 'provider' => UptelligenceWebhook::PROVIDER_GITLAB, + 'secret' => 'gitlab-secret-token', + ]); + + $payload = '{"object_kind":"release","action":"create"}'; + + // GitLab uses X-Gitlab-Token header (direct token comparison) + $this->assertTrue($webhook->verifySignature($payload, 'gitlab-secret-token')); + $this->assertFalse($webhook->verifySignature($payload, 'wrong-token')); + + // Empty signature should fail when secret is set + $this->assertFalse($webhook->verifySignature($payload, '')); + $this->assertFalse($webhook->verifySignature($payload, null)); + } + + /** + * Test that npm signature verification uses hash_equals. + */ + #[Test] + public function it_verifies_npm_signature_with_timing_safe_comparison(): void + { + $webhook = new UptelligenceWebhook([ + 'provider' => UptelligenceWebhook::PROVIDER_NPM, + 'secret' => 'npm-webhook-secret', + ]); + + $payload = '{"event":"package:publish","version":"1.0.0"}'; + $validSignature = hash_hmac('sha256', $payload, 'npm-webhook-secret'); + + $this->assertTrue($webhook->verifySignature($payload, $validSignature)); + $this->assertFalse($webhook->verifySignature($payload, 'invalid-signature')); + } + + /** + * Test that Packagist signature verification uses hash_equals. + */ + #[Test] + public function it_verifies_packagist_signature_with_timing_safe_comparison(): void + { + $webhook = new UptelligenceWebhook([ + 'provider' => UptelligenceWebhook::PROVIDER_PACKAGIST, + 'secret' => 'packagist-secret', + ]); + + $payload = '{"repository":{"url":"https://packagist.org/packages/vendor/package"}}'; + // Packagist uses SHA-1 HMAC + $validSignature = hash_hmac('sha1', $payload, 'packagist-secret'); + + $this->assertTrue($webhook->verifySignature($payload, $validSignature)); + $this->assertFalse($webhook->verifySignature($payload, 'wrong-signature')); + } + + /** + * Test that custom webhook signature verification uses hash_equals. + */ + #[Test] + public function it_verifies_custom_signature_with_timing_safe_comparison(): void + { + $webhook = new UptelligenceWebhook([ + 'provider' => UptelligenceWebhook::PROVIDER_CUSTOM, + 'secret' => 'custom-secret-key', + ]); + + $payload = '{"version":"2.0.0","event":"release"}'; + $validSignature = 'sha256=' . hash_hmac('sha256', $payload, 'custom-secret-key'); + + $this->assertTrue($webhook->verifySignature($payload, $validSignature)); + $this->assertFalse($webhook->verifySignature($payload, 'sha256=invalid')); + } + + /** + * Test that signature verification skips when no secret is configured. + */ + #[Test] + public function it_skips_verification_when_no_secret_configured(): void + { + $webhook = new UptelligenceWebhook([ + 'provider' => UptelligenceWebhook::PROVIDER_GITHUB, + 'secret' => null, + ]); + + $payload = '{"any":"payload"}'; + + // Should return true (skip verification) when no secret is set + $this->assertTrue($webhook->verifySignature($payload, null)); + $this->assertTrue($webhook->verifySignature($payload, 'any-signature')); + } + + /** + * Test that signature verification fails when secret is set but no signature provided. + */ + #[Test] + public function it_fails_when_secret_is_set_but_no_signature_provided(): void + { + $webhook = new UptelligenceWebhook([ + 'provider' => UptelligenceWebhook::PROVIDER_GITHUB, + 'secret' => 'test-secret', + ]); + + $payload = '{"any":"payload"}'; + + $this->assertFalse($webhook->verifySignature($payload, null)); + $this->assertFalse($webhook->verifySignature($payload, '')); + } + + /** + * Test grace period allows previous secret. + */ + #[Test] + public function it_accepts_previous_secret_during_grace_period(): void + { + $webhook = new UptelligenceWebhook([ + 'provider' => UptelligenceWebhook::PROVIDER_GITHUB, + 'secret' => 'new-secret', + 'previous_secret' => 'old-secret', + 'secret_rotated_at' => now(), + 'grace_period_seconds' => 86400, // 24 hours + ]); + + $payload = '{"test":"payload"}'; + + // Both old and new secrets should work during grace period + $newSignature = 'sha256=' . hash_hmac('sha256', $payload, 'new-secret'); + $oldSignature = 'sha256=' . hash_hmac('sha256', $payload, 'old-secret'); + $wrongSignature = 'sha256=' . hash_hmac('sha256', $payload, 'wrong-secret'); + + $this->assertTrue($webhook->verifySignature($payload, $newSignature)); + $this->assertTrue($webhook->verifySignature($payload, $oldSignature)); + $this->assertFalse($webhook->verifySignature($payload, $wrongSignature)); + } + + /** + * Test that previous secret is rejected after grace period expires. + */ + #[Test] + public function it_rejects_previous_secret_after_grace_period(): void + { + $webhook = new UptelligenceWebhook([ + 'provider' => UptelligenceWebhook::PROVIDER_GITHUB, + 'secret' => 'new-secret', + 'previous_secret' => 'old-secret', + 'secret_rotated_at' => now()->subDays(2), // 2 days ago + 'grace_period_seconds' => 86400, // 24 hours (expired) + ]); + + $payload = '{"test":"payload"}'; + + $newSignature = 'sha256=' . hash_hmac('sha256', $payload, 'new-secret'); + $oldSignature = 'sha256=' . hash_hmac('sha256', $payload, 'old-secret'); + + $this->assertTrue($webhook->verifySignature($payload, $newSignature)); + $this->assertFalse($webhook->verifySignature($payload, $oldSignature)); + } + + /** + * Test various malformed signatures are rejected safely. + */ + #[Test] + #[DataProvider('malformedSignatures')] + public function it_safely_rejects_malformed_signatures(string $signature): void + { + $webhook = new UptelligenceWebhook([ + 'provider' => UptelligenceWebhook::PROVIDER_GITHUB, + 'secret' => 'test-secret', + ]); + + $payload = '{"test":"payload"}'; + + $this->assertFalse($webhook->verifySignature($payload, $signature)); + } + + /** + * Data provider for malformed signatures. + */ + public static function malformedSignatures(): array + { + return [ + 'empty string' => [''], + 'whitespace only' => [' '], + 'sha256= without hash' => ['sha256='], + 'sha1= prefix (github expects sha256)' => ['sha1=abc123'], + 'random string' => ['not-a-valid-signature'], + 'unicode characters' => ['sha256=\u0000\u0001\u0002'], + 'very long string' => [str_repeat('a', 10000)], + 'null bytes' => ["sha256=abc\x00def"], + 'partial hash' => ['sha256=abc'], + ]; + } + + /** + * Test that verification handles binary payloads correctly. + */ + #[Test] + public function it_handles_binary_payloads(): void + { + $webhook = new UptelligenceWebhook([ + 'provider' => UptelligenceWebhook::PROVIDER_GITHUB, + 'secret' => 'binary-secret', + ]); + + // Payload with null bytes and binary data + $binaryPayload = "binary\x00payload\xff\xfe"; + $validSignature = 'sha256=' . hash_hmac('sha256', $binaryPayload, 'binary-secret'); + + $this->assertTrue($webhook->verifySignature($binaryPayload, $validSignature)); + } + + /** + * Test that verification handles empty payload. + */ + #[Test] + public function it_handles_empty_payload(): void + { + $webhook = new UptelligenceWebhook([ + 'provider' => UptelligenceWebhook::PROVIDER_GITHUB, + 'secret' => 'empty-payload-secret', + ]); + + $emptyPayload = ''; + $validSignature = 'sha256=' . hash_hmac('sha256', $emptyPayload, 'empty-payload-secret'); + + $this->assertTrue($webhook->verifySignature($emptyPayload, $validSignature)); + } + + /** + * Test that verification handles large payloads. + */ + #[Test] + public function it_handles_large_payloads(): void + { + $webhook = new UptelligenceWebhook([ + 'provider' => UptelligenceWebhook::PROVIDER_GITHUB, + 'secret' => 'large-payload-secret', + ]); + + // 1MB payload + $largePayload = str_repeat('{"data":"' . str_repeat('x', 1000) . '"}', 1000); + $validSignature = 'sha256=' . hash_hmac('sha256', $largePayload, 'large-payload-secret'); + + $this->assertTrue($webhook->verifySignature($largePayload, $validSignature)); + } +}