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

Fixes #1751 - Add error thrown if APP_URL does not match current url #1985

Merged
merged 6 commits into from
Aug 24, 2023

Conversation

ildyria
Copy link
Member

@ildyria ildyria commented Aug 21, 2023

Fixes #1751

This aims to avoid some of the confusions "why is U2F not working?".

@ildyria ildyria requested a review from a team August 21, 2023 21:28
@ildyria ildyria added the Review: easy Easy review expected: probably just need a quick to go through. label Aug 21, 2023
@ildyria ildyria added this to the 4.11.1 milestone Aug 21, 2023
@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #1985 (c4c07df) into master (12bd6d4) will increase coverage by 1.86%.
The diff coverage is 71.42%.

Additional details and impacted files

Copy link
Contributor

@qwerty287 qwerty287 left a comment

Choose a reason for hiding this comment

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

What happens if setting APP_URL with a subpath? Is this allowed or would this be a wrong configuration?

@ildyria
Copy link
Member Author

ildyria commented Aug 22, 2023

What happens if setting APP_URL with a subpath? Is this allowed or would this be a wrong configuration?

Good point, I adapted the code to throw a warning in such cases:
8320d16

Co-authored-by: Martin Stone <1611702+d7415@users.noreply.github.com>
Copy link
Contributor

@d7415 d7415 left a comment

Choose a reason for hiding this comment

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

I wonder whether it's worth adding validation for subpaths (or the host of APP_URLs containing them), but that can always be added later.

Untested, as usual.

@qwerty287
Copy link
Contributor

Not sure if you understood my point, but my question actually was: What happens with both domain and subpath? I.e. APP_URL=https://example.com/xzy. Is this supported/allowed by laravel?

@d7415
Copy link
Contributor

d7415 commented Aug 24, 2023

Is this supported/allowed by laravel?

At a glance, it looks like it is.

@ildyria
Copy link
Member Author

ildyria commented Aug 24, 2023

https://www.w3.org/TR/webauthn-2/#rp-id

The RP ID of a public key credential determines its scope. I.e., it determines the set of origins on which the public key credential may be exercised, as follows:

For example, given a Relying Party whose origin is https://login.example.com:1337, then the following RP IDs are valid: login.example.com (default) and example.com, but not m.login.example.com and not com.

@ildyria ildyria merged commit 3dd7dcc into master Aug 24, 2023
29 checks passed
@ildyria ildyria deleted the diagnostic-app-url branch August 24, 2023 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: easy Easy review expected: probably just need a quick to go through.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebAuthn/U2F: Attestation Error: Relying Party ID not scoped to current
3 participants