Repository navigation
Share every query and form value by default, and no cookie but 51D_ ones even when all Evidence is shared - #135
Merged
Merged
Conversation
… and no cookie but 51D_ ones even when all Evidence is shared
…nes even when all Evidence is shared
…rated lists and share all query string parameters
This was referenced Nov 15, 2026
jwrosewell
marked this pull request as ready for review
November 15, 2026 04:02
This was referenced Nov 15, 2026
Draft
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.
Problem
At
rccdbbee8 usage sharing shared too little by default and too much when all Evidence was shared.51D_or was inqueryWhitelist, which is empty by default (pipeline/fiftyone.pipeline.engines.fiftyone/shareUsageEvidenceKeyFilter.jslines 135 to 141,shareUsage.jslines 174 to 176).shareAll: trueevery 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.jslines 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. Fromusage-sharing-element.mdlines 65 to 81 at b6fae7fe:From lines 98 and 99 of the same page:
What changed
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 start51D_or are inqueryWhitelistare, as before. AqueryWhitelistof null still shares every parameter.51D_, whatever the settings,shareAllincluded. WithtrackSessionon, the session cookie named bycookieis still kept for the repeat check, and is not sent unless its name starts51D_.cookie,queryWhitelist,shareAllandtrackSessionsay so, and the type declarations ofShareUsageandShareUsageEvidenceKeyFiltergive the new option, written asnpm run tscwrites them.rcwith #129, which readsqueryWhitelistandheaderBlacklistgiven as one comma separated string in the same lines ofshareUsage.jsandshareUsageEvidenceKeyFilter.jsand their types, as 48302b8, and keeps both changes.Before you upgrade
51D_and those inqueryWhitelist, setshareAllQueryStringParameters: false.queryWhitelisthas an effect only then.shareAll: true, a cookie whose name does not start51D_is no longer shared, the session cookie included.Tests
pipeline/fiftyone.pipeline.engines.fiftyone/tests/shareUsageDefaults.test.jssends one request with a header, a parameter, a51D_parameter, a session cookie, another site cookie and a51D_cookie through usage sharing to a receiver on a free port. With no options the message holds both parameters and only the51D_cookie. WithshareAllQueryStringParameters: falseit holds only the51D_parameter. WithshareAll: true, with and withouttrackSession, it holds no cookie but the51D_one. It checks the filter alone for the same three cases.shareUsageEvidenceKeyFilter.test.jsthe default rules sharequery.passwordand sharing all Evidence does not sharecookie.sessionid, and the three tests ofqueryWhitelistsetshareAllQueryStringParameters: false. InshareUsage.test.jsandshareUsageAcceptsNoEvidence.test.jsthe messages hold thepasswordandotherparameters, the share all message holds nosessionidcookie, and the test of names with dots setsshareAllQueryStringParameters: false. InshareUsageConfiguration.test.js, which #129 added, the two tests ofqueryWhitelistandheaderBlacklistread from a configuration setshareAllQueryStringParameters: 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, asrcalready 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
rcwith #129, with only the same throwaway workflow added. All 98 pass on Ubuntu and on Windows, lint passes on the package, andnpm run tscwrites the committed types again with no difference butpipeline/fiftyone.pipeline.did/types/owid-js/v1.d.ts, which #127 decides. Run 43656347546 on 48302b8, the merge ofrcbefore 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 bytools/pr-matrix.pywithout macOS. All 7Setting '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.