use web crypto instead of cryptojs

[43081j]

Closes/fixes #275

Checklist

• This PR makes changes to the public API
• I have included links for closing relevant issue numbers
• replace test runner/library
• update docs for interface change

[pamapa]

Thanks for the proof of concept, does not look to invasive :-)

The main thing that questions me is:
https://developer.mozilla.org/en-US/docs/Web/API/Crypto/subtle -> Secure context required
I mean for testing and production system this is good, but how do we deal with development systems (localhost), where we have HTTP and not HTTPS?

[43081j]

@pamapa you're in luck i think. from MDN:

Locally-delivered resources such as those with http://127.0.0.1 URLs, http://localhost and http://*.localhost URLs (e.g. http://dev.whatever.localhost/), and file:// URLs are also considered to have been delivered securely.

[43081j]

could we possibly come up with a path forward if you get time? i could either use this PR or make a new one with whatever approach we settle on. then maybe we just publish it as a pre-release for now?

im happy to then test it for a while in one of our larger projects

[pamapa]

the changes i see look pretty good, i need sometime to test it in one of our projects. Maybe you can do the same. I just copy that source into a subdir of the project and direct the imports accordingly...

[43081j]

sure ill give it a go

the tests will fail in CI fyi until we switch test runner (jsdom is missing textencoder and what not, its pretty far behind)

[pamapa]

works like a charm here, including silent renew...

Test env: localhost + Ping Identity + Chrome + Firefox

[pamapa]

also please adapt the initial comment of this merge request with the one in the template (Fixes... Checklist...)

[pamapa]

My plan is to have a 3.0.0 in Q3/Q4 of this year. The other thing i would like to land is the fix of the current strange claims merge behavior...

[43081j]

we still need to move over to web-test-runner too, which is why this PR is blocked

its a fairly big move i think

[pamapa]

we still need to move over to web-test-runner too, which is why this PR is blocked

Do you have time to adapt the unit-tests as well sometime in the next weeks?

[43081j]

i started doing it on a branch but it kinda spiralled. ill probably throw that away and try again from scratch

will make sure i have a go at it soon so we can at least know how big the task is

[43081j]

@pamapa just to update you on this

the big chunk of work to migrate to WTR seems to actually be migration off jest's assertions and mocks.

migrating jest assertions to chai is straight forward but time consuming (lot of find & replace).

we then have to migrate off the mocks to hanbi (used by modernweb) or sinon

alternatively we try figure out a way to use jest for its assertions/mocks but not its test runner. feels sketchy going half way like that, though...

[pamapa]

the big chunk of work to migrate to WTR seems to actually be migration off jest's assertions and mocks.

I prefer to stay with jest and using the mocks as they are now, unless there is a technical issue with doing so. I am not aware of such an issue. So please do not waste your time with porting to another test framework...

[kitten]

Hiya 👋
I don't want to be too pushy here or hurry this along, but just offer a commit for the missing TextEncoder test setup and mocks for Jest: kitten@7898e34

Totally fine to ignore this, but I thought I'd just post and mention it here, in case it was blocking and this could help this draft PR along ✌️

[43081j]

actually glad this got bumped! just got back from vacation so i will give this branch another look this week if i can

[43081j]

@pamapa an unfortunate update...

jest only ships commonjs and unsurprisingly depends on a lot of node APIs (e.g. url, path, etc).

we can get so far by having WTR/rollup transform the commonjs to ESM on the fly, but we can't really solve the node API stuff (even if we try polyfill them it'll be no end of flakiness i think).

not sure what the sensible thing to do here is now...

also does raise a very good point... that jest module mocks make no sense anymore. they can't ever really work in a browser, and this package needs to support browsers really. so whatever we do, ultimately the mocked modules need to go.

edit: looks like we don't use jest.mock actually (to mock imported modules at least), which is good but doesn't unblock us

[kitten]

Are you referring to writing "real world" browser E2E/integration tests by any chance?

I've been looking into the same a while back for another OSS project, and the options aren't great just yet. The most sensible has been Cypress + Component testing + Vite imho. If that's indeed though what you're looking for, I wish Vitest's Browser Mode was ready already, since it'd be perfect for mixing browser- and unit-tests.

Just to add an opinion, not sure if that's the task at hand for this repo, but if it's a big blocker, I'd suggest not throwing away all unit tests and instead using any browser test as a supplement, to not start from scratch entirely 🤔

Anyway, I'll get out of your hair now. No rush & Cheers! ❤️

[43081j]

Are you referring to writing "real world" browser E2E/integration tests by any chance?

I've been looking into the same a while back for another OSS project, and the options aren't great just yet. The most sensible has been Cypress + Component testing + Vite imho. If that's indeed though what you're looking for, I wish Vitest's Browser Mode was ready already, since it'd be perfect for mixing browser- and unit-tests.

Just to add an opinion, not sure if that's the task at hand for this repo, but if it's a big blocker, I'd suggest not throwing away all unit tests and instead using any browser test as a supplement, to not start from scratch entirely 🤔

Anyway, I'll get out of your hair now. No rush & Cheers! ❤️

web-test-runner is basically one of the go-to solutions these days around the web-first/native community.

its very well tested, widely used, etc. super easy to get setup

the problem is more that the current tests here were built in a node-only environment originally. which is the reason jest works (jest is node-only itself). specifically jest's assertions/mocks, we don't care about jest itself, that is easy to drop (and replace with wtr).

there's various things you can do like transforming CJS on test run (to ESM), and polyfilling node APIs (path/fs/etc). but its all really avoiding the reality that is jest isn't made to work in a browser, we should be using a more modern test assertion and mocking library (WTR handles the test runner part already).

there's already a few options for that. the obvious ones are chai (we recently published an ESM-only version of chai that works natively in browsers) and sinon or hanbi (both should work in browsers, sinon has a much larger footprint).

fwiw too vitest will one day be in a similar place to wtr but ultimately is a thin layer around vite, which itself is a thin layer around rollup. so we'd run into the exact same issues as here

the ultimate solution is obvious but a large change, basically

[pamapa]

this simple patch makes this merge request work:

 test/setup.ts | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/test/setup.ts b/test/setup.ts
index 34b5b46..8178605 100644
--- a/test/setup.ts
+++ b/test/setup.ts
@@ -1,6 +1,21 @@
+import { TextEncoder } from "util";
+
 import { Log } from "../src";
 
 beforeAll(() => {
+    // polyfill or mock features which are not present globally in jsdom
+    globalThis.TextEncoder = TextEncoder;
+    Object.defineProperty(globalThis, "crypto", {
+        value: {
+            getRandomValues: jest.fn(),
+            subtle: {
+                digest: jest.fn().mockImplementation(
+                    () => [0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
+                ),
+            },
+        },
+    });
+
     globalThis.fetch = jest.fn();
 
     const unload = () =>

The main issue is that testEnvironment: "jsdom" does not yet support subtle and getRandomValues. For unit-tests proposes it is the easiest to simply mock them...

[43081j]

this simple patch makes this merge request work

we were already aware of this long ago, that isn't the blocker here.

the whole point of this move to built-in crypto was to make oidc-client-ts work in a browser natively. this PR already does that near enough. the last piece of this PR is to then test the module in a real browser.

the blocker here is that. we want to test the code in a real browser, i.e. use web-test-runner to run our unit tests.

inevitably it'll have to happen, in this PR or a followup PR. one way or another some non-jest tests need setting up, against a real browser (if you want any confidence in what is being shipped).

i can get CI passing any time without any tests, its not what was blocking us or what any of the posts above were trying to explain fwiw.

[pamapa]

the blocker here is that. we want to test the code in a real browser, i.e. use web-test-runner to run our unit tests.

inevitably it'll have to happen, in this PR or a followup PR. one way or another some non-jest tests need setting up, against a real browser (if you want any confidence in what is being shipped).

This improvement is out of scope for this merge request:

1. unit-tests only test single functions or units for correctness. When we use crypto.subtle, the only thing that matters is that we call this function with the correct parameters and correctly use the returned data. crypto.subtle itself acts like a black-box, we do not need to make sure it works correctly within unit-tests.
2. testing library functionality (like sign flow, etc..) within a "real test browser" would be awesome, this kind of test would be no longer an unit-test but an integration test. This integration test does not replace the existing unit-test but can be added additionally, maybe additionally/separately?

Thus i really want to keep the existing unit-tests as is, but an additional integration test framework is appreciate, as long as you are willing to support it. If it is possible to have both unit-tests and integration-tests within the same test framework, that might be nice, but still not part of this merge request.

[43081j]

interesting, its because technically value could be an object but should be a primitive

i've added a tiny isPrimitive type guard in that file to get the type right

[pamapa]

interesting, its because technically value could be an object but should be a primitive

i've added a tiny isPrimitive type guard in that file to get the type right

Please not, you can simply declare the type different e.g.:

simple version:

private readonly _optionalParams: Record<string, string | number> | undefined;

more code version:

private readonly _optionalParams: Partial<Omit<SigninRequestArgs, "state_data" | "extraQueryParams" | "extraTokenParams">>;

And also add a space before and after the pipe:
e.g.
_resource: string|string[]|undefined -> _resource: string | string[] | undefined, ... there are more like this

And _authority seems unused -> lets drop it

[43081j]

The omit list is a bit longer but I'll put the change in 👍

Just didn't before in case you didn't want the property types changing

[43081j]

there you go @pamapa

[pamapa]

alternative, than storing all whats needed for getUrl we simple store the almost calculated url, execpt the async challenge, which can/must still be done in getUrl.

Something like:

public async getUrl(): Promise<string> {
  const parsedUrl = this._url; // type: URL, contains all other parameters already done in constructor
  
  const challenge = await this.state.getChallenge();
  if (challenge) {
    parsedUrl.searchParams.append("code_challenge", challenge);
    parsedUrl.searchParams.append("code_challenge_method", "S256");
  }

  return parsedUrl.href;
}

[huysentruitw]

Seems like getUrl is added because of the challenge that needs to be generated async instead, right?

Can we use the existing async-factory method here to generate a SignInRequest instance and move the logic from the SignInRequest constructor to the factory method instead?

[pamapa]

Seems like getUrl is added because of the challenge that needs to be generated async instead, right?

yes

Can we use the existing async-factory method here to generate a SignInRequest instance and move the logic from the SignInRequest constructor to the factory method instead?

sounds great, instead of the async getUrl, we can add a async setUrl, which calculates the url and can be called in createSigninRequest. Access to the url can stay as its is...

[43081j]

@pamapa what do you think of the last commit?

do you wanna do it a different way? i did remove url from the request params since the constructor no longer needed it this way

[pamapa]

@pamapa what do you think of the last commit?

do you wanna do it a different way? i did remove url from the request params since the constructor no longer needed it this way

looks good

[huysentruitw]

I have made some changes in a Draft PR #1196 because it was too much to add as review comments to this PR. Pardon me for that 😊

But in my draft, the changes should be less intrusive, as I just created async factory methods in SigninState & SigninRequest, so we can leave the rest as-is. I've also made the "fromStorageString" async & fixed the tests.

@43081j feel free to cherry pick my latest commit into your PR here.

[43081j]

rebased and done the refactor

[jimmyvdberg-effectory]

Would be lovely if you could move forward with this. There is now also a vulnerability found in crypto-js about using a weak algorithm. More info can be found here: https://security.snyk.io/vuln/SNYK-JS-CRYPTOJS-6028119

[pamapa]

Would be lovely if you could move forward with this. There is now also a vulnerability found in crypto-js about using a weak algorithm. More info can be found here: https://security.snyk.io/vuln/SNYK-JS-CRYPTOJS-6028119

No worries yet, we do not use CryptoJS.PBKDF2. We use only this:

import CryptoJS from "crypto-js/core.js";
import sha256 from "crypto-js/sha256.js";
import Base64 from "crypto-js/enc-base64.js";
import Utf8 from "crypto-js/enc-utf8.js";

But yes its now deprecated (thanks for the hint), it makes sense to proceed.

[pamapa]

@@43081j thanks for the rebase. What's holding this back merging? I guess you synced this with @huysentruitw in #1196. Can we remove the "wip: " in the title? My plan is to final review and merge this next week...

[43081j]

i think all should be fine, it does use async factory methods now as suggested

it'd be good if you could double check the change in public interface (i.e. docs/ changes). i basically introduced a SigninStateArgs instead of having the type inlined in the constructor, since i needed to reuse it at the time

[pamapa]

@43081j Small things:

As we now have create, lets make the constructor private, such that create must be used and lets align the type name to the new create function (SigninRequestArgs -> SigninRequestCreateArgs) so it matches...

diff --git a/src/OidcClient.test.ts b/src/OidcClient.test.ts
index 577d239..aa3d43d 100644
--- a/src/OidcClient.test.ts
+++ b/src/OidcClient.test.ts
@@ -263,15 +263,15 @@ describe("OidcClient", () => {
 
         it("should deserialize stored state and return state and response", async () => {
             // arrange
-            const item = new SigninState({
+            const item = await SigninState.create({
                 id: "1",
                 authority: "authority",
                 client_id: "client",
                 redirect_uri: "http://app/cb",
                 scope: "scope",
                 request_type: "type",
-            }).toStorageString();
-            jest.spyOn(subject.settings.stateStore, "get").mockImplementation(() => Promise.resolve(item));
+            });
+            jest.spyOn(subject.settings.stateStore, "get").mockImplementation(() => Promise.resolve(item.toStorageString()));
 
             // act
             const { state, response } = await subject.readSigninResponseState("http://app/cb?state=1");
@@ -318,7 +318,7 @@ describe("OidcClient", () => {
 
         it("should deserialize stored state and call validator", async () => {
             // arrange
-            const item = new SigninState({
+            const item = await SigninState.create({
                 id: "1",
                 authority: "authority",
                 client_id: "client",
diff --git a/src/OidcClient.ts b/src/OidcClient.ts
index 652a67d..f776950 100644
--- a/src/OidcClient.ts
+++ b/src/OidcClient.ts
@@ -7,7 +7,7 @@ import { type OidcClientSettings, OidcClientSettingsStore } from "./OidcClientSe
 import { ResponseValidator } from "./ResponseValidator";
 import { MetadataService } from "./MetadataService";
 import type { RefreshState } from "./RefreshState";
-import { SigninRequest, type SigninRequestArgs } from "./SigninRequest";
+import { SigninRequest, type SigninRequestCreateArgs } from "./SigninRequest";
 import { SigninResponse } from "./SigninResponse";
 import { SignoutRequest, type SignoutRequestArgs } from "./SignoutRequest";
 import { SignoutResponse } from "./SignoutResponse";
@@ -20,7 +20,7 @@ import { ClaimsService } from "./ClaimsService";
  * @public
  */
 export interface CreateSigninRequestArgs
-    extends Omit<SigninRequestArgs, "url" | "authority" | "client_id" | "redirect_uri" | "response_type" | "scope" | "state_data"> {
+    extends Omit<SigninRequestCreateArgs, "url" | "authority" | "client_id" | "redirect_uri" | "response_type" | "scope" | "state_data"> {
     redirect_uri?: string;
     response_type?: string;
     scope?: string;
diff --git a/src/SigninRequest.test.ts b/src/SigninRequest.test.ts
index 8708e48..a2e779d 100644
--- a/src/SigninRequest.test.ts
+++ b/src/SigninRequest.test.ts
@@ -1,13 +1,13 @@
 // Copyright (c) Brock Allen & Dominick Baier. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See LICENSE in the project root for license information.
 
-import { SigninRequest, type SigninRequestArgs } from "./SigninRequest";
+import { SigninRequest, type SigninRequestCreateArgs } from "./SigninRequest";
 import { URL_STATE_DELIMITER } from "./utils";
 
 describe("SigninRequest", () => {
 
     let subject: SigninRequest;
-    let settings: SigninRequestArgs;
+    let settings: SigninRequestCreateArgs;
 
     beforeEach(async () => {
         settings = {
diff --git a/src/SigninRequest.ts b/src/SigninRequest.ts
index 0b8ebfb..b44bfb5 100644
--- a/src/SigninRequest.ts
+++ b/src/SigninRequest.ts
@@ -8,7 +8,7 @@ import { SigninState } from "./SigninState";
  * @public
  * @see https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest
  */
-export interface SigninRequestArgs {
+export interface SigninRequestCreateArgs {
     // mandatory
     url: string;
     authority: string;
@@ -54,7 +54,7 @@ export class SigninRequest {
     public readonly url: string;
     public readonly state: SigninState;
 
-    public constructor(args: {
+    private constructor(args: {
         url: string;
         state: SigninState;
     }) {
@@ -73,7 +73,7 @@ export class SigninRequest {
         extraTokenParams,
         disablePKCE,
         ...optionalParams
-    }: SigninRequestArgs): Promise<SigninRequest> {
+    }: SigninRequestCreateArgs): Promise<SigninRequest> {
         if (!url) {
             this._logger.error("create: No url passed");
             throw new Error("url");
diff --git a/src/SigninState.ts b/src/SigninState.ts
index 57c0d80..a53be0e 100644
--- a/src/SigninState.ts
+++ b/src/SigninState.ts
@@ -57,7 +57,7 @@ export class SigninState extends State {
 
     public readonly skipUserInfo: boolean | undefined;
 
-    public constructor(args: SigninStateArgs) {
+    private constructor(args: SigninStateArgs) {
         super(args);
 
         this.code_verifier = args.code_verifier;
diff --git a/src/index.ts b/src/index.ts
index 322bcaa..387328c 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -19,7 +19,7 @@ export type { OidcClientSettings, SigningKey, ExtraHeader } from "./OidcClientSe
 export type { OidcMetadata } from "./OidcMetadata";
 export { SessionMonitor } from "./SessionMonitor";
 export type { SessionStatus } from "./SessionStatus";
-export type { SigninRequest, SigninRequestArgs } from "./SigninRequest";
+export type { SigninRequest, SigninRequestCreateArgs } from "./SigninRequest";
 export type { RefreshState } from "./RefreshState";
 export { SigninResponse } from "./SigninResponse";
 export { SigninState } from "./SigninState";

Would be nice if you could apply this patch on-top/into to your commit...

[43081j]

that should be done now @pamapa

[pamapa]

@43081j and @huysentruitw awesome thanks for contributing and taking care!