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

[JENKINS-34254] Fix RequirePOST form #3187

Merged
merged 5 commits into from
Dec 16, 2017

Conversation

daniel-beck
Copy link
Member

See JENKINS-34254.

Downstream of jenkinsci/stapler#135. Alternative to #3186.

SNAPSHOT dependency on Stapler because I couldn't figure out how to handle the different dates in each stapler component other than not using the stapler.version property at all.

Proposed changelog entries

  • Make the form that shows up when a URL requiring POST is accessed using a different verb work with CSRF protection enabled

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests

Desired reviewers

@jenkinsci/code-reviewers

@oleg-nenashev oleg-nenashev added needs-more-reviews Complex change, which would benefit from more eyes work-in-progress The PR is under active development, not ready to the final review labels Dec 10, 2017
core/pom.xml Outdated
@@ -39,7 +39,7 @@ THE SOFTWARE.

<properties>
<staplerFork>true</staplerFork>
<stapler.version>1.253</stapler.version>
<stapler.version>1.254-SNAPSHOT</stapler.version>
Copy link
Member

Choose a reason for hiding this comment

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

staged release would be nice, but IIUC Stapler settings need to be patched for that

Copy link
Member

Choose a reason for hiding this comment

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

Nope. Add to your ~/.bashrc:

alias deploysnapshot='mvn clean install source:jar deploy:deploy -DaltDeploymentRepository=maven.jenkins-ci.org::default::https://repo.jenkins-ci.org/snapshots/'

Copy link
Member Author

@daniel-beck daniel-beck Dec 11, 2017

Choose a reason for hiding this comment

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

@jglick This still doesn't result in a consistent date for every artifact, which is the reason I used -SNASPSHOT rather than changing all the uses of stapler.version.

Copy link
Member

Choose a reason for hiding this comment

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

1.254 released with just this change, please update.

blurb = The URL you're trying to access requires that requests be sent using POST (like a form submission). \
The button below allows you to retry accessing this URL using POST. \
URL being accessed:
warning = If you were sent here from an untrusted source, please proceed with caution.
Copy link
Member

Choose a reason for hiding this comment

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

not sure why, but GitHub UI does not recognize it as a new variable. Maybe : needs to be escaped

Copy link
Member

Choose a reason for hiding this comment

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

I am guessing Linguist is not very good at parsing line escapes in properties files.

<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">

<l:layout norefresh="true" title="${%Method Not Allowed}">
Copy link
Member

Choose a reason for hiding this comment

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

Does it still need norefresh? IIRC no

Copy link
Member

Choose a reason for hiding this comment

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

Actually I am wrong. #2979 has not been integrated yet

core/pom.xml Outdated
@@ -39,7 +39,7 @@ THE SOFTWARE.

<properties>
<staplerFork>true</staplerFork>
<stapler.version>1.253</stapler.version>
<stapler.version>1.254-SNAPSHOT</stapler.version>
Copy link
Member

Choose a reason for hiding this comment

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

Nope. Add to your ~/.bashrc:

alias deploysnapshot='mvn clean install source:jar deploy:deploy -DaltDeploymentRepository=maven.jenkins-ci.org::default::https://repo.jenkins-ci.org/snapshots/'

blurb = The URL you're trying to access requires that requests be sent using POST (like a form submission). \
The button below allows you to retry accessing this URL using POST. \
URL being accessed:
warning = If you were sent here from an untrusted source, please proceed with caution.
Copy link
Member

Choose a reason for hiding this comment

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

I am guessing Linguist is not very good at parsing line escapes in properties files.

${%blurb}
</p>
<p><tt>${requestURL}</tt></p>
<p><strong>${%warning}</strong></p>
Copy link
Member

Choose a reason for hiding this comment

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

Can we check the Referer [sic]?

Copy link
Member Author

Choose a reason for hiding this comment

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

Check how? Or do you mean we should just print the referer as well, in addition to the URL?

@daniel-beck daniel-beck removed the work-in-progress The PR is under active development, not ready to the final review label Dec 15, 2017
@daniel-beck daniel-beck merged commit 76c9f8b into jenkinsci:master Dec 16, 2017
olivergondza pushed a commit that referenced this pull request Jan 25, 2018
[JENKINS-34254] Fix RequirePOST form

(cherry picked from commit 76c9f8b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-more-reviews Complex change, which would benefit from more eyes
Projects
None yet
3 participants