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

Redacting sensitive data with BaseUri #141

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions components/Components/Authority.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
use League\Uri\Uri;
use League\Uri\UriString;
use Psr\Http\Message\UriInterface as Psr7UriInterface;
use SensitiveParameter;
use Stringable;

final class Authority extends Component implements AuthorityInterface
Expand All @@ -34,7 +33,7 @@ final class Authority extends Component implements AuthorityInterface
public function __construct(
Stringable|string|null $host,
Stringable|string|int|null $port = null,
#[SensitiveParameter] Stringable|string|null $userInfo = null
Stringable|string|null $userInfo = null
) {
$this->host = !$host instanceof HostInterface ? Host::new($host) : $host;
$this->port = !$port instanceof PortInterface ? Port::new($port) : $port;
Expand All @@ -47,7 +46,7 @@ public function __construct(
/**
* @throws SyntaxError If the component contains invalid HostInterface part.
*/
public static function new(#[SensitiveParameter] Stringable|string|null $value = null): self
public static function new(Stringable|string|null $value = null): self
{
$components = UriString::parseAuthority(self::filterComponent($value));

Expand All @@ -64,7 +63,7 @@ public static function new(#[SensitiveParameter] Stringable|string|null $value =
/**
* Create a new instance from a URI object.
*/
public static function fromUri(#[SensitiveParameter] Stringable|string $uri): self
public static function fromUri(Stringable|string $uri): self
{
$uri = self::filterUri($uri);

Expand All @@ -87,7 +86,7 @@ public static function fromUri(#[SensitiveParameter] Stringable|string $uri): se
* port? : ?int
* } $components
*/
public static function fromComponents(#[SensitiveParameter] array $components): self
public static function fromComponents(array $components): self
{
$components += ['user' => null, 'pass' => null, 'host' => null, 'port' => null];

Expand All @@ -104,7 +103,7 @@ public function value(): ?string
}

private static function getAuthorityValue(
#[SensitiveParameter] UserInfoInterface $userInfo,
UserInfoInterface $userInfo,
HostInterface $host,
PortInterface $port
): ?string {
Expand Down Expand Up @@ -180,7 +179,7 @@ public function withPort(Stringable|string|int|null $port): AuthorityInterface
};
}

public function withUserInfo(Stringable|string|null $user, #[SensitiveParameter] Stringable|string|null $password = null): AuthorityInterface
Copy link
Contributor

@kelunik kelunik Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd definitely keep the attribute for things that are clearly secrets.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My issue is currently I feel that we are using a half solution. If we keep the attribute then we should IMHO put it everywhere as to completely hide the password part which is what I did now on the master branch, the couter-argument to that is that it makes accessing most of the API in logs a bit more complex only to hide the password. If we keep it only on some method but not on all method then we are not hiding the component as it will be shown completely in a full trace because internal methods do not use the Attribute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand it correctly you are OK with the attribute in the public API where the parameter is clearly defined ... and we should not bother wth internal calls ?

public function withUserInfo(Stringable|string|null $user, Stringable|string|null $password = null): AuthorityInterface
{
$userInfo = new UserInfo($user, $password);

Expand All @@ -200,7 +199,7 @@ public function withUserInfo(Stringable|string|null $user, #[SensitiveParameter]
*
* Create a new instance from a URI object.
*/
public static function createFromUri(#[SensitiveParameter] UriInterface|Psr7UriInterface $uri): self
public static function createFromUri(UriInterface|Psr7UriInterface $uri): self
{
return self::fromUri($uri);
}
Expand All @@ -215,7 +214,7 @@ public static function createFromUri(#[SensitiveParameter] UriInterface|Psr7UriI
*
* Returns a new instance from a string or a stringable object.
*/
public static function createFromString(#[SensitiveParameter] Stringable|string $authority): self
public static function createFromString(Stringable|string $authority): self
{
return self::new($authority);
}
Expand Down Expand Up @@ -255,7 +254,7 @@ public static function createFromNull(): self
* port? : ?int
* } $components
*/
public static function createFromComponents(#[SensitiveParameter] array $components): self
public static function createFromComponents(array $components): self
{
return self::fromComponents($components);
}
Expand Down
18 changes: 8 additions & 10 deletions components/Components/UserInfo.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
use League\Uri\Exceptions\SyntaxError;
use League\Uri\Uri;
use Psr\Http\Message\UriInterface as Psr7UriInterface;
use SensitiveParameter;
use Stringable;

use function explode;
Expand All @@ -36,7 +35,6 @@ final class UserInfo extends Component implements UserInfoInterface
*/
public function __construct(
Stringable|string|null $username,
#[SensitiveParameter]
Stringable|string|null $password = null
) {
$this->username = $this->validateComponent($username);
Expand All @@ -51,7 +49,7 @@ public function __construct(
/**
* Create a new instance from a URI object.
*/
public static function fromUri(#[SensitiveParameter] Stringable|string $uri): self
public static function fromUri(Stringable|string $uri): self
{
$uri = self::filterUri($uri);

Expand All @@ -64,7 +62,7 @@ public static function fromUri(#[SensitiveParameter] Stringable|string $uri): se
/**
* Create a new instance from an Authority object.
*/
public static function fromAuthority(#[SensitiveParameter] Stringable|string|null $authority): self
public static function fromAuthority(Stringable|string|null $authority): self
{
return match (true) {
$authority instanceof AuthorityInterface => self::new($authority->getUserInfo()),
Expand All @@ -80,7 +78,7 @@ public static function fromAuthority(#[SensitiveParameter] Stringable|string|nul
*
* @param array{user? : ?string, pass? : ?string} $components
*/
public static function fromComponents(#[SensitiveParameter] array $components): self
public static function fromComponents(array $components): self
{
$components += ['user' => null, 'pass' => null];

Expand All @@ -93,7 +91,7 @@ public static function fromComponents(#[SensitiveParameter] array $components):
/**
* Creates a new instance from an encoded string.
*/
public static function new(#[SensitiveParameter] Stringable|string|null $value = null): self
public static function new(Stringable|string|null $value = null): self
{
if ($value instanceof UriComponentInterface) {
$value = $value->value();
Expand Down Expand Up @@ -164,7 +162,7 @@ public function withUser(Stringable|string|null $username): self
};
}

public function withPass(#[SensitiveParameter] Stringable|string|null $password): self
public function withPass(Stringable|string|null $password): self
{
$password = $this->validateComponent($password);

Expand All @@ -185,7 +183,7 @@ public function withPass(#[SensitiveParameter] Stringable|string|null $password)
*
* Create a new instance from a URI object.
*/
public static function createFromUri(#[SensitiveParameter] Psr7UriInterface|UriInterface $uri): self
public static function createFromUri(Psr7UriInterface|UriInterface $uri): self
{
return self::fromUri($uri);
}
Expand All @@ -200,7 +198,7 @@ public static function createFromUri(#[SensitiveParameter] Psr7UriInterface|UriI
*
* Create a new instance from an Authority object.
*/
public static function createFromAuthority(#[SensitiveParameter] AuthorityInterface|Stringable|string $authority): self
public static function createFromAuthority(AuthorityInterface|Stringable|string $authority): self
{
return self::fromAuthority($authority);
}
Expand All @@ -215,7 +213,7 @@ public static function createFromAuthority(#[SensitiveParameter] AuthorityInterf
*
* Creates a new instance from an encoded string.
*/
public static function createFromString(#[SensitiveParameter] Stringable|string $userInfo): self
public static function createFromString(Stringable|string $userInfo): self
{
return self::new($userInfo);
}
Expand Down
11 changes: 11 additions & 0 deletions docs/uri/7.0/base-uri.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,17 @@ echo $baseUri; // display 'http://www.example.com'

The instance also implements PHP's `Stringable` and `JsonSerializable` interface.

In addition to all the non-destructive rules from RFC3968, duting instantiation, the class
nyamsprod marked this conversation as resolved.
Show resolved Hide resolved
will convert the host if possible:

- to its IPv4 decimal representation
- to its compressed IPv6 representation (since version **7.5**)

```php
BaseUri::from('https://0:443/')->getUriString(); // returns 'https://0.0.0.0/
BaseUri::from('FtP://[1050:0000:0000:0000:0005:0000:300c:326b]/path')->getUriString(); // returns 'ftp://[1050::5:0:300c:326b]/path
nyamsprod marked this conversation as resolved.
Show resolved Hide resolved
```

## URI resolution

The `BaseUri::resolve` resolves a URI as a browser would for a relative URI while
Expand Down
3 changes: 1 addition & 2 deletions interfaces/Encoder.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
use Closure;
use League\Uri\Contracts\UriComponentInterface;
use League\Uri\Exceptions\SyntaxError;
use SensitiveParameter;
use Stringable;

use function preg_match;
Expand Down Expand Up @@ -58,7 +57,7 @@ public static function encodeUser(Stringable|string|null $component): ?string
*
* Generic delimiters ":" MUST NOT be encoded
*/
public static function encodePassword(#[SensitiveParameter] Stringable|string|null $component): ?string
public static function encodePassword(Stringable|string|null $component): ?string
{
static $pattern = '/[^'.self::REGEXP_PART_UNRESERVED.self::REGEXP_PART_SUBDELIM.':]+|'.self::REGEXP_PART_ENCODED.'/';

Expand Down
43 changes: 43 additions & 0 deletions uri/BaseUri.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@
use League\Uri\Exceptions\MissingFeature;
use League\Uri\Idna\Converter;
use League\Uri\IPv4\Converter as IPv4Converter;
use League\Uri\IPv6\Converter as IPv6Converter;
use Psr\Http\Message\UriFactoryInterface;
use Psr\Http\Message\UriInterface as Psr7UriInterface;
use Stringable;

use function array_map;
use function array_pop;
use function array_reduce;
use function count;
Expand All @@ -48,6 +50,8 @@ class BaseUri implements Stringable, JsonSerializable, UriAccess
/** @var array<string,int> */
final protected const DOT_SEGMENTS = ['.' => 1, '..' => 1];

final protected const COMPONENT_MASK = '*****';

protected readonly Psr7UriInterface|UriInterface|null $origin;
protected readonly ?string $nullValue;

Expand Down Expand Up @@ -269,6 +273,41 @@ public function hasIdn(): bool
return Converter::isIdn($this->uri->getHost());
}

/**
* Redact the password component and any query pairs whose key is submitted.
*
* All redacted values will be replaced by the 5-star mask.
* The return value MAY not be a valid URI
*
* @param string ...$names
*/
public function redactSensitiveParameters(string ...$names): string
{
$components = UriString::parse($this);
if ($components['pass'] !== null) {
$components['pass'] = self::COMPONENT_MASK;
}

$currentQuery = $components['query'];
if ([] === $names || null === $currentQuery || !str_contains($currentQuery, '=')) {
return UriString::build($components);
}

$names = array_map(Encoder::decodeAll(...), $names);
$pairs = [];
foreach (explode('&', $currentQuery) as $part) {
nyamsprod marked this conversation as resolved.
Show resolved Hide resolved
[$key, ] = explode('=', $part, 2) + [1 => null];
$pairs[] = match (in_array(Encoder::decodeAll($key), $names, true)) {
true => $key.'='.self::COMPONENT_MASK,
false => $part,
};
}

$components['query'] = implode('&', $pairs);

return UriString::build($components);
}

/**
* Resolves a URI against a base URI using RFC3986 rules.
*
Expand Down Expand Up @@ -556,6 +595,10 @@ final protected static function formatHost(Psr7UriInterface|UriInterface $uri):
$converted = null;
}

if (false === filter_var($converted, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) {
$converted = IPv6Converter::compress($host);
}

return match (true) {
null !== $converted => $uri->withHost($converted),
'' === $host,
Expand Down
80 changes: 80 additions & 0 deletions uri/BaseUriTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,10 @@ public static function getOriginProvider(): array
'uri' => Uri::new('https://0:443/'),
'expectedOrigin' => 'https://0.0.0.0',
],
'compressed ipv6' => [
'uri' => 'https://[1050:0000:0000:0000:0005:0000:300c:326b]:443/',
'expectedOrigin' => 'https://[1050::5:0:300c:326b]',
],
];
}

Expand Down Expand Up @@ -624,4 +628,80 @@ public static function opaqueUriProvider(): iterable
'uri' => 'data:',
];
}

#[Test]
#[DataProvider('redactUriUserInfoProvider')]
public function it_can_redact_user_info_password_component(string $uri, string $expected):void
{
self::assertSame($expected, BaseUri::from($uri)->redactSensitiveParameters());
}

public static function redactUriUserInfoProvider(): iterable
{
yield 'empty URI' => [
'uri' => '',
'expected' => '',
];

yield 'URI with user only' => [
'uri' => 'https://user@example.com',
'expected' => 'https://user@example.com',
];

yield 'URI with complete user info only' => [
'uri' => 'https://user:pass@example.com',
'expected' => 'https://user:*****@example.com',
];

yield 'URI without user info only' => [
'uri' => 'https://example.com',
'expected' => 'https://example.com',
];
}

#[Test]
#[DataProvider('redactUrQueryPairsProvider')]
public function it_can_redact_query_parameters(string $uri, array $pairs, string $expected):void
{
self::assertSame($expected, BaseUri::from($uri)->redactSensitiveParameters(...$pairs));
}

public static function redactUrQueryPairsProvider(): iterable
{
yield 'empty URI' => [
'uri' => '',
'pairs' => ['toto', 'foobar'],
'expected' => '',
];

yield 'URI with complete user info only' => [
'uri' => 'https://user:pass@example.com',
'pairs' => ['toto', 'foobar'],
'expected' => 'https://user:*****@example.com',
];

yield 'URI with query pair not matching' => [
'uri' => 'https://example.com?key=value',
'pairs' => ['toto', 'foobar'],
'expected' => 'https://example.com?key=value',
];

yield 'URI with query with one query pair matching' => [
'uri' => 'https://example.com?key=value',
'pairs' => ['key', 'foobar'],
'expected' => 'https://example.com?key=*****',
];

yield 'URI with query containing array like paraneters (1)' => [
'uri' => 'https://example.com?key[]=value&key[]=value&key=value',
'pairs' => ['key', 'foobar'],
'expected' => 'https://example.com?key%5B%5D=value&key%5B%5D=value&key=*****',
];

yield 'URI with query containing array like paraneters (2)' => [
'uri' => 'https://example.com?key[]=value&key[]=value&key=value',
'pairs' => ['key[]', 'foobar'],
'expected' => 'https://example.com?key%5B%5D=*****&key%5B%5D=*****&key=value',
];
}
}
2 changes: 2 additions & 0 deletions uri/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ All Notable changes to `League\Uri` will be documented in this file
- `Uri::getUsername` returns the encoded user component of the URI.
- `Uri::getPassword` returns the encoded password component of the URI.
- `BaseUri::isOpaque` tells whether a URI is opaque.
- `BaseUri::redactSensitiveParameters` to remove sensitive parameters from the URI.

### Fixed

- Adding `SensitiveParameter` attribute in the `Uri` and the `BaseUri` class.
- Improve PSR-7 `Http` class implementation.
- `BaseUri::from` will compress the IPv6 host to its compressed form if possible.

### Deprecated

Expand Down
Loading
Loading