Skip to content

Commit 693bfac

Browse files
oshmyheliukshiftedreality
oshmyheliuk
authored and
shiftedreality
committed
MAGECLOUD-3525: Password masking in logs doesn't work in case of exception (#452)
1 parent 88bf1a8 commit 693bfac

File tree

6 files changed

+178
-76
lines changed

6 files changed

+178
-76
lines changed

‎src/App/Logger/Processor/SanitizeProcessor.php

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,20 +5,25 @@
55
*/
66
namespace Magento\MagentoCloud\App\Logger\Processor;
77

8+
use Magento\MagentoCloud\App\Logger\Sanitizer;
9+
810
/**
9-
* Uses for sanitize sensitive data.
11+
* Logger processor for sanitizing sensitive data.
1012
*/
1113
class SanitizeProcessor
1214
{
1315
/**
14-
* Array of replacements that will be applied to log messages.
15-
*
16-
* @var array
16+
* @var Sanitizer
1717
*/
18-
private $replacements = [
19-
'/-password=\'.*?\'(\s|$)/i' => '-password=\'******\'$1',
20-
'/mysqldump (.* )-p\'[^\']+\'/i' => 'mysqldump $1-p\'******\'',
21-
];
18+
private $sanitizer;
19+
20+
/**
21+
* @param Sanitizer $sanitizer
22+
*/
23+
public function __construct(Sanitizer $sanitizer)
24+
{
25+
$this->sanitizer = $sanitizer;
26+
}
2227

2328
/**
2429
* Finds and replace sensitive data in record message.
@@ -28,9 +33,7 @@ class SanitizeProcessor
2833
*/
2934
public function __invoke(array $record)
3035
{
31-
foreach ($this->replacements as $pattern => $replacement) {
32-
$record['message'] = preg_replace($pattern, $replacement, $record['message']);
33-
}
36+
$record['message'] = $this->sanitizer->sanitize($record['message']);
3437

3538
return $record;
3639
}

‎src/App/Logger/Sanitizer.php

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
declare(strict_types=1);
7+
8+
namespace Magento\MagentoCloud\App\Logger;
9+
10+
/**
11+
* Uses for sanitize sensitive data.
12+
*/
13+
class Sanitizer
14+
{
15+
/**
16+
* Array of replacements that will be applied to log messages.
17+
*
18+
* @var array
19+
*/
20+
private $replacements = [
21+
'/-password=\'.*?\'(\s|$)/i' => '-password=\'******\'$1',
22+
'/mysqldump (.* )-p\'[^\']+\'/i' => 'mysqldump $1-p\'******\'',
23+
];
24+
25+
/**
26+
* Finds and replace sensitive data in record message.
27+
*
28+
* @param string $message
29+
* @return string
30+
*/
31+
public function sanitize(string $message): string
32+
{
33+
foreach ($this->replacements as $pattern => $replacement) {
34+
$message = preg_replace($pattern, $replacement, $message);
35+
}
36+
37+
return $message;
38+
}
39+
}

‎src/Shell/Shell.php

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
namespace Magento\MagentoCloud\Shell;
99

10+
use Magento\MagentoCloud\App\Logger\Sanitizer;
1011
use Magento\MagentoCloud\Filesystem\SystemList;
1112
use Psr\Log\LoggerInterface;
1213
use Monolog\Logger;
@@ -26,14 +27,21 @@ class Shell implements ShellInterface
2627
*/
2728
private $systemList;
2829

30+
/**
31+
* @var Sanitizer
32+
*/
33+
private $sanitizer;
34+
2935
/**
3036
* @param LoggerInterface $logger
3137
* @param SystemList $systemList
38+
* @param Sanitizer $sanitizer
3239
*/
33-
public function __construct(LoggerInterface $logger, SystemList $systemList)
40+
public function __construct(LoggerInterface $logger, SystemList $systemList, Sanitizer $sanitizer)
3441
{
3542
$this->logger = $logger;
3643
$this->systemList = $systemList;
44+
$this->sanitizer = $sanitizer;
3745
}
3846

3947
/**
@@ -99,7 +107,14 @@ function ($message, $line) {
99107
}
100108

101109
if ($status !== 0) {
102-
throw new ShellException("Command $command returned code $status", $status);
110+
throw new ShellException(
111+
sprintf(
112+
"Command %s returned code %d",
113+
$this->sanitizer->sanitize($command),
114+
$status
115+
),
116+
$status
117+
);
103118
}
104119

105120
return $output;

‎src/Test/Unit/App/Logger/Processor/SanitizeProcessorTest.php

Lines changed: 11 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -6,74 +6,25 @@
66
namespace Magento\MagentoCloud\Test\Unit\App\Logger\Processor;
77

88
use Magento\MagentoCloud\App\Logger\Processor\SanitizeProcessor;
9+
use Magento\MagentoCloud\App\Logger\Sanitizer;
10+
use PHPUnit\Framework\MockObject\MockObject;
911
use PHPUnit\Framework\TestCase;
1012

1113
/**
1214
* @inheritdoc
1315
*/
1416
class SanitizeProcessorTest extends TestCase
1517
{
16-
/**
17-
* @param array $record
18-
* @param array $expected
19-
* @dataProvider invokeDataProvider
20-
*/
21-
public function testInvoke(array $record, array $expected)
18+
public function testInvoke()
2219
{
23-
$this->assertEquals($expected, (new SanitizeProcessor)($record));
24-
}
25-
26-
/**
27-
* @return array
28-
*/
29-
public function invokeDataProvider()
30-
{
31-
return [
32-
[
33-
['message' => 'some message'],
34-
['message' => 'some message']
35-
],
36-
[
37-
['message' => 'some message with admin password --admin-password=\'Ks81bUSl13Osd\''],
38-
['message' => 'some message with admin password --admin-password=\'******\''],
39-
],
40-
[
41-
['message' => 'some message with db password --db-password=\'Ks81bUSl13Osd\''],
42-
['message' => 'some message with db password --db-password=\'******\''],
43-
],
44-
[
45-
['message' => 'some message with db password --db-password=\'--db-password\''],
46-
['message' => 'some message with db password --db-password=\'******\''],
47-
],
48-
[
49-
['message' => 'some text --admin-password=\'Ks81bUSl13Osd\' some text'],
50-
['message' => 'some text --admin-password=\'******\' some text'],
51-
],
52-
[
53-
['message' => 'some text --admin-password=\'Ks81bUSl13Osd\' --db-password=\'Ks81bUSl13Osd\' some text'],
54-
['message' => 'some text --admin-password=\'******\' --db-password=\'******\' some text'],
55-
],
56-
[
57-
['message' => 'some text --admin-password=\'' . escapeshellarg("Ks81b'USl'13Osd") . '\''],
58-
['message' => 'some text --admin-password=\'******\''],
59-
],
60-
[
61-
['message' => 'Command: bash -c "set -o pipefail; timeout 3600 mysqldump -h \'127.0.0.1\' -P \'3304\''
62-
. ' -u \'abcdefghijklm_stg\' -p\'OmgSuperSecretPasswordDoNotLeak\' \'abcdefghijklm_stg\''
63-
. ' --single-transaction --no-autocommit --quick | gzip > /tmp/dump-1525977618.sql.gz'],
64-
['message' => 'Command: bash -c "set -o pipefail; timeout 3600 mysqldump -h \'127.0.0.1\' -P \'3304\''
65-
. ' -u \'abcdefghijklm_stg\' -p\'******\' \'abcdefghijklm_stg\''
66-
. ' --single-transaction --no-autocommit --quick | gzip > /tmp/dump-1525977618.sql.gz'],
67-
],
68-
[
69-
['message' => 'Command: bash -c "set -o pipefail; timeout 3600 mysqldump -h \'127.0.0.1\' -P \'3304\''
70-
. ' -u \'abcdefghijklm_stg\' \'abcdefghijklm_stg\' --single-transaction --no-autocommit'
71-
. ' --quick | gzip > /tmp/dump-1525977618.sql.gz'],
72-
['message' => 'Command: bash -c "set -o pipefail; timeout 3600 mysqldump -h \'127.0.0.1\' -P \'3304\''
73-
. ' -u \'abcdefghijklm_stg\' \'abcdefghijklm_stg\' --single-transaction --no-autocommit'
74-
. ' --quick | gzip > /tmp/dump-1525977618.sql.gz'],
75-
],
20+
/** @var Sanitizer|MockObject $sanitizerMock */
21+
$sanitizerMock = $this->createMock(Sanitizer::class);
22+
$sanitizerMock->expects($this->once())
23+
->method('sanitize')
24+
->with('some message')
25+
->willReturn('sanitized message');
7626

77-
];
27+
$sanitizeProcessor = new SanitizeProcessor($sanitizerMock);
28+
$this->assertEquals(['message' => 'sanitized message'], $sanitizeProcessor(['message' => 'some message']));
7829
}
7930
}

‎src/Test/Unit/App/Logger/SanitizerTest.php

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
declare(strict_types=1);
7+
8+
namespace Magento\MagentoCloud\Test\Unit\App\Logger;
9+
10+
use Magento\MagentoCloud\App\Logger\Sanitizer;
11+
use PHPUnit\Framework\TestCase;
12+
13+
/**
14+
* @inheritdoc
15+
*/
16+
class SanitizerTest extends TestCase
17+
{
18+
/**
19+
* @param string $message
20+
* @param string $expectedMesssage
21+
* @dataProvider invokeDataProvider
22+
*/
23+
public function testInvoke(string $message, string $expectedMesssage)
24+
{
25+
$this->assertEquals($expectedMesssage, (new Sanitizer())->sanitize($message));
26+
}
27+
28+
/**
29+
* @return array
30+
*/
31+
public function invokeDataProvider()
32+
{
33+
return [
34+
[
35+
'some message',
36+
'some message',
37+
],
38+
[
39+
'some message with admin password --admin-password=\'Ks81bUSl13Osd\'',
40+
'some message with admin password --admin-password=\'******\'',
41+
],
42+
[
43+
'some message with db password --db-password=\'Ks81bUSl13Osd\'',
44+
'some message with db password --db-password=\'******\'',
45+
],
46+
[
47+
'some message with db password --db-password=\'--db-password\'',
48+
'some message with db password --db-password=\'******\'',
49+
],
50+
[
51+
'some text --admin-password=\'Ks81bUSl13Osd\' some text',
52+
'some text --admin-password=\'******\' some text',
53+
],
54+
[
55+
'some text --admin-password=\'Ks81bUSl13Osd\' --db-password=\'Ks81bUSl13Osd\' some text',
56+
'some text --admin-password=\'******\' --db-password=\'******\' some text',
57+
],
58+
[
59+
'some text --admin-password=\'' . escapeshellarg("Ks81b'USl'13Osd") . '\'',
60+
'some text --admin-password=\'******\'',
61+
],
62+
[
63+
'Command: bash -c "set -o pipefail; timeout 3600 mysqldump -h \'127.0.0.1\' -P \'3304\''
64+
. ' -u \'abcdefghijklm_stg\' -p\'OmgSuperSecretPasswordDoNotLeak\' \'abcdefghijklm_stg\''
65+
. ' --single-transaction --no-autocommit --quick | gzip > /tmp/dump-1525977618.sql.gz',
66+
'Command: bash -c "set -o pipefail; timeout 3600 mysqldump -h \'127.0.0.1\' -P \'3304\''
67+
. ' -u \'abcdefghijklm_stg\' -p\'******\' \'abcdefghijklm_stg\''
68+
. ' --single-transaction --no-autocommit --quick | gzip > /tmp/dump-1525977618.sql.gz',
69+
],
70+
[
71+
'Command: bash -c "set -o pipefail; timeout 3600 mysqldump -h \'127.0.0.1\' -P \'3304\''
72+
. ' -u \'abcdefghijklm_stg\' \'abcdefghijklm_stg\' --single-transaction --no-autocommit'
73+
. ' --quick | gzip > /tmp/dump-1525977618.sql.gz',
74+
'Command: bash -c "set -o pipefail; timeout 3600 mysqldump -h \'127.0.0.1\' -P \'3304\''
75+
. ' -u \'abcdefghijklm_stg\' \'abcdefghijklm_stg\' --single-transaction --no-autocommit'
76+
. ' --quick | gzip > /tmp/dump-1525977618.sql.gz',
77+
],
78+
79+
];
80+
}
81+
}

‎src/Test/Unit/Shell/ShellTest.php

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
*/
66
namespace Magento\MagentoCloud\Test\Unit\Shell;
77

8+
use Magento\MagentoCloud\App\Logger\Sanitizer;
89
use Magento\MagentoCloud\Filesystem\SystemList;
910
use PHPUnit\Framework\MockObject\Matcher\InvokedCount;
1011
use PHPUnit\Framework\MockObject\MockObject;
@@ -35,15 +36,21 @@ class ShellTest extends TestCase
3536
*/
3637
private $systemListMock;
3738

39+
/**
40+
* @var Sanitizer|MockObject
41+
*/
42+
private $sanitizerMock;
43+
3844
/**
3945
* @inheritdoc
4046
*/
4147
protected function setUp()
4248
{
4349
$this->loggerMock = $this->getMockForAbstractClass(LoggerInterface::class);
4450
$this->systemListMock = $this->createMock(SystemList::class);
51+
$this->sanitizerMock = $this->createMock(Sanitizer::class);
4552

46-
$this->shell = new Shell($this->loggerMock, $this->systemListMock);
53+
$this->shell = new Shell($this->loggerMock, $this->systemListMock, $this->sanitizerMock);
4754
}
4855

4956
/**
@@ -71,6 +78,8 @@ public function testExecute($execOutput)
7178
$this->loggerMock->expects($this->once())
7279
->method('info')
7380
->with($command);
81+
$this->sanitizerMock->expects($this->never())
82+
->method('sanitize');
7483
if ($execOutput) {
7584
$this->loggerMock->expects($this->once())
7685
->method('log')
@@ -95,13 +104,13 @@ public function executeDataProvider(): array
95104
* @param InvokedCount $logExpects
96105
* @param array $execOutput
97106
* @expectedException \RuntimeException
98-
* @expectedExceptionMessage Command ls -al returned code 123
107+
* @expectedExceptionMessage Command ls -al --password="***" returned code 123
99108
* @expectedExceptionCode 123
100109
* @dataProvider executeExceptionDataProvider
101110
*/
102111
public function testExecuteException($logExpects, array $execOutput)
103112
{
104-
$command = 'ls -al';
113+
$command = 'ls -al --password="123"';
105114
$magentoRoot = '/magento';
106115
$execCommand = 'cd ' . $magentoRoot . ' && ' . $command . ' 2>&1';
107116

@@ -116,6 +125,10 @@ public function testExecuteException($logExpects, array $execOutput)
116125
$this->systemListMock->expects($this->once())
117126
->method('getMagentoRoot')
118127
->willReturn($magentoRoot);
128+
$this->sanitizerMock->expects($this->once())
129+
->method('sanitize')
130+
->with('ls -al --password="123"')
131+
->willReturn('ls -al --password="***"');
119132

120133
$this->loggerMock->expects($this->once())
121134
->method('info')

0 commit comments

Comments
 (0)