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

_checkOrigin does not allow for complex localhost domains #64

Open
tlokot opened this issue Sep 7, 2022 · 4 comments
Open

_checkOrigin does not allow for complex localhost domains #64

tlokot opened this issue Sep 7, 2022 · 4 comments
Labels
enhancement New feature or request

Comments

@tlokot
Copy link

tlokot commented Sep 7, 2022

My local dev environment makes use of complex localhost FQDN's such as ..localhost (eg. prod.abc.localhost)

I had to alter line 583 in WebAuthn.php to the following to allow these domain names to work:

if ($this->_rpId !== 'localhost' && !\str_ends_with($this->_rpId, '.localhost') && \parse_url($origin, PHP_URL_SCHEME) !== 'https') {
            return false;
}

Is anyone able to check this for security/compliance issues and submit a pull request (I'm happy to submit if someone can point me in the right direction)? I was able to use a Yubikey and TouchID locally after making this change.

@lbuchs
Copy link
Owner

lbuchs commented Sep 8, 2022

localhost is not supposed to have any subdomains.
To do so violates the approved RFC standards.

@tlokot
Copy link
Author

tlokot commented Sep 8, 2022

I can appreciate that at an RFC level, but the localhost exception exists for local development and not everyone has the luxury of putting every development environment on the exact word "localhost". Even though it is against the RFC, MacOS, Windows and Linux all support mapping random aliases to localhost via the host files (or equivalent) and all major web server software matches those names just fine. Because of these mechanisms, the concept of "local" could just as easily be "prod.abc.local" or "prod.abc.dev", etc so long as they all map to 127.0.0.1

One alternative might be to introduce a dnslookup, but this creates too many potential problems for a live server.

Looking around at other libraries and how they've "solved" this issue, what do you think about developers being able to pass through a list of "safe" or "exception" domains, where localhost is always part of that list. eg. https://webauthn-doc.spomky-labs.com/pure-php/advanced-behaviours/dealing-with-localhost#the-hard-way

This keeps the onus of "secure" being placed on the developer, without introducing any dns lookups, while giving the developer control over their environment. It also removes a hard-coded assumption in the code. The code would then become something like:

if (!\in_array($this->_rpId, $secureDomains) && \parse_url($origin, PHP_URL_SCHEME) !== 'https') {
            return false;
}

where $secureDomains is defaulted to ['localhost'] and the developer can add to that list somehow?

Would a solution along these lines still satisfy the spirit of why this exception was created in the first place?

@tlokot
Copy link
Author

tlokot commented Sep 9, 2022

@lbuchs Rather than altering the way _checkOrigin works internally, would you consider raising the internal visibility from private to protected for all methods and properties in WebAuthn.php so it can be easily extended and/or overridden? This would remove any compliance burden from you while making it easier to work with and it wouldn't alter the integrity of your library architecture.

@lbuchs lbuchs added the enhancement New feature or request label Nov 15, 2022
@CrownFish-Hsu
Copy link

@lbuchs Android's request json origin is package name, not https domain. so check origin failed, is right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants