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

Command icon 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:

Comments

prudloff created an issue. See original summary.

adhershmnair made their first commit to this issue’s fork.

adhershmnair’s picture

Added #[\SensitiveParameter] to $connection_options in Connection::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.

adhershmnair’s picture

Version: main » 11.x-dev
Status: Active » Needs review
quietone’s picture

Version: 11.x-dev » main

Issues are fixed on main first.

smustgrave’s picture

Status: Needs review » Needs work

Left comments on the MR.

prudloff’s picture

Status: Needs work » Needs review
longwave’s picture

Status: Needs review » Reviewed & tested by the community

Adding 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.

  • godotislate committed ede017cc on 11.x
    task: #3582506 Connection::open() should have SensitiveParameter
    
    By:...

  • godotislate committed ac8820e1 on main
    task: #3582506 Connection::open() should have SensitiveParameter
    
    By:...
godotislate’s picture

Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed ac8820e to main and ede017c to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.