Problem/Motivation
It seems this was discussed in #3296293: Apply SensitiveParameter attribute but never done.
If open() triggers an exception or an error, the credentials can be visible in the stack trace.
It seems by default Drupal does not show the content of arrays in stack traces:
Drupal\mysql\Driver\Database\mysql\Connection::open(Array) (Line: 50)
test_preprocess_page(Array, 'page', Array) (Line: 354)
But it can happen when using other tools to display the stack trace (for example the Symfony Error Handler):
ErrorException:
Warning: Array to string conversion
at core/modules/mysql/src/Driver/Database/mysql/Connection.php:110
at Drupal\mysql\Driver\Database\mysql\Connection::open(array('password' => 'foo', 'host' => array()))
(modules/custom/test/test.module:50)
Steps to reproduce
ini_set('zend.exception_ignore_args', FALSE);
$options = ['password' => 'foo', 'host' => []];
Connection::open($options);
Proposed resolution
Add #[SensitiveParameter] to the $connection_options argument.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3582506
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3582506-connectionopen-should-have
changes, plain diff MR !15309
Comments
Comment #4
adhershmnair commentedAdded
#[\SensitiveParameter]to$connection_optionsinConnection::open()across the base class and all database drivers (mysql, mysqli, pgsql, sqlite) plus the test stub.This was missed during the original sweep in #3296293. All existing database unit tests pass.
Comment #5
adhershmnair commentedComment #6
quietone commentedIssues are fixed on main first.
Comment #7
smustgrave commentedLeft comments on the MR.
Comment #8
prudloff commentedComment #9
longwaveAdding it to the abstract method signature makes sense to me, it's possible that static analysis can warn in the future if an attribute is applied to an abstract class but missing from the concrete implementation.
Comment #12
godotislateCommitted and pushed ac8820e to main and ede017c to 11.x. Thanks!