mirror of
https://github.com/ivuorinen/monolog-gdpr-filter.git
synced 2026-03-13 00:01:24 +00:00
refactor: replace hardcoded strings with constant references (#100)
* fix(tests): remove error_log calls and clean up ComprehensiveValidationTest * refactor: replace hardcoded strings with MaskConstants and TestConstants references * fix(streaming): replace overcounting '[' heuristic with proper mask detection StreamingProcessor::getStatistics() was counting any message containing '[' as masked, causing false positives. Now checks for specific mask constants (MASK_GENERIC, MASK_BRACKETS, MASK_REDACTED_BRACKETS) instead. Also adds MASK_REDACTED_BRACKETS constant to MaskConstants and removes the now-unnecessary UnusedFunctionCall psalm suppression. * refactor(tests): replace remaining hardcoded literals with constant references Add new constants to TestConstants (MASK_REDACTED_PLAIN, MASK_SECRET_BRACKETS, MASK_SSN_BRACKETS, PATTERN_REDOS_NESTED_STAR, FIELD_USER_SSN, FIELD_USER_DATA) and replace all matching literals across 21 test files. Also removes dead memory_get_usage() call and uses existing TestConstants::IP_ADDRESS_PUBLIC for hardcoded IP. * fix(streaming): replace mask-token heuristic with accurate record comparison in getStatistics() The previous implementation only detected masking when specific mask tokens appeared in the message, missing cases where context was masked or different mask values were used. Compare original vs processed records instead. * refactor(tests): add PATTERN_EMAIL_SIMPLE, MASK_CARD_BRACKETS, EXPECTED_SSN_MASKED constants Replace cross-file duplicate literals with TestConstants references: - Email regex (4 files), '[CARD]' (2 files), 'SSN: [SSN]' (2 files) * fix(streaming): bypass audit logger in getStatistics() by calling orchestrator directly getStatistics() previously routed through processStream()/processChunk() which triggered the audit logger for each record. A read-only statistics method should not produce audit side-effects. Now calls orchestrator.process() directly and processes records one at a time without materializing the entire iterable. * refactor(tests): fix test quality issues and add PATTERN_CREDIT_CARD constant - Replace fail() message that leaked sensitive terms with count-only message - Replace bare 'EMAIL' string with MaskConstants::MASK_EMAIL for consistency - Remove error_log() debug output from CriticalBugRegressionTest - Add TestConstants::PATTERN_CREDIT_CARD and replace inline regex in 3 files
This commit is contained in:
GitHub
@@ -126,9 +126,6 @@ class ComprehensiveValidationTest extends TestCase
|
||||
|
||||
$this->assertInstanceOf(LogRecord::class, $result);
|
||||
$this->assertArrayHasKey('test_value', $result->context);
|
||||
|
||||
// Log successful processing for each type
|
||||
error_log('✅ Successfully processed PHP type: ' . $typeName);
|
||||
}
|
||||
|
||||
$this->assertCount(
|
||||
@@ -158,7 +155,6 @@ class ComprehensiveValidationTest extends TestCase
|
||||
$rateLimiter->isAllowed('memory_test_key_' . $i);
|
||||
}
|
||||
|
||||
memory_get_usage(true);
|
||||
$initialStats = RateLimiter::getMemoryStats();
|
||||
|
||||
// Phase 2: Wait for cleanup window and trigger cleanup
|
||||
@@ -196,12 +192,6 @@ class ComprehensiveValidationTest extends TestCase
|
||||
$cleanupStats['total_keys'],
|
||||
'Keys should not accumulate indefinitely'
|
||||
);
|
||||
|
||||
error_log(sprintf(
|
||||
'✅ Memory management working: Keys before=%d, after=%d',
|
||||
$initialStats['total_keys'],
|
||||
$cleanupStats['total_keys']
|
||||
));
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -226,50 +216,27 @@ class ComprehensiveValidationTest extends TestCase
|
||||
];
|
||||
|
||||
$caughtCount = 0;
|
||||
$totalPatterns = count($definitelyDangerousPatterns) + count($possiblyDangerousPatterns);
|
||||
|
||||
// Test definitely dangerous patterns
|
||||
foreach ($definitelyDangerousPatterns as $pattern => $description) {
|
||||
foreach (array_keys($definitelyDangerousPatterns) as $pattern) {
|
||||
try {
|
||||
PatternValidator::validateAll([sprintf('/%s/', $pattern) => TestConstants::DATA_MASKED]);
|
||||
error_log(sprintf(
|
||||
'⚠️ Pattern not caught: %s (%s)',
|
||||
$pattern,
|
||||
$description
|
||||
));
|
||||
} catch (Throwable) {
|
||||
$caughtCount++;
|
||||
error_log(sprintf(
|
||||
'✅ Caught dangerous pattern: %s (%s)',
|
||||
$pattern,
|
||||
$description
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
// Test possibly dangerous patterns (implementation may vary)
|
||||
foreach ($possiblyDangerousPatterns as $pattern => $description) {
|
||||
foreach (array_keys($possiblyDangerousPatterns) as $pattern) {
|
||||
try {
|
||||
PatternValidator::validateAll([sprintf('/%s/', $pattern) => TestConstants::DATA_MASKED]);
|
||||
error_log(sprintf(
|
||||
'ℹ️ Pattern allowed: %s (%s)',
|
||||
$pattern,
|
||||
$description
|
||||
));
|
||||
} catch (Throwable) {
|
||||
$caughtCount++;
|
||||
error_log(sprintf(
|
||||
'✅ Caught potentially dangerous pattern: %s (%s)',
|
||||
$pattern,
|
||||
$description
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
// At least some dangerous patterns should be caught
|
||||
$this->assertGreaterThan(0, $caughtCount, 'ReDoS protection should catch at least some dangerous patterns');
|
||||
|
||||
error_log(sprintf('✅ ReDoS protection caught %d/%d dangerous patterns', $caughtCount, $totalPatterns));
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -367,17 +334,10 @@ class ComprehensiveValidationTest extends TestCase
|
||||
}
|
||||
|
||||
if ($sensitiveTermsFound !== []) {
|
||||
error_log(sprintf(
|
||||
"⚠️ Scenario '%s': Sensitive terms still present: ",
|
||||
$scenario
|
||||
) . implode(', ', $sensitiveTermsFound));
|
||||
error_log(
|
||||
' Full message: ' . $loggedMessage
|
||||
);
|
||||
} else {
|
||||
error_log(sprintf(
|
||||
"✅ Scenario '%s': No sensitive terms found in sanitized message",
|
||||
$scenario
|
||||
$this->fail(sprintf(
|
||||
"Scenario '%s': %d sensitive term(s) still present in masked output",
|
||||
$scenario,
|
||||
count($sensitiveTermsFound)
|
||||
));
|
||||
}
|
||||
|
||||
@@ -424,8 +384,6 @@ class ComprehensiveValidationTest extends TestCase
|
||||
if ($json === false) {
|
||||
$this->fail('RateLimiter::getMemoryStats() returned false');
|
||||
}
|
||||
|
||||
error_log("✅ Rate limiter statistics: " . $json);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -484,19 +442,8 @@ class ComprehensiveValidationTest extends TestCase
|
||||
$memoryIncrease,
|
||||
'Memory usage should be reasonable for ' . $name
|
||||
);
|
||||
|
||||
error_log(sprintf(
|
||||
"✅ Safely processed extreme value '%s' in %ss using %d bytes",
|
||||
$name,
|
||||
$processingTime,
|
||||
$memoryIncrease
|
||||
));
|
||||
} catch (Throwable $e) {
|
||||
// Some extreme values might cause controlled exceptions
|
||||
error_log(sprintf(
|
||||
"ℹ️ Extreme value '%s' caused controlled exception: ",
|
||||
$name
|
||||
) . $e->getMessage());
|
||||
$this->assertInstanceOf(Throwable::class, $e);
|
||||
}
|
||||
}
|
||||
@@ -528,8 +475,8 @@ class ComprehensiveValidationTest extends TestCase
|
||||
$processor = $this->createProcessor(
|
||||
patterns: [
|
||||
'/\b\d{3}-\d{2}-\d{4}\b/' => MaskConstants::MASK_USSSN,
|
||||
'/[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}/' => MaskConstants::MASK_EMAIL,
|
||||
'/\b\d{4}[\s-]?\d{4}[\s-]?\d{4}[\s-]?\d{4}\b/' => MaskConstants::MASK_CC,
|
||||
TestConstants::PATTERN_EMAIL_SIMPLE => MaskConstants::MASK_EMAIL,
|
||||
TestConstants::PATTERN_CREDIT_CARD => MaskConstants::MASK_CC,
|
||||
],
|
||||
fieldPaths: [
|
||||
TestConstants::FIELD_USER_PASSWORD => FieldMaskConfig::remove(),
|
||||
@@ -618,11 +565,6 @@ class ComprehensiveValidationTest extends TestCase
|
||||
// Rate limiter should provide stats
|
||||
$stats = $rateLimitedLogger->getRateLimitStats();
|
||||
$this->assertIsArray($stats);
|
||||
|
||||
error_log(
|
||||
"✅ Complete integration test passed with "
|
||||
. count($this->auditLog) . " audit log entries"
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -646,9 +588,6 @@ class ComprehensiveValidationTest extends TestCase
|
||||
PatternValidator::clearCache();
|
||||
RateLimiter::clearAll();
|
||||
|
||||
// Log final validation summary
|
||||
error_log("🎯 Comprehensive validation completed successfully");
|
||||
|
||||
parent::tearDown();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -84,7 +84,7 @@ class CriticalBugRegressionTest extends TestCase
|
||||
$testCases = [
|
||||
'integer' => 42,
|
||||
'double' => 3.14,
|
||||
'string' => 'test string',
|
||||
'string' => TestConstants::MESSAGE_TEST_STRING,
|
||||
'boolean_true' => true,
|
||||
'boolean_false' => false,
|
||||
'null' => null,
|
||||
@@ -378,7 +378,7 @@ class CriticalBugRegressionTest extends TestCase
|
||||
{
|
||||
$safePatterns = [
|
||||
'/\b\d{3}-\d{2}-\d{4}\b/' => 'SSN',
|
||||
'/[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}/' => 'EMAIL',
|
||||
TestConstants::PATTERN_EMAIL_SIMPLE => 'EMAIL',
|
||||
'/\b\d{4}\s?\d{4}\s?\d{4}\s?\d{4}\b/' => 'CREDIT_CARD',
|
||||
'/\+?1?[-.\s]?\(?(\d{3})\)?[-.\s]?(\d{3})[-.\s]?(\d{4})/' => 'PHONE',
|
||||
];
|
||||
@@ -467,23 +467,6 @@ class CriticalBugRegressionTest extends TestCase
|
||||
// Note: Current implementation may not fully sanitize all patterns
|
||||
$this->assertStringContainsString('Rule error:', (string) $errorMessage);
|
||||
|
||||
// Test that at least some sanitization occurs (implementation-dependent)
|
||||
$containsSensitiveInfo = false;
|
||||
$sensitiveTerms = ['password=secret123', 'user=secret_user', 'host=sensitive.db.com'];
|
||||
foreach ($sensitiveTerms as $term) {
|
||||
if (str_contains((string) $errorMessage, $term)) {
|
||||
$containsSensitiveInfo = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
// If sensitive info is still present, log a warning for future improvement
|
||||
if ($containsSensitiveInfo) {
|
||||
error_log(
|
||||
"Warning: Error message sanitization may need improvement: " . $errorMessage
|
||||
);
|
||||
}
|
||||
|
||||
// For now, just ensure the error was logged properly
|
||||
$this->assertNotEmpty($errorMessage);
|
||||
}
|
||||
|
||||
@@ -74,8 +74,8 @@ class SecurityRegressionTest extends TestCase
|
||||
{
|
||||
$redosPatterns = [
|
||||
// Nested quantifiers - classic ReDoS
|
||||
'/^(a+)+$/',
|
||||
'/^(a*)*$/',
|
||||
TestConstants::PATTERN_REDOS_VULNERABLE,
|
||||
TestConstants::PATTERN_REDOS_NESTED_STAR,
|
||||
'/^(a+)*$/',
|
||||
|
||||
// Alternation with overlapping
|
||||
@@ -124,8 +124,8 @@ class SecurityRegressionTest extends TestCase
|
||||
$legitimatePatterns = [
|
||||
// Common GDPR patterns
|
||||
'/\b\d{3}-\d{2}-\d{4}\b/' => 'SSN',
|
||||
'/[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}/' => 'EMAIL',
|
||||
'/\b\d{4}[\s-]?\d{4}[\s-]?\d{4}[\s-]?\d{4}\b/' => 'CREDIT_CARD',
|
||||
TestConstants::PATTERN_EMAIL_SIMPLE => MaskConstants::MASK_EMAIL,
|
||||
TestConstants::PATTERN_CREDIT_CARD => 'CREDIT_CARD',
|
||||
'/\+?1?[-.\s]?\(?(\d{3})\)?[-.\s]?(\d{3})[-.\s]?(\d{4})/' => 'PHONE',
|
||||
'/\b\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}\b/' => 'IP_ADDRESS',
|
||||
|
||||
|
||||
Reference in New Issue
Block a user