Repository navigation
Show only the last four characters of a licence key in logs and errors - #78
Merged
Merged
Conversation
Each hidden character is an asterisk, and a key of four or fewer characters is hidden entirely. The update service masks the URL it logs and the message of a failed request, and the Cloud Request Engine masks the URL in its no-data message.
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
For the rule on licence keys in logs and errors, logging.md, License keys (lines 20 to 39 on specifications
rc). It says "A license key MUST NOT appear in full in a log entry or in an error message, at any log level. Where a message names a license key it MUST show only the key's last 4 characters, with each character before them replaced by one*" and "A key of 4 characters or fewer MUST be replaced entirely, by one*for each character. Every package masks the same way, soABCDEFGHis written****EFGHin every language." It also says a message that names a URL carrying a key, or the message of an error raised by an HTTP library, MUST hold only the masked key, and that there MUST be a test of a failed request.The update URL carries
licenseKeys=..., anddatafile_update_service.pylogged it in full for 429, 304, 403, other failures and a failed MD5 check (lines 124 to 153 onrc). A failed request raises an error whose message names the URL as well, and the Cloud Request Engine's no-data message names the response URL.What changed
New
fiftyone_pipeline_core/keymask.pywithmask_licence_keyandmask_licence_keys_in_text. The second finds licence parameters in text (licenseKeys,licenceKey,license_keysand the like, in any case) and masks each key of a list separated by|or%7C.The update service logs a masked URL, and re-raises a failed request with a masked message.
The Cloud Request Engine masks the URL in its no-data message.
The Cloud Request Engine also masks the licence key by value in every exception
validate_responseraises, through the newCloudRequestEngine.masked. The cloud's accessible properties answer to a licence key that grants no cloud product names the key in itserrorstext ("License key(s): '...'"), andvalidate_responseput that text into the exception in full (cloudrequestengine.pylines 339 to 343 and 367 to 376 onrcat f55f0d9). A non-JSON answer's content is masked the same way.A review on 10 November found that cloud errors case. Nothing else in the
srcfolders logs or raises a key or a URL that carries one (searchedlog,raiseandException).Overlap
The data update draft #69 rewrites the same log lines, and the two conflict in
datafile_update_service.py. #69 holds this draft's commits as they were at b919b6b, merged in, and masks its own new messages, so this draft merges first and #69 after it. In #69 a failed request is logged rather than raised, so #69 also changes this draft'stest_an_error_raised_by_the_request_does_not_hold_the_keyto check the logged message.How it was verified
New tests in
pipeline/fiftyone_pipeline_engines/tests/test_licence_key_masking.py.keymask, as the module is new. A red that shows these tests catching a key logged in full is Align the data update service to the specification: HTTP date, expected time, update from memory and logging #69's run 42796630384, on Align the data update service to the specification: HTTP date, expected time, update from memory and logging #69 with this draft merged in and before Align the data update service to the specification: HTTP date, expected time, update from memory and logging #69 masked its own messages, wheretest_no_message_holds_the_keyfails with'ABCDEFGHIJKL' unexpectedly found in "Checking for update from ...".b919b6b9: run 42704807459, 9 passed, core 354 passed.pipeline/fiftyone_pipeline_cloudrequestengine/tests/test_licence_key_errors.pygivevalidate_responsea stub answer whoseerrorstext, and a non-JSON content, name the key. Red, the tests on 7aad19d (this draft withrc451c17e merged in, before the change): run 42869089360, both fail on the key in full. Green, head 930b677: run 42869122613, 2 passed,fiftyone_pipeline_core360 passed.6ef9917mergesrcin, with no conflict.6ef991724891aa75f8359fb3db25e4551c1d287d: run 42895898002. 12 jobs passed and 2 were skipped (the performance comparisons), and all 11Setting 'PullRequestSha'lines equal the head. The Configure log reads "Left out 5 macOS entries".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.
The last release into
mainmerged on 2026-11-04.#69 now holds this draft's later commits too, merged in at 2a90195.