Fix #1602 (#2066)

* initial test case

* fix segfault from JSObjectMakeDeferredPromise

* pass exceptions through from FetchHeader.createFromJS

* not resolved, but getting close

* implement review suggestions

* fix exception check, tests

* Change how header filtering is accomplished

Previously the FetchHeaders implementation relied on converting names and values
 to IDLByteString to catch non-ASCII data, though not always reliably.  This
 resulted in message-less TypeErrors when headers contained invalid characters.
This commit shifts everything to IDLDOMString for the conversion and relies on
 the actual error checking in FetchHeaders.canWriteHeader, resulting in nicer
 error messages.

To ensure that all headers are written as ASCII/UTF8 rather than UTF16, the
 copyTo bindings function checks the encoding and converts if necessary.

* wrapping up FetchHeader fixes

* since utf8 allocates only do so when needed

* Update src/bun.js/bindings/bindings.cpp

Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>

* WebCore__FetchHeaders__has should return on exception path

* strip out log calls from test

---------

Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>

1 files changed, 85 insertions, 23 deletions

diff --git a/src/bun.js/bindings/bindings.cpp b/src/bun.js/bindings/bindings.cpp
index 4b614788d..bb01e4101 100644
--- a/src/bun.js/bindings/bindings.cpp
+++ b/src/bun.js/bindings/bindings.cpp
@@ -97,19 +97,27 @@ static void copyToUWS(WebCore::FetchHeaders* headers, UWSResponse* res)
     auto& internalHeaders = headers->internalHeaders();
 
     for (auto& value : internalHeaders.getSetCookieHeaders()) {
-        res->writeHeader(std::string_view("set-cookie", 10), std::string_view(reinterpret_cast<const char*>(value.characters8()), value.length()));
+        res->writeHeader(std::string_view("set-cookie", 10), std::string_view(
+            value.is8Bit() ? reinterpret_cast<const char*>(value.characters8()) : value.utf8().data(), value.length()
+        ));
     }
 
     for (auto& header : internalHeaders.commonHeaders()) {
         const auto& name = WebCore::httpHeaderNameString(header.key);
         auto& value = header.value;
-        res->writeHeader(std::string_view(reinterpret_cast<const char*>(name.characters8()), name.length()), std::string_view(reinterpret_cast<const char*>(value.characters8()), value.length()));
+        res->writeHeader(
+            std::string_view(name.is8Bit() ? reinterpret_cast<const char*>(name.characters8()) : name.utf8().data(), name.length()),
+            std::string_view(value.is8Bit() ? reinterpret_cast<const char*>(value.characters8()) : value.utf8().data(), value.length())
+        );
     }
 
     for (auto& header : internalHeaders.uncommonHeaders()) {
         auto& name = header.key;
         auto& value = header.value;
-        res->writeHeader(std::string_view(reinterpret_cast<const char*>(name.characters8()), name.length()), std::string_view(reinterpret_cast<const char*>(value.characters8()), value.length()));
+        res->writeHeader(
+            std::string_view(name.is8Bit() ? reinterpret_cast<const char*>(name.characters8()) : name.utf8().data(), name.length()),
+            std::string_view(value.is8Bit() ? reinterpret_cast<const char*>(value.characters8()) : value.utf8().data(), value.length())
+        );
     }
 }
 
@@ -709,9 +717,13 @@ WebCore__FetchHeaders* WebCore__FetchHeaders__createEmpty()
 {
     return new WebCore::FetchHeaders({ WebCore::FetchHeaders::Guard::None, {} });
 }
-void WebCore__FetchHeaders__append(WebCore__FetchHeaders* headers, const ZigString* arg1, const ZigString* arg2)
+void WebCore__FetchHeaders__append(WebCore__FetchHeaders* headers, const ZigString* arg1, const ZigString* arg2,
+                                   JSC__JSGlobalObject* lexicalGlobalObject)
 {
-    headers->append(Zig::toString(*arg1), Zig::toString(*arg2));
+    auto throwScope = DECLARE_THROW_SCOPE(lexicalGlobalObject->vm());
+    WebCore::propagateException(*lexicalGlobalObject, throwScope,
+        headers->append(Zig::toString(*arg1), Zig::toString(*arg2))
+    );
 }
 WebCore__FetchHeaders* WebCore__FetchHeaders__cast_(JSC__JSValue JSValue0, JSC__VM* vm)
 {
@@ -724,12 +736,25 @@ WebCore__FetchHeaders* WebCore__FetchHeaders__createFromJS(JSC__JSGlobalObject*
 {
     Zig::GlobalObject* globalObject = reinterpret_cast<Zig::GlobalObject*>(lexicalGlobalObject);
     EnsureStillAliveScope argument0 = JSC::JSValue::decode(argument0_);
+
     auto throwScope = DECLARE_THROW_SCOPE(lexicalGlobalObject->vm());
-    auto init = argument0.value().isUndefined() ? std::optional<Converter<IDLUnion<IDLSequence<IDLSequence<IDLByteString>>, IDLRecord<IDLByteString, IDLByteString>>>::ReturnType>() : std::optional<Converter<IDLUnion<IDLSequence<IDLSequence<IDLByteString>>, IDLRecord<IDLByteString, IDLByteString>>>::ReturnType>(convert<IDLUnion<IDLSequence<IDLSequence<IDLByteString>>, IDLRecord<IDLByteString, IDLByteString>>>(*lexicalGlobalObject, argument0.value()));
+    // Note that we use IDLDOMString here rather than IDLByteString: while headers
+    //  should be ASCII only, we want the headers->fill implementation to discover
+    //  and error on invalid names and values
+    using TargetType = IDLUnion<IDLSequence<IDLSequence<IDLDOMString>>, IDLRecord<IDLDOMString, IDLDOMString>>;
+    using Converter = std::optional<Converter<TargetType>::ReturnType>;
+    auto init = argument0.value().isUndefined() ? Converter() : Converter(convert<TargetType>(*lexicalGlobalObject, argument0.value()));
     RETURN_IF_EXCEPTION(throwScope, nullptr);
+
     auto* headers = new WebCore::FetchHeaders({ WebCore::FetchHeaders::Guard::None, {} });
-    if (init)
-        headers->fill(WTFMove(init.value()));
+    if (init) {
+        // `fill` doesn't set an exception on the VM if it fails, it returns an
+        //  ExceptionOr<void>.  So we need to check for the exception and, if set,
+        //  translate it to JSValue and throw it.
+        WebCore::propagateException(*lexicalGlobalObject, throwScope,
+            headers->fill(WTFMove(init.value()))
+        );
+    }
     return headers;
 }
 
@@ -741,16 +766,22 @@ JSC__JSValue WebCore__FetchHeaders__toJS(WebCore__FetchHeaders* headers, JSC__JS
 }
 JSC__JSValue WebCore__FetchHeaders__clone(WebCore__FetchHeaders* headers, JSC__JSGlobalObject* arg1)
 {
+    auto throwScope = DECLARE_THROW_SCOPE(arg1->vm());
     Zig::GlobalObject* globalObject = reinterpret_cast<Zig::GlobalObject*>(arg1);
     auto* clone = new WebCore::FetchHeaders({ WebCore::FetchHeaders::Guard::None, {} });
-    clone->fill(*headers);
+    WebCore::propagateException(*arg1, throwScope,
+        clone->fill(*headers)
+    );
     return JSC::JSValue::encode(WebCore::toJSNewlyCreated(arg1, globalObject, WTFMove(clone)));
 }
 
-WebCore__FetchHeaders* WebCore__FetchHeaders__cloneThis(WebCore__FetchHeaders* headers)
+WebCore__FetchHeaders* WebCore__FetchHeaders__cloneThis(WebCore__FetchHeaders* headers, JSC__JSGlobalObject* lexicalGlobalObject)
 {
+    auto throwScope = DECLARE_THROW_SCOPE(lexicalGlobalObject->vm());
     auto* clone = new WebCore::FetchHeaders({ WebCore::FetchHeaders::Guard::None, {} });
-    clone->fill(*headers);
+    WebCore::propagateException(*lexicalGlobalObject, throwScope,
+        clone->fill(*headers)
+    );
     return clone;
 }
 
@@ -764,14 +795,25 @@ void WebCore__FetchHeaders__copyTo(WebCore__FetchHeaders* headers, StringPointer
     auto iter = headers->createIterator();
     uint32_t i = 0;
     unsigned count = 0;
+
     for (auto pair = iter.next(); pair; pair = iter.next()) {
         auto name = pair->key;
         auto value = pair->value;
         names[count] = { i, name.length() };
-        memcpy(&buf[i], name.characters8(), name.length());
+
+        if (name.is8Bit())
+            memcpy(&buf[i], name.characters8(), name.length());
+        else {
+            StringImpl::copyCharacters(&buf[i], name.characters16(), name.length());
+        }
+
         i += name.length();
         values[count++] = { i, value.length() };
-        memcpy(&buf[i], value.characters8(), value.length());
+        if (value.is8Bit())
+            memcpy(&buf[i], value.characters8(), value.length());
+        else
+            StringImpl::copyCharacters(&buf[i], value.characters16(), value.length());
+
         i += value.length();
     }
 }
@@ -883,6 +925,7 @@ void WebCore__FetchHeaders__deref(WebCore__FetchHeaders* arg0)
 
 JSC__JSValue WebCore__FetchHeaders__createValue(JSC__JSGlobalObject* arg0, StringPointer* arg1, StringPointer* arg2, const ZigString* arg3, uint32_t count)
 {
+    auto throwScope = DECLARE_THROW_SCOPE(arg0->vm());
     Vector<KeyValuePair<String, String>> pairs;
     pairs.reserveCapacity(count);
     ZigString buf = *arg3;
@@ -893,25 +936,44 @@ JSC__JSValue WebCore__FetchHeaders__createValue(JSC__JSGlobalObject* arg0, Strin
     }
 
     Ref<WebCore::FetchHeaders> headers = WebCore::FetchHeaders::create();
-    headers->fill(WebCore::FetchHeaders::Init(WTFMove(pairs)));
+    WebCore::propagateException(*arg0, throwScope, 
+        headers->fill(WebCore::FetchHeaders::Init(WTFMove(pairs)))
+    );
     pairs.releaseBuffer();
     return JSC::JSValue::encode(WebCore::toJSNewlyCreated(arg0, reinterpret_cast<Zig::GlobalObject*>(arg0), WTFMove(headers)));
 }
-void WebCore__FetchHeaders__get_(WebCore__FetchHeaders* headers, const ZigString* arg1, ZigString* arg2)
+void WebCore__FetchHeaders__get_(WebCore__FetchHeaders* headers, const ZigString* arg1, ZigString* arg2, JSC__JSGlobalObject* global)
 {
-    *arg2 = Zig::toZigString(headers->get(Zig::toString(*arg1)).releaseReturnValue());
+    auto throwScope = DECLARE_THROW_SCOPE(global->vm());
+    auto result = headers->get(Zig::toString(*arg1));
+    if (result.hasException())
+        WebCore::propagateException(*global, throwScope, result.releaseException());
+    else
+        *arg2 = Zig::toZigString(result.releaseReturnValue());
 }
-bool WebCore__FetchHeaders__has(WebCore__FetchHeaders* headers, const ZigString* arg1)
+bool WebCore__FetchHeaders__has(WebCore__FetchHeaders* headers, const ZigString* arg1, JSC__JSGlobalObject* global)
 {
-    return headers->has(Zig::toString(*arg1)).releaseReturnValue();
+    auto throwScope = DECLARE_THROW_SCOPE(global->vm());
+    auto result = headers->has(Zig::toString(*arg1));
+    if (result.hasException()) {
+        WebCore::propagateException(*global, throwScope, result.releaseException());
+        return false;
+    } else
+        return result.releaseReturnValue();
 }
-void WebCore__FetchHeaders__put_(WebCore__FetchHeaders* headers, const ZigString* arg1, const ZigString* arg2)
+void WebCore__FetchHeaders__put_(WebCore__FetchHeaders* headers, const ZigString* arg1, const ZigString* arg2, JSC__JSGlobalObject* global)
 {
-    headers->set(Zig::toString(*arg1), Zig::toString(*arg2));
+    auto throwScope = DECLARE_THROW_SCOPE(global->vm());
+    WebCore::propagateException(*global, throwScope,
+        headers->set(Zig::toString(*arg1), Zig::toString(*arg2))
+    );
 }
-void WebCore__FetchHeaders__remove(WebCore__FetchHeaders* headers, const ZigString* arg1)
+void WebCore__FetchHeaders__remove(WebCore__FetchHeaders* headers, const ZigString* arg1, JSC__JSGlobalObject* global)
 {
-    headers->remove(Zig::toString(*arg1));
+    auto throwScope = DECLARE_THROW_SCOPE(global->vm());
+    WebCore::propagateException(*global, throwScope,
+        headers->remove(Zig::toString(*arg1))
+    );
 }
 
 void WebCore__FetchHeaders__fastRemove_(WebCore__FetchHeaders* headers, unsigned char headerName)
@@ -3876,4 +3938,4 @@ CPP_DECL JSC__JSValue WebCore__DOMFormData__create(JSC__JSGlobalObject* arg0)
 CPP_DECL WebCore__DOMFormData* WebCore__DOMFormData__fromJS(JSC__JSValue JSValue1)
 {
     return WebCoreCast<WebCore::JSDOMFormData, WebCore__DOMFormData>(JSValue1);
-}
\ No newline at end of file
+}