Skip to content

Share every query and form value by default, and no cookie but 51D_ ones even when all Evidence is shared - #135

Merged
jwrosewell merged 5 commits into
rcfrom
fix/usage-sharing-defaults
Nov 15, 2026
Merged

jwrosewell merged 5 commits into
rcfrom
fix/usage-sharing-defaults

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Nov 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem

At rc cdbbee8 usage sharing shared too little by default and too much when all Evidence was shared.

  1. A query string or form parameter was shared only when its name started 51D_ or was in queryWhitelist, which is empty by default (pipeline/fiftyone.pipeline.engines.fiftyone/shareUsageEvidenceKeyFilter.js lines 135 to 141, shareUsage.js lines 174 to 176).
  2. With shareAll: true every cookie was shared, a site's own session and analytics cookies included, because the filter answered true before it looked at the cookie rule (shareUsageEvidenceKeyFilter.js lines 106 and 107, before line 124).

What the specification says

The defaults for every language were settled on 14 November. Every package shares every query and form value by default, and sharing all Evidence no longer shares cookies other than 51D_ ones. specifications#155 writes that into the usage sharing element's page. From usage-sharing-element.md lines 65 to 81 at b6fae7fe:

It MUST also return false, whatever the configuration and even when all Evidence is shared, for any key whose prefix is cookie. and whose field does not start with 51D_. The 51D_ cookies carry the values the 51Degrees JavaScript sends, and they are the only cookies shared. A cookie a site sets for itself, such as its session cookie, is never shared.

Unless all Evidence is shared, it MUST also return false for any key where:

  • Prefix = header. and field is in a user-configurable block list.
  • Prefix = session. and field does not start with 51D_
  • Prefix = query., the share all query string parameters option is off, and field does not start with 51D_ and is not in a user-configurable allow list of query string parameters that will be shared.

Share all query string parameters is on by default, so every query string and HTTP form parameter is shared unless a user turns it off.

From lines 98 and 99 of the same page:

The session cookie MUST NOT be shared unless its name starts with 51D_, as no other cookie is.

What changed

  1. Usage sharing has the specification's share all query string parameters option, as shareAllQueryStringParameters, which is true unless set to false. While it is true every query string and form parameter is shared. When it is false, only those whose names start 51D_ or are in queryWhitelist are, as before. A queryWhitelist of null still shares every parameter.
  2. A cookie is shared only when its name starts 51D_, whatever the settings, shareAll included. With trackSession on, the session cookie named by cookie is still kept for the repeat check, and is not sent unless its name starts 51D_.
  3. The descriptions of cookie, queryWhitelist, shareAll and trackSession say so, and the type declarations of ShareUsage and ShareUsageEvidenceKeyFilter give the new option, written as npm run tsc writes them.
  4. The branch takes rc with #129, which reads queryWhitelist and headerBlacklist given as one comma separated string in the same lines of shareUsage.js and shareUsageEvidenceKeyFilter.js and their types, as 48302b8, and keeps both changes.

Before you upgrade

  1. Every query string and HTTP form parameter is now shared by default. To share only the parameters whose names start 51D_ and those in queryWhitelist, set shareAllQueryStringParameters: false. queryWhitelist has an effect only then.
  2. With shareAll: true, a cookie whose name does not start 51D_ is no longer shared, the session cookie included.

Tests

  • The new pipeline/fiftyone.pipeline.engines.fiftyone/tests/shareUsageDefaults.test.js sends one request with a header, a parameter, a 51D_ parameter, a session cookie, another site cookie and a 51D_ cookie through usage sharing to a receiver on a free port. With no options the message holds both parameters and only the 51D_ cookie. With shareAllQueryStringParameters: false it holds only the 51D_ parameter. With shareAll: true, with and without trackSession, it holds no cookie but the 51D_ one. It checks the filter alone for the same three cases.
  • Tests written to the old defaults now expect the new ones. In shareUsageEvidenceKeyFilter.test.js the default rules share query.password and sharing all Evidence does not share cookie.sessionid, and the three tests of queryWhitelist set shareAllQueryStringParameters: false. In shareUsage.test.js and shareUsageAcceptsNoEvidence.test.js the messages hold the password and other parameters, the share all message holds no sessionid cookie, and the test of names with dots sets shareAllQueryStringParameters: false. In shareUsageConfiguration.test.js, which #129 added, the two tests of queryWhitelist and headerBlacklist read from a configuration set shareAllQueryStringParameters: false, so the list decides which parameters are shared.

Red, run 43653414818 on b8639ad, the test commit on rc's code, with only a throwaway workflow added. On Ubuntu and on Windows 11 of the package's 96 tests fail, being the five new tests of the default and of sharing all Evidence and the six tests now written to the new defaults. The other 85 pass, the two new tests with share all query string parameters off among them, as rc already shares that way.

Green before taking rc, run 43653422691 on c47c375, with only the same throwaway workflow added. All 96 pass on Ubuntu and on Windows.

Green on this head, run 43657199930 on a0be5a2, which takes rc with #129, with only the same throwaway workflow added. All 98 pass on Ubuntu and on Windows, lint passes on the package, and npm run tsc writes the committed types again with no difference but pipeline/fiftyone.pipeline.did/types/owid-js/v1.d.ts, which #127 decides. Run 43656347546 on 48302b8, the merge of rc before a0be5a2, failed both of #129's configuration tests, because every parameter was shared whatever the list said.

Full build

Run 43657206561 on tmp/pr-matrix-135-c, started by tools/pr-matrix.py without macOS. All 7 Setting 'PullRequestSha' lines give this head, a0be5a2, and Configure printed "Left out 2 macOS entries". Each of the six Build and Test jobs, Ubuntu, Ubuntu ARM and Windows on Node.js 22 and 24, passed 1,556 unit tests with 2 skipped and 113 integration tests with 4 skipped.

macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.

What remains

Nothing in this repository. The other languages make the same change in their own repositories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant