fix: do not resolve query-param keys that name inherited prototype members

[mattbodle]

Summary

Two instances of the same bug class in queryStringParser, one latent and one reachable from a URL alone.

1. Caller-supplied keys resolve through the prototype chain. queryStringParser takes a caller-supplied list of keys and looks each one up on a plain object. obj[name] walks the prototype chain, so a key named constructor resolves to Object.prototype.constructor, passes the truthiness gate, and is copied into the result as though the URL had supplied it.

queryStringParser('https://x.com?foo=bar', ['constructor'])
// before: { constructor: 'function Object() { [native code] }' }
// after:  {}

A real ?constructor=legitimate in the URL still resolves — the guard is an own-property check, not a name ban. There is a test for that.

2. queryStringParserFallback throws on a URL param named hasOwnProperty. The fallback's returned forEach calls params.hasOwnProperty(key), where params' keys come straight off the URL. ?hasOwnProperty=1 gives that map an own property shadowing the inherited method with the string '1', and the next iteration invokes it.

queryStringParser('https://x.com?hasOwnProperty=1&foo=bar', ['foo'])
// before: TypeError: params.hasOwnProperty is not a function
// after:  { foo: 'bar' }

That throws out of queryStringParser, through allowedQueryParams and currentPage(), and out of IntegrationCapture.capture() — on the init path, from a URL alone, with no configuration required. It is therefore strictly more reachable than (1), which needs a configurable key list to become input at all. The fallback is live code for browsers without URL/URLSearchParams, and the spec already exercises it (describe('without URLSearchParams')), which had no prototype-name coverage before this PR.

Both are fixed with one hasOwnProp helper.

Reachability, stated plainly

• (1) is not reachable in production today. Both callers pass hardcoded key lists — integrationCapture's click IDs and pageViewTracker's ALLOWED_QUERY_PARAMS — and neither contains a prototype member name. It becomes reachable input the moment either list can be extended from configuration, which is the next PR.
• (2) is reachable today, on any browser taking the fallback path, from any URL carrying ?hasOwnProperty=.

Object.prototype is not polluted by any of this, before or after. I verified. (1) is a data-hygiene and dedup-integrity bug and (2) is a crash — neither is a prototype-pollution vulnerability, and I would rather label them accurately than escalate.

Why guard the lookup instead of Object.create(null)

Object.create(null) on the lookup map looks like the smaller fix and kills the whole class at the root. It is the wrong call here for two concrete reasons:

1. queryStringParser returns that same map directly when no keys are passed (if (isEmpty(keys)) return lowerCaseUrlParams), so the null prototype would escape to callers.
2. integrationCapture.ts:336 — applyProcessors — calls .hasOwnProperty() on its clickIds argument, and getQueryParams() passes the return value of queryStringParser straight in. A null-prototype object throws there.

So the returned object stays plain, and a test pins that contract.

Correction from the first revision of this description, which cited integrationCapture.ts:289. That line operates on this.clickIds, a fresh spread literal ({...this.clickIds, ...queryParams, ...}), so it is a plain object regardless of what queryStringParser returns. :336 is the citation that actually holds. The same wrong claim was repeated in a test comment and is fixed there too.

The other half, and its missing test

This also switches pageViewTracker's capturedNames() from in to the same own-property check.

That half has no test in this PR, deliberately. capturedNames() only ever tests names drawn from ALLOWED_QUERY_PARAMS; with a hardcoded list containing no prototype member name, in and hasOwnProperty are unobservable through the public helpers.

Correction: the first revision said the two "cannot be told apart by any test". That is false. ALLOWED_QUERY_PARAMS is a mutable export, so a test could push 'constructor' onto it, assert pageKey, and pop it in a finally. The honest reason there is no test here is that the only possible one requires mutating an exported array that every other test in the file reads — shared mutable state in a suite that otherwise has none — and that was not worth the smell for one release. It gains a clean regression test in #1373, where the allowlist becomes configurable and the distinction is observable without touching module state.

Test-quality corrections

The six-key prototype-member test (['foo', 'constructor', '__proto__', 'toString', 'valueOf', 'hasOwnProperty']) is 5/6 inert, and its comment claimed otherwise. queryStringParser lowercases the key before the lookup, so:

key	resolves to	exercises the guard?
constructor	Object.prototype.constructor (truthy)	yes
__proto__	Object.prototype, but assigning it back hits the setter rather than creating an own property	no
toString / valueOf / hasOwnProperty	tostring / valueof / hasownproperty → undefined	no

The list is kept as documentation of the name class, with a comment that says which name does the work — and notes that the .toLowerCase() is therefore load-bearing for the other five, which was previously undocumented.

Verification

Measured, not estimated:

• jest — 700 pass / 0 fail, up from 694 on origin/development (+6: 4 from the original commit, 2 from the fallback fix)
• karma ChromeHeadless — 1089 pass / 0 fail, 10 skipped
• tsc -p . --noEmit clean, eslint src/ test/src/ clean

Correction: the first revision said "+6 jest tests (from 692)". The original commit adds exactly 4 it() blocks and removes 0. The 698 total was right; the baseline was stale. Both numbers above are re-measured.

Mutation-checked — each fix deliberately broken, and the named test that catches it:

mutation	test that fails
fallback guard reverted to params.hasOwnProperty(key)	without URLSearchParams › survives a query param named after a prototype method (and › still returns a param named after a prototype method when asked for it)
own-property guard deleted from queryStringParser	with URLSearchParams › does not resolve keys naming inherited Object.prototype members
guard replaced with a name ban (name in Object.prototype)	with URLSearchParams › still resolves a real query param sharing a prototype member name

Follow-ups, not fixed here

Pre-existing full-harvest bug on development, unrelated to this PR. src/mp-instance.ts:~1546-1553 enables integration capture for any truthy captureIntegrationSpecificIds.V2 value that is not 'none', but only assigns captureMode for 'all' and 'roktonly'. An unrecognised value leaves captureMode undefined → getActiveIntegrationMapping() returns {} → queryStringParser(href, []) takes the no-keys branch → every query param on the page is captured and forwarded. Verified structurally, not fixed here; it wants its own PR and its own risk assessment.

Unsafe obj.hasOwnProperty(...) invocation elsewhere. src/utils.ts:47, :453, :502 and src/integrationCapture.ts:289/:315/:336 all invoke hasOwnProperty as a method on an object whose keys may come from user data. A user attribute literally named hasOwnProperty throws at utils.ts:453. Same class as (2) above; a follow-up ticket rather than scope creep on this stack.

Stack

Part 1 of 4 for customer-configurable APV query params:

1. this PR — hardening
2. feat(apv): union the query-param allowlist with a customer-configured list #1373 — flag + union of ALLOWED_QUERY_PARAMS with a customer list
3. mPServer: SettingTemplate row
4. mPServer: Advanced Settings UI field

🤖 Generated with Claude Code

[cursor]

PR Summary

Medium Risk
Changes URL parsing on SDK init and page-view dedup paths; fixes a production crash on certain URLs but alters edge-case param resolution.

Overview
Hardens query-parameter handling so lookups do not follow Object.prototype and URLs cannot break parsing.

queryStringParser now uses a shared hasOwnProp helper when resolving caller-supplied keys, so names like constructor are not treated as URL params unless they are real own properties on the parsed map. Legitimate ?constructor=… values still work. The no-URLSearchParams fallback’s forEach uses the same helper instead of params.hasOwnProperty, fixing a TypeError when the URL includes ?hasOwnProperty=… (reachable on init via integration capture / page view paths).

pageViewTracker switches capturedNames from in to hasOwnProp for the same reason ahead of configurable allowlists.

Jest coverage adds cases for prototype-named keys, mixed-case key lists, fallback survival, and that filtered results remain plain objects callable with .hasOwnProperty().

^{Reviewed by Cursor Bugbot for commit d17c69f. Bugbot is set up for automated code reviews on this repo. Configure here.}

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[alexs-mparticle]

Can we invert this guard to make it more readable? So that rather than return if it's negative we just assign if possitive?