Skip to content

Unsafe Challenge Overrides #112

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.

Sign up for GitHub

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

Merged
dimitribouniol merged 2 commits into from
Nov 3, 2025
Merged

Unsafe Challenge Overrides #112

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf2308c
Added unsafe setters for changing the challenge returned by the manager
dimitribouniol Oct 9, 2025
495b3ae
Renamed setUnsafeChallenge to unsafeSetChallenge
dimitribouniol Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions Sources/WebAuthn/Ceremonies/Authentication/PublicKeyCredentialRequestOptions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,17 @@ import Foundation
public struct PublicKeyCredentialRequestOptions: Sendable {
/// A challenge that the authenticator signs, along with other data, when producing an authentication assertion.
///
/// When encoding using `Encodable` this is encoded as base64url.
public var challenge: [UInt8]
/// The Relying Party should store the challenge temporarily until the authentication flow is complete. When encoding using `Encodable` this is encoded as base64url.
///
/// - Warning: Although the challenge can be changed, doing so is not recommended and can lead to an insecure implementation of the WebAuthn protocol. See ``setUnsafeChallenge(_:)``.
Copy link
Member

@0xTim 0xTim Nov 3, 2025

Choose a reason for hiding this comment

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

Let's just remove this from the API docs, I don't think we need it

Copy link
Collaborator Author

@dimitribouniol dimitribouniol Nov 3, 2025

Choose a reason for hiding this comment

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

Removed in favor of a SeeAlso

public private(set) var challenge: [UInt8]

/// Unsafely change the challenge that will be delivered to the client.
///
/// - Warning: Although the challenge can be changed, doing so is not recommended and can lead to an insecure implementation of the WebAuthn protocol.
public mutating func setUnsafeChallenge(_ newValue: [UInt8]) {
Copy link
Member

@0xTim 0xTim Nov 3, 2025

Choose a reason for hiding this comment

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

Suggested change
public mutating func setUnsafeChallenge(_ newValue: [UInt8]) {
public mutating func unsafeSetChallenge(_ newValue: [UInt8]) {

Copy link
Collaborator Author

@dimitribouniol dimitribouniol Nov 3, 2025

Choose a reason for hiding this comment

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

Renamed.

challenge = newValue
}

/// A time, in seconds, that the caller is willing to wait for the call to complete. This is treated as a
/// hint, and may be overridden by the client.
Expand Down
11 changes: 10 additions & 1 deletion Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,16 @@ public struct PublicKeyCredentialCreationOptions: Sendable {
///
/// The Relying Party should store the challenge temporarily until the registration flow is complete. When
/// encoding using `Encodable`, the challenge is base64url encoded.
public let challenge: [UInt8]
///
/// - Warning: Although the challenge can be changed, dooing so is not recommended and can lead to an insecure implementation of the WebAuthn protocol. See ``setUnsafeChallenge(_:)``.
Copy link
Member

@0xTim 0xTim Nov 3, 2025

Choose a reason for hiding this comment

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

As above

Copy link
Collaborator Author

@dimitribouniol dimitribouniol Nov 3, 2025

Choose a reason for hiding this comment

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

I'll change them right now!

Copy link
Collaborator Author

@dimitribouniol dimitribouniol Nov 3, 2025

Choose a reason for hiding this comment

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

Removed in favor of a SeeAlso.

Copy link
Collaborator Author

@dimitribouniol dimitribouniol Nov 3, 2025

Choose a reason for hiding this comment

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

Actually, should I have kept the warning on unsafeSetChallenge?

Copy link
Member

@0xTim 0xTim Nov 3, 2025

Choose a reason for hiding this comment

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

Yeah keep the warning on unsafeSetChallenge, we just don't need to include it on the main parameter

Copy link
Collaborator Author

@dimitribouniol dimitribouniol Nov 3, 2025

Choose a reason for hiding this comment

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

Updated!

public private(set) var challenge: [UInt8]

/// Unsafely change the challenge that will be delivered to the client.
///
/// - Warning: Although the challenge can be changed, doing so is not recommended and can lead to an insecure implementation of the WebAuthn protocol.
public mutating func setUnsafeChallenge(_ newValue: [UInt8]) {
challenge = newValue
}

/// Contains names and an identifier for the user account performing the registration.
public var user: PublicKeyCredentialUserEntity
Expand Down
Loading