From b86f46e34f4833d3b963802057ae3196e7ef3dc1 Mon Sep 17 00:00:00 2001 From: Vincent LE GOFF Date: Mon, 20 May 2019 15:06:24 +0200 Subject: [PATCH 01/18] add handling of bad request --- http/server.ts | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/http/server.ts b/http/server.ts index 3886e7672636..cec3a3f10b00 100644 --- a/http/server.ts +++ b/http/server.ts @@ -107,6 +107,7 @@ export class ServerRequest { r: BufReader; w: BufWriter; done: Deferred = deferred(); + err?: any; // error in the request public async *bodyStream(): AsyncIterableIterator { if (this.headers.has("content-length")) { @@ -189,6 +190,10 @@ export class ServerRequest { } async respond(r: Response): Promise { + if (this.err) { + r.status = 400; + r.body = new TextEncoder().encode("Unable to proceed request"); + } // Write our response! await writeResponse(this.w, r); // Signal that this request has been processed and the next pipelined @@ -211,7 +216,16 @@ async function readRequest( return [null, err]; } [req.method, req.url, req.proto] = firstLine.split(" ", 3); - [req.headers, err] = await tp.readMIMEHeader(); + + // if there is an error in the request header + // we proceed the request but we force a 400 + try { + [req.headers, err] = await tp.readMIMEHeader(); + } catch (e) { + req.err = e; + return [req, null]; + } + return [req, err]; } From 0ca2b76d0f668bbf8e4c347cbfc7bece9a8dbd7b Mon Sep 17 00:00:00 2001 From: Vincent LE GOFF Date: Mon, 20 May 2019 15:31:41 +0200 Subject: [PATCH 02/18] comment --- http/server.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/http/server.ts b/http/server.ts index cec3a3f10b00..60e05d696725 100644 --- a/http/server.ts +++ b/http/server.ts @@ -190,10 +190,13 @@ export class ServerRequest { } async respond(r: Response): Promise { + // if an error in the request occurs + // we rewrite it if (this.err) { r.status = 400; r.body = new TextEncoder().encode("Unable to proceed request"); } + // Write our response! await writeResponse(this.w, r); // Signal that this request has been processed and the next pipelined From c51e3ba83c16ee27bed22500a2fdd916f9a16756 Mon Sep 17 00:00:00 2001 From: Vincent LE GOFF Date: Mon, 20 May 2019 15:36:14 +0200 Subject: [PATCH 03/18] lint --- http/server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http/server.ts b/http/server.ts index 60e05d696725..631bc7057ba1 100644 --- a/http/server.ts +++ b/http/server.ts @@ -107,7 +107,7 @@ export class ServerRequest { r: BufReader; w: BufWriter; done: Deferred = deferred(); - err?: any; // error in the request + err?: Error; // error in the request public async *bodyStream(): AsyncIterableIterator { if (this.headers.has("content-length")) { From d01e08f4b19b3697c9e8ae29ca20540f6abe1ee6 Mon Sep 17 00:00:00 2001 From: Vincent LE GOFF Date: Mon, 20 May 2019 16:52:28 +0200 Subject: [PATCH 04/18] fix raw result & tests --- http/server.ts | 2 +- http/server_test.ts | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/http/server.ts b/http/server.ts index 631bc7057ba1..c4948b7692c4 100644 --- a/http/server.ts +++ b/http/server.ts @@ -194,7 +194,7 @@ export class ServerRequest { // we rewrite it if (this.err) { r.status = 400; - r.body = new TextEncoder().encode("Unable to proceed request"); + r.body = new TextEncoder().encode("Unable to proceed request\r\n\r\n"); } // Write our response! diff --git a/http/server_test.ts b/http/server_test.ts index a2a4713d90ff..4a8bfc16fa44 100644 --- a/http/server_test.ts +++ b/http/server_test.ts @@ -55,6 +55,39 @@ test(async function responseWrite(): Promise { } }); +test(async function responseWriteInCaseOfError(): Promise { + const buf = new Buffer(); + const bufw = new BufWriter(buf); + const request = new ServerRequest(); + request.w = bufw; + request.err = new Error("Unable to parse request"); + request.conn = { + localAddr: "", + remoteAddr: "", + rid: -1, + closeRead: (): void => {}, + closeWrite: (): void => {}, + read: async (): Promise => { + return { eof: true, nread: 0 }; + }, + write: async (): Promise => { + return -1; + }, + close: (): void => {} + }; + const response = { + status: 200 + }; + await request.respond(response); + console.log(buf.toString()); + const expected = + "HTTP/1.1 400 Bad Request\r\n" + + "content-length: 29\r\n\r\n" + + "Unable to proceed request\r\n\r\n"; + assertEquals(buf.toString(), expected); + await request.done; +}); + test(async function requestBodyWithContentLength(): Promise { { const req = new ServerRequest(); From fba0ad13a92716cad3f6fc3d0e36bfe494da3d94 Mon Sep 17 00:00:00 2001 From: LE GOFF Vincent Date: Mon, 20 May 2019 20:36:50 +0200 Subject: [PATCH 05/18] fix error handling --- http/server.ts | 16 ++++++---------- http/server_test.ts | 33 --------------------------------- 2 files changed, 6 insertions(+), 43 deletions(-) diff --git a/http/server.ts b/http/server.ts index c4948b7692c4..b545b587b03e 100644 --- a/http/server.ts +++ b/http/server.ts @@ -107,7 +107,6 @@ export class ServerRequest { r: BufReader; w: BufWriter; done: Deferred = deferred(); - err?: Error; // error in the request public async *bodyStream(): AsyncIterableIterator { if (this.headers.has("content-length")) { @@ -190,13 +189,6 @@ export class ServerRequest { } async respond(r: Response): Promise { - // if an error in the request occurs - // we rewrite it - if (this.err) { - r.status = 400; - r.body = new TextEncoder().encode("Unable to proceed request\r\n\r\n"); - } - // Write our response! await writeResponse(this.w, r); // Signal that this request has been processed and the next pipelined @@ -225,8 +217,12 @@ async function readRequest( try { [req.headers, err] = await tp.readMIMEHeader(); } catch (e) { - req.err = e; - return [req, null]; + await writeResponse(req.w, { + status: 400, + body: new TextEncoder().encode("Unable to proceed request\r\n\r\n") + }); + await req.done.resolve(); + return [null, "EOF"]; } return [req, err]; diff --git a/http/server_test.ts b/http/server_test.ts index 4a8bfc16fa44..a2a4713d90ff 100644 --- a/http/server_test.ts +++ b/http/server_test.ts @@ -55,39 +55,6 @@ test(async function responseWrite(): Promise { } }); -test(async function responseWriteInCaseOfError(): Promise { - const buf = new Buffer(); - const bufw = new BufWriter(buf); - const request = new ServerRequest(); - request.w = bufw; - request.err = new Error("Unable to parse request"); - request.conn = { - localAddr: "", - remoteAddr: "", - rid: -1, - closeRead: (): void => {}, - closeWrite: (): void => {}, - read: async (): Promise => { - return { eof: true, nread: 0 }; - }, - write: async (): Promise => { - return -1; - }, - close: (): void => {} - }; - const response = { - status: 200 - }; - await request.respond(response); - console.log(buf.toString()); - const expected = - "HTTP/1.1 400 Bad Request\r\n" + - "content-length: 29\r\n\r\n" + - "Unable to proceed request\r\n\r\n"; - assertEquals(buf.toString(), expected); - await request.done; -}); - test(async function requestBodyWithContentLength(): Promise { { const req = new ServerRequest(); From ebf8cfd0a4894f552c443d032b433231796fe57f Mon Sep 17 00:00:00 2001 From: Vincent LE GOFF Date: Tue, 21 May 2019 15:43:20 +0200 Subject: [PATCH 06/18] export readRequest --- http/server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http/server.ts b/http/server.ts index b545b587b03e..2514bd70d307 100644 --- a/http/server.ts +++ b/http/server.ts @@ -197,7 +197,7 @@ export class ServerRequest { } } -async function readRequest( +export async function readRequest( bufr: BufReader ): Promise<[ServerRequest, BufState]> { const req = new ServerRequest(); From 2999b353f262a7a518724af45699fb09b362c3be Mon Sep 17 00:00:00 2001 From: Vincent LE GOFF Date: Tue, 21 May 2019 15:43:47 +0200 Subject: [PATCH 07/18] add test for readrequest --- http/server_test.ts | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/http/server_test.ts b/http/server_test.ts index a2a4713d90ff..806a94e8610d 100644 --- a/http/server_test.ts +++ b/http/server_test.ts @@ -8,7 +8,12 @@ const { Buffer } = Deno; import { test, runIfMain } from "../testing/mod.ts"; import { assertEquals } from "../testing/asserts.ts"; -import { Response, ServerRequest, writeResponse } from "./server.ts"; +import { + Response, + ServerRequest, + writeResponse, + readRequest +} from "./server.ts"; import { BufReader, BufWriter } from "../io/bufio.ts"; import { StringReader } from "../io/readers.ts"; @@ -283,4 +288,28 @@ test(async function writeStringReaderResponse(): Promise { assertEquals(decoder.decode(line), "0"); }); +test(async function readRequestError(): Promise { + let input = `GET / HTTP/1.1 +malformedHeader +`; + const buf = new Buffer(enc.encode(input)); + const reader = new BufReader(buf); + const conn = { + localAddr: "", + remoteAddr: "", + rid: -1, + closeRead: (): void => {}, + closeWrite: (): void => {}, + read: async (): Promise => { + return { eof: true, nread: 0 }; + }, + write: async (): Promise => { + return -1; + }, + close: (): void => {} + }; + const [_, err] = await readRequest(conn, reader); + assertEquals(err, "EOF"); +}); + runIfMain(import.meta); From 5df854e486c73917af5535973e7bd40f1d9fce30 Mon Sep 17 00:00:00 2001 From: Vincent LE GOFF Date: Tue, 21 May 2019 17:39:22 +0200 Subject: [PATCH 08/18] refactor + port of golang test --- http/server.ts | 24 ++++++++---- http/server_test.ts | 90 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 96 insertions(+), 18 deletions(-) diff --git a/http/server.ts b/http/server.ts index 2514bd70d307..1c3e5c08ac3e 100644 --- a/http/server.ts +++ b/http/server.ts @@ -15,6 +15,14 @@ import { MuxAsyncIterator } from "../util/async.ts"; +export class HttpError extends Error { + public status: number; + constructor(status: number, message: string) { + super(message); + this.status = status; + } +} + function bufWriter(w: Writer): BufWriter { if (w instanceof BufWriter) { return w; @@ -199,7 +207,7 @@ export class ServerRequest { export async function readRequest( bufr: BufReader -): Promise<[ServerRequest, BufState]> { +): Promise<[ServerRequest, BufState | HttpError]> { const req = new ServerRequest(); req.r = bufr; const tp = new TextProtoReader(bufr); @@ -217,12 +225,7 @@ export async function readRequest( try { [req.headers, err] = await tp.readMIMEHeader(); } catch (e) { - await writeResponse(req.w, { - status: 400, - body: new TextEncoder().encode("Unable to proceed request\r\n\r\n") - }); - await req.done.resolve(); - return [null, "EOF"]; + return [req, new HttpError(400, "Unable to proceed request")]; } return [req, err]; @@ -259,7 +262,12 @@ export class Server implements AsyncIterable { if (bufStateErr === "EOF") { // The connection was gracefully closed. - } else if (bufStateErr instanceof Error) { + } else if (bufStateErr instanceof HttpError) { + await writeResponse(req.w, { + status: bufStateErr.status, + body: new TextEncoder().encode(`${bufStateErr.message}\r\n\r\n`) + }); + await req.done.resolve(); // TODO(ry): send something back like a HTTP 500 status. } else if (this.closing) { // There are more requests incoming but the server is closing. diff --git a/http/server_test.ts b/http/server_test.ts index 806a94e8610d..3cfc8ebe877b 100644 --- a/http/server_test.ts +++ b/http/server_test.ts @@ -7,12 +7,13 @@ const { Buffer } = Deno; import { test, runIfMain } from "../testing/mod.ts"; -import { assertEquals } from "../testing/asserts.ts"; +import { assert, assertEquals } from "../testing/asserts.ts"; import { Response, ServerRequest, writeResponse, - readRequest + readRequest, + HttpError } from "./server.ts"; import { BufReader, BufWriter } from "../io/bufio.ts"; import { StringReader } from "../io/readers.ts"; @@ -288,13 +289,8 @@ test(async function writeStringReaderResponse(): Promise { assertEquals(decoder.decode(line), "0"); }); -test(async function readRequestError(): Promise { - let input = `GET / HTTP/1.1 -malformedHeader -`; - const buf = new Buffer(enc.encode(input)); - const reader = new BufReader(buf); - const conn = { +function createConnMock(): Deno.Conn { + return { localAddr: "", remoteAddr: "", rid: -1, @@ -308,8 +304,82 @@ malformedHeader }, close: (): void => {} }; +} + +test(async function readRequestError(): Promise { + let input = `GET / HTTP/1.1 +malformedHeader +`; + const buf = new Buffer(enc.encode(input)); + const reader = new BufReader(buf); + const conn = createConnMock(); const [_, err] = await readRequest(conn, reader); - assertEquals(err, "EOF"); + const e: any = err; + assert(e instanceof HttpError); + assertEquals(e.status, 400); + assertEquals(e.message, "Unable to proceed request"); }); +// Port from Go +// https://github.com/golang/go/blob/master/src/net/http/request_test.go +test(async function testReadRequestError(): Promise { + const testCases = { + 0: { + in: "GET / HTTP/1.1\r\nheader: foo\r\n\r\n", + header: new Headers({ header: "foo" }), + err: null + }, + 1: { in: "GET / HTTP/1.1\r\nheader:foo\r\n", err: "EOF" }, + 2: { in: "", err: "EOF" }, + // 3: { + // in: "HEAD / HTTP/1.1\r\nContent-Length:4\r\n\r\n", + // err: "http: method cannot contain a Content-Length" + // }, + 4: { + in: "HEAD / HTTP/1.1\r\n\r\n", + header: new Headers(), + err: null + }, + // Multiple Content-Length values should either be + // deduplicated if same or reject otherwise + // See Issue 16490. + // 5: { + // in: + // "POST / HTTP/1.1\r\nContent-Length: 10\r\nContent-Length: 0\r\n\r\nGopher hey\r\n", + // err: "cannot contain multiple Content-Length headers" + // }, + // 6: { + // in: + // "POST / HTTP/1.1\r\nContent-Length: 10\r\nContent-Length: 6\r\n\r\nGopher\r\n", + // err: "cannot contain multiple Content-Length headers" + // }, + 7: { + in: + "PUT / HTTP/1.1\r\nContent-Length: 6 \r\nContent-Length: 6\r\nContent-Length:6\r\n\r\nGopher\r\n", + err: null, + header: new Headers({ "Content-Length": "6" }) + }, + // 8: { + // in: "PUT / HTTP/1.1\r\nContent-Length: 1\r\nContent-Length: 6 \r\n\r\n", + // err: "cannot contain multiple Content-Length headers" + // }, + // 9: { + // in: "POST / HTTP/1.1\r\nContent-Length:\r\nContent-Length: 3\r\n\r\n", + // err: "cannot contain multiple Content-Length headers" + // }, + 10: { + in: "HEAD / HTTP/1.1\r\nContent-Length:0\r\nContent-Length: 0\r\n\r\n", + header: new Headers({ "Content-Length": "0" }), + err: null + } + }; + for (const p in testCases) { + const test = testCases[p]; + const buf = new Buffer(enc.encode(test.in).buffer); + const reader = new BufReader(buf); + const conn = createConnMock(); + const [_, err] = await readRequest(conn, reader); + assertEquals(test.err, err); + } +}); runIfMain(import.meta); From d6ee15b2a25bf143c9b6aeaccda22a795adb6704 Mon Sep 17 00:00:00 2001 From: Vincent LE GOFF Date: Tue, 21 May 2019 18:00:08 +0200 Subject: [PATCH 09/18] testing refactor --- http/server_test.ts | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/http/server_test.ts b/http/server_test.ts index 3cfc8ebe877b..3c460397a0e7 100644 --- a/http/server_test.ts +++ b/http/server_test.ts @@ -314,7 +314,7 @@ malformedHeader const reader = new BufReader(buf); const conn = createConnMock(); const [_, err] = await readRequest(conn, reader); - const e: any = err; + const e: any = err; // eslint-disable-line @typescript-eslint/no-explicit-any assert(e instanceof HttpError); assertEquals(e.status, 400); assertEquals(e.message, "Unable to proceed request"); @@ -326,18 +326,18 @@ test(async function testReadRequestError(): Promise { const testCases = { 0: { in: "GET / HTTP/1.1\r\nheader: foo\r\n\r\n", - header: new Headers({ header: "foo" }), + headers: [{ key: "header", value: "foo" }], err: null }, - 1: { in: "GET / HTTP/1.1\r\nheader:foo\r\n", err: "EOF" }, - 2: { in: "", err: "EOF" }, + 1: { in: "GET / HTTP/1.1\r\nheader:foo\r\n", err: "EOF", headers: [] }, + 2: { in: "", err: "EOF", headers: [] }, // 3: { // in: "HEAD / HTTP/1.1\r\nContent-Length:4\r\n\r\n", // err: "http: method cannot contain a Content-Length" // }, 4: { in: "HEAD / HTTP/1.1\r\n\r\n", - header: new Headers(), + headers: [], err: null }, // Multiple Content-Length values should either be @@ -353,12 +353,12 @@ test(async function testReadRequestError(): Promise { // "POST / HTTP/1.1\r\nContent-Length: 10\r\nContent-Length: 6\r\n\r\nGopher\r\n", // err: "cannot contain multiple Content-Length headers" // }, - 7: { - in: - "PUT / HTTP/1.1\r\nContent-Length: 6 \r\nContent-Length: 6\r\nContent-Length:6\r\n\r\nGopher\r\n", - err: null, - header: new Headers({ "Content-Length": "6" }) - }, + // 7: { + // in: + // "PUT / HTTP/1.1\r\nContent-Length: 6 \r\nContent-Length: 6\r\nContent-Length:6\r\n\r\nGopher\r\n", + // err: null, + // headers: [{ key: "Content-Length", value: "6" }] + // }, // 8: { // in: "PUT / HTTP/1.1\r\nContent-Length: 1\r\nContent-Length: 6 \r\n\r\n", // err: "cannot contain multiple Content-Length headers" @@ -369,7 +369,7 @@ test(async function testReadRequestError(): Promise { // }, 10: { in: "HEAD / HTTP/1.1\r\nContent-Length:0\r\nContent-Length: 0\r\n\r\n", - header: new Headers({ "Content-Length": "0" }), + headers: [{ key: "Content-Length", value: "0" }], err: null } }; @@ -378,8 +378,11 @@ test(async function testReadRequestError(): Promise { const buf = new Buffer(enc.encode(test.in).buffer); const reader = new BufReader(buf); const conn = createConnMock(); - const [_, err] = await readRequest(conn, reader); + const [req, err] = await readRequest(conn, reader); assertEquals(test.err, err); + for (const h of test.headers) { + assertEquals(req.headers.get(h.key), h.value); + } } }); runIfMain(import.meta); From c2ad5eea9c6eca556daa0992b2be8f9a7812e238 Mon Sep 17 00:00:00 2001 From: Vincent LE GOFF Date: Tue, 21 May 2019 18:03:58 +0200 Subject: [PATCH 10/18] disable buggy test --- http/server_test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/http/server_test.ts b/http/server_test.ts index 3c460397a0e7..dee243c12cb3 100644 --- a/http/server_test.ts +++ b/http/server_test.ts @@ -367,11 +367,11 @@ test(async function testReadRequestError(): Promise { // in: "POST / HTTP/1.1\r\nContent-Length:\r\nContent-Length: 3\r\n\r\n", // err: "cannot contain multiple Content-Length headers" // }, - 10: { - in: "HEAD / HTTP/1.1\r\nContent-Length:0\r\nContent-Length: 0\r\n\r\n", - headers: [{ key: "Content-Length", value: "0" }], - err: null - } + // 10: { + // in: "HEAD / HTTP/1.1\r\nContent-Length:0\r\nContent-Length: 0\r\n\r\n", + // headers: [{ key: "Content-Length", value: "0" }], + // err: null + // } }; for (const p in testCases) { const test = testCases[p]; From 6c4d7c2dc06e0cdec41fc5532319d4e1722976e1 Mon Sep 17 00:00:00 2001 From: Vincent LE GOFF Date: Tue, 21 May 2019 18:06:05 +0200 Subject: [PATCH 11/18] format --- http/server_test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http/server_test.ts b/http/server_test.ts index dee243c12cb3..866f2ebbcf12 100644 --- a/http/server_test.ts +++ b/http/server_test.ts @@ -339,7 +339,7 @@ test(async function testReadRequestError(): Promise { in: "HEAD / HTTP/1.1\r\n\r\n", headers: [], err: null - }, + } // Multiple Content-Length values should either be // deduplicated if same or reject otherwise // See Issue 16490. From c3aef687e509239e5e8241cd501186ea6dab4da9 Mon Sep 17 00:00:00 2001 From: Vincent LE GOFF Date: Tue, 21 May 2019 18:36:30 +0200 Subject: [PATCH 12/18] Revert to regular error --- http/server.ts | 16 ++++------------ http/server_test.ts | 6 ++---- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/http/server.ts b/http/server.ts index 1c3e5c08ac3e..aae92bb9ef3c 100644 --- a/http/server.ts +++ b/http/server.ts @@ -15,14 +15,6 @@ import { MuxAsyncIterator } from "../util/async.ts"; -export class HttpError extends Error { - public status: number; - constructor(status: number, message: string) { - super(message); - this.status = status; - } -} - function bufWriter(w: Writer): BufWriter { if (w instanceof BufWriter) { return w; @@ -207,7 +199,7 @@ export class ServerRequest { export async function readRequest( bufr: BufReader -): Promise<[ServerRequest, BufState | HttpError]> { +): Promise<[ServerRequest, BufState | Error]> { const req = new ServerRequest(); req.r = bufr; const tp = new TextProtoReader(bufr); @@ -225,7 +217,7 @@ export async function readRequest( try { [req.headers, err] = await tp.readMIMEHeader(); } catch (e) { - return [req, new HttpError(400, "Unable to proceed request")]; + return [req, new Error("Unable to proceed request")]; } return [req, err]; @@ -262,9 +254,9 @@ export class Server implements AsyncIterable { if (bufStateErr === "EOF") { // The connection was gracefully closed. - } else if (bufStateErr instanceof HttpError) { + } else if (bufStateErr instanceof Error) { await writeResponse(req.w, { - status: bufStateErr.status, + status: 400, body: new TextEncoder().encode(`${bufStateErr.message}\r\n\r\n`) }); await req.done.resolve(); diff --git a/http/server_test.ts b/http/server_test.ts index 866f2ebbcf12..d162938176df 100644 --- a/http/server_test.ts +++ b/http/server_test.ts @@ -12,8 +12,7 @@ import { Response, ServerRequest, writeResponse, - readRequest, - HttpError + readRequest } from "./server.ts"; import { BufReader, BufWriter } from "../io/bufio.ts"; import { StringReader } from "../io/readers.ts"; @@ -315,8 +314,7 @@ malformedHeader const conn = createConnMock(); const [_, err] = await readRequest(conn, reader); const e: any = err; // eslint-disable-line @typescript-eslint/no-explicit-any - assert(e instanceof HttpError); - assertEquals(e.status, 400); + assert(e instanceof Error); assertEquals(e.message, "Unable to proceed request"); }); From b99eab8136697af9f896ce11b73acd2b13ede2f8 Mon Sep 17 00:00:00 2001 From: Vincent LE GOFF Date: Tue, 21 May 2019 19:08:31 +0200 Subject: [PATCH 13/18] review --- http/server_test.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/http/server_test.ts b/http/server_test.ts index d162938176df..9b968d4def09 100644 --- a/http/server_test.ts +++ b/http/server_test.ts @@ -309,8 +309,7 @@ test(async function readRequestError(): Promise { let input = `GET / HTTP/1.1 malformedHeader `; - const buf = new Buffer(enc.encode(input)); - const reader = new BufReader(buf); + const reader = new BufReader(new StringReader(input)); const conn = createConnMock(); const [_, err] = await readRequest(conn, reader); const e: any = err; // eslint-disable-line @typescript-eslint/no-explicit-any @@ -319,7 +318,7 @@ malformedHeader }); // Port from Go -// https://github.com/golang/go/blob/master/src/net/http/request_test.go +// https://github.com/golang/go/blob/master/src/net/http/request_test.go#L380-L446 test(async function testReadRequestError(): Promise { const testCases = { 0: { @@ -373,8 +372,7 @@ test(async function testReadRequestError(): Promise { }; for (const p in testCases) { const test = testCases[p]; - const buf = new Buffer(enc.encode(test.in).buffer); - const reader = new BufReader(buf); + const reader = new BufReader(new StringReader(test.in)); const conn = createConnMock(); const [req, err] = await readRequest(conn, reader); assertEquals(test.err, err); From 4bbd381a4253739aeedfa879f559b0a9b2d210aa Mon Sep 17 00:00:00 2001 From: LE GOFF Vincent Date: Tue, 21 May 2019 22:59:11 +0200 Subject: [PATCH 14/18] review --- http/server_test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/http/server_test.ts b/http/server_test.ts index 9b968d4def09..57d34ac55c67 100644 --- a/http/server_test.ts +++ b/http/server_test.ts @@ -317,8 +317,8 @@ malformedHeader assertEquals(e.message, "Unable to proceed request"); }); -// Port from Go -// https://github.com/golang/go/blob/master/src/net/http/request_test.go#L380-L446 +// Ported from Go +// https://github.com/golang/go/blob/go1.12.5/src/net/http/request_test.go#L377-L443 test(async function testReadRequestError(): Promise { const testCases = { 0: { From 51fdd758480078d5991925dfc012354d1b2daec1 Mon Sep 17 00:00:00 2001 From: LE GOFF Vincent Date: Wed, 22 May 2019 08:29:28 +0200 Subject: [PATCH 15/18] review --- http/server.ts | 6 ++++-- http/server_test.ts | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/http/server.ts b/http/server.ts index aae92bb9ef3c..f3f7a91393cf 100644 --- a/http/server.ts +++ b/http/server.ts @@ -197,9 +197,10 @@ export class ServerRequest { } } +// TODO(#427) readRequest should not depend on conn. export async function readRequest( bufr: BufReader -): Promise<[ServerRequest, BufState | Error]> { +): Promise<[ServerRequest, BufState]> { const req = new ServerRequest(); req.r = bufr; const tp = new TextProtoReader(bufr); @@ -210,11 +211,12 @@ export async function readRequest( if (err) { return [null, err]; } - [req.method, req.url, req.proto] = firstLine.split(" ", 3); // if there is an error in the request header // we proceed the request but we force a 400 try { + [req.method, req.url, req.proto] = firstLine.split(" ", 3); + [req.headers, err] = await tp.readMIMEHeader(); } catch (e) { return [req, new Error("Unable to proceed request")]; diff --git a/http/server_test.ts b/http/server_test.ts index 57d34ac55c67..24e5d4011344 100644 --- a/http/server_test.ts +++ b/http/server_test.ts @@ -319,6 +319,7 @@ malformedHeader // Ported from Go // https://github.com/golang/go/blob/go1.12.5/src/net/http/request_test.go#L377-L443 +// TODO(zekth) fix tests test(async function testReadRequestError(): Promise { const testCases = { 0: { From 33a2fee1059e068fecf69b33725eb1e7818c6d0b Mon Sep 17 00:00:00 2001 From: LE GOFF Vincent Date: Wed, 22 May 2019 08:29:50 +0200 Subject: [PATCH 16/18] format --- http/server.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/http/server.ts b/http/server.ts index f3f7a91393cf..0d3b715fd16b 100644 --- a/http/server.ts +++ b/http/server.ts @@ -216,7 +216,6 @@ export async function readRequest( // we proceed the request but we force a 400 try { [req.method, req.url, req.proto] = firstLine.split(" ", 3); - [req.headers, err] = await tp.readMIMEHeader(); } catch (e) { return [req, new Error("Unable to proceed request")]; From 6edddf9944d68c9f00c39d76e7a5a078489904f8 Mon Sep 17 00:00:00 2001 From: LE GOFF Vincent Date: Wed, 22 May 2019 08:40:29 +0200 Subject: [PATCH 17/18] review --- http/server.ts | 17 ++++++++--------- http/server_test.ts | 33 +++++++++------------------------ 2 files changed, 17 insertions(+), 33 deletions(-) diff --git a/http/server.ts b/http/server.ts index 0d3b715fd16b..1533bb9c448c 100644 --- a/http/server.ts +++ b/http/server.ts @@ -212,14 +212,9 @@ export async function readRequest( return [null, err]; } - // if there is an error in the request header - // we proceed the request but we force a 400 - try { - [req.method, req.url, req.proto] = firstLine.split(" ", 3); - [req.headers, err] = await tp.readMIMEHeader(); - } catch (e) { - return [req, new Error("Unable to proceed request")]; - } + [req.method, req.url, req.proto] = firstLine.split(" ", 3); + + [req.headers, err] = await tp.readMIMEHeader(); return [req, err]; } @@ -244,7 +239,11 @@ export class Server implements AsyncIterable { let req: ServerRequest; while (!this.closing) { - [req, bufStateErr] = await readRequest(bufr); + try { + [req, bufStateErr] = await readRequest(bufr); + } catch (err) { + bufStateErr = err; + } if (bufStateErr) break; req.w = w; yield req; diff --git a/http/server_test.ts b/http/server_test.ts index 24e5d4011344..e3baebd53854 100644 --- a/http/server_test.ts +++ b/http/server_test.ts @@ -288,33 +288,19 @@ test(async function writeStringReaderResponse(): Promise { assertEquals(decoder.decode(line), "0"); }); -function createConnMock(): Deno.Conn { - return { - localAddr: "", - remoteAddr: "", - rid: -1, - closeRead: (): void => {}, - closeWrite: (): void => {}, - read: async (): Promise => { - return { eof: true, nread: 0 }; - }, - write: async (): Promise => { - return -1; - }, - close: (): void => {} - }; -} - test(async function readRequestError(): Promise { let input = `GET / HTTP/1.1 malformedHeader `; const reader = new BufReader(new StringReader(input)); - const conn = createConnMock(); - const [_, err] = await readRequest(conn, reader); - const e: any = err; // eslint-disable-line @typescript-eslint/no-explicit-any - assert(e instanceof Error); - assertEquals(e.message, "Unable to proceed request"); + let err; + try { + await readRequest(reader); + } catch (e) { + err = e; + } + assert(err instanceof Error); + assertEquals(err.message, "malformed MIME header line: malformedHeader"); }); // Ported from Go @@ -374,8 +360,7 @@ test(async function testReadRequestError(): Promise { for (const p in testCases) { const test = testCases[p]; const reader = new BufReader(new StringReader(test.in)); - const conn = createConnMock(); - const [req, err] = await readRequest(conn, reader); + const [req, err] = await readRequest(reader); assertEquals(test.err, err); for (const h of test.headers) { assertEquals(req.headers.get(h.key), h.value); From f3bed064a22888f0b461e649425309c6fb2cb49b Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Wed, 22 May 2019 16:22:32 -0700 Subject: [PATCH 18/18] fixes --- http/server.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/http/server.ts b/http/server.ts index 1533bb9c448c..809cf39ffe99 100644 --- a/http/server.ts +++ b/http/server.ts @@ -197,7 +197,6 @@ export class ServerRequest { } } -// TODO(#427) readRequest should not depend on conn. export async function readRequest( bufr: BufReader ): Promise<[ServerRequest, BufState]> { @@ -211,11 +210,8 @@ export async function readRequest( if (err) { return [null, err]; } - [req.method, req.url, req.proto] = firstLine.split(" ", 3); - [req.headers, err] = await tp.readMIMEHeader(); - return [req, err]; } @@ -255,12 +251,11 @@ export class Server implements AsyncIterable { if (bufStateErr === "EOF") { // The connection was gracefully closed. } else if (bufStateErr instanceof Error) { + // An error was thrown while parsing request headers. await writeResponse(req.w, { status: 400, body: new TextEncoder().encode(`${bufStateErr.message}\r\n\r\n`) }); - await req.done.resolve(); - // TODO(ry): send something back like a HTTP 500 status. } else if (this.closing) { // There are more requests incoming but the server is closing. // TODO(ry): send a back a HTTP 503 Service Unavailable status.