Test: pin the core and dev version tests to their own manifest and a semver shape (stacked on #417) - #619
Open
AmaadMartin wants to merge 1 commit into
Conversation
Stacked on the core/dev version tests: the equality assertion alone can pass vacuously. release-please's linked-versions plugin holds the root, core, dev and integrations manifests at the same version forever, so a read that resolved to the wrong package.json still compares equal; asserting the package name pins the read to the intended manifest. The semver match stops a future refactor that makes both sides undefined from keeping the test green.
AmaadMartin
pushed a commit
that referenced
this pull request
Aug 6, 2026
* fix(security): prevent prototype pollution via untrusted map keys
appName, userId, sessionId and state keys arrive straight off request
paths and bodies on the dev server. Held in plain `{}` maps, a key of
`__proto__` aliases Object.prototype instead of creating an own
property, so a single unauthenticated request writes onto
Object.prototype for the lifetime of the process. The appendEvent
stateDelta path makes the planted value fully attacker-controlled.
Key the affected maps with Object.create(null) so these names become
ordinary own properties:
- InMemorySessionService: sessions, userState, appState.
- InMemoryCredentialService: credentials. Also stops an inherited
credentialKey such as `toString` resolving to a Function rather
than undefined.
- AdkApiServer: runnerCache, traceDict, sessionTraceDict. Here `in`
matched inherited names, so `appName in runnerCache` reported a hit
and yielded a Function where a Runner was expected, and
GET /debug/trace/toString returned 200 instead of 404.
An app literally named __proto__ keeps working, and no longer leaks
phantom sessions across apps.
* fix(security): address review on prototype pollution fix
Close the same primitive one level up, on the same request path, and drop
the duplicated helper.
- `trimTempState` / `trimTempDeltaState` copied caller-controlled keys into
plain object literals, so a `{"state": {"__proto__": {...}}}` request body
re-parented the new session state onto the attacker's object. `State.get`
and `State.has` use `in`, so every key on it read back as session state.
Both filtered maps are now null-prototype.
- `updateSessionState` wrote the delta into `session.state`, which is not
always null-prototype, so fixing the delta map alone would have routed the
attacker key straight into the plain assignment. It now uses
`Object.defineProperty`, which always creates an own property.
- `InMemoryMemoryService.sessionEvents` inner map is keyed by `session.id`,
which holds no `/` and so can be exactly `__proto__`; such a session was
silently dropped from the `Object.values` scan in `searchMemory`.
- Replace the three `createNullProtoMap()` copies with direct
`Object.create(null)` assignments; the cast was a no-op because
`Object.create` returns `any`. The rationale moves onto the fields.
Tests: the `__proto__` state-key test asserted `({}).baseUrl`, which is
undefined before and after the fix. It now reads the key back through a
*sibling* session, because `updateSessionState` also writes the prefixed key
into the originating session's own state and masks the `userState` loss.
Cleanup list gains `u1` and `s1`, both of which land on `Object.prototype`
when the fix is reverted. Three new tests cover the sinks above; all 10
guard tests were confirmed to fail on the unfixed tree.
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
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
N/A — no existing issue.
Problem:
core/src/version.tsanddev/src/version.tsare hand-maintained constants that release-please is expected to keep in lockstep with theirpackage.json(via theextra-files/x-release-please-versionwiring inrelease-please-config.json), but nothing in the repo asserts that it actually does. A stale constant is silently user-visible:core/src/version.tsfeeds thegoogle-adk/<version>client label incore/src/utils/client_labels.ts:41andcore/src/telemetry/tracing.ts:27, anddev/src/version.tsis whatadk --versionprints (dev/src/cli/cli.ts:210). This rot is not hypothetical —integrations/test/version_test.tsstill asserts a hardcoded'1.3.0'against a source constant of'1.5.0'.A manifest-comparison test alone, however, can pass vacuously in two ways:
linked-versionsplugin holds the root,core,devandintegrationsmanifests at the same version forever. A test that read the wrongpackage.json(e.g. the repo root's) would still compare equal and stay green, so the read is not actually pinned to the workspace it claims to check.undefined,expect(version).toBe(pkg.version)would still pass.Solution: This PR is stacked on
fix/version-tests-core-dev(#417), which adds the two test files. It adds only the two hardening assertions that close the vacuous-pass holes, +10 lines per file:expect(pkg.name).toBe('@google/adk')/toBe('@google/adk-devtools')— pins the manifest read to the intended workspace.expect(version).toMatch(SEMVER_PATTERN)—major.minor.patchwith an optional pre-release suffix, so the equality assertion cannot be satisfied by twoundefineds.The pre-existing
expect(version).toBe(pkg.version)assertion is unchanged; both new assertions are additive. No existing test was modified, weakened, skipped or deleted.Collision check (required, run before any code was written).
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000returned 518 open PRs; filtering for version/manifest keywords surfaced two that add these exact two paths:fix/version-tests-core-devcore/test/version_test.ts,dev/test/version_test.tsreadFileSync(new URL('../package.json', import.meta.url))→expect(version).toBe(pkg.version)feat/core-dev-version-consistency-testsBoth are OPEN and non-draft, and they already collide with each other. Since #417 overlaps rather than fully lands the intended change (it has neither the name guard nor the semver guard), this PR stacks on #417's branch instead of branching from
mainand opening a third competing copy of the same two files. Recommendation: land #417 with this PR on top, and close #258 as a duplicate.Scope deliberately declined:
dev/test/version_test.tsimportsversionfrom'../src/version.js'by relative path because@google/adk-devtoolsdoes not export it (dev/src/index.tsexports onlyAdkApiClientandAdkApiServer) and no@google/adk-devtoolsvitest alias exists. Adding that export would widen a published package's public API, and adding the alias would changevitest.config.ts— both out of scope for a test-only change. All 14 existingdev/test/**/*_test.tsfiles reach dev internals the same way. Thecoreside does import from@google/adk, per the public-API-import guideline.No production source, no
package.json, nopackage-lock.json, novitest.config.ts, norelease-please-config.json, and noCHANGELOG.mdwas touched.git diff fix/version-tests-core-dev --statis exactly two files, +20/-2.integrations/test/version_test.tswas deliberately left alone — its stale literal is owned by a separate queued task (#479).Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
CI is
absenton this PR, by design — not green..github/workflows/validation.yamland.github/workflows/license-check.ymlboth trigger onpull_request: branches: [main], and this PR's base isfix/version-tests-core-dev, so no build/test job will ever run against it; only the trivialauto-assigncheck appears, which is not validation. The commands above were therefore run locally against the exact pushed commit (848a8f05) and are the validation for this change. The test jobs will run on #417 once this is squashed into it or once its base becomesmain.npm run ts:checkexits 2 with 281 errors, but this is entirely pre-existing: the identical command on the unmodified base (fork/main) also exits 2 with 281 errors, and not one of them is inversion_test.ts. They areBASE_AGENT_SIGNATURE_SYMBOLassignability errors in unrelated test files. This change adds zero type errors. (ts:checkis not part of.github/workflows/validation.yaml.)Mutation testing — proof each new assertion can fail. Run one at a time, each reverted before the next;
git diff -- core/src dev/srcwas confirmed empty afterwards.core/src/version.ts→'9.9.9'AssertionError: expected '9.9.9' to be '1.5.0' // Object.is equalitydev/src/version.ts→'9.9.9'AssertionError: expected '9.9.9' to be '1.5.0' // Object.is equalitycore/src/version.ts→''AssertionError: expected '' to match /^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?$/'../../package.json')AssertionError: expected 'adk' to be '@google/adk' // Object.is equalityAssertionError: expected 'adk' to be '@google/adk-devtools' // Object.is equalityMutation 6 is the point of this PR: with #417's assertion alone, a read that resolves to the wrong manifest is green, because the root
package.jsonis also1.5.0. The name assertion is what turns that into a failure, so it is load-bearing rather than decorative. Mutation 3 likewise shows the semver guard is not dead weight — it, not the equality check, is what catches an empty constant.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Prints
1.5.0, matchingdev/package.json's"version": "1.5.0"— the user-visible surface the dev-side test protects. No integration test was added: there is no cross-component behaviour here, and one would duplicate the unit test at higher cost.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.