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 expectedType for verifyAuthenticationResponse and verifyRegistrationResponse #436

Merged
merged 3 commits into from
Sep 28, 2023

Conversation

fabiancook
Copy link
Contributor

This enables the functionality for authentication types like payment.get

This would be used in this code right here: https://github.com/opennetwork/logistics/blob/7a305ea1a51d2276e2b77ebbc8be42e80c361893/src/listen/auth/webauthn.ts#L593C9-L604C12

const {verified, authenticationInfo} = await verifyAuthenticationResponse({
  response: payment.details,
  expectedChallenge,
  expectedOrigin: WEBAUTHN_RP_ORIGIN || origin,
  expectedRPID: WEBAUTHN_RP_ID || hostname,
  expectedType: "payment.get",
  authenticator: {
    credentialID: new Uint8Array(toArrayBuffer(found.credentialId, true)),
    credentialPublicKey: new Uint8Array(toArrayBuffer(found.credentialPublicKey, true)),
    transports: found.authenticatorTransports,
    counter: found.credentialCounter ?? 0
  }
});

Related to #402

This isn't complete support for secure payment confirmation, but its the only runtime level change needed. Some types need to be widened to allow additional extensions.

@MasterKale
Copy link
Owner

MasterKale commented Sep 12, 2023

Hey, regarding the issues you're having getting tests working, I discovered a bootstrapping issue with the monorepo related to how workspaces are set up. I needed to explicitly perform a special sequence of commands to get root dependencies installed which make it possible to actually build the various packages after a fresh clone that will ultimately get pnpm to understand the dnt-built workspace folders are in place for symlinking.

tl;dr: I just pushed up a new pnpm command to the master branch that you can run to hopefully get tests working locally:

pnpm run bootstrap-monorepo

Give it a try and let me know what you think

@fabiancook
Copy link
Contributor Author

Upgrading deno and running that command worked :)

Tests are now running locally, but I do get a couple of failures, and it seems I'm running into some changes from https://github.com/MasterKale/SimpleWebAuthn/blob/master/packages/server/deno.lock that my local deno must be looking to change.

@fabiancook
Copy link
Contributor Author

Added my tests:

should throw an error if type not the expected type ... ok (3ms)
should throw an error if type not in list of expected types ... ok (3ms)

should throw when response type is not expected value ... ok (3ms)
should throw when response type is not in list of expected types ... ok (3ms)

should validate when attestation type is not webauthn.create and expected type provided ... ok (3ms)

Locally I get these couple errors:

should verify Android KeyStore response ... FAILED (11ms)
should validate Android-Key response ... FAILED (6ms)


error: Error: Cannot get schema for 'n' target
    at U.get (https://esm.sh/v132/@peculiar/[email protected]/deno/asn1-schema.mjs:2:5714)
    at Function.fromASN (https://esm.sh/v132/@peculiar/[email protected]/deno/asn1-schema.mjs:2:8533)
    at Function.parse (https://esm.sh/v132/@peculiar/[email protected]/deno/asn1-schema.mjs:2:8441)
    at verifyAttestationAndroidKey (file:///Users/fabiancook/src/SimpleWebAuthn/packages/server/src/registration/verifications/verifyAttestationAndroidKey.ts:77:39)
    at verifyRegistrationResponse (file:///Users/fabiancook/src/SimpleWebAuthn/packages/server/src/registration/verifyRegistrationResponse.ts:255:22)
    at eventLoopTick (ext:core/01_core.js:183:11)
    at async file:///Users/fabiancook/src/SimpleWebAuthn/packages/server/src/registration/verifications/verifyAttestationAndroidKey.test.ts:16:24
should validate Android-Key response => ./src/registration/verifyRegistrationResponse.test.ts:596:6
error: Error: Cannot get schema for 'n' target
    at U.get (https://esm.sh/v132/@peculiar/[email protected]/deno/asn1-schema.mjs:2:5714)
    at Function.fromASN (https://esm.sh/v132/@peculiar/[email protected]/deno/asn1-schema.mjs:2:8533)
    at Function.parse (https://esm.sh/v132/@peculiar/[email protected]/deno/asn1-schema.mjs:2:8441)
    at verifyAttestationAndroidKey (file:///Users/fabiancook/src/SimpleWebAuthn/packages/server/src/registration/verifications/verifyAttestationAndroidKey.ts:77:39)
    at verifyRegistrationResponse (file:///Users/fabiancook/src/SimpleWebAuthn/packages/server/src/registration/verifyRegistrationResponse.ts:255:22)
    at eventLoopTick (ext:core/01_core.js:183:11)
    at async file:///Users/fabiancook/src/SimpleWebAuthn/packages/server/src/registration/verifyRegistrationResponse.test.ts:598:24

@MasterKale
Copy link
Owner

@fabiancook I regenerated server's Deno lock file which should resolve those issues you're seeing when running tests. Try rebasing off of the latest master branch and see if it fixes the problem on your end too.

@fabiancook
Copy link
Contributor Author

I can see the tests run successfully for me as part of pnpm run bootstrap-monorepo but not when I run pnpm run test directly

> @simplewebauthn/[email protected] test
> node test_runner.js

See:

image

But then for pnpm run test:

image

deno --version
deno 1.36.4 (release, aarch64-apple-darwin)
v8 11.6.189.12
typescript 5.1.6
node --version
v20.6.1

Can see deno v1.37.0 popped up 2 days ago

running it with the updated version of:

deno --version
deno 1.37.0 (release, aarch64-apple-darwin)
v8 11.8.172.3
typescript 5.2.2
pnpm run test

> [email protected] test /Users/fabiancook/src/virtualstate/SimpleWebAuthn
> pnpm run test:browser; pnpm run test:server


> [email protected] test:browser /Users/fabiancook/src/virtualstate/SimpleWebAuthn
> lerna run test --scope=@simplewebauthn/browser

lerna notice cli v7.1.5
lerna notice filter including "@simplewebauthn/browser"
lerna info filter [ '@simplewebauthn/browser' ]

> @simplewebauthn/browser:test  [existing outputs match the cache, left as is]


> @simplewebauthn/[email protected] test /Users/fabiancook/src/virtualstate/SimpleWebAuthn/packages/browser
> jest

 PASS  src/index.test.ts
 PASS  src/helpers/browserSupportsWebAuthn.test.ts
 PASS  src/methods/startRegistration.test.ts
 PASS  src/helpers/webAuthnAbortService.test.ts
 PASS  src/helpers/platformAuthenticatorIsAvailable.test.ts
 PASS  src/methods/startAuthentication.test.ts

Test Suites: 6 passed, 6 total
Tests:       2 skipped, 59 passed, 61 total
Snapshots:   0 total
Time:        2.465 s, estimated 3 s
Ran all test suites.

 ————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————

 >  Lerna (powered by Nx)   Successfully ran target test for project @simplewebauthn/browser (23ms)
 
   Nx read the output from the cache instead of running the command for 1 out of 1 tasks.
 

> [email protected] test:server /Users/fabiancook/src/virtualstate/SimpleWebAuthn
> lerna run test --scope=@simplewebauthn/server

lerna notice cli v7.1.5
lerna notice filter including "@simplewebauthn/server"
lerna info filter [ '@simplewebauthn/server' ]

> @simplewebauthn/server:test

> @simplewebauthn/[email protected] test /Users/fabiancook/src/virtualstate/SimpleWebAuthn/packages/server
> deno test -A src/
error: The source code is invalid, as it does not match the expected hash in the lock file.
  Specifier: https://esm.sh/@peculiar/[email protected]
  Lock file: /Users/fabiancook/src/virtualstate/SimpleWebAuthn/packages/server/deno.lock
 ELIFECYCLE  Test failed. See above for more details.

 ————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————

 >  Lerna (powered by Nx)   Ran target test for project @simplewebauthn/server (685ms)
 
    ✖    1/1 failed
    ✔    0/1 succeeded [0 read from cache]
 
 ELIFECYCLE  Command failed with exit code 1.
 ELIFECYCLE  Test failed. See above for more details.
 ```
 
 Still fails. Is something off with how I am installing it? I can see CI passes... 

@MasterKale MasterKale changed the title Expected type for verifyAuthenticationResponse and verifyRegistrationResponse Add expectedType for verifyAuthenticationResponse and verifyRegistrationResponse Sep 27, 2023
@MasterKale
Copy link
Owner

Hey @fabiancook, I pulled down your branch and rebased on the latest master, and the tests passed just fine:

should throw when response type is not expected value ... ok (5ms)
should throw when response type is not in list of expected types ... ok (4ms)
...
should throw an error if type not the expected type ... ok (4ms)
should throw an error if type not in list of expected types ... ok (5ms)

CI is happy too so I think this is good to go.

@MasterKale MasterKale added this to the v8.2.0 milestone Sep 28, 2023
@MasterKale MasterKale added the package:server @simplewebauthn/server label Sep 28, 2023
@MasterKale MasterKale merged commit d9f85db into MasterKale:master Sep 28, 2023
@MasterKale
Copy link
Owner

This is now available in the newly published @simplewebauthn/[email protected] ✌️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:server @simplewebauthn/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants