Skip to content

Commit

Permalink
fix: security alerts fix (uncontrolled data usage, potential XSS vuln…
Browse files Browse the repository at this point in the history 
…erabilities & inefficient regular expressions)
  • Loading branch information
@Jumas
Jumas committed Jun 6, 2024
1 parent 43e0758 commit a695f89
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 34 deletions.
49 changes: 32 additions & 17 deletions app/src/main/java/ch/sbb/polarion/extension/generic/GenericUiServlet.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package ch.sbb.polarion.extension.generic;

import com.polarion.alm.shared.util.Pair;
import com.polarion.core.util.logging.Logger;
import org.apache.commons.io.IOUtils;
import org.jetbrains.annotations.NotNull;
Expand All @@ -16,11 +17,22 @@
import java.io.Serial;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.Arrays;
import java.util.List;
import java.util.zip.ZipEntry;
import java.util.zip.ZipFile;

public abstract class GenericUiServlet extends HttpServlet {

private static final List<Pair<String, String>> ALLOWED_FILE_TYPES = Arrays.asList(
Pair.of(".js", "text/javascript"),
Pair.of(".html", "text/html"),
Pair.of(".css", "text/css"),
Pair.of(".png", "image/png"),
Pair.of(".svg", "image/svg+xml"),
Pair.of(".gif", "image/gif")
);

private static final Logger logger = Logger.getLogger(GenericUiServlet.class);

@Serial
Expand All @@ -34,36 +46,39 @@ protected GenericUiServlet(String webAppName) {

@VisibleForTesting
static void setContentType(@NotNull String uri, @NotNull HttpServletResponse response) {
if (uri.endsWith(".js")) {
response.setContentType("text/javascript");
} else if (uri.endsWith(".html")) {
response.setContentType("text/html");
} else if (uri.endsWith(".png")) {
response.setContentType("image/png");
} else if (uri.endsWith(".css")) {
response.setContentType("text/css");
}
response.setContentType(ALLOWED_FILE_TYPES.stream().filter(f -> uri.endsWith(f.left())).map(Pair::right).findFirst()
.orElseThrow(() -> new IllegalArgumentException("Unsupported file type")));
}

@Override
protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
String uri = request.getRequestURI();
String relativeUri = uri.substring("/polarion/".length());
String resourceUri = getInternalResourcePath(request.getRequestURI());
try {
if (relativeUri.startsWith(webAppName + "/ui/generic/")) {
serveGenericResource(response, relativeUri.substring((webAppName + "/ui/generic/").length()));
} else if (relativeUri.startsWith(webAppName + "/ui/")) {
serveResource(response, relativeUri.substring((webAppName + "/ui").length()));
if (resourceUri.startsWith("generic/")) {
serveGenericResource(response, resourceUri.substring("generic/".length()));
} else {
serveResource(response, "/" + resourceUri);
}
} catch (IOException e) {
logger.error("Cannot copy resource '" + relativeUri + "': " + e.getMessage(), e);
logger.error("Cannot copy resource '" + resourceUri + "': " + e.getMessage(), e);
throw e;
} catch (Exception e) {
logger.error("Unexpected error by getting resource '" + relativeUri + "': " + e.getMessage(), e);
logger.error("Unexpected error by getting resource '" + resourceUri + "': " + e.getMessage(), e);
throw new ServletException(e.getMessage(), e);
}
}

private String getInternalResourcePath(String fullUri) {
String acceptablePath = "/polarion/" + webAppName + "/ui/";
if (!fullUri.startsWith(acceptablePath)) {
throw new IllegalArgumentException("Unsupported resource path");
}
if (ALLOWED_FILE_TYPES.stream().noneMatch(t -> fullUri.endsWith(t.left()))) {
throw new IllegalArgumentException("Unsupported file type");
}
return fullUri.substring(acceptablePath.length());
}

@VisibleForTesting
void serveGenericResource(@NotNull HttpServletResponse response, @NotNull String uri) throws IOException, URISyntaxException {
final URI currentJar = GenericUiServlet.class.getProtectionDomain().getCodeSource().getLocation().toURI();
Expand Down
4 changes: 2 additions & 2 deletions app/src/main/resources/js/common.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ const SbbCommon = {
Prism.languages.properties = {
comment: /^[ \t]*[#!].*$/m,
//ORIGINAL value: {pattern: /(^[ \t]*(?:\\(?:\r\n|[\s\S])|[^\\\s:=])+(?: *[=:] *(?! )| ))(?:\\(?:\r\n|[\s\S])|[^\\\r\n])+/m, lookbehind: !0, alias: "attr-value"},
value: {pattern: /(^[ \t]*(?:\\(?:\r\n|[\s\S])|[^\\=])+(?: *= *(?! )| ))(?:\\(?:\r\n|[\s\S])|[^\\\r\n])+/m, lookbehind: !0, alias: "attr-value"},
value: {pattern: /(^[ \t]*(?:\\[\s\S]|[^\\=])+(?: *= *(?! )| ))(?:\\(?:\r\n|[\s\S])|[^\\\r\n])+/m, lookbehind: !0, alias: "attr-value"},
//ORIGINAL key: {pattern: /^[ \t]*(?:\\(?:\r\n|[\s\S])|[^\\\s:=])+(?= *[=:]| )/m, alias: "attr-name"},
key: {pattern: /^[ \t]*(?:\\(?:\r\n|[\s\S])|[^\\=])+(?= *[=:]| )/m, alias: "attr-name"},
key: {pattern: /^[ \t]*(?:\\[\s\S]|[^\\=])+(?= *[=:]| )/m, alias: "attr-name"},
punctuation: /[=:]/
};
}
Expand Down
14 changes: 12 additions & 2 deletions app/src/main/resources/js/custom-select.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ SbbCustomSelect.prototype.selectMultipleValues = function(values) {

SbbCustomSelect.prototype.handleChange = function(event) {
if (this.mutiselect) {
this.selectElement.innerHTML = "<option>" + this.getSelectedText().join(", ") + "</option>";
this.setSelectedOptionValue(this.getSelectedText().join(", "));
if (this.changeListener) {
this.changeListener(this.checkboxContainer.querySelectorAll('input[type="checkbox"]:checked'));
}
Expand All @@ -146,7 +146,7 @@ SbbCustomSelect.prototype.handleChange = function(event) {
}
});

this.selectElement.innerHTML = "<option>" + selectedCheckbox.parentElement.textContent + "</option>";
this.setSelectedOptionValue(selectedCheckbox.parentElement.textContent);
this.checkboxContainer.querySelectorAll('label').forEach(function (label) {
label.classList.remove("selected");
if (label.textContent === selectedCheckbox.parentElement.textContent) {
Expand All @@ -161,4 +161,14 @@ SbbCustomSelect.prototype.handleChange = function(event) {
}
this.checkboxContainer.style.display = "none";
}

// Using code like:
// this.selectElement.innerHTML = "<option>" + document.createTextNode(selectedCheckbox.parentElement.textContent).textContent + "</option>"
// results in XSS vulnerability. The code below solves this issue.
SbbCustomSelect.prototype.setSelectedOptionValue = function(optionText) {
const optionElement = document.createElement("option");
optionElement.textContent = optionText;
this.selectElement.innerHTML = '';
this.selectElement.appendChild(optionElement);
};
}
35 changes: 22 additions & 13 deletions app/src/test/java/ch/sbb/polarion/extension/generic/GenericUiServletTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import javax.servlet.http.HttpServletResponse;
import java.io.Serial;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.*;

Expand All @@ -25,37 +26,45 @@ void testSetContentType() {
GenericUiServlet.setContentType("/sub_path/file.html", response);
verify(response, times(1)).setContentType("text/html");

response = mock(HttpServletResponse.class);
GenericUiServlet.setContentType("https://localhost/styles.css", response);
verify(response, times(1)).setContentType("text/css");

response = mock(HttpServletResponse.class);
GenericUiServlet.setContentType("/img.png", response);
verify(response, times(1)).setContentType("image/png");

response = mock(HttpServletResponse.class);
GenericUiServlet.setContentType("https://localhost/styles.css", response);
verify(response, times(1)).setContentType("text/css");
GenericUiServlet.setContentType("/img.svg", response);
verify(response, times(1)).setContentType("image/svg+xml");

response = mock(HttpServletResponse.class);
GenericUiServlet.setContentType("unknown_file.xml", response);
verify(response, times(0)).setContentType(any());
GenericUiServlet.setContentType("/img.gif", response);
verify(response, times(1)).setContentType("image/gif");

HttpServletResponse servletResponse = mock(HttpServletResponse.class);
IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> GenericUiServlet.setContentType("unknown_file.xml", servletResponse));
assertEquals("Unsupported file type", exception.getMessage());
}

@Test
@SneakyThrows
void testService() {
// unreal case (at least we assume that uri will start with /polarion/)
assertThrows(StringIndexOutOfBoundsException.class, () -> callServlet("/badUrl"));
// error case (at least we assume that uri will start with /polarion/)
IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> callServlet("/badUrl/someImg.png"));
assertEquals("Unsupported resource path", exception.getMessage());

// does nothing coz we do not process sub-paths other than /ui/generic/ and /ui/
TestServlet servlet = callServlet("/polarion/testServletName/unknownPath");
verify(servlet, times(0)).serveGenericResource(any(), any());
verify(servlet, times(0)).serveResource(any(), any());
// we expect uri starting by /polarion/{web_app_name}/ui/
exception = assertThrows(IllegalArgumentException.class, () -> callServlet("/polarion/testServletName/unknownPath"));
assertEquals("Unsupported resource path", exception.getMessage());

// generic resource
servlet = callServlet("/polarion/testServletName/ui/generic/genericUri");
verify(servlet, times(1)).serveGenericResource(any(), eq("genericUri"));
TestServlet servlet = callServlet("/polarion/testServletName/ui/generic/genericUri/someImage.gif");
verify(servlet, times(1)).serveGenericResource(any(), eq("genericUri/someImage.gif"));
verify(servlet, times(0)).serveResource(any(), any());

// regular resource
servlet = callServlet("/polarion/testServletName/ui/regularUri");
servlet = callServlet("/polarion/testServletName/ui/regularUri/someStyle.css");
verify(servlet, times(0)).serveGenericResource(any(), any());
verify(servlet, times(1)).serveResource(any(), any());
}
Expand Down

0 comments on commit a695f89

Please sign in to comment.