Problem/Motivation

The $password argument is sensitive and should not be leaked in stack traces:

RedisException:
php_network_getaddresses: getaddrinfo for foo failed: Temporary failure in name resolution

  at modules/contrib/redis/src/Client/PhpRedis.php:32
  at Redis->connect('foo', 42)
     (modules/contrib/redis/src/Client/PhpRedis.php:32)
  at Drupal\redis\Client\PhpRedis->getClient('foo', 42, 'bar')
     (modules/custom/test/test.module:70)

Steps to reproduce

  ini_set('zend.exception_ignore_args', FALSE);
  (new PhpRedis())->getClient('foo', 42, 'bar');

Proposed resolution

Add #[SensitiveParameter] to the $password argument.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork redis-3585855

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:

About issue forks

Comments

prudloff created an issue. See original summary.

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

tribekk opened merge request !95

tribekk’s picture

tribekk commented
Status: Active » Needs review

Opened MR !95.

The 2.x branch passes Redis connection details through the `$settings` array, including the password. This marks the factory settings parameter as `#[\SensitiveParameter]` on the interface and the PhpRedis, Predis, and Relay implementations.

Checked locally:
- composer validate --no-check-publish
- composer install --no-interaction
- php -l over src
- Reflection check that all getClient() settings parameters have SensitiveParameter

tribekk’s picture

tribekk commented

Credit would be appreciated if this is useful.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

a MR for 8.x-1.x also makes sense I think.

@prudloff I guess the attribute also makes sense on an array parameter that contains the password?

prudloff’s picture

prudloff commented

@berdir Yes, ideally any parameter that contains a password should have this.

tribekk opened merge request !97

  • berdir committed e0c6a95f on 2.x authored by tribekk 
    fix: #3585855 ClientInterface::getClient() implementations should use...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Fixed

Merged 2.x and 8.x-1.x. this is going to break several merge requests/patches on 8.x-1.x, oh well.

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.

  • berdir committed 66656990 on 8.x-1.x authored by tribekk 
    fix: #3585855 ClientInterface::getClient() implementations should use...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.