Question about UriCompliance.checkUriCompliance

[leonchen83]

jetty.project/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java

Line 365 in 2584eb0

listener.onComplianceViolation(new ComplianceViolation.Event(compliance, violation, uri.toString()));

should above code to be following?

    public static String checkUriCompliance(UriCompliance compliance, HttpURI uri, ComplianceViolation.Listener listener)
    {
        for (UriCompliance.Violation violation : UriCompliance.Violation.values())
        {
            if (uri.hasViolation(violation) && (compliance == null || !compliance.allows(violation))) {
                if (listener != null)
                    listener.onComplianceViolation(new ComplianceViolation.Event(compliance, violation, uri.toString()));
                return violation.getDescription();
            }
        }
        return null;
    }

[joakime]

I do not understand your question.
Can you elaborate?

[gregw]

@joakime The problem is that the current code will return a violation description OR report it to a listener. The proposed code will report a violation to the listener and return the description.

I also this this method should be gated with hasViolations() and should report multiple violations.

[gregw]

@joakime I think something like the following would be a better implementation:

    public static String checkUriCompliance(UriCompliance compliance, HttpURI uri, ComplianceViolation.Listener listener)
    {
        if (uri.hasViolations())
        {
            StringBuilder violations = null;
            for (UriCompliance.Violation violation : UriCompliance.Violation.values())
            {
                if (uri.hasViolation(violation) && (compliance == null || !compliance.allows(violation)))
                {
                    if (listener != null)
                        listener.onComplianceViolation(new ComplianceViolation.Event(compliance, violation, uri.toString()));
                    
                    if (violations == null)
                        violations = new StringBuilder();
                    else 
                        violations.append(", ");
                    violations.append(violation.getDescription());
                }
            }
            if (violations != null)
                return violations.toString();
        }
        return null;
    }

[gregw]

Oh! are we only reporting violations to listeners if they are allows by a compliance mode? Maybe we should always report them to a listener, but only throw if they are not allowed?

Either way, this method needs to be protected with hasViolations so it isn't creating an iterator for every URI that doesn't have a violation.

[gregw]

@joakime would the following be better:

    public static String checkUriCompliance(UriCompliance compliance, HttpURI uri, ComplianceViolation.Listener listener)
    {
        if (uri.hasViolations())
        {
            StringBuilder violations = null;
            for (UriCompliance.Violation violation : UriCompliance.Violation.values())
            {
                if (uri.hasViolation(violation))
                {
                    // always report any violation to the listeners
                    if (listener != null)
                        listener.onComplianceViolation(new ComplianceViolation.Event(compliance, violation, uri.toString()));
                    
                    // Only produce a violation message to throw if the violations is not allowed
                    if (compliance == null || !compliance.allows(violation))
                    {
                        if (violations == null)
                            violations = new StringBuilder();
                        else
                            violations.append(", ");
                        violations.append(violation.getDescription());
                    }
                }
            }
            if (violations != null)
                return violations.toString();
        }
        return null;
    }

[uschindler]

I was wondering about this code, too. Yes the listener should always be called.

At the moment I am also thinking of a better way to have "per context" UriCompliance settings. I was about to use the new feature, but stopped using it due to this. At moment I set the global Uri compliance to allow most violations and adding a handler per context to have restructive settings. This looks a bit reversed and has problems when somebody uses encoded ".." to trick the contexts (it is not an issue for me, because its different virtual hosts). I'd more like to relax UriCompliance only for specific contexts. Should I open a different issue, to maybe implement the whole URICompliance stuff as a handler (which is added by default)?

P.S.: I am using this code:

/** should be inserted into the {@code ContextHandler} of webapp or context */
final class UriComplianceHandler extends Handler.Wrapper {
  
  public static final UriCompliance URI_COMPLIANCE_STRICT = UriCompliance.RFC3986;
  public static final UriCompliance URI_COMPLIANCE_RELAXED = new UriCompliance("RELAXED",
      EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_ENCODING, Violation.AMBIGUOUS_EMPTY_SEGMENT));
  
  private final UriCompliance compliance;
  
  UriComplianceHandler(UriCompliance compliance) {
    this.compliance = compliance;
  }

  @Override
  public boolean handle(Request request, Response response, Callback callback) throws Exception {
    final var uri = request.getHttpURI();
    if (uri.hasViolations()) {
      String badMessage = UriCompliance.checkUriCompliance(compliance, uri, HttpChannel.from(request).getComplianceViolationListener());
      if (badMessage != null) {
        Response.writeError(request, response, callback, HttpStatus.BAD_REQUEST_400, badMessage);
        return true;
      }
    }
    return super.handle(request, response, callback);
  }
}

In general I tend to think the whole URI compliance should be implemented like this as a handler in the server root.

[joakime]

PR #11444 opened