Repository: go-webauthn/webauthn
Pull request #423 (2025-05-09 20:49:52)

test(protocol): add test cases to authenticator_test
Add test cases for `AuthenticatorData`'s functions

- Re-factor tests to also check error details
- Add tests to cover failure cases
- Remove newline from 2 `ErrVerification` to be consistent with other errors

coderabbitai[bot]: <!-- This is an auto-generated comment: summarize by coderabbit.ai -->
<!-- This is an auto-generated comment: review in progress by coderabbit.ai -->

> [!NOTE]
> Currently processing new changes in this PR. This may take a few minutes, please wait...
> 
> <details>
> <summary>📥 Commits</summary>
> 
> Reviewing files that changed from the base of the PR and between 5a289d5e104faacaa28018d33b0a53d0b49b0273 and f0be60b337ea8e42b9c77ac81059da32177796ab.
> 
> </details>
> 
> <details>
> <summary>📒 Files selected for processing (2)</summary>
> 
> * `protocol/authenticator.go` (1 hunks)
> * `protocol/authenticator_test.go` (7 hunks)
> 
> </details>
> 
> ```ascii
>  ____________________________________________________
> < Fluent in over six million forms of bug detection. >
>  ----------------------------------------------------
>   \
>    \   (\__/)
>        (•ㅅ•)
>        / 　 づ
> ```

<!-- end of auto-generated comment: review in progress by coderabbit.ai -->
<!-- usage_tips_start -->

> [!TIP]
> <details>
> <summary>You can disable the changed files summary in the walkthrough.</summary>
> 
> Disable the `reviews.changed_files_summary` setting in your project's settings in CodeRabbit to disable the changed files summary in the walkthrough.
> 
> </details>

<!-- usage_tips_end -->
<!-- finishing_touch_checkbox_start -->

<details open="true">
<summary>✨ Finishing Touches</summary>

- [ ] <!-- {"checkboxId": "7962f53c-55bc-4827-bfbf-6a18da830691"} --> 📝 Generate Docstrings

</details>

<!-- finishing_touch_checkbox_end -->
<!-- tips_start -->

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

<details>
<summary>❤️ Share</summary>

- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai)
- [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai)
- [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai)
- [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)

</details>

<details>
<summary>🪧 Tips</summary>

### Chat

There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=go-webauthn/webauthn&utm_content=423):

- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  - `I pushed a fix in commit <commit_id>, please review it.`
  - `Generate unit testing code for this file.`
  - `Open a follow-up GitHub issue for this discussion.`
- Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples:
  - `@coderabbitai generate unit testing code for this file.`
  -	`@coderabbitai modularize this function.`
- PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  - `@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.`
  - `@coderabbitai read src/utils.ts and generate unit testing code.`
  - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.`
  - `@coderabbitai help me debug CodeRabbit configuration file.`

### Support

Need help? Create a ticket on our [support page](https://www.coderabbit.ai/contact-us/support) for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

### CodeRabbit Commands (Invoked using PR comments)

- `@coderabbitai pause` to pause the reviews on a PR.
- `@coderabbitai resume` to resume the paused reviews.
- `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
- `@coderabbitai full review` to do a full review from scratch and review all the files again.
- `@coderabbitai summary` to regenerate the summary of the PR.
- `@coderabbitai generate docstrings` to [generate docstrings](https://docs.coderabbit.ai/finishing-touches/docstrings) for this PR.
- `@coderabbitai generate sequence diagram` to generate a sequence diagram of the changes in this PR.
- `@coderabbitai resolve` resolve all the CodeRabbit review comments.
- `@coderabbitai configuration` to show the current CodeRabbit configuration for the repository.
- `@coderabbitai help` to get help.

### Other keywords and placeholders

- Add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed.
- Add `@coderabbitai summary` to generate the high-level summary at a specific location in the PR description.
- Add `@coderabbitai` anywhere in the PR title to generate the title automatically.

### CodeRabbit Configuration File (`.coderabbit.yaml`)

- You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository.
- Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json`

### Documentation and Community

- Visit our [Documentation](https://docs.coderabbit.ai) for detailed information on how to use CodeRabbit.
- Join our [Discord Community](http://discord.gg/coderabbit) to get help, request features, and share feedback.
- Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.

</details>

<!-- tips_end -->

Patch:
diff --git a/protocol/authenticator.go b/protocol/authenticator.go
index c27726d..d3c351a 100644
--- a/protocol/authenticator.go
+++ b/protocol/authenticator.go
@@ -397,14 +397,14 @@ func (a *AuthenticatorData) Verify(rpIdHash []byte, appIDHash []byte, userVerifi
 	// Registration Step 14 & Assertion Step 16
 	// Verify that the User Present bit of the flags in authData is set.
 	if !a.Flags.UserPresent() {
-		return ErrVerification.WithInfo(fmt.Sprintln("User presence flag not set by authenticator"))
+		return ErrVerification.WithInfo(fmt.Sprintf("User presence flag not set by authenticator"))
 	}
 
 	// Registration Step 15 & Assertion Step 17
 	// If user verification is required for this assertion, verify that
 	// the User Verified bit of the flags in authData is set.
 	if userVerificationRequired && !a.Flags.UserVerified() {
-		return ErrVerification.WithInfo(fmt.Sprintln("User verification required but flag not set by authenticator"))
+		return ErrVerification.WithInfo(fmt.Sprintf("User verification required but flag not set by authenticator"))
 	}
 
 	// Registration Step 17 & Assertion Step 18


Test patch:
diff --git a/protocol/authenticator_test.go b/protocol/authenticator_test.go
index af03c04..2765dcf 100644
--- a/protocol/authenticator_test.go
+++ b/protocol/authenticator_test.go
@@ -2,8 +2,19 @@ package protocol
 
 import (
 	"encoding/base64"
+	"encoding/binary"
+	"encoding/hex"
+	"fmt"
 	"reflect"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	noneAuthDataBase64 = "pkLSG3xtVeHOI8U5mCjSx0m/am7y/gPMnhDN9O1ttItBAAAAAAAAAAAAAAAAAAAAAAAAAAAAQMAxl6G32ykWaLrv/ouCs5HoGsvONqBtOb7ZmyMs8K8PccnwyyqPzWn/yZuyQmQBguvjYSvH6gDBlFG65quUDCSlAQIDJiABIVggyJGP+ra/u/eVjqN4OeYXUShRWxrEeC6Sb5/bZmJ9q8MiWCCHIkRdg5oRb1RHoFVYUpogcjlObCKFsV1ls1T+uUc6rA=="
+	attAuthDataBase64  = "lWkIjx7O4yMpVANdvRDXyuORMFonUbVZu4/Xy7IpvdRBAAAAAAAAAAAAAAAAAAAAAAAAAAAAQIniszxcGnhupdPFOHJIm6dscrWCC2h8xHicBMu91THD0kdOdB0QQtkaEn+6KfsfT1o3NmmFT8YfXrG734WfVSmlAQIDJiABIVggyoHHeiUw5aSbt8/GsL9zaqZGRzV26A4y3CnCGUhVXu4iWCBMnc8za5xgPzIygngAv9W+vZTMGJwwZcM4sjiqkcb/1g=="
 )
 
 func TestAuthenticatorFlags_UserPresent(t *testing.T) {
@@ -147,30 +158,104 @@ func TestAuthenticatorData_Unmarshal(t *testing.T) {
 		rawAuthData []byte
 	}
 
-	noneAuthData, _ := base64.StdEncoding.DecodeString("pkLSG3xtVeHOI8U5mCjSx0m/am7y/gPMnhDN9O1TCItBAAAAAAAAAAAAAAAAAAAAAAAAAAAAQMAxl6G32ykWaLrv/ouCs5HoGsvONqBtOb7ZmyMs8K8PccnwyyqPzWn/yZuyQmQBguvjYSvH6gDBlFG65quUDCSlAQIDJiABIVggyJGP+ra/u/eVjqN4OeYXUShRWxrEeC6Sb5/bZmJ9q8MiWCCHIkRdg5oRb1RHoFVYUpogcjlObCKFsV1ls1T+uUc6rA==")
-	attAuthData, _ := base64.StdEncoding.DecodeString("lWkIjx7O4yMpVANdvRDXyuORMFonUbVZu4/Xy7IpvdRBAAAAAAAAAAAAAAAAAAAAAAAAAAAAQIniszxcGnhupdPFOHJIm6dscrWCC2h8xHicBMu91THD0kdOdB0QQtkaEn+6KfsfT1o3NmmFT8YfXrG734WfVSmlAQIDJiABIVggyoHHeiUw5aSbt8/GsL9zaqZGRzV26A4y3CnCGUhVXu4iWCBMnc8za5xgPzIygngAv9W+vZTMGJwwZcM4sjiqkcb/1g==")
+	noneAuthData, _ := base64.StdEncoding.DecodeString(noneAuthDataBase64)
+	attAuthData, _ := base64.StdEncoding.DecodeString(attAuthDataBase64)
+	// Empty data
+	badAuthData1 := []byte{}
+	// Attested credential data missing
+	badAuthData2 := make([]byte, minAttestedAuthLength-1)
+	copy(badAuthData2, attAuthData)
+	// Flags not set but data exists
+	badAuthData3 := make([]byte, len(attAuthData))
+	copy(badAuthData3, attAuthData)
+	badAuthData3[32] &= 0b0011_1111
+	// Extensions data missing
+	badAuthData4 := make([]byte, len(attAuthData))
+	copy(badAuthData4, attAuthData)
+	badAuthData4[32] |= 0b1000_0000
+	// Leftover bytes
+	badAuthData5 := make([]byte, len(attAuthData))
+	copy(badAuthData5, attAuthData)
+	badAuthData5 = append(badAuthData5, []byte("Hello World")...)
 
 	tests := []struct {
-		name    string
-		fields  fields
-		args    args
-		wantErr bool
+		name   string
+		fields fields
+		args   args
+
+		errString  string
+		errType    string
+		errDetails string
+		errInfo    string
 	}{
 		{
-			"None Marshall Successfully",
-			fields{},
-			args{
+			name:   "None Marshall Successfully",
+			fields: fields{},
+			args: args{
 				noneAuthData,
 			},
-			false,
 		},
 		{
-			"Att Data Marshall Successfully",
-			fields{},
-			args{
+			name:   "Att Data Marshall Successfully",
+			fields: fields{},
+			args: args{
 				attAuthData,
 			},
-			false,
+		},
+		{
+			name:   "Authenticator data too short",
+			fields: fields{},
+			args: args{
+				badAuthData1,
+			},
+			errString:  "Authenticator data length too short",
+			errType:    "invalid_request",
+			errDetails: "Authenticator data length too short",
+			errInfo:    fmt.Sprintf("Expected data greater than %d bytes. Got %d bytes", minAuthDataLength, len(badAuthData1)),
+		},
+		{
+			name:   "Attested credential missing",
+			fields: fields{},
+			args: args{
+				badAuthData2,
+			},
+			errString:  "Attested credential flag set but data is missing",
+			errType:    "invalid_request",
+			errDetails: "Attested credential flag set but data is missing",
+			errInfo:    "",
+		},
+		{
+			name:   "Attested credential missing",
+			fields: fields{},
+			args: args{
+				badAuthData3,
+			},
+			errString:  "Attested credential flag not set",
+			errType:    "invalid_request",
+			errDetails: "Attested credential flag not set",
+			errInfo:    "",
+		},
+		{
+			name:   "Extensions data missing",
+			fields: fields{},
+			args: args{
+				badAuthData4,
+			},
+			errString:  "Extensions flag set but extensions data is missing",
+			errType:    "invalid_request",
+			errDetails: "Extensions flag set but extensions data is missing",
+			errInfo:    "",
+		},
+		{
+			name:   "Leftover bytes",
+			fields: fields{},
+			args: args{
+				badAuthData5,
+			},
+			errString:  "Leftover bytes decoding AuthenticatorData",
+			errType:    "invalid_request",
+			errDetails: "Leftover bytes decoding AuthenticatorData",
+			errInfo:    "",
 		},
 	}
 
@@ -183,9 +268,17 @@ func TestAuthenticatorData_Unmarshal(t *testing.T) {
 				AttData:  tt.fields.AttData,
 				ExtData:  tt.fields.ExtData,
 			}
-			if err := a.Unmarshal(tt.args.rawAuthData); (err != nil) != tt.wantErr {
-				t.Errorf("AuthenticatorData.Unmarshal() error = %v, wantErr %v", err, tt.wantErr)
+
+			err := a.Unmarshal(tt.args.rawAuthData)
+			if tt.errString != "" {
+				assert.EqualError(t, err, tt.errString)
+
+				AssertIsProtocolError(t, err, tt.errType, tt.errDetails, tt.errInfo)
+
+				return
 			}
+
+			require.NoError(t, err)
 		})
 	}
 }
@@ -203,13 +296,78 @@ func TestAuthenticatorData_unmarshalAttestedData(t *testing.T) {
 		rawAuthData []byte
 	}
 
+	noneAuthData, _ := base64.StdEncoding.DecodeString(noneAuthDataBase64)
+	attAuthData, _ := base64.StdEncoding.DecodeString(attAuthDataBase64)
+	// Data length too short
+	badAuthData1 := make([]byte, len(attAuthData))
+	copy(badAuthData1, attAuthData)
+	binary.BigEndian.PutUint16(badAuthData1[53:], 256)
+	// ID length too long
+	badAuthData2 := make([]byte, len(attAuthData)+maxCredentialIDLength+1)
+	copy(badAuthData2, attAuthData)
+	binary.BigEndian.PutUint16(badAuthData2[53:], maxCredentialIDLength+1)
+	// Malformed public key
+	badAuthData3 := make([]byte, 119)
+	copy(badAuthData3, attAuthData[:119])
+	badData, _ := hex.DecodeString("83FF20030102")
+	badAuthData3 = append(badAuthData3, badData...)
+
 	tests := []struct {
-		name    string
-		fields  fields
-		args    args
-		wantErr bool
+		name       string
+		fields     fields
+		args       args
+		errString  string
+		errType    string
+		errDetails string
+		errInfo    string
 	}{
-		// TODO: Add test cases.
+		{
+			name:   "None Marshall Successfully",
+			fields: fields{},
+			args: args{
+				noneAuthData,
+			},
+		},
+		{
+			name:   "Att Data Marshall Successfully",
+			fields: fields{},
+			args: args{
+				attAuthData,
+			},
+		},
+		{
+			name:   "Data length too short",
+			fields: fields{},
+			args: args{
+				badAuthData1,
+			},
+			errString:  "Authenticator attestation data length too short",
+			errType:    "invalid_request",
+			errDetails: "Authenticator attestation data length too short",
+			errInfo:    "",
+		},
+		{
+			name:   "ID length too long",
+			fields: fields{},
+			args: args{
+				badAuthData2,
+			},
+			errString:  "Authenticator attestation data credential id length too long",
+			errType:    "invalid_request",
+			errDetails: "Authenticator attestation data credential id length too long",
+			errInfo:    "",
+		},
+		{
+			name:   "Could not unmarshal Credential Public Key",
+			fields: fields{},
+			args: args{
+				badAuthData3,
+			},
+			errString:  "Could not unmarshal Credential Public Key: cbor: unexpected \"break\" code",
+			errType:    "invalid_request",
+			errDetails: "Could not unmarshal Credential Public Key: cbor: unexpected \"break\" code",
+			errInfo:    "",
+		},
 	}
 
 	for _, tt := range tests {
@@ -221,9 +379,16 @@ func TestAuthenticatorData_unmarshalAttestedData(t *testing.T) {
 				AttData:  tt.fields.AttData,
 				ExtData:  tt.fields.ExtData,
 			}
-			if err := a.unmarshalAttestedData(tt.args.rawAuthData); (err != nil) != tt.wantErr {
-				t.Errorf("AuthenticatorData.unmarshalAttestedData() error = %v, wantErr %v", err, tt.wantErr)
+			err := a.unmarshalAttestedData(tt.args.rawAuthData)
+			if tt.errString != "" {
+				assert.EqualError(t, err, tt.errString)
+
+				AssertIsProtocolError(t, err, tt.errType, tt.errDetails, tt.errInfo)
+
+				return
 			}
+
+			require.NoError(t, err)
 		})
 	}
 }
@@ -269,12 +434,67 @@ func TestAuthenticatorData_Verify(t *testing.T) {
 	}
 
 	tests := []struct {
-		name    string
-		fields  fields
-		args    args
-		wantErr bool
+		name       string
+		fields     fields
+		args       args
+		errString  string
+		errType    string
+		errDetails string
+		errInfo    string
 	}{
-		// TODO: Add test cases.
+		{
+			name: "Success",
+			fields: fields{
+				RPIDHash: []byte{1, 2, 3},
+				Flags:    AuthenticatorFlags(0x05),
+			},
+			args: args{
+				rpIdHash: []byte{1, 2, 3},
+			},
+			errString: "",
+		},
+		{
+			name: "RP hash mismatch",
+			fields: fields{
+				RPIDHash: []byte{0xff},
+			},
+			args: args{
+				rpIdHash: []byte{0xaa},
+			},
+			errString:  "Error validating the authenticator response",
+			errType:    "verification_error",
+			errDetails: "Error validating the authenticator response",
+			errInfo:    "RP Hash mismatch. Expected ff and Received aa",
+		},
+		{
+			name: "UP flag not set",
+			fields: fields{
+				RPIDHash: []byte{1, 2, 3},
+				Flags:    AuthenticatorFlags(0x04),
+			},
+			args: args{
+				rpIdHash: []byte{1, 2, 3},
+			},
+			errString:  "Error validating the authenticator response",
+			errType:    "verification_error",
+			errDetails: "Error validating the authenticator response",
+			errInfo:    "User presence flag not set by authenticator",
+		},
+		{
+			name: "User verification required",
+			fields: fields{
+				RPIDHash: []byte{1, 2, 3},
+				Flags:    AuthenticatorFlags(0x01),
+			},
+			args: args{
+				rpIdHash:                 []byte{1, 2, 3},
+				userVerificationRequired: true,
+			},
+			errString:  "Error validating the authenticator response",
+			errType:    "verification_error",
+			errDetails: "Error validating the authenticator response",
+			errInfo:    "User verification required but flag not set by authenticator",
+		},
 	}
 
 	for _, tt := range tests {
@@ -286,9 +506,16 @@ func TestAuthenticatorData_Verify(t *testing.T) {
 				AttData:  tt.fields.AttData,
 				ExtData:  tt.fields.ExtData,
 			}
-			if err := a.Verify(tt.args.rpIdHash, nil, tt.args.userVerificationRequired); (err != nil) != tt.wantErr {
-				t.Errorf("AuthenticatorData.Verify() error = %v, wantErr %v", err, tt.wantErr)
+			err := a.Verify(tt.args.rpIdHash, nil, tt.args.userVerificationRequired)
+			if tt.errString != "" {
+				assert.EqualError(t, err, tt.errString)
+
+				AssertIsProtocolError(t, err, tt.errType, tt.errDetails, tt.errInfo)
+
+				return
 			}
+
+			require.NoError(t, err)
 		})
 	}
 }