Skip to content
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

Fixes some loose secrets handling #108

Merged
merged 1 commit into from
Feb 13, 2025
Merged

Conversation

JamesDawson
Copy link
Contributor

Updates the following composite actions:

  • prepare-env-vars-and-secrets
  • set-env-vars-and-secrets

Summary of changes:

  • Removes logging of base64 strings
  • Adds explicit GHA masking for the base64 strings
  • Consume any base64 strings containing secrets via environment variables, to avoid another logging disclosure scenario (where GHA logs the contents of the script being run)

- Removes logging of base64 strings
- Adds explicit GHA masking for the base64 strings
- Consume the base64 strings via environment variables in GHA scripts, to avoid another logging disclosure scenario
Copy link

github-actions bot commented Feb 12, 2025

Test Results

1 tests   1 ✅  0s ⏱️
1 suites  0 💤
1 files    0 ❌

Results for commit 83d91d5.

♻️ This comment has been updated with latest results.

Copy link

Code Coverage Summary Report - Linux (No TFM)

Summary
Generated on: 02/12/2025 - 14:37:57
Parser: Cobertura
Assemblies: 2
Classes: 2
Files: 2
Line coverage: 100% (4 of 4)
Covered lines: 4
Uncovered lines: 0
Coverable lines: 4
Total lines: 21
Covered branches: 0
Total branches: 0
Method coverage: Feature is only available for sponsors

Coverage

TestLib - 100%
Name Line Branch
TestLib 100% ****
TestLib.Class1 100%
TestLib.Tests - 100%
Name Line Branch
TestLib.Tests 100% ****
TestLib.Tests.Tests 100%

Copy link

Code Coverage Summary Report - Linux (net8.0)

Summary
Generated on: 02/12/2025 - 14:39:23
Parser: Cobertura
Assemblies: 2
Classes: 2
Files: 2
Line coverage: 100% (4 of 4)
Covered lines: 4
Uncovered lines: 0
Coverable lines: 4
Total lines: 21
Covered branches: 0
Total branches: 0
Method coverage: Feature is only available for sponsors

Coverage

TestLib - 100%
Name Line Branch
TestLib 100% ****
TestLib.Class1 100%
TestLib.Tests - 100%
Name Line Branch
TestLib.Tests 100% ****
TestLib.Tests.Tests 100%

Copy link

Code Coverage Summary Report - Windows (net8.0)

Summary
Generated on: 2/12/2025 - 2:41:31 PM
Parser: Cobertura
Assemblies: 2
Classes: 2
Files: 2
Line coverage: 100% (4 of 4)
Covered lines: 4
Uncovered lines: 0
Coverable lines: 4
Total lines: 21
Covered branches: 0
Total branches: 0
Method coverage: Feature is only available for sponsors

Coverage

TestLib - 100%
Name Line Branch
TestLib 100% ****
TestLib.Class1 100%
TestLib.Tests - 100%
Name Line Branch
TestLib.Tests 100% ****
TestLib.Tests.Tests 100%

Copy link
Member

@mwadams mwadams left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I would like a second approver for this though.

Copy link

@idg10 idg10 left a comment

Choose a reason for hiding this comment

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

So if I've understood correctly, there are three changes here:

  1. we no longer dump out the base64 with the secrets
  2. we no longer dump out the base64 with the non-secret environment variables
  3. we tell GitHub that if it ever sees the text of the base64 with the secrets in log output, it should mask it.

So 1 is the problem originally identified. 3 is an additional 'belt and braces' step to ensure that if anything else tries to do the same thing, it'll be blocked. And 2 doesn't seem directly concerned with secret handling, but I'm guessing that both of these Write-Host lines were diagnostic code that wasn't meant to be left in.

@JamesDawson JamesDawson merged commit 7d4d7fb into main Feb 13, 2025
19 checks passed
@JamesDawson JamesDawson deleted the feature/update-secrets-handling branch February 13, 2025 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants