Crumb exclusion should be more forgiving if the user leaves off the trailing slash #152

i386 · 2016-11-05T06:22:03Z

Discovered that if I left out the trailing slash on my webhook configuration in Github (e.g. /github-webook rather than /github-webhook/) then incoming requests would not be covered by the crumb exclusion.

This change also adds tests for GitHubWebHookCrumbExclusion.

PTAL @jenkinsci/code-reviewers @KostyaSha

This change is

jglick · 2016-11-07T22:33:28Z

What is the purpose of this? If Jenkins receives a request to /github-webhook it will simply redirect you to /github-webhook/ before servicing GitHubWebHook.doIndex:

$ curl -IL https://jenkins.ci.cloudbees.com/github-webhook
HTTP/1.1 302 Found
Date: Mon, 07 Nov 2016 22:32:45 GMT
Expires: Thu, 01 Jan 1970 00:00:00 GMT
Location: https://jenkins.ci.cloudbees.com/github-webhook/
Server: Jetty(9.2.z-SNAPSHOT)
Set-Cookie: JSESSIONID.f61ecab1=1cu3g6hvkmjo21mdyuj1ixkhgu;Path=/;Secure;HttpOnly
X-Content-Type-Options: nosniff
Connection: keep-alive

HTTP/1.1 405 Method Not Allowed
Content-Type: text/plain;charset=UTF-8
Date: Mon, 07 Nov 2016 22:32:45 GMT
Server: Jetty(9.2.z-SNAPSHOT)
X-Content-Type-Options: nosniff
Connection: keep-alive

so you would better use the form ending with / to begin with.

KostyaSha · 2016-11-10T21:39:37Z

I also don't understand profit of it. Url format should be also documented. A lot of APIs are sensitive and users should follow docs.

lanwen · 2016-11-11T11:25:32Z

Agree with @KostyaSha. So many code to avoid user mistake...

i386 · 2016-11-16T00:35:34Z

@jglick from my testing it seems Github wasn't following the redirect.

michaelneale · 2016-11-16T00:37:47Z

github does not follow the redirect. @lanwen its a net increase of 4 lines of code for something that bites users daily - is that a lot of code? (did you see that most of it is test cases) @jglick github does not follow the redirect: isaacs/github#574. You do realise this PR is about the "github plugin" so the redirect won't work (github won't change this, they haven't so far, they are not going to). Did you read the PR?

i386 · 2016-11-16T00:38:47Z

@KostyaSha @lanwen I strongly disagree - it's very easy to leave a slash off of the end of the URL and not pick up on why it's not working. This removes the problem all together at the cost of a few lines of code in the Git plugin (and I even wrote some tests!)

We can make more users satisfied with Jenkins if we are more forgiving with their mistakes. So closing this PR as WONTFIX is 🐛

daniel-beck · 2016-11-16T00:40:41Z

I'll just leave this link here:
https://en.wikipedia.org/wiki/Robustness_principle

KostyaSha · 2016-11-16T00:45:37Z

github does not follow the redirect.

Where did you found this?

i386 · 2016-11-16T00:46:38Z

@KostyaSha isaacs/github#574

I also ran into this problem myself which motivated me to take time out of my weekend to make this change.

michaelneale · 2016-11-16T00:47:53Z

@KostyaSha years of using github with random webhook services that send 302's

KostyaSha · 2016-11-16T00:49:46Z

Could you please stop editing your comments? I can't collect everything to follow.

KostyaSha · 2016-11-16T00:53:54Z

src/main/java/com/cloudbees/jenkins/GitHubWebHookCrumbExclusion.java

        }
-        return false;
+        pathInfo = !pathInfo.endsWith("/") ? pathInfo + '/' : pathInfo;


Please place comment here and with link to source of crazy GH API nuances.

KostyaSha · 2016-11-16T00:54:48Z

src/main/java/com/cloudbees/jenkins/GitHubWebHookCrumbExclusion.java

-        if (pathInfo != null && pathInfo.equals(getExclusionPath())) {
-            chain.doFilter(req, resp);
-            return true;
+        if (pathInfo == null || pathInfo.equals("")) {


Use StringUtils.isEmpty() (AFAIR) with static import

KostyaSha · 2016-11-16T00:56:24Z

@KostyaSha isaacs/github#574

Second time on this week i shocked how GH handle HTTP.

KostyaSha · 2016-11-16T00:57:29Z

@michaelneale blocked me on GH so i have no notifications about his comments.

KostyaSha

comments

i386 · 2016-11-16T01:08:24Z

@KostyaSha I've updated the PR with the requested changes.

KostyaSha · 2016-11-16T01:11:02Z

src/main/java/com/cloudbees/jenkins/GitHubWebHookCrumbExclusion.java

+            return false;
+        }
+        // Github will not follow redirects https://github.com/isaacs/github/issues/574
+        pathInfo = !pathInfo.endsWith("/") ? pathInfo + '/' : pathInfo;


please swap arguments to exclude negative logic
pathInfo = pathInfo.endsWith("/") ? pathInfo : pathInfo + '/' ;

KostyaSha · 2016-11-16T01:14:11Z

src/main/java/com/cloudbees/jenkins/GitHubWebHookCrumbExclusion.java

+            return false;
+        }
+        // Github will not follow redirects https://github.com/isaacs/github/issues/574
+        pathInfo = pathInfo.endsWith("/") ?  pathInfo : pathInfo + '/';


extra space...

i386 · 2016-11-16T01:22:41Z

Thanks @KostyaSha

…es off the trailing slash

KostyaSha · 2016-11-16T01:24:34Z

Will keep for @lanwen review. 4am. 💤

i386 · 2016-11-16T01:24:55Z

Squashed these commits.

KostyaSha · 2016-11-16T01:25:23Z

@i386 no need, github has squash button.

KostyaSha · 2016-11-16T01:25:59Z

KostyaSha

lgtm

lanwen · 2016-11-16T09:37:27Z

ok guys, you've convinced me

lanwen · 2016-11-16T09:43:47Z

released as 1.23.1

michaelneale · 2016-11-16T09:54:55Z

thanks! @lanwen hahaha yes. I have no idea why github don't follow, but I suspect that it doesn't as it adds latency that is "hidden" (my theory). Anyway, they do what they do, and they are the 400 pound gorrilla so we have to follow their rules ;)

i386 · 2016-11-16T10:46:21Z

Thank you!

i386 force-pushed the more-forgiving-config branch 4 times, most recently from 3a1df8f to 098e1cb Compare November 7, 2016 21:31

lanwen closed this Nov 11, 2016

KostyaSha reviewed Nov 16, 2016

View reviewed changes

KostyaSha reopened this Nov 16, 2016

KostyaSha requested changes Nov 16, 2016

View reviewed changes

KostyaSha reviewed Nov 16, 2016

View reviewed changes

KostyaSha approved these changes Nov 16, 2016

View reviewed changes

GitHubWebHookCrumbExclusion should be more forgiving if the user leav…

b522499

…es off the trailing slash

i386 force-pushed the more-forgiving-config branch from d25704e to b522499 Compare November 16, 2016 01:24

KostyaSha approved these changes Nov 16, 2016

View reviewed changes

lanwen merged commit f784e19 into jenkinsci:master Nov 16, 2016

KostyaSha mentioned this pull request Jan 9, 2017

Web hook not triggering build KostyaSha/github-integration-plugin#184

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crumb exclusion should be more forgiving if the user leaves off the trailing slash #152

Crumb exclusion should be more forgiving if the user leaves off the trailing slash #152

i386 commented Nov 5, 2016 •

edited

Loading

jglick commented Nov 7, 2016

KostyaSha commented Nov 10, 2016

lanwen commented Nov 11, 2016

i386 commented Nov 16, 2016

michaelneale commented Nov 16, 2016 •

edited

Loading

i386 commented Nov 16, 2016 •

edited

Loading

daniel-beck commented Nov 16, 2016

KostyaSha commented Nov 16, 2016

i386 commented Nov 16, 2016 •

edited

Loading

michaelneale commented Nov 16, 2016

KostyaSha commented Nov 16, 2016

KostyaSha Nov 16, 2016

KostyaSha Nov 16, 2016

KostyaSha commented Nov 16, 2016

KostyaSha commented Nov 16, 2016

KostyaSha left a comment

i386 commented Nov 16, 2016

KostyaSha Nov 16, 2016

i386 Nov 16, 2016

KostyaSha Nov 16, 2016

i386 Nov 16, 2016

i386 commented Nov 16, 2016

KostyaSha commented Nov 16, 2016

i386 commented Nov 16, 2016

KostyaSha commented Nov 16, 2016

KostyaSha commented Nov 16, 2016

KostyaSha left a comment

lanwen commented Nov 16, 2016

lanwen commented Nov 16, 2016

michaelneale commented Nov 16, 2016

i386 commented Nov 16, 2016

Crumb exclusion should be more forgiving if the user leaves off the trailing slash #152

Crumb exclusion should be more forgiving if the user leaves off the trailing slash #152

Conversation

i386 commented Nov 5, 2016 • edited Loading

jglick commented Nov 7, 2016

KostyaSha commented Nov 10, 2016

lanwen commented Nov 11, 2016

i386 commented Nov 16, 2016

michaelneale commented Nov 16, 2016 • edited Loading

i386 commented Nov 16, 2016 • edited Loading

daniel-beck commented Nov 16, 2016

KostyaSha commented Nov 16, 2016

i386 commented Nov 16, 2016 • edited Loading

michaelneale commented Nov 16, 2016

KostyaSha commented Nov 16, 2016

KostyaSha Nov 16, 2016

Choose a reason for hiding this comment

KostyaSha Nov 16, 2016

Choose a reason for hiding this comment

KostyaSha commented Nov 16, 2016

KostyaSha commented Nov 16, 2016

KostyaSha left a comment

Choose a reason for hiding this comment

i386 commented Nov 16, 2016

KostyaSha Nov 16, 2016

Choose a reason for hiding this comment

i386 Nov 16, 2016

Choose a reason for hiding this comment

KostyaSha Nov 16, 2016

Choose a reason for hiding this comment

i386 Nov 16, 2016

Choose a reason for hiding this comment

i386 commented Nov 16, 2016

KostyaSha commented Nov 16, 2016

i386 commented Nov 16, 2016

KostyaSha commented Nov 16, 2016

KostyaSha commented Nov 16, 2016

KostyaSha left a comment

Choose a reason for hiding this comment

lanwen commented Nov 16, 2016

lanwen commented Nov 16, 2016

michaelneale commented Nov 16, 2016

i386 commented Nov 16, 2016

i386 commented Nov 5, 2016 •

edited

Loading

michaelneale commented Nov 16, 2016 •

edited

Loading

i386 commented Nov 16, 2016 •

edited

Loading

i386 commented Nov 16, 2016 •

edited

Loading