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.

[jankapunkt]

@FlorianB-DE please also take a look at these tests, using the "native" Request signature which seems to be working when passing down as-is.

[FlorianB-DE]

Sorry to disagree but I've not only encountered the problem myself but you can easily verify the issue yourself. If you want to, you can look at the native Request Documentation on MDN and try to search "query". You will see that "query" is not a field in the Request class. So you can't instanciate a Request into node-oauth Request like:

import {Request as Oauth2Request} from "node-oauth2-server";

const nativeRequest = new Request(/* some parameters */);
const oauthRequest = new Oauth2Request(nativeRequest);

Since this fails, due to the query parameter missing, I can imagine that some workflows might work but maybe not all. Or is the query parameter not used? I'm not very deep into the inner workings of the library but there should be a reason this is required in the first place, right?

[jankapunkt]

@FlorianB-DE yes, query is used in various places to extract access_token, scope, state or redirect_uri, depending on the workflow position.

Since this is a server-side lib, I wonder how you actually pass down the "native" request to the library. Can you please show me a short snippet of your request handler on the server?

[FlorianB-DE]

E.g. Sveltekit uses Fetch API standards for Server-Side request handling: https://svelte.dev/docs/kit/web-standards#Fetch-APIs-Request

[jankapunkt]

@FlorianB-DE I get that but from what I see you should rather write an adapter for it, instead of passing it raw to the application. This is what the library is designed for. I did not realize this when you opened the PR but the correct way to support svelte kit right now is to write an adapter, specific to the framework.

You can get inspiration by the express adapter: https://github.com/node-oauth/express-oauth-server

There you can parse the request to construct a request that fits the OAuth2Server API.

One could argue that fetch is native and standard, however it's just recently standard in node and we aim for backwards-compatibility across major versions. Apologies again for not realizing this in the first place.

If you think this makes sense and intend to write an adapter for svelte kit then we can also write it in a new repo in this org to let others benefit from it, too. let me know if this is considerable to you.

[KeithGillette]

@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

I revisited this and found the following oddity.

This is an excerpt of our implementation that broke with 5.2.1:

import OAuth2Server, {
	AuthorizationCode,
	AuthorizationCodeModel,
	AuthorizeOptions,
	Client,
	Falsey,
	RefreshToken,
	RefreshTokenModel,
	Request as OAuthRequest,
	Response as OAuthResponse,
	Token,
	User as OAuthUser,
} from '@node-oauth/oauth2-server';
import { Request as ExpressRequest, Response as ExpressResponse } from 'express';

// ...
	public authenticateRouteMiddleware(request: ExpressRequest, response: ExpressResponse, next: () => void): void {
		const oAuthRequest = new OAuthRequest({
			headers: { authorization: request.headers.authorization },
			method: request.method,
			query: request.query,
			body: request.body,
		});
// ...

with the following error on the query property of the argument:

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

However, the following simplified implementation works fine:

// ...
	public authenticateRouteMiddleware(request: ExpressRequest, response: ExpressResponse, next: () => void): void {
		const oAuthRequest = new OAuthRequest(request);
// ...