fix: do not consume request before calling onUnhandledRequest (#153)

* test(node-middleware): do not consume `request` before calling `onUnhandledRequest`

* fix: do not consume `request` before calling `onUnhandledRequest`

src/middleware/node/middleware.ts

@@ -11,6 +11,25 @@ export async function middleware(
  request: IncomingMessage,
  response: ServerResponse
 ) {
+ // request.url mayb include ?query parameters which we don't want for `route`
+ // hence the workaround using new URL()
+ const { pathname } = new URL(request.url as string, "http://localhost");
+ const route = [request.method, pathname].join(" ");
+ const routes = {
+ getLogin: `GET ${options.pathPrefix}/login`,
+ getCallback: `GET ${options.pathPrefix}/callback`,
+ createToken: `POST ${options.pathPrefix}/token`,
+ getToken: `GET ${options.pathPrefix}/token`,
+ patchToken: `PATCH ${options.pathPrefix}/token`,
+ deleteToken: `DELETE ${options.pathPrefix}/token`,
+ deleteGrant: `DELETE ${options.pathPrefix}/grant`,
+ };
+
+ if (!Object.values(routes).includes(route)) {
+ options.onUnhandledRequest(request, response);
+ return;
+ }
+
  let parsedRequest;
  try {
  parsedRequest = await parseRequest(request);
@@ -24,10 +43,10 @@ export async function middleware(
  })
  );
  }
- const { route, headers, query, body } = parsedRequest;
+ const { headers, query, body } = parsedRequest;
 
  try {
- if (route === `GET ${options.pathPrefix}/login`) {
+ if (route === routes.getLogin) {
  const url = app.getAuthorizationUrl({
  state: query.state,
  scopes: query.scopes?.split(","),
@@ -39,7 +58,7 @@ export async function middleware(
  return response.end();
  }
 
- if (route === `GET ${options.pathPrefix}/callback`) {
+ if (route === routes.getCallback) {
  if (query.error) {
  throw new Error(
  `[@octokit/oauth-app] ${query.error} ${query.error_description}`
@@ -66,7 +85,7 @@ export async function middleware(
  return response.end();
  }
 
- if (route === `POST ${options.pathPrefix}/token`) {
+ if (route === routes.createToken) {
  // @ts-ignore body is guaraenteed to exist
  const { state: oauthState, code, redirectUrl } = body;
 
@@ -88,7 +107,7 @@ export async function middleware(
  return response.end(JSON.stringify({ token, scopes }));
  }
 
- if (route === `GET ${options.pathPrefix}/token`) {
+ if (route === routes.getToken) {
  const token = headers.authorization?.substr("token ".length);
 
  if (!token) {
@@ -107,7 +126,7 @@ export async function middleware(
  return response.end(JSON.stringify(result));
  }
 
- if (route === `PATCH ${options.pathPrefix}/token`) {
+ if (route === routes.patchToken) {
  const token = headers.authorization?.substr("token ".length);
 
  if (!token) {
@@ -126,7 +145,7 @@ export async function middleware(
  return response.end(JSON.stringify(result));
  }
 
- if (route === `DELETE ${options.pathPrefix}/token`) {
+ if (route === routes.deleteToken) {
  const token = headers.authorization?.substr("token ".length) as string;
 
  if (!token) {
@@ -143,24 +162,21 @@ export async function middleware(
  return response.end();
  }
 
- if (route === `DELETE ${options.pathPrefix}/grant`) {
- const token = headers.authorization?.substr("token ".length) as string;
-
- if (!token) {
- throw new Error(
- '[@octokit/oauth-app] "Authorization" header is required'
- );
- }
-
- await app.deleteAuthorization({
- token,
- });
+ // route === routes.deleteGrant
+ const token = headers.authorization?.substr("token ".length) as string;
 
- response.writeHead(204);
- return response.end();
+ if (!token) {
+ throw new Error(
+ '[@octokit/oauth-app] "Authorization" header is required'
+ );
  }
 
- options.onUnhandledRequest(request, response);
+ await app.deleteAuthorization({
+ token,
+ });
+
+ response.writeHead(204);
+ return response.end();
  } catch (error) {
  response.writeHead(400, {
  "content-type": "application/json",

src/middleware/node/parse-request.ts

@@ -4,7 +4,6 @@ import { IncomingMessage } from "http";
 import fromEntries from "fromentries";
 
 type ParsedRequest = {
- route: string;
  headers: {
  authorization?: string;
  };
@@ -28,17 +27,13 @@ type ParsedRequest = {
 export async function parseRequest(
  request: IncomingMessage
 ): Promise<ParsedRequest> {
- const { pathname, searchParams } = new URL(
- request.url as string,
- "http://localhost"
- );
- const route = [request.method, pathname].join(" ");
+ const { searchParams } = new URL(request.url as string, "http://localhost");
 
  const query = fromEntries(searchParams);
  const headers = request.headers;
 
  if (!["POST", "PATCH"].includes(request.method as string)) {
- return { route, headers, query };
+ return { headers, query };
  }
 
  return new Promise((resolve, reject) => {
@@ -48,10 +43,10 @@ export async function parseRequest(
  .on("data", (chunk) => bodyChunks.push(chunk))
  .on("end", async () => {
  const bodyString = Buffer.concat(bodyChunks).toString();
- if (!bodyString) return resolve({ route, headers, query });
+ if (!bodyString) return resolve({ headers, query });
 
  try {
- resolve({ route, headers, query, body: JSON.parse(bodyString) });
+ resolve({ headers, query, body: JSON.parse(bodyString) });
  } catch (error) {
  reject(error);
  }

test/node-middleware.test.ts

@@ -1,4 +1,4 @@
-import { createServer } from "http";
+import { createServer, IncomingMessage, ServerResponse } from "http";
 import { URL } from "url";
 
 import fetch from "node-fetch";
@@ -299,6 +299,47 @@ describe("getNodeMiddleware(app)", () => {
  });
  });
 
+ it("POST /unrelated", async () => {
+ expect.assertions(4);
+
+ const app = new OAuthApp({
+ clientId: "0123",
+ clientSecret: "0123secret",
+ });
+
+ const server = createServer(
+ getNodeMiddleware(app, {
+ onUnhandledRequest: (request, response) => {
+ expect(request.method).toEqual("POST");
+ expect(request.url).toEqual("/unrelated");
+
+ // test that the request has not yet been consumed with .on("data")
+ expect(request.complete).toEqual(false);
+
+ response.writeHead(200);
+ response.end();
+ },
+ })
+ ).listen();
+ // @ts-ignore complains about { port } although it's included in returned AddressInfo interface
+ const { port } = server.address();
+
+ const { status, headers } = await fetch(
+ `http://localhost:${port}/unrelated`,
+ {
+ method: "POST",
+ body: JSON.stringify({ ok: true }),
+ headers: {
+ "content-type": "application/json",
+ },
+ }
+ );
+
+ server.close();
+
+ expect(status).toEqual(200);
+ });
+
  // errors
 
  it("GET /unknown", async () => {