Problem/Motivation

The $password in InstallCommand::install() is sensitive and should not appear in stack traces.
The risk of leaking a stack trace is probably more limited for CLI scripts but it is still worth hardening.

Steps to reproduce

Proposed resolution

Add #[SensitiveParameter] to the $password argument.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3582510

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.

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

adhershmnair opened merge request !15311

adhershmnair’s picture

adhershmnair commented
Version: main » 11.x-dev
Status: Active » Needs review

Added #[\SensitiveParameter] to the $password parameter in InstallCommand::install(). This prevents the password from appearing in stack traces if the method throws an exception. Follow-up to #3296293.

sourav_paul’s picture

sourav_paul
Location Kolkata
commented
Status: Needs review » Needs work

Hey @adhershmnair, good catch 🙌
Two things before this goes for review:

CI pipeline first — please make sure MR !15311 is fully green (no PHPCS/PHPStan/test failures) before tagging for review.
Target main — core changes should land in main first (active dev branch), then get backported to 11.x / 10.4.x.

The #[\SensitiveParameter] addition itself looks right.

Thanks.

prudloff’s picture

prudloff commented

The password is passed to install_drupal() as part of $parameters so I wonder if this argument should also be marked sensitive (not sure, it can contain a lot of different things).

adhershmnair’s picture

adhershmnair commented
Status: Needs work » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Seems legit.

godotislate’s picture

godotislate
he/him
commented

Re #6: it looks like the password passed to install_drupal from InstallCommand::install() is just randomly generated? I don't see other uses of install_drupal where a password is passed in.

Edited to add: Never mind, I was looking at the wrong branch, and I guess it doesn't make it any less sensitive anyway.

quietone’s picture

quietone commented
Version: 11.x-dev » main

Issues are fixed on main first.

godotislate closed merge request !15311

  • godotislate committed 7362bf47 on main 
    task: #3582510 Add SensitiveParameter attribute to InstallCommand::...

  • godotislate committed ac797858 on 11.x 
    task: #3582510 Add SensitiveParameter attribute to InstallCommand::...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 7362bf4 to main and ac79785 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.

Status: Fixed » Closed (fixed)

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