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 SECURITY-1025 #11

Merged
merged 8 commits into from
Nov 2, 2023
Merged

Fix SECURITY-1025 #11

merged 8 commits into from
Nov 2, 2023

Conversation

lacostej
Copy link
Contributor

@lacostej lacostej commented Oct 30, 2023

I hope this fixes SECURITY-1025. Any way to check if there are other paths to protect?

Testing done

Submitter checklist

Preview Give feedback

@MarkEWaite
Copy link

Could anyone please replay this PR build with the updated Jenkinsfile?

Done

@lacostej
Copy link
Contributor Author

I think I now have handled all cases that needed to be handled.

image

@@ -264,6 +265,7 @@ public Object getDynamic(String token, StaplerRequest req, StaplerResponse rsp)
/**
* Schedules the execution
*/
@POST

Choose a reason for hiding this comment

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

With addition of these annotations UI buttons seem to be broken.
I've create a dummy freestyle job and added 2 batch tasks to it. Then when I go to JENKINS/job/JOB_NAME/batchTasks/task/TASK_NAME/ and click "Build Now" I get a 404. Same with "Delete Task".

I suspect that culprits are https://github.com/jenkinsci/batch-task-plugin/blob/master/src/main/resources/hudson/plugins/batch_task/BatchTask/sidepanel.jelly#L18 and https://github.com/jenkinsci/batch-task-plugin/blob/master/src/main/resources/hudson/plugins/batch_task/BatchTask/delete.jelly#L9C11-L9C23

For l:task there exists a post attribute, so the fix should be straightforward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perfect. Note that I removed the delete.jelly as it does no seem to be used.

I am still able to delete tasks without it.

* @param expectedStatus if expecting a failing http status code or null if not
* @throws IOException if so
*/
public static HtmlPage post(JenkinsRule.WebClient webClient, String relative,

Choose a reason for hiding this comment

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

FYI utility methods exist in jenkins-test-harness that allow you to send post requests already.
See https://github.com/jenkinsci/jenkins/blob/9012a5b35aca298e4cfa23c6ca95242690c232c3/test/src/test/java/hudson/model/ViewTest.java#L264-L267 for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I adjusted my helper method.

Copy link

@yaroslavafenkin yaroslavafenkin left a comment

Choose a reason for hiding this comment

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

Works well in local testing.

@@ -326,6 +327,7 @@ public String getUrlName() {
/**
* Handles incremental log output.
*/
@POST

Choose a reason for hiding this comment

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

This is not really needed, no CSRF here.

@lacostej lacostej merged commit b6bbc46 into jenkinsci:master Nov 2, 2023
1 check was pending
@lacostej lacostej deleted the SECURITY-1025 branch November 2, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants