Repository navigation
Share every query value by default, and no cookie but 51D_ ones even when all Evidence is shared - #160
Draft
jwrosewell wants to merge 3 commits into
Draft
Share every query value by default, and no cookie but 51D_ ones even when all Evidence is shared#160jwrosewell wants to merge 3 commits into
jwrosewell wants to merge 3 commits into
Conversation
…D_ ones The specification now has share all query string parameters on by default, and even when all Evidence is shared it shares no cookie whose name does not start with 51D_. The session cookie is still compared by the tracker when track session is on. The tests that took the old defaults now take the new ones, and the tests of the allow list turn share all query string parameters off.
…when all Evidence is shared The usage sharing specification now has share all query string parameters on by default, and returns false for every cookie whose name does not start with 51D_, whatever the configuration and even when all Evidence is shared. The builder, the element and the filter take the new default from one constant, and the cookie rule now runs before the share all Evidence check. The tracker's filter still keeps the session cookie when track session is on, so that the session is compared, and the filter for the record never keeps it, so it is never shared.
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.
Usage sharing now shares every query string and HTTP form value by default, and shares no cookie but the
51D_ones even when all Evidence is shared, as specifications#155 now says.Why
James Rosewell decided at about 20:55 UTC on 14 November that the rule the cloud service follows is every language package's default: "Every package shares every query and form value by default, and Share all Evidence no longer shares cookies other than 51D_ ones (the raw Cookie header never)." specifications#155 wrote it into the usage sharing element's page, merged into the specifications'
rcat 33de68a0.What the specification says
usage-sharing-element.md lines 65 to 69 at 30b3741f:
Lines 76 to 81:
The options table (line 425) gives share all query string parameters a default of true. Lines 90 to 99 keep the session cookie for the repeat check when track session is on, and say "The session cookie MUST NOT be shared unless its name starts with
51D_, as no other cookie is."What changed
SHARE_USAGE_DEFAULT_SHARE_ALL_QUERY_STRING_PARAMETERS, true, and the builder (ShareUsageBuilderBase, its field, Javadoc and@DefaultValue), the element's constructors that take no value for it (ShareUsageBase,ShareUsageElement) and the filter's shorter constructor (EvidenceKeyFilterShareUsage) take their default from it.51D_, or when it is the session cookie and the filter was made with the session included. The element makes the filter for the record with the session left out (ShareUsageBase.java line 547), so the session cookie is never shared, and the tracker's filter keeps it when track session is on (line 566), so the session is still compared.Tests
Tests first, 4ce76a71. New in
EvidenceKeyFilterShareUsageTests:shareAllQueryStringParametersOff_OnlyNamedAnd51D,shareAllEvidence_NoCookieBut51D(a51D_cookie in any case is kept,_gaand another cookie are not, and the session cookie only by a filter made with it) andtrackerWithShareAllEvidence_SessionCookieOnlyWhenTracked.defaultRulesnow expectsquery.passwordandQuery.Utm_Sourceshared andserver.request-querynot. The tests of the allow list, and the element and options tests that took the old defaults, now say which default they test.Red 43673395639 ran the tests with
rcde74c8c and no change, plus a throwaway workflow, Java 17. 9 of 50 failed, exactly the new and changed tests:EvidenceKeyFilterShareUsageTestsdefaultRules,shareAllEvidence_NoCookieBut51DandtrackerWithShareAllEvidence_SessionCookieOnlyWhenTracked,ShareUsageElementTestsShareUsageElement_DefaultSharesOnlyAcceptedEvidence("password parameter") andShareUsageElement_WebRequest_SharesWhatNoOtherElementAccepts("A query string parameter from the web request was not shared by default."), and fourShareUsageOptionsTests.UsageSharingEvidenceTestspassed 7 of 7.Green 43673749176 ran the head plus the same workflow.
EvidenceKeyFilterShareUsageTests15 of 15,ShareUsageElementTests26 with 1 skipped as onrc,ShareUsageOptionsTests9 of 9 andUsageSharingEvidenceTests7 of 7 passed. An earlier green, 43673384594, stopped at a missing static import inShareUsageElement, which the fix commit now holds.Full build
Under way, without macOS under the 9 November decision. This section is replaced with the run, its result and the head it built when it ends.
Before you upgrade
51D_parameters and those named, as before, setsetShareAllQueryStringParameters(false)on the builder, orShareAllQueryStringParametersto false in the options file.51D_is no longer shared, the session cookie included. Before, share all Evidence shared every cookie.What it releases
pipeline.engines.fiftyonechanges, so it and the packages that depend on it are in Monday's plan under version per package.Base branch
rcat de74c8c, merged into the branch at fab13be.