Skip to content

Commit 783a335

Browse files
harrishancockclaude
andcommitted
fix: revive dead FormData Content-Type warning (#6067)
The logWarning() in Body::Body that warns when a FormData body is paired with a custom Content-Type header was dead code. The comparison used ConstMimeType::operator==(kj::StringPtr), which compared the full substring after '/' against the subtype — so 'form-data; boundary=...' never matched 'form-data'. Fix the comparison by parsing the content-type string into a MimeType first, then comparing against MimeType::FORM_DATA. The MimeType equality operator correctly ignores MIME parameters like boundary. Also remove ConstMimeType::operator==(kj::StringPtr) entirely — it was a footgun with only this one (buggy) caller, and any future use would hit the same parameter-ignoring trap. Add a streaming tail worker test that verifies the warning is emitted and captured in the tracing system when a Request is constructed with a FormData body and a custom Content-Type header. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent ee6d283 commit 783a335

File tree

9 files changed

+160
-16
lines changed

9 files changed

+160
-16
lines changed

src/workerd/api/cache.c++

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,9 @@ jsg::Promise<void> Cache::put(jsg::Lock& js,
335335
// this behavior. Silently discarding 304s maintains backwards compatibility and is actually
336336
// still spec-conformant.
337337

338+
// TODO(cleanup): This guard predates streaming tail workers. logWarning() already emits to
339+
// the tracer when one exists, so this check suppresses the warning for tail-worker-only
340+
// users. Consider replacing with a check that also accounts for getWorkerTracer().
338341
if (context.isInspectorEnabled()) {
339342
context.logWarning(
340343
"Ignoring attempt to Cache.put() a 304 status response. 304 responses "

src/workerd/api/headers.c++

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,9 @@ static_assert(std::size(COMMON_HEADER_NAMES) == (Headers::MAX_COMMON_HEADER_ID +
275275

276276
void maybeWarnIfBadHeaderString(kj::StringPtr name, kj::StringPtr str) {
277277
KJ_IF_SOME(context, IoContext::tryCurrent()) {
278+
// TODO(cleanup): This guard predates streaming tail workers. logWarning() already emits to
279+
// the tracer when one exists, so this check suppresses the warning for tail-worker-only
280+
// users. Consider replacing with a check that also accounts for getWorkerTracer().
278281
if (context.isInspectorEnabled()) {
279282
if (!simdutf::validate_ascii(str.begin(), str.size())) {
280283
// The string contains non-ASCII characters. While any 8-bit value is technically valid

src/workerd/api/http.c++

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -174,14 +174,16 @@ Body::Body(jsg::Lock& js, kj::Maybe<ExtractedBody> init, Headers& headers)
174174
// The spec allows the user to override the Content-Type, if they wish, so we only set
175175
// the Content-Type if it doesn't already exist.
176176
headers.setCommon(capnp::CommonHeaderName::CONTENT_TYPE, kj::mv(ct));
177-
} else if (MimeType::FORM_DATA == ct) {
178-
// Custom content-type request/responses with FormData are broken since they require a
179-
// boundary parameter only the FormData serializer can provide. Let's warn if a dev does this.
180-
IoContext::current().logWarning(
181-
"A FormData body was provided with a custom Content-Type header when constructing "
182-
"a Request or Response object. This will prevent the recipient of the Request or "
183-
"Response from being able to parse the body. Consider omitting the custom "
184-
"Content-Type header.");
177+
} else KJ_IF_SOME(parsed, MimeType::tryParse(ct)) {
178+
if (MimeType::FORM_DATA == parsed) {
179+
// Custom content-type request/responses with FormData are broken since they require a
180+
// boundary parameter only the FormData serializer can provide. Let's warn if a dev does this.
181+
IoContext::current().logWarning(
182+
"A FormData body was provided with a custom Content-Type header when constructing "
183+
"a Request or Response object. This will prevent the recipient of the Request or "
184+
"Response from being able to parse the body. Consider omitting the custom "
185+
"Content-Type header.");
186+
}
185187
}
186188
}
187189
return kj::mv(i.impl);
@@ -271,6 +273,9 @@ jsg::Promise<kj::String> Body::text(jsg::Lock& js) {
271273
// search-and-replace across your whole site and you forgot that it'll apply to images too.
272274
// When running in the fiddle, let's warn the developer if they do this.
273275
auto& context = IoContext::current();
276+
// TODO(cleanup): This guard predates streaming tail workers. logWarning() already emits to
277+
// the tracer when one exists, so this check suppresses the warning for tail-worker-only
278+
// users. Consider replacing with a check that also accounts for getWorkerTracer().
274279
if (context.isInspectorEnabled()) {
275280
KJ_IF_SOME(type, headersRef.getCommon(js, capnp::CommonHeaderName::CONTENT_TYPE)) {
276281
maybeWarnIfNotText(js, type);
@@ -1058,6 +1063,9 @@ jsg::Ref<Response> Response::constructor(jsg::Lock& js,
10581063
"Response with null body status (101, 204, 205, or 304) cannot have a body.");
10591064

10601065
auto& context = IoContext::current();
1066+
// TODO(cleanup): This guard predates streaming tail workers. logWarning() already emits to
1067+
// the tracer when one exists, so this check suppresses the warning for tail-worker-only
1068+
// users. Consider replacing with a check that also accounts for getWorkerTracer().
10611069
if (context.isInspectorEnabled()) {
10621070
context.logWarning(kj::str("Constructing a Response with a null body status (", statusCode,
10631071
") and a non-null, "

src/workerd/api/streams/internal.c++

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ namespace {
2323
// papers over is fixed.
2424
[[noreturn]] void throwTypeErrorAndConsoleWarn(kj::StringPtr message) {
2525
KJ_IF_SOME(context, IoContext::tryCurrent()) {
26+
// TODO(cleanup): This guard predates streaming tail workers. logWarning() already emits to
27+
// the tracer when one exists, so this check suppresses the warning for tail-worker-only
28+
// users. Consider replacing with a check that also accounts for getWorkerTracer().
2629
if (context.isInspectorEnabled()) {
2730
context.logWarning(message);
2831
}
@@ -918,6 +921,10 @@ ReadableStreamController::Tee ReadableStreamInternalController::tee(jsg::Lock& j
918921
auto makeTee = [&](kj::Own<ReadableStreamSource> b1,
919922
kj::Own<ReadableStreamSource> b2) -> Tee {
920923
doClose(js);
924+
// TODO(cleanup): This guard predates streaming tail workers. The WarnIfUnusedStream
925+
// wrapper calls logWarning(), which already emits to the tracer when one exists, so this
926+
// check suppresses the warning for tail-worker-only users. Consider replacing with a
927+
// check that also accounts for getWorkerTracer().
921928
if (ioContext.isInspectorEnabled()) {
922929
b1 = kj::heap<WarnIfUnusedStream>(js, kj::mv(b1), ioContext);
923930
b2 = kj::heap<WarnIfUnusedStream>(js, kj::mv(b2), ioContext);

src/workerd/api/tests/BUILD.bazel

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,15 @@ wd_test(
270270
data = ["form-data-test-ts.ts"],
271271
)
272272

273+
wd_test(
274+
src = "warnings-test.wd-test",
275+
args = ["--experimental"],
276+
data = [
277+
"warnings-tail.js",
278+
"warnings-test.js",
279+
],
280+
)
281+
273282
wd_test(
274283
size = "large",
275284
src = "global-scope-test.wd-test",
Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
// Copyright (c) 2026 Cloudflare, Inc.
2+
// Licensed under the Apache 2.0 license found in the LICENSE file or at:
3+
// https://opensource.org/licenses/Apache-2.0
4+
import * as assert from 'node:assert';
5+
6+
// ================================================================================================
7+
// Expected warnings registry.
8+
//
9+
// Add new entries here when introducing logWarning() calls. Each entry is a substring that must
10+
// appear in at least one warn-level tail event's message. The trigger worker (warnings-test.js)
11+
// must exercise a code path that produces each warning.
12+
//
13+
// At present, the FormData warning is the only logWarning() call that is easily testable here.
14+
// Several other warnings (e.g. .text() on non-text body, null-body status with zero-length body,
15+
// Cache.put() with 304, non-ASCII header values) are guarded by isInspectorEnabled() checks that
16+
// predate streaming tail workers. Those guards suppress the warning for tail-worker-only users.
17+
// If we relax those checks (see TODO(cleanup) comments at each site), we could add tests for
18+
// those warnings here too.
19+
const EXPECTED_WARNINGS = [
20+
// From Body::Body (http.c++) — FormData body with a custom Content-Type header.
21+
'FormData body was provided',
22+
];
23+
// ================================================================================================
24+
25+
let allEvents = [];
26+
27+
export default {
28+
tailStream(event) {
29+
allEvents.push(event.event);
30+
return (event) => {
31+
allEvents.push(event.event);
32+
};
33+
},
34+
};
35+
36+
export const test = {
37+
async test() {
38+
// HACK: Tail events from the trigger worker's fetch invocation are delivered asynchronously
39+
// across service boundaries. There is no JS-level synchronization primitive to await their
40+
// arrival — a plain `await promise` doesn't drive the I/O loop that delivers them. We must
41+
// use scheduler.wait() to yield while the runtime processes pending I/O.
42+
const TIMEOUT_MS = 5000;
43+
const POLL_MS = 10;
44+
const deadline = Date.now() + TIMEOUT_MS;
45+
46+
const unseen = () =>
47+
EXPECTED_WARNINGS.filter(
48+
(substring) =>
49+
!allEvents.some(
50+
(e) =>
51+
e.type === 'log' &&
52+
e.level === 'warn' &&
53+
e.message?.[0]?.includes(substring)
54+
)
55+
);
56+
57+
while (unseen().length > 0 && Date.now() < deadline) {
58+
await scheduler.wait(POLL_MS);
59+
}
60+
61+
const missing = unseen();
62+
assert.strictEqual(
63+
missing.length,
64+
0,
65+
`${missing.length} expected warning(s) were not observed after ${TIMEOUT_MS}ms:\n` +
66+
missing.map((s) => ` - "${s}"`).join('\n')
67+
);
68+
},
69+
};
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// Copyright (c) 2026 Cloudflare, Inc.
2+
// Licensed under the Apache 2.0 license found in the LICENSE file or at:
3+
// https://opensource.org/licenses/Apache-2.0
4+
5+
export default {
6+
async fetch(request) {
7+
const formData = new FormData();
8+
formData.append('key', 'value');
9+
// Constructing a Request with a FormData body and a custom Content-Type header should
10+
// produce a logWarning, because the custom Content-Type will lack the boundary parameter
11+
// that the FormData serializer generates, preventing the recipient from parsing the body.
12+
const req = new Request('http://example.com', {
13+
method: 'POST',
14+
body: formData,
15+
headers: { 'Content-Type': 'multipart/form-data' },
16+
});
17+
return new Response('ok');
18+
},
19+
};
20+
21+
export const test = {
22+
async test(ctrl, env) {
23+
// Invoke via service binding so the fetch() handler runs in a traced invocation
24+
// (test() handlers are intentionally not traced by the test runner).
25+
await env.SELF.fetch('http://dummy');
26+
},
27+
};
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
using Workerd = import "/workerd/workerd.capnp";
2+
3+
const unitTests :Workerd.Config = (
4+
services = [
5+
( name = "trigger",
6+
worker = (
7+
modules = [
8+
( name = "worker", esModule = embed "warnings-test.js" )
9+
],
10+
bindings = [
11+
( name = "SELF", service = "trigger" ),
12+
],
13+
compatibilityFlags = ["experimental", "nodejs_compat"],
14+
streamingTails = ["tail"],
15+
),
16+
),
17+
( name = "tail",
18+
worker = (
19+
modules = [
20+
( name = "worker", esModule = embed "warnings-tail.js" )
21+
],
22+
compatibilityFlags = ["experimental", "nodejs_compat"],
23+
),
24+
),
25+
],
26+
);

src/workerd/util/mimetype.h

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,6 @@ class ConstMimeType final {
3232
return this == &other || (type_ == other.type_ && subtype_ == other.subtype_);
3333
}
3434

35-
inline bool operator==(kj::StringPtr other) const {
36-
KJ_IF_SOME(slashPos, other.findFirst('/')) {
37-
return strcaseeq(type_, other.first(slashPos)) &&
38-
strcaseeq(subtype_, other.slice(slashPos + 1));
39-
}
40-
return false;
41-
}
42-
4335
bool operator==(const MimeType& other) const;
4436
operator MimeType() const;
4537
MimeType clone() const;

0 commit comments

Comments
 (0)