Repository navigation
Share only the evidence usage sharing accepts unless asked for all - #50
Merged
Merged
Conversation
Usage sharing should share what the specification's accepted evidence section allows unless the caller asks to share everything: headers other than Cookie, cookies, session values and query string parameters starting 51D_, the parameters the caller names, and any other evidence. The email address, the salt for hashed email identifiers, the query string and the path are never shared. Naming the session cookie keeps it in evidence but never sends it, and an ordinary cookie must not raise an error once any option is set. The evidence filter must also remove keys no element accepts without raising "dictionary changed size during iteration". These tests fail on rc, where every key is shared by default.
The usage sharing filter shared every piece of evidence whenever no cookie, parameter list or header list was given, which is how the builders add it, so every cookie, form field and query string parameter was sent. Sharing everything is now only done when share_all is True, which defaults to False, as the specification's share all evidence option does. An empty parameter list shares only parameters starting 51D_, and None shares them all. The filter now also - splits a key at its first dot, so a name containing a dot is matched whole, and a key with no dot is shared rather than raising IndexError, - never shares id.email, id.salt, request-query or request-path, in any mode, as .NET does, - does not share the Cookie header, which carries every cookie, unless all evidence is shared, - matches the header and parameter lists without regard to case, - sets include_session, whose absence raised AttributeError for any ordinary cookie once an option was set. ShareUsage builds three filters once, as .NET does. The pipeline keeps the named session cookie, the data sent never includes it, and the tracker leaves out the session id and sequence number, which made every request look new. The session id and sequence number stay in evidence because the usage data carries them separately. Evidence.filter() looped over the evidence while deleting from it, which raised "dictionary changed size during iteration" whenever it removed a key. It now loops over a copy of the keys.
.NET keeps two settings apart. SetTrackSession, false by default, decides whether the session cookie is used to tell sessions apart, and SetAspSessionCookieName only names the cookie, with no effect unless tracking is on. Here naming the cookie turned tracking on. The session cookie test now turns tracking on with track_session. Two new tests check that the cookie alone has no effect, and that with tracking on a repeat is shared again for a new session but not within the same session.
ShareUsage takes track_session, False by default, as .NET's SetTrackSession is. The pipeline's filter and the tracker's filter include the session cookie named by cookie only when it is True, so the cookie has no effect otherwise, as .NET's SetAspSessionCookieName has none. The data sent still leaves the session cookie out unless share_all is True. The usage sharing example's configuration, which lists every option, now lists track_session and share_all with their defaults.
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.
This is the fix for #48 and #49, which can close once it reaches main. Session tracking follows .NET, which is the model for usage sharing.
Problem
ShareUsageEvidenceKeyFilterswitched on sharing everything whenever no cookie and no lists were given.51d_raisedAttributeError, because the filter readinclude_sessionand nothing set it (#49). The two on-premise web examples set"cookie": "", so they reached this on any request carrying an ordinary cookie.Evidence.filter()deleted keys from the dictionary it was looping over, which raisesdictionary changed size during iterationwhenever a key is removed.cookie, where .NET has two settings.SetTrackSession, false by default (Constants.cs lines 88 to 95), decides whether the session cookie is used to tell sessions apart, andSetAspSessionCookieNameonly names the cookie and "has no effect if TrackSession is false". The first version of this pull request made namingcookieturn tracking on.Change
The filter follows the accepted evidence section and the configuration options of the usage sharing specification, and the .NET filter.
ShareUsagetakesshare_all, False by default, as the specification's "Share all Evidence" is. Onlyshare_all=Trueshares everything.51d_, query string and form parameters only when they start51d_or are onquery_whitelist, and any other evidence. An emptyquery_whitelistshares only51d_parameters, andNoneshares every parameter, as .NET'sSetShareAllQueryStringParametersdoes.id.email,id.salt,request-queryandrequest-pathare never shared, in any mode, matched without regard to case as the whole key or after a., as in .NET'sEvidenceKeyFilterShareUsage.., soquery.valid.emailis checked asvalid.email. A key with no.is shared, where it used to raiseIndexError, andShareUsageData.try_addusesfindso it no longer raisesValueErroron such a key.ShareUsagebuilds three filters once instead of a new filter for every key, as .NET's ShareUsageBase.cs lines 636 to 666 does. The filter for the data sent leaves the session cookie out, and the tracker's filter leaves outquery.session-idandquery.sequence, which differ on every request and made every request look new to the tracker. The session id and sequence are added to the parameters kept, as .NET does, because the usage data carries them separately.ShareUsagetakestrack_session, False by default, as .NET'sSetTrackSessionis. Only when it is True do the pipeline's filter and the tracker's filter include the session cookie named bycookie. A repeat from a new session is then shared again, while a repeat within the same session is not.cookieonly names the session cookie, and has no effect unlesstrack_sessionis True. The session cookie is not sent unlessshare_allis True.Evidence.filter()loops over a copy of the keys.track_sessionandshare_allwith their defaults.For review
share_allis set. The specification said the default block list "SHOULD include thecookiesHTTP header". HTTP has no header calledCookies, and the request header that carries cookies isCookie(RFC 6265 section 5.4). The web integration adds it asheader.cookiewith every cookie in it (web.py), so without this the session and sign-in cookies the cookie rule leaves out would still be sent. The51d_cookies are still shared, as their owncookie.entries. specifications#89, now merged into rc, put theCookieheader in the specification, and .NET now blocks it too (51degrees-dotnet#81).cookiekeeps its default of None. .NET's default,asp.net_sessionid, is the ASP.NET session cookie, and Java's,JSESSIONID, is the servlet one. Python web frameworks have no common session cookie name, as Flask usessessionand Djangosessionid, so no name would be right by default.track_session=Truetherefore needscookieset to have any effect, and the docstring says so.cookiehas no effect at all otherwise. What is sent is the same.share_alloverrides session tracking, as in .NET. Withshare_all=True, every cookie is shared and compared, the session cookie included, whatevertrack_sessionsays. .NET's share all filters do the same (ShareUsageBase.cs lines 659 to 666).Tests
tests/test_share_usage_evidencekeyfilter.pyis new. It ports the .NET filter tests and adds the cases the issues list:query.id.email,header.id.email,cookie.a.b,session.org.example.attribute,query.valid.email, the session cookie, and lists in mixed case.tests/test_usage.pysends through a real receiver and checks what reaches it: the default settings, the never-shared keys,share_all, a parameter name with a dot, an empty and aNoneparameter list, an ordinary cookie with a header list set, a repeat that differs only in its session id and sequence, and a key with no prefix.tests/test_usage.pycover session tracking.test_session_cookieturns tracking on and checks the session cookie is kept but not sent.test_session_cookie_without_session_trackingnames the cookie without turning tracking on, and checks that tracking is off by default, the cookie is not kept, and the same evidence from a second session is not sent again.test_session_trackingturns tracking on and checks that the same evidence is sent again for a new session but not for a repeat in the same session, and that the cookie is never sent.tests/test_core.pychecks that evidence no element accepts is removed without an error. The JavaScript and set header elements accept every key, so the test leaves them out.Checks
AttributeError: 'ShareUsageEvidenceKeyFilter' object has no attribute 'include_session',RuntimeError: dictionary changed size during iteration, the session cookie and email address reaching the receiver, and2 != 1for the repeat. Every existing test passed (run 42192496992, first attempt).test_session_cookieandtest_session_trackingraisedTypeError: ShareUsage.__init__() got an unexpected keyword argument 'track_session', andtest_session_cookie_without_session_trackingfailed with2 != 1, because naming the cookie turned tracking on. The other 31 usage sharing tests passed, and the usage sharing example ran from its configuration (run 42320909238).Found, not changed
SequenceElementEvidenceKeyFilterandJavaScriptBuilderEvidenceKeyFilterdefinefilter()rather thanfilter_evidence_key(), andSetHeaderElementhas no filter of its own, so all three accept every key. A pipeline built with the default settings therefore keeps all evidence, whatever the other elements accept. What usage sharing sends is decided by its own filter, so this change does not depend on it. That is #52.test_usage.pydefinestest_includes_headertwice, so the first one, which checks a header is shared, never runs.fiftyone_devicedetection_examples/onpremise/shareusage.pyturns off certificate checks for everyrequests.postin the process. That is #51.Release note
Usage sharing now shares only the evidence the specification accepts unless
share_all=Trueis given, never shares an email address, its salt, the query string or the path, and no longer raises an error for ordinary cookies once an option is set. A newtrack_sessionoption, False by default, uses the session cookie named bycookieto tell sessions apart, as .NET'sSetTrackSessiondoes.