Skip to content

Commit

Permalink
fix: shimming of http.request() with node v18.17.0 caused a subtle …
Browse files Browse the repository at this point in the history
…edge case breakage (#3511)

Before this change, a call with a non-Function callback:

    http.request(urlString, {}, 'this-is-not-a-cb-function')

would accidentally *not* fail because of the agent's instrumentation.
This is because the Node.js internal `isURL()` check changed such that
the result of `urlToHttpOptions(url)` passes `isURL()`.
  • Loading branch information
trentm authored Jul 29, 2023
1 parent 5fe6fe1 commit a696dd9
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 2 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ Notes:
[float]
===== Bug fixes
* Fix wrapping of `http.request()` for node v18.17.0. Before this change, a
call with a non-Function callback -- `http.request(urlString, {}, 'this-is-not-a-cb-function')`
-- would accidentally *not* fail because of the agent's instrumentation.
* Fix tedious instrumentation to recognize "connection.prepare()" usage in
tedious@16.2.0 and later.
Expand Down
7 changes: 5 additions & 2 deletions lib/instrumentation/http-shared.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,16 @@ if (!urlToHttpOptions) {
}
return options
}
} else if (semver.satisfies(process.version, '>=19.9.0 <20')) {
} else if (semver.satisfies(process.version, '>=19.9.0 <20') ||
semver.satisfies(process.version, '>=18.17.0 <19')) {
// Starting in node v19.9.0 (as of https://github.com/nodejs/node/pull/46989)
// `urlToHttpOptions(url)` returns an object which is considered a `url`
// argument by `http.request()` -- because of the internal `isURL(url)` test.
// Starting with node v18.17.0, the same is true with the internal switch
// to the "Ada" lib for URL parsing.
safeUrlToHttpOptions = function (url) {
const options = urlToHttpOptions(url)
// Specically we are dropping the `Symbol(context)` field.
// Specifically we are dropping the `Symbol(context)` field.
Object.getOwnPropertySymbols(options).forEach(sym => {
delete options[sym]
})
Expand Down

0 comments on commit a696dd9

Please sign in to comment.