Validate hosted domain of user #54
Conversation
| */ | ||
| public function __construct($hostedDomainConfigured, $hostedDomainOfUser) | ||
| { | ||
| parent::__construct("Hosted domain mismatch '$hostedDomainOfUser' !== '$hostedDomainConfigured'"); |
There was a problem hiding this comment.
This could be used as an XSS attack vector if the site has exception handling output. I think it would be better to only show the $hostedDomainConfigured value, as that will be developer supplied.
| * @param string $hostedDomainConfigured | ||
| * @param string|null $hostedDomainOfUser | ||
| */ | ||
| public function __construct($hostedDomainConfigured, $hostedDomainOfUser) |
There was a problem hiding this comment.
Generally I like to see exceptions maintain the same constructor signature, with a static constructor:
class HostedDomainException extends \Exception
{
public static function notMatchingDomain($configuredDomain)
{
return new static("User is not part of domain $configuredDomain");
}
}This preserves the ability to use $code and $previousException when necessary.
src/Provider/Google.php
Outdated
| if ($this->hostedDomain === '*') { | ||
| if (empty($user->getHostedDomain())) { | ||
| throw new HostedDomainException($this->hostedDomain, $user->getHostedDomain()); | ||
| } |
There was a problem hiding this comment.
This implies a significant change to current functionality, requiring all users to set hostedDomain. I think this check should be skipped unless hostedDomain option has been set, at least until the next major version.
There was a problem hiding this comment.
I think I'm missing your point - this only gets checked if hostedDomain is set to '*' or is not empty, so it doesn't require everyone to configure a hostedDomain.
It does affect everyone with hostedDomain configured though I expect those people don't realize how easily that can be bypassed and already assume the library validates it.
|
Rebase on |
If hostedDomain is configured then validate that user is from the hostedDomain. Per https://developers.google.com/identity/protocols/OpenIDConnect#hd-param 'Be sure to validate that the returned ID token has an hd claim value that matches what you expect (e.g. mycolledge.edu). Unlike the request parameter, the ID token claim is contained within a security token from Google, so the value can be trusted.'
- Use static constructor to preserve default constructors - Don't include user's hd/domain incase it contains XSS'able data
|
👍Code has been rebased on |
|
Released in version 2.2.0. |
If hostedDomain is configured then validate that user is from the hostedDomain.
Per https://developers.google.com/identity/protocols/OpenIDConnect#hd-param 'Be
sure to validate that the returned ID token has an hd claim value that matches
what you expect (e.g. mycolledge.edu). Unlike the request parameter, the ID
token claim is contained within a security token from Google, so the value can
be trusted.'