Expected "Hello World" response body, got "HTTP/1.1 20"

[talentlessguy]

Issue

I get this error when I'm asserting a response body. For some reason it asserts with a raw HTTP response:

res.format(obj) > should send text by default
Error: expected "Hello World" response body, got "HTTP/1.1 20"
    at error (https://deno.land/x/superdeno@4.4.0/src/test.ts:637:15)
    at Test.#assertBody (https://deno.land/x/superdeno@4.4.0/src/test.ts:535:16)
    at Test.#assertFunction (https://deno.land/x/superdeno@4.4.0/src/test.ts:617:13)
    at Test.#assert (https://deno.land/x/superdeno@4.4.0/src/test.ts:480:35)
    at https://deno.land/x/superdeno@4.4.0/src/test.ts:455:23
    at async close (https://deno.land/x/superdeno@4.4.0/src/close.ts:48:46)

failures:

        res.format(obj) > should send text by default

test result: FAILED. 121 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out (4184ms)

Setup:

deno 1.12.0 (release, x86_64-unknown-linux-gnu) v8 9.2.230.14 typescript 4.3.2

• SuperDeno Version: 4.4.0

Details

import { describe, it, run } from 'https://deno.land/x/tincan@0.2.1/mod.ts'
import { App, Handler, AppConstructor, Request, Response } from 'https://deno.land/x/tinyhttp/mod.ts'

const runServer = (h: Handler) => {
  const app = new App()

  app.use(h)

  const request = superdeno(app.attach)

  return request
}

it('should send text by default', async () => {
    const request = runServer((req, res) => {
      formatResponse(req, res, () => {})({
        text: (req: Request) => req.respond({ body: `Hello World` })
      }).end()
    })

    await request.get('/').expect(200).expect('Hello World')
  })

[cmorten]

A curious one 🤔 having fixed up your example ( missing import for formatResponse confused me for a bit! ) can see that the response body to the fetch which SuperDeno makes under the hood is actually the full HTTP response, not just the body as one might expect.

import { App, Handler, Request } from "https://deno.land/x/tinyhttp/mod.ts";
import { formatResponse } from "https://deno.land/x/tinyhttp/extensions/res/format.ts";
import { superdeno } from "./mod.ts";

const runServer = (h: Handler) => {
  const app = new App();

  app.use(h);

  return superdeno(app.attach);
};

Deno.test("should send text by default", async () => {
  const request = runServer((req, res) => {
    formatResponse(req, res, () => {})({
      text: (req: Request) => req.respond({ body: "a".repeat(200) }),
    }).end();
  });

  const response = await request.get("/");

  console.log(response.text);
});

$ deno test -A tinyhttp.test.ts
Check file:///Users/craigmorten/git/asos-craigmorten/superdeno/tinyhttp.test.ts
running 1 test from file:///Users/craigmorten/git/asos-craigmorten/superdeno/tinyhttp.test.ts
test should send text by default ...HTTP/1.1 200 OK
content-length: 0
content-type: text/plain
vary: Accept
x-powered-by: tinyhttp

aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
 ok (49ms)

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out (609ms)

Yet to determine if the issue lies with how superdeno sets up the server, or a bug with tinyhttp itself...

[cmorten]

Extracting the server logic of superdeno out into a file with the above tinyhttp example we get something like...

// deno-lint-ignore-file
import { serve } from "https://deno.land/std@0.101.0/http/server.ts";
import { App, Handler, Request } from "https://deno.land/x/tinyhttp/mod.ts";
import { formatResponse } from "https://deno.land/x/tinyhttp/extensions/res/format.ts";

const server = serve({ port: 0 });

console.log(server.listener.addr);

const handler: Handler = (req, res) => {
  formatResponse(req, res, () => {})({
    text: (req: Request) => req.respond({ body: "a".repeat(200) }),
  }).end();
};

const app = new App();
app.use(handler);

for await (const request of server) {
  const response = await app.attach(request as any);

  console.log({ response });
}

$ deno run -A tinyhttp.ts 
Check file:///Users/craigmorten/git/asos-craigmorten/superdeno/tinyhttp.ts
{ transport: "tcp", hostname: "0.0.0.0", port: 64565 }
{ response: 1 }
{ response: 2 }
{ response: 3 }
{ response: 4 }
{ response: 5 }
{ response: 6 }

Yielding the following in the browser:

which leads me to believe either tinyhttp or the http std lib have a bug.

Also hitting the server with curl:

$ curl -X GET http://0.0.0.0:64565
HTTP/1.1 200 OK
content-length: 0
content-type: text/plain
vary: Accept
x-powered-by: tinyhttp

aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa%                                               
$ curl -I http://0.0.0.0:64565
HTTP/1.1 200 OK
content-length: 200

[cmorten]

I'm not familiar with tinyhttp, the internals of formatResponse, nor app.attach, but if you can get an example of app.attach working with the serve method from Deno's std/http then I would expect superdeno to start working with it also.

[talentlessguy]

@cmorten ah my bad, the source for formatResponse is here:

https://github.com/deno-libs/tinyhttp/blob/755c42572a0ba617bb1e97ff374d4d1dc27fd834/extensions/res/format.ts#L11-L41

it's almost the same as in Opine's res.format

regarding app.attach... it just immediately mounts the app handler to a server, so nothing fancy.

you can check the source here: https://github.com/deno-libs/tinyhttp/blob/755c42572a0ba617bb1e97ff374d4d1dc27fd834/app.ts#L143

tomorrow I'll try to make a reproduction with serve instead and I'll see what's happening

[cmorten]

Is it the .end() as well as the text prop for content negotiation calling respond() that is doing it?

If https://github.com/deno-libs/tinyhttp/blob/92b9a4455b2cf2f5a71bf6478bc63085aeeb9151/extensions/res/end.ts#L6 is the right place then we might be responding to the same request twice in quick succession resulting in a form of (unsuccessful) HTTP smuggling?

[talentlessguy]

@cmorten I am not sure, but yeah res.end just calls req.respond but with res object being passed as an argument

[cmorten]

@talentlessguy yep dropping the .end() from the example results in the desired response, I think we're calling .respond() twice!

In the Node world authors typically use something like https://nodejs.org/api/http.html#http_response_headerssent to conditionally wrap their .end() / .send() functionality to prevent double response ( well it is a single response, just the second body being added to the body buffer immediately after the first REF: https://deno.land/std@0.102.0/http/server.ts#L87 ). I don't believe we have a similarly nice getter in Deno and authors are typically writing their own logic around that, e.g. see Oak.

I don't know if the std/http lib has anything that can guard against this if consumers of the framework are able to call the respond() method on the underlying request... I suspect not - so if transparency of the std lib method is a feature of the tinyhttp framework then probably just need to document somegotchas!

For instance, the following is currently a possibility:

// deno-lint-ignore-file
import { serve } from "https://deno.land/std@0.101.0/http/server.ts";
import { App, Handler, Request } from "https://deno.land/x/tinyhttp/mod.ts";
import { formatResponse } from "https://deno.land/x/tinyhttp/extensions/res/format.ts";

const server = serve({ port: 0 });

console.log(server.listener.addr);

const handler: Handler = (req, res) => {
  formatResponse(req, res, () => {})({
    text: (req: Request) => req.respond({ body: "a".repeat(600) }),
  }).end().end().end(); // :D
};

const app = new App();
app.use(handler);

for await (const request of server) {
  const response = await app.attach(request as any);

  console.log({ response });
}

[talentlessguy]

@cmorten ok now it's clear, thanks for explaining

should I.. file an issue on Deno repo then?

[asos-craigmorten]

Probably worth reaching out either in an issue or discord to query this - not sure what the response will be, suspect the inclination may be to not do much given there are talks of deprecating the current std/http and replacing with a native server equivalent ( REF: https://github.com/denoland/deno_std/discussions/1034 ) - maybe worth raising to make sure any future design takes these kind of things into account.

[asos-craigmorten]

Issue lies upstream.