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 iterating headers with set-cookie #4048

Merged
merged 6 commits into from
Aug 8, 2023
Merged

fix iterating headers with set-cookie #4048

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6dc5035
fix iterating headers with `set-cookie`
dylan-conway Aug 7, 2023
4110307
a test
dylan-conway Aug 7, 2023
f925ec2
move work to `HTTPHeaderMap::set`
dylan-conway Aug 8, 2023
2bd21c6
append set-cookie after sort
dylan-conway Aug 8, 2023
10c9d80
remove compare function
dylan-conway Aug 8, 2023
04638d6
Merge branch 'main' into dylan/fix-set-cookie-headers
dylan-conway Aug 8, 2023
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
45 changes: 37 additions & 8 deletions src/bun.js/bindings/webcore/FetchHeaders.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

#include "config.h"
#include "FetchHeaders.h"
#include "HTTPHeaderNames.h"

#include "HTTPParsers.h"

Expand Down Expand Up @@ -261,30 +262,58 @@ void FetchHeaders::filterAndFill(const HTTPHeaderMap& headers, Guard guard)
}
}

static NeverDestroyed<const String> setCookieLowercaseString(MAKE_STATIC_STRING_IMPL("set-cookie"));
static const WTF::String setCookieLowercaseString = WTF::staticHeaderNames[static_cast<uint8_t>(HTTPHeaderName::SetCookie)];

static bool compareIteratorKeys(const String& a, const String& b)
{
// null in the iterator's m_keys represents Set-Cookie.
return WTF::codePointCompareLessThan(
a.isNull() ? "set-cookie"_s : a,
b.isNull() ? "set-cookie"_s : b);
}

std::optional<KeyValuePair<String, String>> FetchHeaders::Iterator::next()
{
if (m_keys.isEmpty() || m_updateCounter != m_headers->m_updateCounter) {
bool hasSetCookie = !m_headers->getSetCookieHeaders().isEmpty();
m_keys.resize(0);
m_keys.reserveCapacity(m_headers->m_headers.size());
m_keys.reserveCapacity(m_headers->m_headers.size() + (hasSetCookie ? 1 : 0));
for (auto& header : m_headers->m_headers)
m_keys.uncheckedAppend(header.asciiLowerCaseName());
std::sort(m_keys.begin(), m_keys.end(), WTF::codePointCompareLessThan);
if (hasSetCookie)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to pay this performance overhead here? Wouldn't it be fine if we put the Set-Cookie header either first or last?

Copy link
Member Author

Choose a reason for hiding this comment

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

we do, this is part of the fetch spec https://fetch.spec.whatwg.org/#convert-header-names-to-a-sorted-lowercase-set

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was also expected in the spec for keys to be unique which isn’t true for set-cookies

I think we should just always put it at the end and call it a day. Each call to next() shouldn’t slow it down

Copy link
Member Author

Choose a reason for hiding this comment

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

updated to append after the sort

m_keys.uncheckedAppend(String());
std::sort(m_keys.begin(), m_keys.end(), compareIteratorKeys);

m_currentIndex += m_cookieIndex;
if (hasSetCookie) {
size_t setCookieKeyIndex = std::lower_bound(m_keys.begin(), m_keys.end(), String(), compareIteratorKeys) - m_keys.begin();
if (m_currentIndex < setCookieKeyIndex)
m_cookieIndex = 0;
else {
m_cookieIndex = std::min(m_currentIndex - setCookieKeyIndex, m_headers->getSetCookieHeaders().size());
m_currentIndex -= m_cookieIndex;
}
} else
m_cookieIndex = 0;

m_updateCounter = m_headers->m_updateCounter;
m_cookieIndex = 0;
}

auto& setCookieHeaders = m_headers->m_headers.getSetCookieHeaders();

while (m_currentIndex < m_keys.size()) {
auto key = m_keys[m_currentIndex++];
auto key = m_keys[m_currentIndex];

if (!setCookieHeaders.isEmpty() && key == setCookieLowercaseString) {
auto cookie = setCookieHeaders[m_cookieIndex++];
return KeyValuePair<String, String> { WTFMove(key), WTFMove(cookie) };
if (key.isNull()) {
if (m_cookieIndex < setCookieHeaders.size()) {
String value = setCookieHeaders[m_cookieIndex++];
return KeyValuePair<String, String> { setCookieLowercaseString, WTFMove(value) };
}
m_currentIndex++;
continue;
}

m_currentIndex++;
auto value = m_headers->m_headers.get(key);
if (!value.isNull())
return KeyValuePair<String, String> { WTFMove(key), WTFMove(value) };
Expand Down
10 changes: 1 addition & 9 deletions src/bun.js/bindings/webcore/HTTPHeaderMap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,15 +236,7 @@ String HTTPHeaderMap::get(HTTPHeaderName name) const
void HTTPHeaderMap::set(HTTPHeaderName name, const String& value)
{
if (name == HTTPHeaderName::SetCookie) {
auto cookieName = extractCookieName(value);
size_t length = m_setCookieHeaders.size();
const auto& cookies = m_setCookieHeaders.data();
for (size_t i = 0; i < length; ++i) {
if (extractCookieName(cookies[i]) == cookieName) {
m_setCookieHeaders[i] = value;
return;
}
}
m_setCookieHeaders.clear();
m_setCookieHeaders.append(value);
return;
}
Expand Down
25 changes: 2 additions & 23 deletions src/bun.js/bindings/webcore/HTTPHeaderMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,9 @@ class HTTPHeaderMap {
: m_table(table)
, m_commonHeadersIt(commonHeadersIt)
, m_uncommonHeadersIt(uncommonHeadersIt)
, m_setCookiesIter(setCookiesIter)
{
if (!updateKeyValue(m_commonHeadersIt)) {
if (!updateSetCookieHeaderPosition(setCookiesIter)) {
updateKeyValue(m_uncommonHeadersIt);
}
updateKeyValue(m_uncommonHeadersIt);
}
}

Expand Down Expand Up @@ -107,9 +104,6 @@ class HTTPHeaderMap {
if (m_commonHeadersIt != m_table.m_commonHeaders.end()) {
if (updateKeyValue(++m_commonHeadersIt))
return *this;
} else if (m_setCookiesIter != m_table.m_setCookieHeaders.end()) {
if (updateSetCookieHeaderPosition(++m_setCookiesIter))
return *this;
} else {
++m_uncommonHeadersIt;
}
Expand All @@ -122,7 +116,7 @@ class HTTPHeaderMap {
bool operator!=(const HTTPHeaderMapConstIterator &other) const { return !(*this == other); }
bool operator==(const HTTPHeaderMapConstIterator &other) const
{
return m_commonHeadersIt == other.m_commonHeadersIt && m_uncommonHeadersIt == other.m_uncommonHeadersIt && m_setCookiesIter == other.m_setCookiesIter;
return m_commonHeadersIt == other.m_commonHeadersIt && m_uncommonHeadersIt == other.m_uncommonHeadersIt;
}

private:
Expand All @@ -145,22 +139,9 @@ class HTTPHeaderMap {
return true;
}

bool updateSetCookieHeaderPosition(Vector<String, 0>::const_iterator cookieI)
{
if (cookieI == m_table.m_setCookieHeaders.end()) {
return false;
}

m_keyValue.key = httpHeaderNameString(HTTPHeaderName::SetCookie).toStringWithoutCopying();
m_keyValue.keyAsHTTPHeaderName = HTTPHeaderName::SetCookie;
m_keyValue.value = *cookieI;
return true;
}

const HTTPHeaderMap &m_table;
CommonHeadersVector::const_iterator m_commonHeadersIt;
UncommonHeadersVector::const_iterator m_uncommonHeadersIt;
Vector<String, 0>::const_iterator m_setCookiesIter;
KeyValue m_keyValue;
};
typedef HTTPHeaderMapConstIterator const_iterator;
Expand All @@ -178,14 +159,12 @@ class HTTPHeaderMap {
{
m_commonHeaders.clear();
m_uncommonHeaders.clear();
m_setCookieHeaders.clear();
}

void shrinkToFit()
{
m_commonHeaders.shrinkToFit();
m_uncommonHeaders.shrinkToFit();
m_setCookieHeaders.shrinkToFit();
}

WEBCORE_EXPORT String get(const String &name) const;
Expand Down
12 changes: 10 additions & 2 deletions test/js/web/fetch/fetch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,9 +276,17 @@ describe("Headers", () => {
headers.set("set-Cookie", "foo=baz");
headers.set("set-cookie", "bar=qat");
const actual = [...headers];
expect(actual).toEqual([["set-cookie", "bar=qat"]]);
});

it("should include set-cookie headers in array", () => {
const headers = new Headers();
headers.append("Set-Cookie", "foo=bar");
headers.append("Content-Type", "text/plain");
const actual = [...headers];
expect(actual).toEqual([
["set-cookie", "foo=baz"],
["set-cookie", "bar=qat"],
["content-type", "text/plain"],
["set-cookie", "foo=bar"],
]);
});
});
Expand Down