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

Remove logging of any SAS tokens in Actions/Cache and Actions/Artifact #1982

Merged
merged 12 commits into from
Mar 17, 2025

Conversation

salmanmkc
Copy link
Contributor

@salmanmkc salmanmkc commented Mar 10, 2025

Issue

Currently you are able to see the SAS token for downloading and uploading files when debugging.

This is not secure as technically anyone with access to the logs can use the SAS token to download or upload files.

Fix

Fix: masking the SAS token, so that you are unable to see it anymore, it will show three stars: , e.g. sig= in the URL

The code handles malformed URLs as well, encoding the raw, encoded & decoded URL in-case. Code also checks for nested parameters and for any sig fields, in the case where for some reason signature_upload_url or the keys in the object could change.

Examples now changed approach to just mask the signature part:

Cache

Cache uploading
Cache uploading with masked SAS

Cache downloading
Cache downloading with masked SAS

Artifact

Artifact uploading
image

Artifact downloading
image

### Cache _(old approach)_

Cache uploading (old approach)
image

Cache downloading (old approach)
image

Artifact (old approach)

Uploading to artifact (old approach)
image

Downloading from artifact (old approach)
image

Questions

Discussion outcome was to have duplicate code rather than using a shared utility function in Core, which was the previous approach as indicated via previous commits.

This PR will need a release of:

Sorry, something went wrong.

@salmanmkc salmanmkc requested review from a team as code owners March 10, 2025 14:10
…tures, nested parameters, and moved to a utility file
Copy link
Contributor

@robherley robherley left a comment

Choose a reason for hiding this comment

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

Have a few comments, I don't think it's necessary to use regex for these cases and looking at the URL itself (for just the well known top level body keys) should be plenty without getting too complicated 👍

@salmanmkc salmanmkc requested a review from a team as a code owner March 13, 2025 11:48
Copy link
Contributor

@robherley robherley left a comment

Choose a reason for hiding this comment

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

👍 Two small comments, otherwise nice work!

Copy link
Collaborator

@thboop thboop left a comment

Choose a reason for hiding this comment

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

lgtm

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.

None yet

4 participants