Skip to content

Log environment variables sorted by key while redacting values of unsafe ones #3543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Jun 3, 2025

Improves logging of environment variables by sorting them by key and redacting the values for the ones that are likely to contain secrets.

Fixes: #3542

  • ran the linter to address style issues (tox -e fix)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

@ssbarnea ssbarnea force-pushed the fix/3542 branch 3 times, most recently from 511fcac to 7f508e0 Compare June 4, 2025 12:01
@ssbarnea ssbarnea requested a review from gaborbernat June 4, 2025 12:47
@ssbarnea
Copy link
Member Author

ssbarnea commented Jun 4, 2025

@gaborbernat Any chance you could look at it again today? I think that I addressed the requests.

This patch is a blocker for improving the security of GHA pipelines as I would not want to disable log collection for tox. Thanks.

@ssbarnea ssbarnea changed the title Improve logging of environment variables Log environment variables sorted by key while redacting values of some unsafe ones Jun 10, 2025
@ssbarnea ssbarnea changed the title Log environment variables sorted by key while redacting values of some unsafe ones Log environment variables sorted by key while redacting values of unsafe ones Jun 10, 2025
@ssbarnea ssbarnea force-pushed the fix/3542 branch 2 times, most recently from ca01304 to 37a1156 Compare June 10, 2025 16:10
@ssbarnea ssbarnea dismissed gaborbernat’s stale review June 10, 2025 16:15

Please review again, I made the required changes, dropped the notice, added documentation entry, tested all keywords.

@ssbarnea
Copy link
Member Author

@gaborbernat Sorry to bother you again. Is there anything else I need to change?

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ssbarnea
Copy link
Member Author

@gaborbernat The py314 failure is unrelated and seems to be a bug in coveragepy. I will try to help @nedbat with a patch if I do not run out of time. That also made me realize that I need to fix few bugs in tox-uv in order to run coveragepy tests..., side effect raising tox-dev/tox-uv#210

@ssbarnea
Copy link
Member Author

@gaborbernat Any idea about what to do about py314 errors related to coverage?

CoverageWarning: Couldn't import C tracer: No module named 'coverage.tracer' (no-ctracer)

They started to fail on main last night, so unrelated to this patch. Should we attempt to capture/hide these warnings to avoid our test failures?

@nedbat
Copy link
Contributor

nedbat commented Jun 12, 2025

I think I will skip that warning on platforms where I don't ship a built wheel, though I'd be curious whether the installation of coverage on 3.14 shows that it tried to compile the extension or not?

@nedbat
Copy link
Contributor

nedbat commented Jun 13, 2025

Coverage 7.9.1 is released to quiet that warning on 3.14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

logged environment variables are not sorted
3 participants