Escape user-supplied values when setting build description #146
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| package stashpullrequestbuilder.stashpullrequestbuilder; | ||
|
|
||
| import static org.hamcrest.CoreMatchers.is; | ||
| import static org.hamcrest.object.HasToString.hasToString; | ||
| import static org.junit.Assert.assertThat; | ||
|
|
||
| import org.junit.Test; | ||
|
|
||
| public class StashCauseTest { | ||
|
|
||
| @Test | ||
| public void short_description_doesnt_allow_injection() { | ||
|
|
||
| StashCause stashCause = | ||
| new StashCause( | ||
| "", | ||
| "", | ||
| "", | ||
| "", | ||
| "", | ||
| PULL_REQUEST_ID, | ||
| OWNER, | ||
| DESTINATION_REPOSITORY_NAME, | ||
| PULL_REQUEST_TITLE, | ||
| "", | ||
| "", | ||
| "", | ||
| "", | ||
| null); | ||
|
|
||
| assertThat( | ||
| 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. |
||
| } | ||
|
|
||
| @Test | ||
| public void will_produce_valid_PR_url() { | ||
|
|
||
| StashCause stashCause = | ||
| new StashCause( | ||
| "https://stash.host", | ||
| "", | ||
| "", | ||
| "", | ||
| "", | ||
| PULL_REQUEST_ID, | ||
| OWNER, | ||
| DESTINATION_REPOSITORY_NAME, | ||
| PULL_REQUEST_TITLE, | ||
| "", | ||
| "", | ||
| "", | ||
| "", | ||
| null); | ||
|
|
||
| assertThat( | ||
| stashCause.getPrUrl(), | ||
| hasToString( | ||
| "https://stash.host/projects/owner%3C%3E&'%22/repos/name%3C%3E&'%22/pull-requests/id%3C%3E&'%22")); | ||
| } | ||
|
|
||
| @Test | ||
| public void will_escape_PR_url() { | ||
|
|
||
| StashCause stashCause = | ||
| new StashCause( | ||
| "https://stash.host", | ||
| "", | ||
| "", | ||
| "", | ||
| "", | ||
| PULL_REQUEST_ID, | ||
| OWNER, | ||
| DESTINATION_REPOSITORY_NAME, | ||
| PULL_REQUEST_TITLE, | ||
| "", | ||
| "", | ||
| "", | ||
| "", | ||
| null); | ||
|
|
||
| assertThat( | ||
| stashCause.getEscapedUrl(), | ||
| is( | ||
| "https://stash.host/projects/owner%3C%3E&'%22/repos/name%3C%3E&'%22/pull-requests/id%3C%3E&'%22")); | ||
| } | ||
|
|
||
| @Test | ||
| public void will_escape_PR_description() { | ||
|
|
||
| StashCause stashCause = | ||
| new StashCause( | ||
| "https://stash.host", | ||
| "", | ||
| "", | ||
| "", | ||
| "", | ||
| PULL_REQUEST_ID, | ||
| OWNER, | ||
| DESTINATION_REPOSITORY_NAME, | ||
| PULL_REQUEST_TITLE, | ||
| "", | ||
| "", | ||
| "", | ||
| "", | ||
| null); | ||
|
|
||
| assertThat( | ||
| stashCause.getEscapedDescription(), | ||
| is("PR #id<>&'" title<>&'"")); | ||
| } | ||
|
|
||
| public static final String OWNER = "owner<>&'\""; | ||
| public static final String DESTINATION_REPOSITORY_NAME = "name<>&'\""; | ||
| public static final String PULL_REQUEST_ID = "id<>&'\""; | ||
| public static final String PULL_REQUEST_TITLE = "title<>&'\""; | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using slf4j-api for poll logging. Dependency management should be preferred over exclusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.