Skip to content

Remove the prototype from Fortify plugin #70

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

Closed
wants to merge 5 commits into from
Closed

Conversation

akaryakina
Copy link
Contributor

This is to remove usages of Prototype from the Fortify plugin to address #64

I tested this locally with the latest Jenkins LTS version with a user that has Prototype disabled.

I'm asking if anyone could code review the changes since I'm not a javascript expert.

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

This all looks correct. The POST method and CSRF crumb handling all looks correct.

After doing a number of these conversions, we noticed that it was common convention for POST methods with parameters to set the Content-Type: application/x-www-form-urlencoded header and send the parameters in the POST body rather than the query string. And this matches how Prototype used to do it, as well. But in practice there is no downside to sending these parameters in the URL query string, either (unless one considers it a downside that some web servers will log this data in access logs, which I don't consider to be a downside). In any case, we cleaned this up in Jenkins core in jenkinsci/jenkins#8279, and you might consider doing the same here, but that is an optional code cleanup, and this PR is fine to deliver as-is.

Comment on lines +207 to +213
<dependency>
<groupId>jakarta.mail</groupId>
<artifactId>jakarta.mail-api</artifactId>
<version>2.1.2</version>
<type>jar</type>
<scope>compile</scope>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to the Prototype changes, but in Jenkins plugins we typically prefer plugin-to-plugin dependencies for libraries that are packaged as Jenkins plugins:

<dependency>
    <groupId>io.jenkins.plugins</groupId>
    <artifactId>jakarta-mail-api</artifactId>
    <version>2.0.1-3</version>
</dependency>

The reasoning is explained here and can be summarized as follows. By depending on this library via a Jenkins plugin, your users will get security updates as part of the regular plugin update process. When you bundle the library into the Fortify plugin, that benefit is lost.

@akaryakina
Copy link
Contributor Author

@basil Thank you SO MUCH for looking at the code! Yes, the POST parameters were exactly the item that bothered me, and I am very grateful to you for spending your time. It is really not a good idea to expose parameters this way for a security plugin, even though they are not supposed to be passing any sensitive information. But when we pass a piece of text that the user might have typed in a form, we could accidentally expose private information and just make it easier for everyone to collect it.
As for the javax -> jakarta, that's a very good idea, indeed. I'll try to modify that to a plugin dependency as soon as I am able to update the minimal required jenkins version.

@akaryakina akaryakina self-assigned this Nov 6, 2023
@akaryakina akaryakina closed this Nov 6, 2023
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.

2 participants