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

Adding flexibility in client origin scheme validation to align with real world implementations #2018

Merged
merged 7 commits into from
Feb 21, 2024

Conversation

abergs
Copy link
Contributor

@abergs abergs commented Jan 29, 2024

I suggest adding a little bit of flexibility to the requirements on validating the scheme to be https. This is in response to the real world implementation by clients, where clients (browsers, chrome) allow webauthn on localhost running on the http-scheme. We've been receiving negative feedback for following this part of the spec. I wanted to suggest adding just a little bit of flexibility here, hopefully without opening a can of DNS worms.

I might be sticking my shin out here, since I know the topic of localhost has been brought up in previous calls with varying (dis)-agreement. E.g issue #1204 morphed into a discussion on DNS.

Either I'm misinterpreting the current writing, but to me it's quite clear about not allowing http in any case.

Original:
CleanShot 2024-01-29 at 10 31 55

Updated:
image


Preview | Diff

Clients today want to allow localhost on http, but are forbidden by spec due to scheme validation
@abergs
Copy link
Contributor Author

abergs commented Jan 29, 2024

@MasterKale any thoughts? I don't wanna hog the meeting minutes on Wednesday, if this is dumb I'd rather close it before then.

edit: ipr failed because my accounts didn't line up. It's been fixed.

Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

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

Seems fair to me.

index.bs Outdated Show resolved Hide resolved
Co-authored-by: Emil Lundberg <emil@emlun.se>
Copy link
Contributor

@MasterKale MasterKale left a comment

Choose a reason for hiding this comment

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

This seems fine to me. The spec mentions secure contexts a couple of times but it's far from obvious that http://localhost is one especially if all you're going off of is WHATWG.

I suggest we also specifically call out here that http://localhost addresses are valid. Let's include a localhost example here too? And maybe a mention of an address like http://localhost:8000 being valid too because many JS frameworks will start local dev servers on a different port than 80.

Copy link
Member

@timcappalli timcappalli left a comment

Choose a reason for hiding this comment

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

makes sense to me

@abergs
Copy link
Contributor Author

abergs commented Jan 29, 2024

@MasterKale I agree and even thought about adding an example too, but didn't want to be too specific in order to open the door for more question.

I'll add second PR with a suggested example so that it can be independently accepted/rejected.

Copy link
Member

@nsatragno nsatragno left a comment

Choose a reason for hiding this comment

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

Drive-by review

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@abergs
Copy link
Contributor Author

abergs commented Feb 14, 2024

Incorporated @nsatragno feedback, it now spells out the behaviour:

CleanShot 2024-02-14 at 16 50 46

Copy link
Member

@nsatragno nsatragno left a comment

Choose a reason for hiding this comment

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

Approved, thank you!

Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

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

Thanks!

index.bs Outdated Show resolved Hide resolved
@abergs
Copy link
Contributor Author

abergs commented Feb 14, 2024

CleanShot 2024-02-14 at 21 07 34

@emlun I tried to keep this PR focused to this section of the spec. I noticed that there are more usages of [=host=] elsewhere. I can adress those in a separate PR if [=origin/host=] is more suitable(?)

@barryp
Copy link

barryp commented Feb 15, 2024

Is this going to cover the case of having names in a /etc/hosts file like

127.0.0.1 foo.localhost
127.0.0.1 bar.localhost 

and then trying to access http://foo.localhost or http://bar.localhost? I'm doing this on my development laptop with several sites.

Seems like those are considered Secure Contexts elsewhere.

@abergs
Copy link
Contributor Author

abergs commented Feb 15, 2024

@barryp While I sympathise with your use case, I'm desperately trying to stay away from #1204. I'd be happy to get this in to get a 80% win, and then hash out the remaining 20% in a separate issue/pr if that makes sense.

@nicksteele nicksteele merged commit 3c71812 into w3c:main Feb 21, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Feb 21, 2024
SHA: 3c71812
Reason: push, by nicksteele

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@emlun
Copy link
Member

emlun commented Feb 21, 2024

@emlun I tried to keep this PR focused to this section of the spec. I noticed that there are more usages of [=host=] elsewhere. I can adress those in a separate PR if [=origin/host=] is more suitable(?)

Sorry for the delayed response - yeah, I don't think we need to change any other occurrences. (Probably, I might take a look into that later)

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.

7 participants