Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(http): don't expose body on GET/HEAD requests #12260

Merged
merged 3 commits into from
Oct 11, 2021
Merged

fix(http): don't expose body on GET/HEAD requests #12260

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
459bc87
fix(http): don't expose body on GET/HEAD requests
lucacasonato Sep 28, 2021
d426f02
apply ben's suggestion
lucacasonato Oct 2, 2021
788d270
Merge remote-tracking branch 'origin/main' into no_body_for_get_head
lucacasonato Oct 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions cli/tests/unit/http_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,40 @@ unitTest({ permissions: { net: true } }, async function httpServerBasic() {
await promise;
});

unitTest(
{ permissions: { net: true } },
async function httpServerGetRequestBody() {
const promise = (async () => {
const listener = Deno.listen({ port: 4501 });
const conn = await listener.accept();
listener.close();
const httpConn = Deno.serveHttp(conn);
const e = await httpConn.nextRequest();
assert(e);
const { request, respondWith } = e;
assertEquals(request.body, null);
await respondWith(new Response("", { headers: {} }));
httpConn.close();
})();

const conn = await Deno.connect({ port: 4501 });
// Send GET request with a body + content-length.
const encoder = new TextEncoder();
const body =
`GET / HTTP/1.1\r\nHost: 127.0.0.1:4501\r\nContent-Length: 5\r\n\r\n12345`;
const writeResult = await conn.write(encoder.encode(body));
assertEquals(body.length, writeResult);

const resp = new Uint8Array(200);
const readResult = await conn.read(resp);
assertEquals(readResult, 115);

conn.close();

await promise;
},
);

unitTest(
{ permissions: { net: true } },
async function httpServerStreamResponse() {
Expand Down
7 changes: 6 additions & 1 deletion ext/http/01_http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,12 @@
let body = null;
if (typeof requestRid === "number") {
SetPrototypeAdd(this.managedResources, requestRid);
body = createRequestBodyStream(this, requestRid);
// There might be a body, but we don't expose it for GET/HEAD requests.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to refer to the relevant portion(s) of the spec here?

Copy link
Member Author

@lucacasonato lucacasonato Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the Request creation that is internal to our HTTP server, so there is no spec. The new Request constructor that is exposed to users enforces this in step 34 of the constructor steps: https://fetch.spec.whatwg.org/#dom-request

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the Request creation that is internal to our HTTP server, so there is no spec.

Then why do we have to hide bodies on incoming requests?
And if we do, what about other methods that can't have bodies - OPTIONS and TRACE (DELETE?).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because users can not do new Request(incomingReq) (this throws) for an unmodified incoming GET request from the HTTP server. See #11927. We are creating an invalid Request instance because we are creating it internally, bypassing the usual request constructor checks. There is no possibility on the web platform to create a Request object where method === "GET" && body !== null. Because of this we should not have one either.

OPTIONS, TRACE, and DELETE requests can have bodies in fetch on the web platform. GET and HEAD can not.

// It will be closed automatically once the request has been handled and
// the response has been sent.
if (method !== "GET" && method !== "HEAD") {
body = createRequestBodyStream(this, requestRid);
}
}

const innerRequest = newInnerRequest(
Expand Down
9 changes: 7 additions & 2 deletions ext/http/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use hyper::http;
use hyper::server::conn::Http;
use hyper::service::Service as HyperService;
use hyper::Body;
use hyper::Method;
use hyper::Request;
use hyper::Response;
use serde::Deserialize;
Expand Down Expand Up @@ -244,13 +245,17 @@ fn prepare_next_request(
let url = req_url(&req, scheme, addr)?;

let is_websocket = is_websocket_request(&req);
let has_body = if let Some(exact_size) = req.size_hint().exact() {
let maybe_has_body = if let Some(exact_size) = req.size_hint().exact() {
lucacasonato marked this conversation as resolved.
Show resolved Hide resolved
exact_size > 0
} else {
true
};
let can_method_have_body =
!matches!(*req.method(), Method::GET | Method::HEAD);
let should_expose_body =
is_websocket || (maybe_has_body && can_method_have_body);

let maybe_request_rid = if is_websocket || has_body {
let maybe_request_rid = if should_expose_body {
let request_rid = state.resource_table.add(RequestResource {
conn_rid,
inner: AsyncRefCell::new(RequestOrStreamReader::Request(Some(req))),
Expand Down