Skip to content

Commit

Permalink
Sanitize ADR inputs for HTML elements (finos#761)
Browse files Browse the repository at this point in the history
  • Loading branch information
grahampacker-ms committed Feb 27, 2025
1 parent f689014 commit 1d08997
Show file tree
Hide file tree
Showing 5 changed files with 153 additions and 6 deletions.
5 changes: 5 additions & 0 deletions calm-hub/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@
<groupId>com.fasterxml.jackson.datatype</groupId>
<artifactId>jackson-datatype-jsr310</artifactId>
</dependency>
<dependency>
<groupId>com.googlecode.owasp-java-html-sanitizer</groupId>
<artifactId>owasp-java-html-sanitizer</artifactId>
<version>20240325.1</version>
</dependency>

<!-- Testing -->
<dependency>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,10 @@ public class MongoAdrIntegration {

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

private final String TITLE = "My ADR";
private final String PROBLEM_STATEMENT = "My problem is...";
private final String TITLE = "<b>My ADR</b>";
private final String EXPECTED_TITLE = "My ADR";
private final String PROBLEM_STATEMENT = "<a>My problem is...</a>";
private final String EXPECTED_PROBLEM_STATEMENT = "My problem is...";
private final List<String> DECISION_DRIVERS = List.of("a", "b", "c");
private final Option OPTION_A = new Option("Option A", "optionDescription", List.of("a"), List.of("b"));
private final Option OPTION_B = new Option("Option B", "optionDescription", List.of("c"), List.of("d"));
Expand All @@ -56,7 +58,7 @@ public class MongoAdrIntegration {
private final NewAdrRequest newAdr = new NewAdrRequest(TITLE, PROBLEM_STATEMENT, DECISION_DRIVERS,
CONSIDERED_OPTIONS, DECISION_OUTCOME, LINKS);

private final Adr adr = new Adr.AdrBuilder(newAdr).setStatus(Status.draft).build();
private final Adr adr = new Adr.AdrBuilder(newAdr).setTitle(EXPECTED_TITLE).setContextAndProblemStatement(EXPECTED_PROBLEM_STATEMENT).setStatus(Status.draft).build();

@BeforeEach
public void setupAdrs() {
Expand Down
13 changes: 10 additions & 3 deletions calm-hub/src/main/java/org/finos/calm/resources/AdrResource.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.finos.calm.domain.exception.AdrPersistenceException;
import org.finos.calm.domain.exception.AdrRevisionNotFoundException;
import org.finos.calm.domain.exception.NamespaceNotFoundException;
import org.finos.calm.sanitizer.AdrSanitizer;
import org.finos.calm.store.AdrStore;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -40,10 +41,12 @@
public class AdrResource {

private final AdrStore store;
private final AdrSanitizer adrSanitizer;
private final Logger logger = LoggerFactory.getLogger(AdrResource.class);

public AdrResource(AdrStore store) {
public AdrResource(AdrStore store, AdrSanitizer adrSanitizer) {
this.store = store;
this.adrSanitizer = adrSanitizer;
}

/**
Expand Down Expand Up @@ -83,7 +86,9 @@ public Response getAdrsForNamespace(@PathParam("namespace") String namespace) {
description = "Creates an ADR for a given namespace with an allocated ID and revision 1"
)
public Response createAdrForNamespace(@PathParam("namespace") String namespace, NewAdrRequest newAdrRequest) {
Adr adr = new Adr.AdrBuilder(newAdrRequest)
NewAdrRequest safeNewAdrRequest = adrSanitizer.sanitizeNewAdrRequest(newAdrRequest);

Adr adr = new Adr.AdrBuilder(safeNewAdrRequest)
.setStatus(Status.draft)
.setCreationDateTime(LocalDateTime.now())
.setUpdateDateTime(LocalDateTime.now())
Expand Down Expand Up @@ -119,7 +124,9 @@ public Response createAdrForNamespace(@PathParam("namespace") String namespace,
description = "Updates an ADR for a given namespace. Creates a new revision."
)
public Response updateAdrForNamespace(@PathParam("namespace") String namespace, @PathParam("adrId") int adrId, NewAdrRequest newAdrRequest) {
Adr adr = new Adr.AdrBuilder(newAdrRequest)
NewAdrRequest safeNewAdrRequest = adrSanitizer.sanitizeNewAdrRequest(newAdrRequest);

Adr adr = new Adr.AdrBuilder(safeNewAdrRequest)
.setUpdateDateTime(LocalDateTime.now())
.build();

Expand Down
50 changes: 50 additions & 0 deletions calm-hub/src/main/java/org/finos/calm/sanitizer/AdrSanitizer.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package org.finos.calm.sanitizer;

import jakarta.enterprise.context.ApplicationScoped;
import org.finos.calm.domain.adr.Decision;
import org.finos.calm.domain.adr.Link;
import org.finos.calm.domain.adr.NewAdrRequest;
import org.finos.calm.domain.adr.Option;
import org.owasp.html.HtmlPolicyBuilder;
import org.owasp.html.PolicyFactory;

@ApplicationScoped
public class AdrSanitizer {

//strict policy that strips all HTML elements
private final PolicyFactory strictPolicy = new HtmlPolicyBuilder().toFactory();

public NewAdrRequest sanitizeNewAdrRequest(NewAdrRequest newAdrRequest) {
return new NewAdrRequest(
newAdrRequest.title() == null ? null : strictPolicy.sanitize(newAdrRequest.title()),
newAdrRequest.contextAndProblemStatement() == null ? null : strictPolicy.sanitize(newAdrRequest.contextAndProblemStatement()),
newAdrRequest.decisionDrivers() == null ? null : newAdrRequest.decisionDrivers().stream().map(strictPolicy::sanitize).toList(),
newAdrRequest.consideredOptions() == null ? null : newAdrRequest.consideredOptions().stream().map(this::sanitizeOption).toList(),
newAdrRequest.decisionOutcome() == null ? null : sanitizeDecision(newAdrRequest.decisionOutcome()),
newAdrRequest.links() == null ? null : newAdrRequest.links().stream().map(this::sanitizeLink).toList()
);
}

private Option sanitizeOption(Option option) {
return new Option(
option.name() == null ? null : strictPolicy.sanitize(option.name()),
option.description() == null ? null : strictPolicy.sanitize(option.description()),
option.positiveConsequences() == null ? null : option.positiveConsequences().stream().map(strictPolicy::sanitize).toList(),
option.negativeConsequences() == null ? null : option.negativeConsequences().stream().map(strictPolicy::sanitize).toList()
);
}

private Decision sanitizeDecision(Decision decision) {
return new Decision(
decision.chosenOption() == null ? null : sanitizeOption(decision.chosenOption()),
decision.rationale() == null ? null : strictPolicy.sanitize(decision.rationale())
);
}

private Link sanitizeLink(Link link) {
return new Link(
link.rel() == null ? null : strictPolicy.sanitize(link.rel()),
link.href() == null ? null : strictPolicy.sanitize(link.href())
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package org.finos.calm.sanitizer;

import io.quarkus.test.InjectMock;
import io.quarkus.test.junit.QuarkusTest;
import jakarta.inject.Inject;
import org.finos.calm.domain.adr.Decision;
import org.finos.calm.domain.adr.Link;
import org.finos.calm.domain.adr.NewAdrRequest;
import org.finos.calm.domain.adr.Option;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.junit.jupiter.MockitoExtension;

import java.util.List;

import static org.junit.jupiter.api.Assertions.assertEquals;

@QuarkusTest
class TestAdrSanitizerShould {

@Inject
AdrSanitizer adrSanitizer;

@Test
public void remove_html_tags_from_adr() {
NewAdrRequest unsafeNewAdrRequest = new NewAdrRequest(
"<a href=\"http://my-url.com\"><b><i>My ADR</i></b></a>",
"<img> <body>problem statement",
List.of("<p><h1>decision driver"),
List.of(new Option(
"<p><h1>name",
"<p><h1>description",
List.of("<p><h1> blah"),
List.of("<p><h1> blah")
)),
new Decision(new Option(
"<p><h1>name",
"<p><h1>description",
List.of("<p><h1> blah"),
List.of("<p><h1> blah")),
"blah <h1>"
),
List.of(new Link("<p><h1> blah", "<p><h1> blah"))
);

NewAdrRequest expectedSafeNewAdrRequest = new NewAdrRequest(
"My ADR",
" problem statement",
List.of("decision driver"),
List.of(new Option(
"name",
"description",
List.of(" blah"),
List.of(" blah")
)),
new Decision(new Option(
"name",
"description",
List.of(" blah"),
List.of(" blah")),
"blah "
),
List.of(new Link(" blah", " blah"))
);

assertEquals(expectedSafeNewAdrRequest, adrSanitizer.sanitizeNewAdrRequest(unsafeNewAdrRequest));
}

@Test
public void handle_null_values_in_adr() {
NewAdrRequest newAdrRequest = new NewAdrRequest(
null,
null,
null,
null,
null,
null
);

assertEquals(newAdrRequest, adrSanitizer.sanitizeNewAdrRequest(newAdrRequest));
}

}

0 comments on commit 1d08997

Please sign in to comment.