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

Jump to bottom

[JENKINS-50154] Fix webhook payload signature generation when Unicode symbols are present in it #242

Merged
merged 3 commits into from Jan 3, 2021
Merged

[JENKINS-50154] Fix webhook payload signature generation when Unicode symbols are present in it #242

merged 3 commits into from Jan 3, 2021

Conversation

ababushk
Copy link
Contributor

@ababushk ababushk commented Dec 2, 2020

This fixes a bug when plugin rejects webhook payload came from GitHub due to incorrect signature although secret token is configured correctly in Jenkins and GitHub repo/org settings.
It turned out that signature was generated incorrectly when Unicode characters were used in repo's description (in our case it was the ™ character).

I have a concern of using already deprecated method in new code, but the alternative is to add new dependency to pom.xml (apache-commons.text)

@ababushk
Rename CryptoUtilTest to GHWebhookSignatureTest
bb56162 
Previous name wasn't very clear
@ababushk
Copy link
Contributor Author

ababushk commented Dec 3, 2020

Hmm, all those CheckStyle errors are TODO comments, should I fix that?

@limitedAtonement
Copy link

I don't know why I was notified about this pull request. I don't know the Jenkins code base and I'm no expert in Java unicode handling (though unescapedPayload.getBytes("latin1") makes me a wee bit nervous in an ignorant, fear, uncertainty sort of way). I think the only thing I can add is: seven commits is unnecessary. One commit should be fine, or if you really want to break it up, two: one for the test and one for the feature. If you want to chronicle your development saga, do so in commit messages.

Thanks for the fix and for making Jenkins better!

@limitedAtonement
Copy link

Oh yes, I also meant to say, as an outsider, I don't see adding a dependency to apache-commons.text as a liability, especially if done for the sake of avoiding deprecated function calls.

@ababushk
Copy link
Contributor Author

ababushk commented Dec 8, 2020

@limitedAtonement I'm sorry you've received unwanted notification, but thank you for the review! I'm completely agree about number of commits, I've just got used to the way it is configured in my team's repo - all commits in PR are squashed before merge. I also agree about new dependency - I think it is better on the long run.

@ababushk
[GHWebhookSignature] Process escaped Unicode characters in a JSON pay…
653f8a5 
…load

[GHWebhookSignatureTest] Add test to check signature generation for unicode payloads

[GHWebhookSignatureTest] Fix test data

We should've pass all those \uXXXX to function, but Java was keeping them as unicode characters inside

[GHWebhookSignature] Use modules available via pom.xml to perform unescape

[GHWebhookSignatureTest] Explain test data choice

[GHWebhookSignatureTest] Remove escaped unicode from comments
@ababushk
[GHWebhookSignature] Use non-deprecated unescapeJava
a728333
@ababushk ababushk force-pushed the JENKINS_50154_unicode_payload branch from b912826 to a728333 Compare December 8, 2020 12:03
@ababushk
Copy link
Contributor Author

ababushk commented Dec 8, 2020

@KostyaSha could you please take a look?

@jonathonbattista
Copy link

Any ETA on this?

Webhooks are completely unusable for us.

@ababushk
Copy link
Contributor Author

@jonathonbattista as a workaround you can build a plugin from my fork https://github.com/ababushk/github-plugin/tree/JENKINS_50154_unicode_payload . You need Java and Maven installed on your system. After you clone the repo, execute mvn hpi:hpi - this will build a *.hpi file which you can install in Jenkins' Update Center's advanced options.

@timja
Copy link
Member

timja commented Dec 31, 2020

Checkstyle errors will be fixed in #243

oleg-nenashev
oleg-nenashev approved these changes Dec 31, 2020
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev 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!

@KostyaSha
Copy link
Member

i think we can try it

@KostyaSha KostyaSha merged commit c1aa272 into jenkinsci:master Jan 3, 2021
@eyalzek
Copy link

eyalzek commented Feb 10, 2021

Jobs stopped triggering for us on merges/new PR after updating to v1.33.0. Can this be the reason? We had to downgrade to 1.32.0

@enys
Copy link

enys commented Feb 15, 2021

It looks like I have the same problem:

<title>Error 400 Provided signature [c0180c723bbe3ddb266be493f5c9c1977896ea15] did not match to calculated</title>
</head>

I have commented the Jira issue.

@eagletmt
Copy link

Same here. I also had to downgrade to 1.32.0 due to the signature mismatch errors.

This PR looks completely broken to me.

  1. new String(unescapedPayload.getBytes("latin1"), UTF_8) replaces all non-latin1 characters to ?
    • So a valid JSON payload (e.g., {"description": "説明"} or {"description": "\u8aac\u660e"}) is converted to broken one
  2. "\u00e2\u0084\u00a2" is not a JSON string for ™ character
    • Its codepoint is U+2122. Thus the correct JSON string should be "\u2122". [e2, 84, a2] is a UTF-8 byte sequence of ™.

@ababushk
Copy link
Contributor Author

@KostyaSha could we revert this while I'm searching for the fix?

@eyalzek
Copy link

eyalzek commented Feb 16, 2021

Previous version is still available here https://updates.jenkins-ci.org/download/plugins/github/1.32.0/github.hpi

seems like it was already removed from the plugin registry though, you can install the link above manually if necessary.

@KostyaSha
Copy link
Member

KostyaSha commented Feb 17, 2021 •
edited

@ababushk could you PR revert? i can merge release then (automatic on GH web doesn't work)

ababushk added a commit to ababushk/github-plugin that referenced this pull request Feb 17, 2021
@ababushk
Revert "Merge pull request jenkinsci#242 from ababushk/JENKINS_50154_…
10418c4 
…unicode_payload"

This reverts commit c1aa272, reversing
changes made to e394f77.
@ababushk
Copy link
Contributor Author

I've created a PR to revert this #246
I apologize to everyone who suffered from this change - looks like the bug is not in the plugin.
I've tried to setup webhooks on Jenkins instance behind corporate firewall using this instruction https://www.jenkins.io/blog/2019/01/07/webhook-firewalls/, but I've taken pysmee (https://github.com/Akrog/pysmee) instead of official smee.io client written in JavaScript. If the bug exists, it is in pysmee - official client passes webhooks correctly.

KostyaSha added a commit that referenced this pull request Feb 17, 2021
@KostyaSha
Merge pull request #246 from ababushk/master
4419c21 
Revert "Merge pull request #242 from ababushk/JENKINS_50154_unicode_p…
@KostyaSha
Copy link
Member

merged revert and released 1.33.1

@jlebon jlebon mentioned this pull request Jun 15, 2021
Closed
jlebon added a commit to jlebon/jenkins that referenced this pull request Jun 15, 2021
@jlebon
base-plugins: bump github to 1.33.1
6251779 
v1.33.0 has a regression breaking GitHub webhooks. It was fixed in
v1.33.1:

jenkinsci/github-plugin#242

This is the only change in v1.33.1 so should be safe:

jenkinsci/github-plugin@v1.33.0...v1.33.1
jlebon added a commit to jlebon/jenkins that referenced this pull request Jun 15, 2021
@jlebon
base-plugins: bump github to 1.33.1
98ab7b2 
v1.33.0 has a regression breaking GitHub webhooks. It was fixed in
v1.33.1:

jenkinsci/github-plugin#242

This is the only change in v1.33.1 so should be safe:

jenkinsci/github-plugin@v1.33.0...v1.33.1

See also: coreos/fedora-coreos-pipeline#352
@jlebon jlebon mentioned this pull request Jun 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@timja timja timja approved these changes

@KostyaSha KostyaSha Awaiting requested review from KostyaSha

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
9 participants
@ababushk @limitedAtonement @jonathonbattista @timja @KostyaSha @eyalzek @enys @eagletmt @oleg-nenashev