Upgrade @oslojs/oauth2 dependency
#108
There are no files selected for viewing
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,16 +3,8 @@ | |
| "version": "2.1.0", | ||
| "description": "A strategy to use and implement OAuth2 framework for authentication with federated services like Google, Facebook, GitHub, etc.", | ||
| "license": "MIT", | ||
| "funding": ["https://github.com/sponsors/sergiodxa"], | ||
| "keywords": ["remix", "remix-auth", "auth", "authentication", "strategy"], | ||
| "author": { | ||
| "name": "Sergio Xalambrí", | ||
| "email": "[email protected]", | ||
|
|
@@ -31,23 +23,22 @@ | |
| "typecheck": "tsc --noEmit", | ||
| "quality": "biome check .", | ||
| "quality:fix": "biome check . --apply-unsafe", | ||
| "exports": "bun run ./scripts/exports.ts", | ||
| "unused": "knip" | ||
| }, | ||
| "sideEffects": false, | ||
| "type": "module", | ||
| "engines": { | ||
| "node": "^18.0.0 || ^20.0.0 || >=20.0.0" | ||
| }, | ||
| "files": ["build", "package.json", "README.md"], | ||
| "exports": { | ||
| ".": "./build/index.js", | ||
| "./package.json": "./package.json" | ||
| }, | ||
| "dependencies": { | ||
| "@oslojs/crypto": "^0.6.2", | ||
| "@oslojs/encoding": "^0.4.1", | ||
| "@oslojs/oauth2": "^0.5.0", | ||
| "debug": "^4.3.4" | ||
| }, | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| /** | ||
| * A lot of the code here was originally implemented by @pilcrowOnPaper for a | ||
| * previous version of `@oslojs/oauth2`, as Pilcrow decided to change the | ||
| * direction of the library to focus on response parsing, I decided to copy the | ||
| * old code and adapt it to the new structure of the library. | ||
| */ | ||
| import { sha256 } from "@oslojs/crypto/sha2"; | ||
| import { encodeBase64urlNoPadding } from "@oslojs/encoding"; | ||
|
|
||
| export namespace AuthorizationCode { | ||
| export class AuthorizationURL extends URL { | ||
| constructor(authorizationEndpoint: string, clientId: string) { | ||
| super(authorizationEndpoint); | ||
| this.searchParams.set("response_type", "code"); | ||
| this.searchParams.set("client_id", clientId); | ||
| } | ||
|
|
||
| public setRedirectURI(redirectURI: string): void { | ||
| this.searchParams.set("redirect_uri", redirectURI); | ||
| } | ||
|
|
||
| public addScopes(...scopes: string[]): void { | ||
| if (scopes.length < 1) { | ||
| return; | ||
| } | ||
| let scopeValue = scopes.join(" "); | ||
| const existingScopes = this.searchParams.get("scope"); | ||
| if (existingScopes !== null) scopeValue = ` ${existingScopes}`; | ||
| this.searchParams.set("scope", scopeValue); | ||
| } | ||
|
|
||
| public setState(state: string): void { | ||
| this.searchParams.set("state", state); | ||
| } | ||
|
|
||
| public setS256CodeChallenge(codeVerifier: string): void { | ||
| const codeChallengeBytes = sha256(new TextEncoder().encode(codeVerifier)); | ||
| const codeChallenge = encodeBase64urlNoPadding(codeChallengeBytes); | ||
|
There was a problem hiding this comment. I think My understanding is that There was a problem hiding this comment. I've made a fork of oslojs/encoding to check the compatibility between Executing this locally with both It seems that using Buffer could indeed be faster or at least be of similar speed. At the same time this could allow to remove the dependency on @oslojs/encoding. There was a problem hiding this comment. The effect here becomes even more pronounced when I increase the string sizes: import { bench, expect } from "vitest";
import { encodeBase64urlNoPadding } from "./base64.js";
const iterations = 1_000;
const from = 4_000;
const to = from + 100;
bench(
"encodeBase64urlNoPadding: Oslo",
() => {
for (let i = from; i <= to; i++) {
const bytes = new Uint8Array(i);
crypto.getRandomValues(bytes);
const oslo = encodeBase64urlNoPadding(bytes);
expect(oslo).not.toEqual("");
}
},
{ iterations }
);
bench(
"encodeBase64urlNoPadding: Node",
() => {
for (let i = from; i <= to; i++) {
const bytes = new Uint8Array(i);
crypto.getRandomValues(bytes);
const node = Buffer.from(bytes).toString("base64url");
expect(node).not.toEqual("");
}
},
{ iterations }
);
I've looked into the code of node a bit and found that indeed the buffer encoding ends up in native code and from what I could gather leads somewhere to There was a problem hiding this comment. It seems to me that remix itself makes use of Buffers in some places. An example would be Personally I'd be a fan of ditching the There was a problem hiding this comment. Buffer doesn't exists on Cloudflare, without Remix's There was a problem hiding this comment. I see - this wasn't on my radar - thanks 🙂 |
||
| this.searchParams.set("code_challenge", codeChallenge); | ||
| this.searchParams.set("code_challenge_method", "S256"); | ||
| } | ||
|
|
||
| public setPlainCodeChallenge(codeVerifier: string): void { | ||
| this.searchParams.set("code_challenge", codeVerifier); | ||
| this.searchParams.set("code_challenge_method", "plain"); | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| /** | ||
| * A lot of the code here was originally implemented by @pilcrowOnPaper for a | ||
| * previous version of `@oslojs/oauth2`, as Pilcrow decided to change the | ||
| * direction of the library to focus on response parsing, I decided to copy the | ||
| * old code and adapt it to the new structure of the library. | ||
| */ | ||
| import { encodeBase64urlNoPadding } from "@oslojs/encoding"; | ||
|
|
||
| export namespace Generator { | ||
| export function codeVerifier(): string { | ||
| const randomValues = new Uint8Array(32); | ||
| crypto.getRandomValues(randomValues); | ||
| return encodeBase64urlNoPadding(randomValues); | ||
| } | ||
|
|
||
| export function state(): string { | ||
| const randomValues = new Uint8Array(32); | ||
| crypto.getRandomValues(randomValues); | ||
| return encodeBase64urlNoPadding(randomValues); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| /** | ||
| * A lot of the code here was originally implemented by @pilcrowOnPaper for a | ||
| * previous version of `@oslojs/oauth2`, as Pilcrow decided to change the | ||
| * direction of the library to focus on response parsing, I decided to copy the | ||
| * old code and adapt it to the new structure of the library. | ||
| */ | ||
| import { encodeBase64 } from "@oslojs/encoding"; | ||
|
|
||
| export namespace OAuth2Request { | ||
| export abstract class Context { | ||
| public method: string; | ||
| public body = new URLSearchParams(); | ||
| public headers = new Headers(); | ||
|
|
||
| constructor(method: string) { | ||
| this.method = method; | ||
| this.headers.set("Content-Type", "application/x-www-form-urlencoded"); | ||
| this.headers.set("Accept", "application/json"); | ||
| this.headers.set("User-Agent", "oslo"); | ||
| } | ||
|
|
||
| public setClientId(clientId: string): void { | ||
| this.body.set("client_id", clientId); | ||
| } | ||
|
|
||
| public authenticateWithRequestBody( | ||
| clientId: string, | ||
| clientSecret: string, | ||
| ): void { | ||
| this.setClientId(clientId); | ||
| this.body.set("client_secret", clientSecret); | ||
| } | ||
|
|
||
| public authenticateWithHTTPBasicAuth( | ||
| clientId: string, | ||
| clientSecret: string, | ||
| ): void { | ||
| const authorizationHeader = `Basic ${encodeBase64( | ||
| new TextEncoder().encode(`${clientId}:${clientSecret}`), | ||
| )}`; | ||
| this.headers.set("Authorization", authorizationHeader); | ||
| } | ||
|
|
||
| toRequest(url: ConstructorParameters<URL>["0"]) { | ||
| return new Request(url, { | ||
| method: this.method, | ||
| body: this.body, | ||
| headers: this.headers, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // biome-ignore lint/suspicious/noShadowRestrictedNames: It's namespaced | ||
| export class Error extends globalThis.Error { | ||
| public request: Request; | ||
| public context: OAuth2Request.Context; | ||
| public description: string | null; | ||
| public uri: string | null; | ||
| public responseHeaders: Headers; | ||
|
|
||
| constructor( | ||
| message: string, | ||
| request: Request, | ||
| context: OAuth2Request.Context, | ||
| responseHeaders: Headers, | ||
| options?: { description?: string; uri?: string }, | ||
| ) { | ||
| super(message); | ||
| this.request = request; | ||
| this.context = context; | ||
| this.responseHeaders = responseHeaders; | ||
| this.description = options?.description ?? null; | ||
| this.uri = options?.uri ?? null; | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether this line could instead use the SubtleCrypto digest method. It would allow for the execution of native, possibly more optimised code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm - this surprises me:
I've made a fork of oslojs/crypto to check the compatibility and compare the performance using vitest bench.
To my surprise it appears that the Oslo implementation of sha256 may be faster than the SubtleCrypto one:
It makes me wonder whether this is because of SubtleCrypto being async or something, and I may look into it further - but at least initially it is contrary to my intuition and claim.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm - maybe a weakness of my benchmark setup was to only ever hash 5 bytes.
The results become different when I modify the code to look like this:
So there's certainly a question of input size at play here. At the same time I see a chance for
remix-auth-oauth2to have a dependency less.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into node it seems
crypto.subtle.digestis provided bydigestinlib/internal/crypto/webcrypto.js, which seems to schedule aHashJobdefined insrc/crypto/crypto_hash.h.It makes sense to me that this would be faster than oslojs, but I see a possible question as to the input size we're expecting, too. Given that
Generator.codeVerifieronly provides 32 bytes of randomness we may generally be dealing with shorter sequences of bytes.For 32 bytes of input to the hash function I've still got Oslo ahead in hash timings. Generally hashing seems to be so fast here that it should barely make a difference - and that it shouldn't matter much whether it's done sync or async.
I think I'd have a slight preference for SubtleCrypto on the grounds of using fewer dependencies and using something that's less likely to cause issues if used for bigger inputs while still being okay for smaller input sizes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a suggestion to drop
@oslojs/cryptohere: #109