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

Warn if URLSearchParams has() or delete() are called with multiple args #635

Merged
merged 3 commits into from
May 12, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 20 additions & 2 deletions src/workerd/api/url-standard.c++
Original file line number Diff line number Diff line change
Expand Up @@ -1943,7 +1943,16 @@ void URLSearchParams::append(jsg::UsvString name, jsg::UsvString value) {
update();
}

void URLSearchParams::delete_(jsg::UsvString name) {
void URLSearchParams::delete_(jsg::UsvString name, jsg::Optional<jsg::Value> value) {
KJ_IF_MAYBE(v, value) {
// The WHATWG recently added new arguments to the delete() and has() methods.
// We need to determine if adding those will break anyone if they are added
// without a compat flag. For now, we're just logging so we can know for sure.
// If we get this warning even once in production we'll have to introduce the
// new arguments behind a compat flag.
// https://github.com/whatwg/url/pull/735
LOG_WARNING_ONCE("URLSearchParams.prototype.delete() called with a second argument.");
}
auto pivot = std::remove_if(list.begin(), list.end(),
[&name](auto& kv) { return kv.name == name; });
list.truncate(pivot - list.begin());
Expand All @@ -1965,7 +1974,16 @@ kj::Array<jsg::UsvStringPtr> URLSearchParams::getAll(jsg::UsvString name) {
return result.releaseAsArray();
}

bool URLSearchParams::has(jsg::UsvString name) {
bool URLSearchParams::has(jsg::UsvString name, jsg::Optional<jsg::Value> value) {
KJ_IF_MAYBE(v, value) {
// The WHATWG recently added new arguments to the delete() and has() methods.
// We need to determine if adding those will break anyone if they are added
// without a compat flag. For now, we're just logging so we can know for sure.
// If we get this warning even once in production we'll have to introduce the
// new arguments behind a compat flag.
// https://github.com/whatwg/url/pull/735
LOG_WARNING_ONCE("URLSearchParams.prototype.has() called with a second argument.");
}
for (auto& entry : list) {
if (entry.name == name) return true;
}
Expand Down
4 changes: 2 additions & 2 deletions src/workerd/api/url-standard.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,10 @@ class URLSearchParams: public jsg::Object {
}

void append(jsg::UsvString name, jsg::UsvString value);
void delete_(jsg::UsvString name);
void delete_(jsg::UsvString name, jsg::Optional<jsg::Value> value);
kj::Maybe<jsg::UsvStringPtr> get(jsg::UsvString name);
kj::Array<jsg::UsvStringPtr> getAll(jsg::UsvString name);
bool has(jsg::UsvString name);
bool has(jsg::UsvString name, jsg::Optional<jsg::Value> value);
void set(jsg::UsvString name, jsg::UsvString value);
void sort();

Expand Down
9 changes: 9 additions & 0 deletions src/workerd/util/sentry.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,15 @@ inline kj::StringPtr maybeOmitColoFromSentry(uint32_t coloId) {
KJ_LOG(severity, __VA_ARGS__); \
}

// Log this to Sentry once ever per process. Typically will be better to use LOG_ERROR_PERIODICALLY.
jasnell marked this conversation as resolved.
Show resolved Hide resolved
#define LOG_WARNING_ONCE(...) \
do { \
static bool logOnce KJ_UNUSED = [&]() { \
KJ_LOG(WARNING, __VA_ARGS__); \
return true; \
}(); \
} while (0)

// Log this to Sentry once ever per process. Typically will be better to use LOG_ERROR_PERIODICALLY.
#define LOG_ERROR_ONCE(...) \
do { \
Expand Down