Repository: octokit/oauth-app.js
Pull request #153 (2020-11-02 23:40:05)

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


Patch:
diff --git a/src/middleware/node/middleware.ts b/src/middleware/node/middleware.ts
index b439126..f7494aa 100644
--- a/src/middleware/node/middleware.ts
+++ b/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",
diff --git a/src/middleware/node/parse-request.ts b/src/middleware/node/parse-request.ts
index 7baa03b..0fad90c 100644
--- a/src/middleware/node/parse-request.ts
+++ b/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 patch:
diff --git a/test/node-middleware.test.ts b/test/node-middleware.test.ts
index 34bf1a5..4f86edb 100644
--- a/test/node-middleware.test.ts
+++ b/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 () => {