Skip to content

Commit

Permalink
Add ProblemDetails for invalid policy conditions
Browse files Browse the repository at this point in the history
The frontend can properly interpret `ProblemDetails` and present a more helpful error message to users than the current generic "request failed" one.

Signed-off-by: nscuro <[email protected]>
  • Loading branch information
nscuro committed Jul 6, 2024
1 parent cda1b86 commit 6246966
Show file tree
Hide file tree
Showing 7 changed files with 122 additions and 51 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
import org.dependencytrack.parser.cyclonedx.InvalidBomException;
import org.dependencytrack.persistence.QueryManager;
import org.dependencytrack.resources.v1.problems.InvalidBomProblemDetails;
import org.dependencytrack.resources.v1.problems.ProblemDetails;
import org.dependencytrack.resources.v1.vo.BomSubmitRequest;
import org.dependencytrack.resources.v1.vo.BomUploadResponse;
import org.dependencytrack.resources.v1.vo.IsTokenBeingProcessedResponse;
Expand Down Expand Up @@ -522,13 +521,7 @@ static void validate(final byte[] bomBytes) {
if (!e.getValidationErrors().isEmpty()) {
problemDetails.setErrors(e.getValidationErrors());
}

final Response response = Response.status(Response.Status.BAD_REQUEST)
.header("Content-Type", ProblemDetails.MEDIA_TYPE_JSON)
.entity(problemDetails)
.build();

throw new WebApplicationException(response);
throw new WebApplicationException(problemDetails.toResponse());
} catch (RuntimeException e) {
LOGGER.error("Failed to validate BOM", e);
final Response response = Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
import org.dependencytrack.policy.cel.CelPolicyScriptHost;
import org.dependencytrack.policy.cel.CelPolicyScriptHost.CacheMode;
import org.dependencytrack.policy.cel.CelPolicyType;
import org.dependencytrack.resources.v1.vo.CelExpressionError;
import org.projectnessie.cel.common.CELError;
import org.dependencytrack.resources.v1.problems.InvalidCelExpressionProblemDetails;
import org.dependencytrack.resources.v1.problems.ProblemDetails;
import org.projectnessie.cel.tools.ScriptCreateException;

import javax.validation.Validator;
Expand All @@ -50,8 +50,6 @@
import javax.ws.rs.Produces;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
import java.util.ArrayList;
import java.util.Map;

/**
* JAX-RS resources for processing policies.
Expand Down Expand Up @@ -165,18 +163,22 @@ private void maybeValidateExpression(final PolicyCondition policyCondition) {
}

if (policyCondition.getViolationType() == null) {
throw new BadRequestException(Response.status(Response.Status.BAD_REQUEST).entity("Expression conditions must define a violation type").build());
final var problemDetails = new ProblemDetails();
problemDetails.setStatus(400);
problemDetails.setTitle("Invalid policy condition");
problemDetails.setDetail("Expression conditions must define a violation type");
throw new BadRequestException(problemDetails.toResponse());
}

try {
CelPolicyScriptHost.getInstance(CelPolicyType.COMPONENT).compile(policyCondition.getValue(), CacheMode.NO_CACHE);
} catch (ScriptCreateException e) {
final var celErrors = new ArrayList<CelExpressionError>();
for (final CELError error : e.getIssues().getErrors()) {
celErrors.add(new CelExpressionError(error.getLocation().line(), error.getLocation().column(), error.getMessage()));
}

throw new BadRequestException(Response.status(Response.Status.BAD_REQUEST).entity(Map.of("celErrors", celErrors)).build());
final var problemDetails = new InvalidCelExpressionProblemDetails(e.getIssues());
problemDetails.setStatus(400);
problemDetails.setTitle("Invalid policy expression");
problemDetails.setDetail("The provided policy expression contains %d error(s)"
.formatted(problemDetails.getErrors().size()));
throw new BadRequestException(problemDetails.toResponse());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,7 @@ public Response toResponse(final JsonMappingException exception) {
problemDetails.setStatus(400);
problemDetails.setTitle("The provided JSON payload could not be mapped");
problemDetails.setDetail(createDetail(exception));

return Response
.status(Response.Status.BAD_REQUEST)
.type(ProblemDetails.MEDIA_TYPE_JSON)
.entity(problemDetails)
.build();
return problemDetails.toResponse();
}

private static String createDetail(final JsonMappingException exception) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* This file is part of Dependency-Track.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
* Copyright (c) OWASP Foundation. All Rights Reserved.
*/
package org.dependencytrack.resources.v1.problems;

import io.swagger.annotations.ApiModelProperty;
import io.swagger.annotations.ApiParam;
import org.projectnessie.cel.Issues;

import java.util.List;

/**
* @since 5.5.0
*/
public class InvalidCelExpressionProblemDetails extends ProblemDetails {

public record CelExpressionError(
@ApiParam(value = "The line in which the error was identified") Integer line,
@ApiParam(value = "The column in which the error was identified") Integer column,
@ApiParam(value = "The message describing the error") String message
) {
}

@ApiModelProperty(value = "Errors identified during expression compilation")
private final List<CelExpressionError> errors;

public InvalidCelExpressionProblemDetails(final Issues issues) {
this.errors = issues.getErrors().stream()
.map(error -> new CelExpressionError(
error.getLocation().line(),
error.getLocation().column(),
error.getMessage()
))
.toList();
}

public List<CelExpressionError> getErrors() {
return errors;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import io.swagger.annotations.ApiModel;
import io.swagger.annotations.ApiModelProperty;

import javax.ws.rs.core.HttpHeaders;
import javax.ws.rs.core.Response;
import java.net.URI;

/**
Expand All @@ -30,7 +32,10 @@
*/
@ApiModel(
description = "An RFC 9457 problem object",
subTypes = InvalidBomProblemDetails.class
subTypes = {
InvalidBomProblemDetails.class,
InvalidCelExpressionProblemDetails.class
}
)
@JsonInclude(JsonInclude.Include.NON_NULL)
public class ProblemDetails {
Expand Down Expand Up @@ -70,6 +75,14 @@ public class ProblemDetails {
)
private URI instance;

public Response toResponse() {
return Response
.status(status)
.header(HttpHeaders.CONTENT_TYPE, MEDIA_TYPE_JSON)
.entity(this)
.build();
}

public URI getType() {
return type;
}
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,32 @@ public void testCreateExpressionCondition() {
""");
}

@Test
public void testCreateExpressionConditionWithoutViolationType() {
final Policy policy = qm.createPolicy("policy", Policy.Operator.ANY, Policy.ViolationState.FAIL);

final Response response = jersey.target("%s/%s/condition".formatted(V1_POLICY, policy.getUuid()))
.request()
.header(X_API_KEY, apiKey)
.put(Entity.entity("""
{
"subject": "EXPRESSION",
"value": "true"
}
""", MediaType.APPLICATION_JSON));

assertThat(response.getStatus()).isEqualTo(400);
assertThat(response.getHeaderString("Content-Type")).isEqualTo("application/problem+json");
assertThatJson(getPlainTextBody(response))
.isEqualTo("""
{
"status": 400,
"title": "Invalid policy condition",
"detail": "Expression conditions must define a violation type"
}
""");
}

@Test
public void testCreateExpressionConditionWithError() {
final Policy policy = qm.createPolicy("policy", Policy.Operator.ANY, Policy.ViolationState.FAIL);
Expand All @@ -87,10 +113,14 @@ public void testCreateExpressionConditionWithError() {
""", MediaType.APPLICATION_JSON));

assertThat(response.getStatus()).isEqualTo(400);
assertThat(response.getHeaderString("Content-Type")).isEqualTo("application/problem+json");
assertThatJson(getPlainTextBody(response))
.isEqualTo("""
{
"celErrors": [
"status": 400,
"title": "Invalid policy expression",
"detail": "The provided policy expression could not be compiled",
"errors": [
{
"line": 1,
"column": 9,
Expand Down Expand Up @@ -151,12 +181,16 @@ public void testUpdateExpressionConditionWithError() {
""".formatted(condition.getUuid()), MediaType.APPLICATION_JSON));

assertThat(response.getStatus()).isEqualTo(400);
assertThat(response.getHeaderString("Content-Type")).isEqualTo("application/problem+json");
assertThatJson(getPlainTextBody(response))
.isEqualTo("""
{
"celErrors": [
"status": 400,
"title": "Invalid policy expression",
"detail": "The provided policy expression could not be compiled",
"errors": [
{
"line": 1,
"line":1,
"column": 9,
"message": "undefined field 'doesNotExist'"
}
Expand Down

0 comments on commit 6246966

Please sign in to comment.