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(compiler)!: webhook payload containing message with special characters causes failure #793

Merged

Conversation

NickHackman
Copy link
Contributor

Use the implementation made upstream in drone/envsubst#27 to fix escape sequence handling to prevent yaml parsing to fail due to invalid escape sequences.

Issue: go-vela/community#702

Use the implementation made upstream in drone/envsubst#27 to fix escape sequence handling to prevent yaml parsing to fail due to invalid escape sequences.

Issue: go-vela/community#702
go.mod Outdated
@@ -53,6 +53,7 @@ require (
github.com/chenzhuoyu/base64x v0.0.0-20221115062448-fe3a3abad311 // indirect
github.com/cpuguy83/go-md2man/v2 v2.0.2 // indirect
github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f // indirect
github.com/drone/envsubst v1.0.3 // indirect
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an indirect dependency

❯ go mod why github.com/drone/envsubst
# github.com/drone/envsubst
github.com/go-vela/server/api
github.com/go-vela/types/pipeline
github.com/drone/envsubst

Going to update this dependency in types/pipeline as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR to update types/pipeline go-vela/types#284

@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #793 (4f2edf2) into main (3fd6215) will not change coverage.
The diff coverage is n/a.

❗ Current head 4f2edf2 differs from pull request most recent head 8e4ed33. Consider uploading reports for the commit 8e4ed33 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #793   +/-   ##
=======================================
  Coverage   57.73%   57.73%           
=======================================
  Files         263      263           
  Lines       15911    15911           
=======================================
  Hits         9187     9187           
  Misses       6310     6310           
  Partials      414      414           
Impacted Files Coverage Δ
compiler/native/substitute.go 70.00% <ø> (ø)

NickHackman added a commit to NickHackman/vela-types that referenced this pull request Mar 17, 2023
NickHackman added a commit to NickHackman/vela-types that referenced this pull request Mar 17, 2023
@NickHackman NickHackman marked this pull request as ready for review March 17, 2023 19:21
@NickHackman NickHackman requested a review from a team as a code owner March 17, 2023 19:21
cognifloyd
cognifloyd previously approved these changes Mar 18, 2023
@NickHackman
Copy link
Contributor Author

"full-review" failed due to other lint checks done by golangci-lint

jbrockopp
jbrockopp previously approved these changes Apr 8, 2023
Copy link
Contributor

@jbrockopp jbrockopp left a comment

Choose a reason for hiding this comment

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

LGTM

@plyr4
Copy link
Contributor

plyr4 commented Apr 13, 2023

looks great, thanks @NickHackman :D
one hesitation is that it changes the escape functionality in the critical path for most of the executor container code.
one example: https://github.com/go-vela/worker/blob/main/executor/linux/step.go#L59-L71
some users might have put workarounds in place for having bogus escape sequences in secrets.

im excited for the fix, so i'd say admins should be watchful on rolling this change out, and also should mark this as a breaking change in the PR and in the release notes.
either way, love it!

plyr4
plyr4 previously approved these changes Apr 13, 2023
@plyr4 plyr4 changed the title fix(compiler): webhook payload containing message with special characters causes failure fix(compiler)!: webhook payload containing message with special characters causes failure Apr 13, 2023
@NickHackman
Copy link
Contributor Author

@plyr4 good call out, this would impact users who have tried to hack around the issue in the first place. Hopefully that's a very small subset of users who need remediation here

@plyr4 plyr4 dismissed their stale review April 14, 2023 20:54

with further testing theres a bit more to the issue at large

@ecrupper
Copy link
Contributor

@NickHackman Hi Nick! Thanks for the PR. I pulled down both the changes in this PR as well as your changes in the types PR. I had a few buggy payloads to test, and it does indeed look like the escape character error is resolved, however some new YAML parser error arose, such as mapping values not allowed in this context. While I do think we should probably update this library, I'm wondering if there's more to this issue + perhaps another way to solve? Perhaps some sanitization on our end? I will update the issue with a payload I tested that got another parser error. Curious what others think in terms of whether we merge this in as a "step in the right direction" or if we prefer a catch-all sanitization solution?

@NickHackman
Copy link
Contributor Author

@ecrupper Awesome, fire away with examples here. I assume any issue we face is coming from the upstream library. I'll do my best to patch things if we can get cases that fail.

Ideally these issues are all fixed in the upstream library. Vela shouldn't need to patch it, but whatever is seen as best

ecrupper added a commit to go-vela/types that referenced this pull request Apr 18, 2023
@NickHackman NickHackman dismissed stale reviews from jbrockopp and cognifloyd via 8e4ed33 April 18, 2023 19:10
@ecrupper
Copy link
Contributor

Will update the original issue with anything else we find related to compiler errors from weird character sequences. For now, we know this fixes a few things and doesn't break anything from what we can tell. Merging! Thanks again @NickHackman !

@ecrupper ecrupper merged commit 3fec46c into go-vela:main Apr 18, 2023
@NickHackman NickHackman deleted the webook-message-escaped-causes-failure branch April 18, 2023 19:18
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.

6 participants