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

Fix multiline secret redaction when output with \r\n #3050

Merged
merged 2 commits into from
Oct 27, 2024

Conversation

DrJosh9000
Copy link
Contributor

@DrJosh9000 DrJosh9000 commented Oct 23, 2024

Description

Make the string replacer engine match \r as part of \n.

Context

https://coda.io/d/Escalations-Feedback_dHnUHNps1YO/Pipelines-In-Progress-Board_suIFKYEb#Escalations-Pipelines_tuQbCBf6/r774&view=modal

Changes

  • Partial matches now track string position and matched byte count separately. This is because, if \r\n is treated as a variation of \n, the number of matched bytes will be off by the number of \r bytes that were swallowed.
  • Special-case the byte comparison when the needle byte is \n and the incoming byte doesn't match the needle byte. If the incoming byte is \r in this case, then keep the partial match for the next byte but don't increment the position in the needle (so the next byte to match should be \n, but could be another \r).
  • Add a test.

Testing

  • Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
  • Code is formatted (with go fmt ./...)
  • Manually tested a build with an environment hook that adds a multiline secret and a pipeline that runs printenv.

@DrJosh9000 DrJosh9000 force-pushed the fix-multiline-secrets-with-crlf branch from 1fbd517 to d97c28c Compare October 27, 2024 23:15
Copy link
Contributor

@moskyb moskyb left a comment

Choose a reason for hiding this comment

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

this LGTM, with the caveat: do we need to match \r+\n, or would it be okay to only match \r\n? is sequences of multiple carriage returns and then a newline a common thing?

i'm perfectly happy for that to be the case, but it might be worth writing a test for it, just to show that it's intended behaviour?

@DrJosh9000
Copy link
Contributor Author

DrJosh9000 commented Oct 27, 2024

When looking at some raw logs with xxd, it often has 0d0d0a (\r\r\n) at line breaks, and I'm not really sure exactly where the double-up is coming from. So yeah, maybe it should be \r\n or \r{1,2}\n.

@DrJosh9000 DrJosh9000 force-pushed the fix-multiline-secrets-with-crlf branch 2 times, most recently from 4668e2b to 8b42a7c Compare October 27, 2024 23:38
@DrJosh9000 DrJosh9000 force-pushed the fix-multiline-secrets-with-crlf branch from 8b42a7c to f7e923c Compare October 27, 2024 23:38
@DrJosh9000 DrJosh9000 merged commit 1772973 into main Oct 27, 2024
1 check was pending
@DrJosh9000 DrJosh9000 deleted the fix-multiline-secrets-with-crlf branch October 27, 2024 23:59
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.

2 participants