Remove logging of any SAS tokens in Actions/Cache and Actions/Artifact #1982
Conversation
| * @returns A masked URL where the sig parameter value is replaced with '***' if found, | ||
| * or the original URL if no sig parameter is present. | ||
| */ | ||
| maskSigUrl(url: string): string { |
There was a problem hiding this comment.
Instead of having this as a method on clients (therefore needing to export/test the entire client) how about making a singular maskSignedURLSignature utility function?
There was a problem hiding this comment.
Yeah, tried to do that here, discussed with @Link- and was told it's ok to have duplicated code, so reverted the approach with the utility function and put it into each client. Ran into some issue with testing it locally using npm link for some reason. Logically I think it should work with the utility function in core, so open to trying that out, but hit that issue with the test saying the utility function doesn't exist, despite the compiled code showing it and the typescript compiler not complaining.
There was a problem hiding this comment.
This is the same conversation we had for cache / artifacts regarding the clients and shared components. It might not be worth it to block this small fix by a larger effort to address the use of a shared library
There was a problem hiding this comment.
I didn't necessarily mean making it shared across packages, more of a utility function in each. Since it's on the client, we're exporting it now (for tests?). It doesn't need to be on the client class since it it doesn't use any class attributes and could be easier to test just that part.
But this is all nonblocking for the change
There was a problem hiding this comment.
Ah okay, ya that sounds like a good idea, I can put it in util.ts
| maskSigUrl(url: string): string { | ||
| const sigIndex = url.indexOf('sig=') | ||
| if (sigIndex !== -1) { | ||
| const sigValue = url.substring(sigIndex + 4) | ||
| setSecret(sigValue) | ||
| return `${url.substring(0, sigIndex + 4)}***` | ||
| } | ||
| return url | ||
| } |
There was a problem hiding this comment.
Unless the signature is guaranteed to be the last query parameter, this will mask more than the signature right?
How about something like:
const params = new URL(url).searchParams
const signature = params.get("sig")
setSecret(signature)
setSecret(encodeURIComponent(signature))
// ...But this way we mask just the signature (and it's encoded URI version) so if the runner ends up printing just the sig param or somehow out of order parameters the mask will still work.
(Also you'll need to catch the new URL exception if we don't pass a valid url string)
There was a problem hiding this comment.
Yeah previously I had that, this was a previous workflow run:
Code: 47c4fa8#diff-ad88f3760254954a760927536fc9ca883893c68d7662005a09bf32abd9d500cc
@Link- thoughts? I know you said you preferred masking in the current approach vs just the sig param
There was a problem hiding this comment.
I know you said you preferred masking in the current approach vs just the sig param
When we discussed this, we spoke about the potential ways of abuse & what makes sense to mask vs what makes sense to keep for troubleshooting purposes. I don't believe we discussed specific instructions on how to write the code for that :)
@robherley's suggestion solves for the edge cases of masking the sig field only. The implementation that he commented on needs to be revisited irrespective of whether we mask the signature only or other parts of the URL, because you have no guarantees where that parameter will be.
The question remains, is masking the signature alone sufficient or do we need to mask further fields? What is the appropriate tradeoff here?
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.
Examples:
Cache
Cache uploading
Cache downloading
Artifact
Uploading to artifact
Downloading from artifact
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: