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.

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

Ensure Content-Length set on requests with known length bodies #981

Merged
merged 1 commit into from
Aug 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 17 additions & 0 deletions src/workerd/api/global-scope.c++
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,23 @@ kj::Promise<DeferredProxy<void>> ServiceWorkerGlobalScope::request(
body = Body::ExtractedBody(jsStream.addRef());
}

// If the request doesn't specify "Content-Length" or "Transfer-Encoding", set "Content-Length"
// to the body length if it's known. This ensures handlers for worker-to-worker requests can
// access known body lengths if they're set, without buffering bodies.
if (body != nullptr &&
headers.get(kj::HttpHeaderId::CONTENT_LENGTH) == nullptr &&
headers.get(kj::HttpHeaderId::TRANSFER_ENCODING) == nullptr) {
// We can't use headers.set() here as headers is marked const. Instead, we call set() on the
// JavaScript headers object, ignoring the REQUEST guard that usually makes them immutable.
KJ_IF_MAYBE(l, requestBody.tryGetLength()) {
Copy link
Member

Choose a reason for hiding this comment

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

If the length isn't known, should we set Transfer-Encoding: chunked to emulate what an actual HTTP connection would do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, sure! 🙂

jsHeaders->setUnguarded(jsg::ByteString(kj::str("Content-Length")),
jsg::ByteString(kj::str(*l)));
} else {
jsHeaders->setUnguarded(jsg::ByteString(kj::str("Transfer-Encoding")),
jsg::ByteString(kj::str("chunked")));
}
}

auto jsRequest = jsg::alloc<Request>(
method, url, Request::Redirect::MANUAL, kj::mv(jsHeaders),
jsg::alloc<Fetcher>(IoContext::NEXT_CLIENT_CHANNEL,
Expand Down
38 changes: 38 additions & 0 deletions src/workerd/api/http-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,50 @@ import assert from "node:assert";
let scheduledLastCtrl;

export default {
async fetch(request, env, ctx) {
const { pathname } = new URL(request.url);
if (pathname === "/body-length") {
return Response.json(Object.fromEntries(request.headers));
}
return new Response(null, { status: 404 });
},

async scheduled(ctrl, env, ctx) {
scheduledLastCtrl = ctrl;
if (ctrl.cron === "* * * * 30") ctrl.noRetry();
},

async test(ctrl, env, ctx) {
// Call `fetch()` with known body length
{
const body = new FixedLengthStream(3);
const writer = body.writable.getWriter();
void writer.write(new Uint8Array([1, 2, 3]));
void writer.close();
const response = await env.SERVICE.fetch("http://placeholder/body-length", {
method: "POST",
body: body.readable,
});
const headers = new Headers(await response.json());
assert.strictEqual(headers.get("Content-Length"), "3");
assert.strictEqual(headers.get("Transfer-Encoding"), null);
}

// Check `fetch()` with unknown body length
{
const body = new IdentityTransformStream();
const writer = body.writable.getWriter();
void writer.write(new Uint8Array([1, 2, 3]));
void writer.close();
const response = await env.SERVICE.fetch("http://placeholder/body-length", {
method: "POST",
body: body.readable,
});
const headers = new Headers(await response.json());
assert.strictEqual(headers.get("Content-Length"), null);
assert.strictEqual(headers.get("Transfer-Encoding"), "chunked");
}

// Call `scheduled()` with no options
{
const result = await env.SERVICE.scheduled();
Expand Down
4 changes: 4 additions & 0 deletions src/workerd/api/http.c++
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,10 @@ bool Headers::has(jsg::ByteString name) {

void Headers::set(jsg::ByteString name, jsg::ByteString value) {
checkGuard();
setUnguarded(kj::mv(name), kj::mv(value));
}

void Headers::setUnguarded(jsg::ByteString name, jsg::ByteString value) {
requireValidHeaderName(name);
auto key = toLower(name);
value = normalizeHeaderValue(kj::mv(value));
Expand Down
3 changes: 3 additions & 0 deletions src/workerd/api/http.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ class Headers: public jsg::Object {
// is not permitted to be combined into a single instance.
bool has(jsg::ByteString name);
void set(jsg::ByteString name, jsg::ByteString value);
void setUnguarded(jsg::ByteString name, jsg::ByteString value);
// Like set(), but ignores the header guard if set. This can only be called from C++, and may be
// used to mutate headers before dispatching a request.
void append(jsg::ByteString name, jsg::ByteString value);
void delete_(jsg::ByteString name);
void forEach(jsg::Lock& js,
Expand Down
Loading