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

Add new error codes #2095

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Add new error codes #2095

wants to merge 16 commits into from

Conversation

MasterKale
Copy link
Contributor

@MasterKale MasterKale commented Jul 11, 2024

This PR proposes new error codes to be raised across various WebAuthn interactions. There is an assumption that the user has meaningfully interacted with some part of the ceremony to consent to informing RP's why the ceremony failed.

Fixes #2096.

An explainer is available here: https://github.com/w3c/webauthn/wiki/Explainer:-New-Error-Codes-(2024-Edition)

Note: This PR is targeting #2047 and should not be merged until that PR has been merged.

  • OptOutError
  • UserVerificationError
  • TimeoutError

Preview | Diff

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.

Some general thoughts:

  • I am in favour of adding more specificity for error states after the user has chosen to go ahead with the authentication ceremony. This would not result in a significant impact to the privacy properties of WebAuthn and if it solves a pain point for RPs, we should do it. Essentially, most circumstances where the user indicates their consent to sign in, an error is shown, and then the user taps "Okay" or "Cancel" could result in more specific errors.
  • However, it's not clear to me that cancelling out of a ceremony constitutes a form of user consent. This PR will allow RPs to distinguish between "the user's authenticator has no passkeys or the authenticator decided the user denied consent" (NotAllowedError) and "the user cancelled out of the ceremony (likely because they did have passkeys but did not consent)" (UserCancelledError). This is especially tricky because the actual browser implementation differs from the letter of the standard with respect to dispatching requests to authenticators, e.g. Chrome will default to the platform authenticator if it knows there's a credential there.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
@@ -1946,11 +1946,17 @@ a numbered step. If outdented, it (today) is rendered as a bullet in the midst o
<dl class="switch">
: If |lifetimeTimer| expires,
:: [=set/For each=] |authenticator| in |issuedRequests| invoke the [=authenticatorCancel=] operation on |authenticator|
and [=set/remove=] |authenticator| from |issuedRequests|.
and [=set/remove=] |authenticator| from |issuedRequests|. Throw a "[=create/TimeoutError=]" {{DOMException}}.
Copy link
Member

Choose a reason for hiding this comment

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

General feedback: this PR needs to update section 14, in particular 14.5.2

For example, one such information leak is if the client returns a failure response as soon as the user denies consent to proceed with an authentication ceremony . In this case the Relying Party could detect that the ceremony was canceled by the user and not the timeout, and thus conclude that at least one of the credentials listed in the allowCredentials parameter is available to the user.

This PR is making it possible to distinguish between these cases.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
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.

Before getting into specifics: I noted in #2096 (comment) that we can't simply use any value as the name of a DOMException:

When creating or throwing a DOMException, specifications must use one of these names. If a specification author believes none of these names are a good fit for their case, they must file an issue to discuss adding a new name to the shared namespace [...]

So we would need to upstream these new name definitions, or use DOMException derived interfaces instead.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
Copy link
Contributor

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

I support adding these error codes after the cleanups suggested by other commenters.

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Base automatically changed from 1859-differentiate-errors to main August 7, 2024 19:50
@MasterKale
Copy link
Contributor Author

I'm inclined to break this PR up to try and get some of the less contentious new error codes into L3 before the upcoming deadline (which those are, I'll try and identify before today's WG meeting.)

@MasterKale
Copy link
Contributor Author

I've taken HybridPrerequisitesError and UserHybridCancellationError out of the running for now for sake of pursuing incremental improvement in WebAuthn's error messages instead.

Comment on lines +1970 to +1974
If the user agent is informing the user that
the last used |authenticator| cannot collect [=user verification=] when
<code>|pkOptions|.{{PublicKeyCredentialCreationOptions/authenticatorSelection}}.{{AuthenticatorSelectionCriteria/userVerification}}</code>
is set to {{UserVerificationRequirement/required}},
throw a "{{UserVerificationError}}" {{DOMException}}.
Copy link
Member

Choose a reason for hiding this comment

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

The equivalent error on the authenticator layer is error code equivalent to "ConstraintError", so I think that could be used here too?

This would lump this together with another ConstraintError thrown when authenticatorSelection.userVerification == "required" and mediation == "conditional", but that is also an error expressing that UV was required but couldn't be performed, so I think that can be okay?

Then we could also add a case to pass through error code equivalent to "ConstraintError" from the authenticator layer, like we do with InvalidStateError.

index.bs Outdated Show resolved Hide resolved
Comment on lines +2295 to +2297
: {{UnknownError}}
:: The [=authenticator=] could not process the supplied options,
or encountered an error while creating the new credential.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think error code equivalent to "UnknownError" is actually passed through by the client operation, is it? I think it's currently only InvalidStateError that's passed through like that.

@nadalin nadalin added the @Risk Items that are at risk for L3 label Sep 11, 2024
@nadalin nadalin modified the milestones: L3-WD-02, Futures (catch-all) Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@Risk Items that are at risk for L3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return more nuanced errors
6 participants