The oauth2-server.Request is not compatible to Fetch API Request

[FlorianB-DE]

Specify your setup

• Operating System: Linux Debian 12 Buster X86_64
• Node version: Bun 1.2.15
• npm version: Bun 1.2.15
• version of @node-oauth/oauth2-server: 5.2.0

Describe the bug

The oauth2-server.Request is not compatible to Fetch API Request, "Missing parameter: query"

To Reproduce

Steps to reproduce the behavior:

1. use a framework like SvelteKit that uses Fetch API Requests
2. build a request handler that passes down Request to oauth2server
3. See error:

invalid_argument: Missing parameter: `query`
 statusCode: 500,
     status: 500,
       code: 500,

      at new OAuthError (/app/node_modules/@node-oauth/oauth2-server/lib/errors/oauth-error.js:13:5)
      at new InvalidArgumentError (/app/node_modules/@node-oauth/oauth2-server/lib/errors/invalid-argument-error.js:21:5)
      at new Request (/app/node_modules/@node-oauth/oauth2-server/lib/request.js:21:13)

Expected behavior

Try to get query via request.url (e.g.: new URL(request.url).searchParams

Screenshots

Additional context
In the examples (e.g. express server) requests are passed down to the oauth server. While express is probably the most used framework, I don't see why the default Request shouldn't be supported.

[FlorianB-DE]

Also note to that:
The type declarations state:

headers?: Record<string, string>;
method?: string;
query?: Record<string, string>;

Which is wrong use of typescript since it leads to a runtime Error which is bypassing typescripts checks. Correct would be to omit all of the questions marks.

[jankapunkt]

@FlorianB-DE released in 5.2.1. Closing this, feel free to reopen if necessary

[FlorianB-DE]

Well I mean it's still not compatible but at least now the typescript type checker also knows this :D

[jankapunkt]

I reopened. Let me check what's possible without hard breaking things.

[netizen01]

Weighing in here...

The headers and query properties need to accept string arrays as well.

headers?: Record<string, string | string[]>;
query?: Record<string, string | string[]>;

Please amend your pull request appropriately...

[KeithGillette]

I'm not 100% sure this is relevant, but I'm experiencing the following error attempting to update to 5.2.1 from 5.2.0 passing an Express 5 ExpressRequest.query as the query property in a new OAuthRequest argument object.

error TS2322: Type 'ParsedQs' is not assignable to type 'Record<string, string>'.
  'string' index signatures are incompatible.
    Type 'string | ParsedQs | (string | ParsedQs)[]' is not assignable to type 'string'.
      Type 'ParsedQs' is not assignable to type 'string'.

Express5 ReqQuery = ParsedQs:

    interface ParsedQs {
        [key: string]: undefined | string | ParsedQs | (string | ParsedQs)[];
    }

[netizen01]

I'm not 100% sure this is relevant, but I'm experiencing the following error attempting to update to 5.2.1 from 5.2.0 passing an Express 5 ExpressRequest.query as the query property in a new OAuthRequest argument object.

error TS2322: Type 'ParsedQs' is not assignable to type 'Record<string, string>'.
  'string' index signatures are incompatible.
    Type 'string | ParsedQs | (string | ParsedQs)[]' is not assignable to type 'string'.
      Type 'ParsedQs' is not assignable to type 'string'.

Express5 ReqQuery = ParsedQs:

interface ParsedQs {
    [key: string]: undefined | string | ParsedQs | (string | ParsedQs)[];
}

This is the same issue that I have.

Releasing 5.2.1 with typing the headers and query parameters has broken a lot of compile time checks due to slight variations in different middleware's typing of those same properties.

This update should probably be rolled back or thoughtfully considered as to how it can be changed to accommodate the numerous scaffolding projects out there (Express, Koa, ....)

[jankapunkt]

Thank you guys for reporting this. Is there a way to incorporate these kinds of checks into the test suite for example using tsc?

[FlorianB-DE]

You can probably use tsc but I guess there is too much variation to cover all possible use cases no?
You can also union it with unknown aka Record<string, string | unknown>

[jankapunkt]

@netizen01 @KeithGillette I am currently workin on this to make it work with express request but also fetch api. I wonder why the express request is not covered by http.IncomingMessage as express request is a direct instance of its prototype.

var req = Object.create(http.IncomingMessage.prototype)

any input on this would be much appreciated.

// cc @dhensby

[jankapunkt]

Dear all, I created a new PR that uses tsd to explicitly write tests for types compatibility and to avoid regressions like this one in the future!

I think this is a necessary step towards a full typescript codebase in the long run. Please also use it as a basis for further discussion on how we can solve this issue.