fix(mock): non-string path matchers under ignoreTrailingSlash, and DataView reply bodies - #5619
Open
marko1olo wants to merge 4 commits into
Open
fix(mock): non-string path matchers under ignoreTrailingSlash, and DataView reply bodies#5619marko1olo wants to merge 4 commits into
marko1olo wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5619 +/- ##
==========================================
+ Coverage 93.49% 93.51% +0.01%
==========================================
Files 110 110
Lines 38429 38461 +32
==========================================
+ Hits 35931 35965 +34
+ Misses 2498 2496 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ilingSlash
getMockDispatch normalizes both sides of a path comparison before matching.
On the ignoreTrailingSlash branch it calls removeTrailingSlash on the
registered matcher, but that helper went straight to path.endsWith, so a
matcher that was not a string threw "path.endsWith is not a function" out of
the dispatch. The TypeError carries no code, so buildMockDispatch's catch
rethrew it rather than falling back to net connect, and the caller saw a
TypeError instead of either a match or a MockNotMatchedError.
MockPool.intercept documents path as {string|RegExp|Function}, and safeUrl —
the other normalizer applied on the same line — already returns non-strings
untouched. removeTrailingSlash now does the same: a RegExp or function has no
trailing slash to strip, so it is handed to matchValue unchanged and tested
against the request path that has already had its own trailing slashes
removed. That is the semantics ignoreTrailingSlash promises, so /^\/foo$/ now
matches a request for /foo/ as well as /foo. The guard also covers the
resolved request path, which getMockDispatch already allows to be a
non-string.
Signed-off-by: marko1olo <marko1olo@users.noreply.github.com>
getResponseData recognises Buffer, Uint8Array and ArrayBuffer as byte
containers and passes them straight through; everything else that is
typeof 'object' falls to JSON.stringify. A DataView is none of the three, so
reply(200, dataView) serialized to the two characters "{}" and the mock
delivered those instead of the body. JSON.stringify sees no own enumerable
properties on a DataView, so the corruption is silent: a valid, parseable,
empty JSON object rather than an error. The reply type is TData | Buffer |
string with TData extends object, so TypeScript accepts a DataView and gives
no warning either.
Returning the DataView itself is not enough. handleReply wraps the result in
Buffer.from(), and Buffer.from() of a DataView hits the fromObject path where
length is undefined while buffer is an ArrayBuffer, which yields an empty
buffer -- the body would still be lost, just as zero bytes rather than as
"{}". So the new branch produces a Uint8Array over exactly the region the view
covers, honouring byteOffset and byteLength; a view onto a slice of a larger
ArrayBuffer sends its own bytes and not the whole backing store.
The branch is keyed on ArrayBuffer.isView rather than an instanceof DataView
check, which also covers Uint8ClampedArray, Int16Array and the rest. Those are
byte containers too, and their current output -- {"0":1,"1":2} from index keys
-- is no more useful than "{}". The branch sits after the existing Buffer,
Uint8Array and ArrayBuffer cases so those keep returning the caller's exact
object, unchanged and uncopied.
Signed-off-by: marko1olo <marko1olo@users.noreply.github.com>
Contributor
Author
|
CI failures here are unrelated to this PR:
Both are h2 test flakes; this PR only touches |
marko1olo
force-pushed
the
fix/mock-utils-bytes-and-matchers
branch
from
July 30, 2026 07:28
4d66958 to
825a739
Compare
metcoder95
reviewed
Jul 30, 2026
| // rather than a plain object. Buffer.from() cannot read one directly, so | ||
| // expose the bytes it covers instead of letting it reach JSON.stringify. | ||
| return new Uint8Array(data.buffer, data.byteOffset, data.byteLength) | ||
| } else if (data && typeof data === 'object') { |
Member
There was a problem hiding this comment.
Suggested change
| } else if (data && typeof data === 'object') { | |
| } else if (typeof data === 'object') { |
Contributor
Author
There was a problem hiding this comment.
Done in 29d60a2e. Thanks!
metcoder95
approved these changes
Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent MockAgent defects, one commit each.
ignoreTrailingSlashthrows on a non-string path matcherremoveTrailingSlash()callspath.endsWith('/')unconditionally, but it is only reached on theignoreTrailingSlashbranch, where the argument is the registered matcher — documented indocs/docs/api/MockPool.md:97as{string|RegExp|Function}.safeUrl()a few lines up gets this right with atypeofguard. Sothrows a
TypeErrorinstead of matching.A
DataViewreply body is delivered as{}getResponseData()checks forBuffer, thenUint8Array, thenArrayBuffer, and everything else that is an object falls toJSON.stringify. ADataViewis none of the first three, soreply(200, someDataView)sent the two characters{}.Measured through
getResponseData()onmainbefore the change:BufferUint8ArrayArrayBufferDataView{}The fix returns a
Uint8Arrayover the same memory, and deliberately passesbyteOffsetandbyteLength:A
DataViewis frequently a window into a larger buffer, sonew Uint8Array(data.buffer)alone would send bytes the caller never offered. There is a test for exactly that case.Tests
Added to
test/mock-utils.jsandtest/mock-interceptor.js, in the style of the neighbours. Revertinglib/mock/mock-utils.jsand keeping the tests:With both fixes in place:
mock-agent.js99/99,mock-utils.js36/36,mock-interceptor.js86/86,mock-pool.js16/16,mock-client.js18/18,mock-scope.js5/5,mock-errors.js2/2,mock-interceptor-unused-assertions.js7/7,mock-call-history.js46/46. eslint clean.Possible drawbacks
ArrayBuffer.isView()widens the second fix pastDataViewto every non-Uint8Arraytyped array, and that is an observable behaviour change.reply(200, new Int32Array([1, 2]))previously delivered{"0":1,"1":2}and now delivers 8 raw bytes. I think the byte reading is obviously the intended one for a typed array, but anyone who was relying on the index-keyed JSON would notice. Say the word and I will narrow it toDataViewonly.Related and worth knowing:
content-lengthfor aDataViewreply also becomes correct as a side effect, becauseUint8Array.lengthis the byte length where the old string had length 2.Not exercised: no real socket — both changes are on the mock dispatch path, and
getResponseData/removeTrailingSlashhave no callers outsidelib/mock/. Single local Node on Windows, so the CI matrix is untested from here, andtest:typescript,test:fuzzingandtest:wptwere not run.