Repository navigation
fix: stop logging notification-rule command arguments - #1198
Conversation
These commands take a signing secret and custom headers as arguments; suppress argv logging for them.
sophiecarreras
left a comment
There was a problem hiding this comment.
Thanks. Two things.
-
The
lintjob fails intowncrier_check: a changelog entry has to start with a capital letter, and+notification-rule-no-arg-logging.fixed.mdstarts with a backtick. Starting it with a word, for example "Thebucket notification-rule createandupdatecommands no longer log ...", should fix it. (The test jobs show as skipped because lint failed.) -
Hiding argv in
console_tooldoes not fully stop the secret reaching the debug log. With--verboseor--debug-logs, b2sdk still logscalling Bucket.set_notification_rules(... rules=[... 'hmacSha256SigningSecret': ..., 'customHeaders': {...}])fromb2sdk._internal.bucket, so both the signing secret and the custom-header values are written (to stderr with--verbose, and tob2_cli.logwith--debug-logs). I reproduced it by runningcreatewith--verbosewhile capturing all loggers at DEBUG with the patch applied: theconsole_toolline reads "(arguments hidden)", but the b2sdk line above still carries the values. The new test only reads records from theb2._internal.console_toollogger, so it passes without seeing this.
Could the test assert on all loggers (and on the stderr stream), and the PR either redact those values or note the remaining gap? The comment "b2sdk request logging is handled separately" does not match what I see today. Redacting in the b2sdk call-trace logging, or not enabling DEBUG tracing for these commands, would both close it.
For the scope of this change, the approach looks right otherwise: Update inherits from the base so both commands are covered, I found no other argv-borne secrets (SSE-C keys come from environment variables), and hiding --webhook-url too is a good call. code/snyk also reports "1 new Code Analysis issue"; I could not open the detail, it may be the hard-coded test secret.
sophiecarreras
left a comment
There was a problem hiding this comment.
Review from Sofia, with Claude's help. Read only, not run.
One blocker: lint is red. The Changelog validation step fails with File +notification-rule-no-arg-logging.fixed.md is not a valid changelog entry: The first character is not a capital letter. The fragment starts with a backtick. Start it with a capital letter, for example "The bucket notification-rule create and ...".
One question about the test. test_notification_rules__sensitive_arguments_are_not_logged asserts only on records from the b2._internal.console_tool logger. With --verbose, b2sdk's own request logging could still print the signing secret and header values from the request body, and this test would not see it. The PR calls that out of scope; can you check whether b2sdk logs the request body at --verbose? If it does, the secret still reaches the debug log by another route.
The change itself (FORBID_LOGGING_ARGUMENTS = True on the shared base, with create and update both parametrized) looks right.
Comment only, not an approval.
Limit SDK bucket debug logging while notification-rule commands run and restore the previous logging level afterward. Cover stderr and debug files in regression tests and correct the changelog fragment.
|
Thanks @sophiecarreras — both points addressed in
Re-requesting your review. |
Context
The
bucket notification-rule create/updatecommands did not setFORBID_LOGGING_ARGUMENTS, so the command dispatcher logged their full argv — including the--sign-secretand--custom-headervalues.Change
Set
FORBID_LOGGING_ARGUMENTS = TrueonBucketNotificationRuleCreateBase(inherited by create and update), so these commands' arguments are not written to the command log. (b2sdk's own request logging is separate and out of scope.)Verification
New test: the
--sign-secretvalue no longer appears in the dispatcher log for create/update, while a normal command still logs its args; removing the flag fails the test. Changelog fragment added.