Escape user-supplied values when setting build description #146

jakub-bochenski · 2019-08-22T16:02:23Z

@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.

proski

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?

proski · 2019-08-22T17:00:20Z

src/main/resources/ESAPI.properties

@@ -0,0 +1 @@
+ESAPI.Encoder=org.owasp.esapi.reference.DefaultEncoder


The final newline is missing. It would be better to have it.

proski · 2019-08-22T17:01:13Z

src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashCause.java

+                      + this.getDestinationRepositoryName()
+                      + "/pull-requests/"
+                      + this.getPullRequestId(),
+                  null));


Please use String.format and drop this. (also in getEscapedDescription)

proski · 2019-08-22T17:04:00Z

src/test/java/stashpullrequestbuilder/stashpullrequestbuilder/StashCauseTest.java

+        stashCause.getShortDescription(),
+        is(
+            "<a href='&#x2f;projects&#x2f;owner&#x25;3C&#x25;3E&amp;&#x27;&#x25;22&#x2f;repos&#x2f;name&#x25;3C&#x25;3E&amp;&#x27;&#x25;22&#x2f;pull-requests&#x2f;id&#x25;3C&#x25;3E&amp;&#x27;&#x25;22'>"
+                + "PR &#x23;id&lt;&gt;&amp;&#x27;&quot; title&lt;&gt;&amp;&#x27;&quot; </a>"));


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.

proski · 2019-08-22T17:04:57Z

src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashCause.java

-        + " "
-        + this.getPullRequestTitle()
-        + " </a>";
+    return "<a href='" + getEscapedUrl() + "'>" + getEscapedDescription() + " </a>";


Please consider removing space before </a>.

proski · 2019-08-22T17:23:38Z

pom.xml

+      <exclusions>
+        <exclusion>
+          <groupId>org.slf4j</groupId>
+          <artifactId>slf4j-api</artifactId>


We are using slf4j-api for poll logging. Dependency management should be preferred over exclusion.

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.

proski · 2019-08-22T17:41:11Z

@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.

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.

proski · 2019-08-23T04:39:48Z

The size of stash-pullrequest-builder.hpi jumps from tens of kilobytes to 8 megabytes. That's quite an overkill for quoting symbols. Not a blocker for a security fix, but something we should try to replace quickly.

Bundled jars:

3.1M xalan-2.7.2.jar
1.4M xercesImpl-2.12.0.jar
736K commons-collections4-4.2.jar
656K xmlgraphics-commons-2.3.jar
431K esapi-2.2.0.0.jar
380K bsh-2.0b6.jar
355K commons-configuration-1.10.jar
318K batik-css-1.11.jar
306K xom-1.2.10.jar
278K commons-lang-2.6.jar
270K serializer-2.7.2.jar
241K commons-beanutils-1.9.3.jar
216K xml-apis-1.4.01.jar
210K commons-io-2.6.jar
141K antisamy-1.5.8.jar
126K batik-util-1.11.jar
123K nekohtml-1.9.22.jar
84K xml-apis-ext-1.3.04.jar
69K commons-fileupload-1.3.3.jar
69K stash-pullrequest-builder.jar
12K batik-i18n-1.11.jar
8.1K batik-constants-1.11.jar

jakub-bochenski · 2019-08-23T09:41:10Z

The size of stash-pullrequest-builder.hpi jumps from tens of kilobytes to 8 megabytes.

Wow, good catch. I guess I understand why they call it "Enterprise Security" now :)

jakub-bochenski · 2019-08-23T10:00:04Z

I was sloppy with this code because I don't really like this way of addressing it.
After thinking about this a bit more I'm convinced we should rather go with the badges approach.

The security implications are not that serious (none for the antisamy formatter actually) unless you use something like anything-goes formatter.

jakub-bochenski · 2019-08-23T10:01:56Z

Do we need any special encoding?

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 sinlgeQuote for that.

proski · 2019-11-28T20:02:10Z

src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashCause.java

+                      + this.getPullRequestId(),
+                  null));
+    } catch (URISyntaxException e) {
+      throw new IllegalStateException(e);


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.

hiding the exception was intentional: my rationale was that it's unlikely or impossible to happen given the arguments

Escape user-supplied values when setting build description

a32ce15

proski suggested changes Aug 22, 2019

View reviewed changes

proski reviewed Nov 28, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Escape user-supplied values when setting build description #146

Escape user-supplied values when setting build description #146

jakub-bochenski commented Aug 22, 2019

proski left a comment

proski Aug 22, 2019

proski Aug 22, 2019

proski Aug 22, 2019

proski Aug 22, 2019

proski Aug 22, 2019

proski Aug 23, 2019

proski commented Aug 22, 2019

proski commented Aug 23, 2019

jakub-bochenski commented Aug 23, 2019

jakub-bochenski commented Aug 23, 2019

jakub-bochenski commented Aug 23, 2019

proski Nov 28, 2019

jakub-bochenski Nov 29, 2019 •

edited

Loading

		@@ -0,0 +1 @@
		ESAPI.Encoder=org.owasp.esapi.reference.DefaultEncoder

Escape user-supplied values when setting build description #146

Are you sure you want to change the base?

Escape user-supplied values when setting build description #146

Conversation

jakub-bochenski commented Aug 22, 2019

proski left a comment

Choose a reason for hiding this comment

proski Aug 22, 2019

Choose a reason for hiding this comment

proski Aug 22, 2019

Choose a reason for hiding this comment

proski Aug 22, 2019

Choose a reason for hiding this comment

proski Aug 22, 2019

Choose a reason for hiding this comment

proski Aug 22, 2019

Choose a reason for hiding this comment

proski Aug 23, 2019

Choose a reason for hiding this comment

proski commented Aug 22, 2019

proski commented Aug 23, 2019

jakub-bochenski commented Aug 23, 2019

jakub-bochenski commented Aug 23, 2019

jakub-bochenski commented Aug 23, 2019

proski Nov 28, 2019

Choose a reason for hiding this comment

jakub-bochenski Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

jakub-bochenski Nov 29, 2019 •

edited

Loading