Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark parameters containing user credentials as sensitive #878

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jul 8, 2024

PHP 8.2 introduced the SensitiveParameter attribute.

The effect of the attribute is that the value of the parameter is no longer directly shown in stack traces; instead, starting with PHP 8.2, the parameter will be presented as a SensitiveParameterValue object.

As the attribute only applies to parameters, it (unfortunately) has no effect on serialization of the object. See: https://3v4l.org/StoQO Might be an idea to start a discussion about an SensitiveProperty attribute on the PHP Internals mailing list, but that's outside the scope of this PR.

For now, this PR marks the $args parameter for the Auth\Basic class constructor and the Proxy\Http constructor as sensitive as both of these are supposed to contain user credentials (user name, password) for accessing a protected URL.

Includes updating the example code for custom authentication to also use the attribute.

Open question

The $options array passed to a large range of Requests methods can also contain credentials. Should this parameter also be marked as sensitive in all appropriate places ?

Refs:

@jrfnl jrfnl added this to the 2.1.0 milestone Jul 8, 2024
@jrfnl jrfnl requested a review from schlessera July 8, 2024 04:03
@jrfnl jrfnl force-pushed the feature/mark-sensitive-parameters branch from ee3e53a to a3761c0 Compare July 8, 2024 09:32
PHP 8.2 introduced the `SensitiveParameter` attribute.

The effect of the attribute is that the value of the parameter is no longer directly shown in stack traces; instead, starting with PHP 8.2, the parameter will be presented as a `SensitiveParameterValue` object.

As the attribute only applies to parameters, it (unfortunately) has no effect on serialization of the object. See: https://3v4l.org/StoQO
Might be an idea to start a discussion about an `SensitiveProperty` attribute on the PHP Internals mailing list, but that's outside the scope of this PR.

For now, this PR marks the `$args` parameter for the `Auth\Basic` class constructor and the `Proxy\Http` constructor as sensitive as both of these are supposed to contain user credentials (user name, password) for accessing a protected URL.

Includes updating the example code for custom authentication to also use the attribute.

**Open question**: the `$options` array passed to a large range of Requests methods can [also contain credentials](https://github.com/WordPress/Requests/blob/ebb9f65855c860bc33005b3d8bccf6444e598fba/src/Requests.php#L395-L399). Should this parameter also be marked as sensitive in all appropriate places ?

Refs:
* https://www.php.net/manual/en/class.sensitiveparameter.php
* https://wiki.php.net/rfc/redact_parameters_in_back_traces
@jrfnl jrfnl force-pushed the feature/mark-sensitive-parameters branch from a3761c0 to ef400a8 Compare July 8, 2024 10:18
@jrfnl jrfnl closed this Jul 8, 2024
@jrfnl jrfnl deleted the feature/mark-sensitive-parameters branch July 8, 2024 12:37
@jrfnl jrfnl restored the feature/mark-sensitive-parameters branch July 8, 2024 12:38
@jrfnl jrfnl reopened this Jul 8, 2024
@schlessera
Copy link
Member

schlessera commented Jul 29, 2024

The $options array passed to a large range of Requests methods can also contain credentials. Should this parameter also be marked as sensitive in all appropriate places ?

Wrapping the entire $options array as a sensitive parameter would hinder a lot of the debugging efforts, so I think that might be too generic to block.

How about creating a polyfill for SensitiveParameterValue on PHP < 8.2 instead and wrapping only $options>auth and $options>proxy in that class?

See https://www.php.net/manual/en/class.sensitiveparametervalue.php

@jrfnl jrfnl removed this from the 2.1.0 milestone Jul 29, 2024
@jrfnl
Copy link
Member Author

jrfnl commented Jul 29, 2024

Let's think this over some more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants