Escape user-supplied values when setting build description #146
Conversation
There was a problem hiding this comment.
I don't see ESAPI being used in Jenkins or in any plugin I use for reference. Escaping is mostly done using hudson.Util https://javadoc.jenkins.io/hudson/Util.html#escape-java.lang.String-
Do we need any special encoding?
| @@ -0,0 +1 @@ | |||
| ESAPI.Encoder=org.owasp.esapi.reference.DefaultEncoder | |||
There was a problem hiding this comment.
The final newline is missing. It would be better to have it.
| + this.getDestinationRepositoryName() | ||
| + "/pull-requests/" | ||
| + this.getPullRequestId(), | ||
| null)); |
There was a problem hiding this comment.
Please use String.format and drop this. (also in getEscapedDescription)
| stashCause.getShortDescription(), | ||
| is( | ||
| "<a href='/projects/owner%3C%3E&'%22/repos/name%3C%3E&'%22/pull-requests/id%3C%3E&'%22'>" | ||
| + "PR #id<>&'" title<>&'" </a>")); |
There was a problem hiding this comment.
A shorter string would be more readable. We are testing that the escaping is done at all. It would be too much to test the underlying quoting algorithm.
| + " " | ||
| + this.getPullRequestTitle() | ||
| + " </a>"; | ||
| return "<a href='" + getEscapedUrl() + "'>" + getEscapedDescription() + " </a>"; |
There was a problem hiding this comment.
Please consider removing space before </a>.
| <exclusions> | ||
| <exclusion> | ||
| <groupId>org.slf4j</groupId> | ||
| <artifactId>slf4j-api</artifactId> |
There was a problem hiding this comment.
We are using slf4j-api for poll logging. Dependency management should be preferred over exclusion.
There was a problem hiding this comment.
I tried that and I don't like the result. Essentially we would promise the code that it would find httpclient 4.5.8 and slf4j 1.7.26 on the server, which is not true. Or we would have to bundle those versions, which would bloat the hpi even more.
If it's a security issue, let's fix it now and do cleanups later. I agree that adding links to descriptions should be avoided it possible. The user needs to install a plugin and enable safe HTML rendering as described under "Recommended": https://wiki.jenkins.io/display/JENKINS/Stash+pullrequest+builder+plugin I was even thinking of having a global option for Stash Pull Request Builder to enable links in descriptions. The help would describe how to enable safe HTML. |
|
The size of Bundled jars: |
Wow, good catch. I guess I understand why they call it "Enterprise Security" now :) |
|
I was sloppy with this code because I don't really like this way of addressing it. The security implications are not that serious (none for the antisamy formatter actually) unless you use something like anything-goes formatter. |
https://javadoc.jenkins.io/hudson/Util.html#escape-java.lang.String- doesn't work for html attributes, but now I see I could probbably use |
| + this.getPullRequestId(), | ||
| null)); | ||
| } catch (URISyntaxException e) { | ||
| throw new IllegalStateException(e); |
There was a problem hiding this comment.
sb-contrib detects an issue here: "Unconstrained method stashpullrequestbuilder.stashpullrequestbuilder.StashCause.getPrUrl() converts checked exception to unchecked [stashpullrequestbuilder.stashpullrequestbuilder.StashCause] At StashCause.java:[line 160] EXS_EXCEPTION_SOFTENING_NO_CONSTRAINTS"
Let's use checked exceptions, the compiler would force us to handle them.
There was a problem hiding this comment.
hiding the exception was intentional: my rationale was that it's unlikely or impossible to happen given the arguments
@proski I wonder WDYT about this change.
This is a hack to cover a common case when using a html-enabled markup formatted.
However I've learned that updating build description the way this plugin does it now is fundamentally wrong. It should be instead adding a build badge. Build badges also have an API to make it easier to escape input for html.