-
Notifications
You must be signed in to change notification settings - Fork 15
PLT-1764: Sanitize sensitive fields in FormatterTrait object normaliz… #21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,16 @@ | |
|
|
||
| trait FormatterTrait | ||
| { | ||
| /** @var string[] */ | ||
| private static $sensitiveKeys = [ | ||
| 'password', | ||
| 'secret', | ||
| 'apikey', | ||
| 'apisecret', | ||
| 'apisecretkey', | ||
| 'secretkey', | ||
| 'credentials', | ||
| ]; | ||
| /** | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. blank line sparator? |
||
| * Normalizes given data with pre-processing for Doctrine entities and collections. | ||
| * | ||
|
|
@@ -96,6 +106,12 @@ private function normalizeObject($data) | |
| continue; | ||
| } | ||
|
|
||
| $normalizedKey = preg_replace('/[^a-z]/', '', strtolower($fixedKey)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we make sanitization enabled by default, but allow disabling it via a configuration option? There may be cases (e.g., debugging in a controlled environment) where logging the actual values is preferred over redacting them. |
||
| if (in_array($normalizedKey, self::$sensitiveKeys, true)) { | ||
| $result[$fixedKey] = '***'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the value at a sensitive key is an object or array, the whole thing gets replaced with ***, not sure if this is expected, we might lose some useful non-sensitive data for debugging |
||
| continue; | ||
| } | ||
|
|
||
| $result[$fixedKey] = $value; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Paysera\LoggingExtraBundle\Tests\Unit; | ||
|
|
||
| use PHPUnit\Framework\TestCase; | ||
| use Paysera\LoggingExtraBundle\Service\Formatter\FormatterTrait; | ||
|
|
||
| class FormatterTraitTest extends TestCase | ||
| { | ||
| /** | ||
| * @dataProvider sensitiveKeysProvider | ||
| */ | ||
| public function testSensitiveKeysAreRedacted(string $propertyName) | ||
| { | ||
| $object = new \stdClass(); | ||
| $object->$propertyName = 'sensitive_value'; | ||
| $object->name = 'visible'; | ||
|
|
||
| $result = $this->normalizeObject($object); | ||
|
|
||
| $this->assertSame('***', $result[$propertyName]); | ||
| $this->assertSame('visible', $result['name']); | ||
| } | ||
|
|
||
| public function sensitiveKeysProvider(): array | ||
| { | ||
| return [ | ||
| 'password' => ['password'], | ||
| 'secret' => ['secret'], | ||
| 'apiKey' => ['apiKey'], | ||
| 'apiSecret' => ['apiSecret'], | ||
| 'secretKey' => ['secretKey'], | ||
| 'credentials' => ['credentials'], | ||
| ]; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing: 'apiSecretKey' => ['apiSecretKey'], |
||
| } | ||
|
|
||
| /** | ||
| * @dataProvider caseInsensitiveProvider | ||
| */ | ||
| public function testSensitiveKeysAreCaseInsensitive(string $propertyName) | ||
| { | ||
| $object = new \stdClass(); | ||
| $object->$propertyName = 'sensitive_value'; | ||
|
|
||
| $result = $this->normalizeObject($object); | ||
|
|
||
| $this->assertSame('***', $result[$propertyName]); | ||
| } | ||
|
|
||
| public function caseInsensitiveProvider(): array | ||
| { | ||
| return [ | ||
| 'Password' => ['Password'], | ||
| 'PASSWORD' => ['PASSWORD'], | ||
| 'pAsSwOrD' => ['pAsSwOrD'], | ||
| 'SECRET' => ['SECRET'], | ||
| 'ApiKey' => ['ApiKey'], | ||
| 'APIKEY' => ['APIKEY'], | ||
| 'Credentials' => ['Credentials'], | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * @dataProvider separatorVariantsProvider | ||
| */ | ||
| public function testSensitiveKeysWithSeparators(string $propertyName) | ||
| { | ||
| $object = new \stdClass(); | ||
| $object->$propertyName = 'sensitive_value'; | ||
|
|
||
| $result = $this->normalizeObject($object); | ||
|
|
||
| $this->assertSame('***', $result[$propertyName]); | ||
| } | ||
|
|
||
| public function separatorVariantsProvider(): array | ||
| { | ||
| return [ | ||
| 'api_key' => ['api_key'], | ||
| 'api-key' => ['api-key'], | ||
| 'api.key' => ['api.key'], | ||
| 'api_secret' => ['api_secret'], | ||
| 'api-secret' => ['api-secret'], | ||
| 'secret_key' => ['secret_key'], | ||
| 'secret-key' => ['secret-key'], | ||
| 'api_secret_key' => ['api_secret_key'], | ||
| 'api-secret-key' => ['api-secret-key'], | ||
| 'API_KEY' => ['API_KEY'], | ||
| 'API_SECRET' => ['API_SECRET'], | ||
| 'SECRET_KEY' => ['SECRET_KEY'], | ||
| ]; | ||
| } | ||
|
|
||
| public function testNonSensitivePropertiesPassThrough() | ||
| { | ||
| $object = new \stdClass(); | ||
| $object->name = 'John'; | ||
| $object->email = 'john@example.com'; | ||
| $object->age = 30; | ||
| $object->active = true; | ||
|
|
||
| $result = $this->normalizeObject($object); | ||
|
|
||
| $this->assertSame('John', $result['name']); | ||
| $this->assertSame('john@example.com', $result['email']); | ||
| $this->assertSame(30, $result['age']); | ||
| $this->assertTrue($result['active']); | ||
| } | ||
|
|
||
| public function testDoubleUnderscorePrefixedPropertiesAreExcluded() | ||
| { | ||
| $object = new \stdClass(); | ||
| $object->__internal = 'hidden'; | ||
| $object->name = 'visible'; | ||
|
|
||
| $result = $this->normalizeObject($object); | ||
|
|
||
| $this->assertArrayNotHasKey('__internal', $result); | ||
| $this->assertSame('visible', $result['name']); | ||
| } | ||
|
|
||
| public function testMixedSensitiveAndNonSensitiveProperties() | ||
| { | ||
| $object = new \stdClass(); | ||
| $object->username = 'admin'; | ||
| $object->password = 'super_secret'; | ||
| $object->email = 'admin@example.com'; | ||
| $object->apiKey = 'key-123'; | ||
| $object->role = 'admin'; | ||
| $object->credentials = ['token' => 'abc']; | ||
|
|
||
| $result = $this->normalizeObject($object); | ||
|
|
||
| $this->assertSame('admin', $result['username']); | ||
| $this->assertSame('***', $result['password']); | ||
| $this->assertSame('admin@example.com', $result['email']); | ||
| $this->assertSame('***', $result['apiKey']); | ||
| $this->assertSame('admin', $result['role']); | ||
| $this->assertSame('***', $result['credentials']); | ||
| } | ||
|
|
||
| private function normalizeObject(object $data): array | ||
| { | ||
| $formatter = new class { | ||
| use FormatterTrait; | ||
|
|
||
| public function callNormalizeObject(object $data): array | ||
| { | ||
| return $this->normalizeObject($data); | ||
| } | ||
| }; | ||
|
|
||
| return $formatter->callNormalizeObject($data); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static array $sensitiveKeys = [...]