[TEX-537] Remove "unframing gRPC-web+json request" functionality (#567)

Removes the functionality to unframe gRPC-web+json requests (added in #501 and #503) in favour of the proper solution to handle grpc-web requests in full (added in #548). This functionality didn't work quite as expected anyway, so it's good to clean it up in favour of the the full, proper working solution.
airtasker · Jan 5, 2024 · 83d9ebb · 83d9ebb
1 parent ec48c83
commit 83d9ebb
Show file tree

Hide file tree

Showing 5 changed files with 1 addition and 181 deletions.
diff --git a/src/cli.ts b/src/cli.ts
@@ -82,10 +82,6 @@ async function main(argv: string[]) {
       "--https-ca <filename.pem>",
       "Enable HTTPS server where the certificate was generated by this CA. Useful if you are using a self-signed certificate. Also requires --https-key and --https-cert.",
     )
-    .option(
-      "--unframe-grpc-web-json-requests-hostname [hostname...]",
-      "Rewrite received requests whose content-type is application/grpc-web+json to be application/json, mutating the body of the request accordingly. This is useful if you want plain text tape records rather than binary data. The gRPC server needs to support receiving unframed requests for this option to be useful. To accept all host headers, use `*`, otherwise specifiy one or more host header values to whitelist.",
-    )
     .option<RewriteRules>(
       "--rewrite-before-diff [s/find/replace/g...]",
       "Provide regex-based rewrite rules over strings before passing them to the diffing algorithm. The regex rules use sed-style syntax. s/find/replace/ with an optional regex modifier suffixes. Capture groups can be used using sed-style \\N syntax. This is only used during replaying existing tapes.",
@@ -106,8 +102,6 @@ async function main(argv: string[]) {
   const httpsCA: string = options.httpsCa || "";
   const httpsKey: string = options.httpsKey;
   const httpsCert: string = options.httpsCert;
-  const unframeGrpcWebJsonRequestsHostnames: string[] =
-    options.unframeGrpcWebJsonRequestsHostname;
   const rewriteBeforeDiffRules: RewriteRules = options.rewriteBeforeDiff;
   const exactRequestMatching: boolean =
     options.exactRequestMatching === undefined
@@ -163,7 +157,6 @@ async function main(argv: string[]) {
     httpsCA,
     httpsKey,
     httpsCert,
-    unframeGrpcWebJsonRequestsHostnames,
     rewriteBeforeDiffRules,
     exactRequestMatching,
   });

diff --git a/src/server.ts b/src/server.ts
@@ -29,7 +29,6 @@ export class RecordReplayServer {
   private defaultTape: string;
   private replayedTapes: Set<TapeRecord> = new Set();
   private preventConditionalRequests?: boolean;
-  private unframeGrpcWebJsonRequestsHostnames: string[];
   private rewriteBeforeDiffRules: RewriteRules;
   private exactRequestMatching: boolean;
 
@@ -45,7 +44,6 @@ export class RecordReplayServer {
     httpsCA?: string;
     httpsKey?: string;
     httpsCert?: string;
-    unframeGrpcWebJsonRequestsHostnames?: string[];
     rewriteBeforeDiffRules?: RewriteRules;
     exactRequestMatching?: boolean;
   }) {
@@ -58,8 +56,6 @@ export class RecordReplayServer {
     this.persistence = new Persistence(options.tapeDir, redactHeaders);
     this.defaultTape = options.defaultTapeName;
     this.preventConditionalRequests = options.preventConditionalRequests;
-    this.unframeGrpcWebJsonRequestsHostnames =
-      options.unframeGrpcWebJsonRequestsHostnames || [];
     this.rewriteBeforeDiffRules =
       options.rewriteBeforeDiffRules || new RewriteRules();
     this.exactRequestMatching =
@@ -255,9 +251,6 @@ export class RecordReplayServer {
    * Potentially rewrite the request before processing it.
    */
   private rewriteRequest(request: HttpRequest) {
-    // Grab the `host` header of the request.
-    const hostname = (request.headers.host || null) as string | null;
-
     // Potentially prevent 304 responses from being able to be generated.
     if (
       this.preventConditionalRequests &&
@@ -267,58 +260,6 @@ export class RecordReplayServer {
       delete request.headers["if-modified-since"];
       delete request.headers["if-none-match"];
     }
-
-    // Potentially unframe a grpc-web+json request.
-    if (
-      request.method === "POST" &&
-      request.headers["content-type"] === "application/grpc-web+json" &&
-      (this.unframeGrpcWebJsonRequestsHostnames.includes("*") ||
-        (hostname != null &&
-          this.unframeGrpcWebJsonRequestsHostnames.includes(hostname)))
-    ) {
-      this.rewriteGrpcWebJsonRequest(request);
-    }
-  }
-
-  /**
-   * Rewrite a gRPC-web+json request to be unframed.
-   */
-  private rewriteGrpcWebJsonRequest(request: HttpRequest) {
-    /**
-     * From the gRPC specification (https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md)
-     *
-     * The repeated sequence of Length-Prefixed-Message items is delivered in DATA frames:
-     *   Length-Prefixed-Message → Compressed-Flag Message-Length Message
-     *   Compressed-Flag → 0 / 1 # encoded as 1 byte unsigned integer
-     *   Message-Length → {length of Message} # encoded as 4 byte unsigned integer (big endian)
-     *   Message → *{binary octet}
-     */
-    const compressionFlag = request.body.readUInt8(0);
-    const messageLength = request.body.readUInt32BE(1);
-    if (compressionFlag !== 0) {
-      console.error(
-        `The gRPC-web compression flag was set to 1. Do not know how to handle compressed request paylods. Aborting gRPC-web+json rewrite.`,
-      );
-      return;
-    }
-
-    // Sanity check the content length.
-    const rawContentLength = request.headers["content-length"] as
-      | string
-      | undefined;
-    if (rawContentLength !== undefined) {
-      const contentLength = parseInt(rawContentLength, 10);
-      if (contentLength !== messageLength + 5) {
-        console.log(
-          `Unexpected content length. Header says "${rawContentLength}". gRPC payload length preamble says "${messageLength}".`,
-        );
-      }
-    }
-
-    // Rewrite the request to be unframed.
-    request.headers["content-length"] = messageLength.toString();
-    request.headers["content-type"] = "application/json";
-    request.body = request.body.subarray(5);
   }
 
   private async fetchResponse(

diff --git a/src/tests/passthrough.spec.ts b/src/tests/passthrough.spec.ts
@@ -1,10 +1,9 @@
 import axios from "axios";
-import { PROXAY_HOST, TEST_SERVER_PORT } from "./config";
+import { PROXAY_HOST } from "./config";
 import { setupServers } from "./setup";
 import {
   BINARY_PATH,
   BINARY_RESPONSE,
-  GRPC_WEB_JSON_PATH,
   SIMPLE_TEXT_PATH,
   SIMPLE_TEXT_RESPONSE,
   UTF8_PATH,
@@ -37,107 +36,3 @@ describe("Passthrough", () => {
     });
   });
 });
-
-describe("Passthrough with grpc-web+json unframing with explicit whitelisted hostname", () => {
-  setupServers({
-    mode: "passthrough",
-    unframeGrpcWebJsonRequestsHostnames: [`localhost:${TEST_SERVER_PORT}`],
-  });
-
-  test("response: simple text", async () => {
-    const response = await axios.get(`${PROXAY_HOST}${SIMPLE_TEXT_PATH}`);
-    expect(response.data).toBe(SIMPLE_TEXT_RESPONSE);
-  });
-
-  test("response: utf-8", async () => {
-    const response = await axios.get(`${PROXAY_HOST}${UTF8_PATH}`);
-    expect(response.data).toBe(UTF8_RESPONSE);
-  });
-
-  test("response: binary", async () => {
-    const response = await axios.get(`${PROXAY_HOST}${BINARY_PATH}`, {
-      responseType: "arraybuffer",
-    });
-    expect(response.data).toEqual(BINARY_RESPONSE);
-  });
-
-  test("can pick any tape", async () => {
-    await axios.post(`${PROXAY_HOST}/__proxay/tape`, {
-      tape: "new-tape",
-    });
-  });
-
-  test("unframes a grpc-web+json request", async () => {
-    const requestBody = Buffer.from([
-      0, 0, 0, 0, 31, 123, 34, 101, 109, 97, 105, 108, 34, 58, 34, 102, 111,
-      111, 46, 98, 97, 114, 64, 101, 120, 97, 109, 112, 108, 101, 46, 99, 111,
-      109, 34, 125,
-    ]);
-    const response = await axios.post(
-      `${PROXAY_HOST}${GRPC_WEB_JSON_PATH}`,
-      requestBody,
-      {
-        headers: {
-          "content-type": "application/grpc-web+json",
-          host: `localhost:${TEST_SERVER_PORT}`,
-        },
-      },
-    );
-    expect(response.headers["content-type"]).toBe(
-      "application/json; charset=utf-8",
-    );
-    expect(response.data).toEqual({ email: "foo.bar@example.com" });
-  });
-});
-
-describe("Passthrough with grpc-web+json unframing with wildcard whitelisted hostname", () => {
-  setupServers({
-    mode: "passthrough",
-    unframeGrpcWebJsonRequestsHostnames: ["*"],
-  });
-
-  test("response: simple text", async () => {
-    const response = await axios.get(`${PROXAY_HOST}${SIMPLE_TEXT_PATH}`);
-    expect(response.data).toBe(SIMPLE_TEXT_RESPONSE);
-  });
-
-  test("response: utf-8", async () => {
-    const response = await axios.get(`${PROXAY_HOST}${UTF8_PATH}`);
-    expect(response.data).toBe(UTF8_RESPONSE);
-  });
-
-  test("response: binary", async () => {
-    const response = await axios.get(`${PROXAY_HOST}${BINARY_PATH}`, {
-      responseType: "arraybuffer",
-    });
-    expect(response.data).toEqual(BINARY_RESPONSE);
-  });
-
-  test("can pick any tape", async () => {
-    await axios.post(`${PROXAY_HOST}/__proxay/tape`, {
-      tape: "new-tape",
-    });
-  });
-
-  test("unframes a grpc-web+json request", async () => {
-    const requestBody = Buffer.from([
-      0, 0, 0, 0, 31, 123, 34, 101, 109, 97, 105, 108, 34, 58, 34, 102, 111,
-      111, 46, 98, 97, 114, 64, 101, 120, 97, 109, 112, 108, 101, 46, 99, 111,
-      109, 34, 125,
-    ]);
-    const response = await axios.post(
-      `${PROXAY_HOST}${GRPC_WEB_JSON_PATH}`,
-      requestBody,
-      {
-        headers: {
-          "content-type": "application/grpc-web+json",
-          host: `localhost:${TEST_SERVER_PORT}`,
-        },
-      },
-    );
-    expect(response.headers["content-type"]).toBe(
-      "application/json; charset=utf-8",
-    );
-    expect(response.data).toEqual({ email: "foo.bar@example.com" });
-  });
-});
diff --git a/src/tests/setup.ts b/src/tests/setup.ts
@@ -9,13 +9,11 @@ export function setupServers({
   mode,
   tapeDirName = mode,
   defaultTapeName = "default",
-  unframeGrpcWebJsonRequestsHostnames,
   exactRequestMatching,
 }: {
   mode: Mode;
   tapeDirName?: string;
   defaultTapeName?: string;
-  unframeGrpcWebJsonRequestsHostnames?: string[];
   exactRequestMatching?: boolean;
 }) {
   const tapeDir = path.join(__dirname, "tapes", tapeDirName);
@@ -40,7 +38,6 @@ export function setupServers({
       host: TEST_SERVER_HOST,
       timeout: 100,
       enableLogging: true,
-      unframeGrpcWebJsonRequestsHostnames,
       exactRequestMatching,
     });
     await Promise.all([

diff --git a/src/tests/testserver.ts b/src/tests/testserver.ts
@@ -15,8 +15,6 @@ export const BINARY_RESPONSE = Buffer.from([
   12, 48, 249, 104, 255, 33, 203, 179,
 ]);
 
-export const GRPC_WEB_JSON_PATH = "/grpc-web-json";
-
 /**
  * A test server used as a fake backend.
  */
@@ -42,10 +40,6 @@ export class TestServer {
     this.app.get(BINARY_PATH, (_req, res) => {
       res.send(BINARY_RESPONSE);
     });
-    this.app.post(GRPC_WEB_JSON_PATH, (req, res) => {
-      res.setHeader("content-type", req.headers["content-type"] as string);
-      res.send(req.body);
-    });
     this.app.get(JSON_IDENTITY_PATH, (req, res) => {
       res.json({ data: req.path, requestCount: this.requestCount });
     });