Fixes #10482 - RewriteHandler with multiple HeaderPatternRules. (#10572)

Reviewed the implementation of RewriteHandler, that was broken for those rules that were overriding `Rule.Handler.handle()`.

The problem was that the handling was not forwarded along the chain of rules, so only the last one was applied.

Now the wrapping at the constructor produces RH3(RH2(RH1(Request))), but the handling is performed from the innermost towards the outermost.
In this way, the order of rules is respected, both in the wrapping at rule application, and in the `Rule.Handler` handling.

Signed-off-by: Simone Bordet <[email protected]>

jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/RewriteCustomizer.java

@@ -17,6 +17,7 @@
 
 import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.io.RuntimeIOException;
+import org.eclipse.jetty.rewrite.handler.Rule;
 import org.eclipse.jetty.rewrite.handler.RuleContainer;
 import org.eclipse.jetty.server.HttpConfiguration.Customizer;
 import org.eclipse.jetty.server.Request;
@@ -35,12 +36,20 @@ public Request customize(Request request, HttpFields.Mutable responseHeaders)
         try
         {
             // TODO: rule are able to complete the request/response, but customizers cannot.
-            Handler input = new Handler(request);
+            Handler input = new Input(request);
             return matchAndApply(input);
         }
         catch (IOException e)
         {
             throw new RuntimeIOException(e);
         }
     }
+
+    private static class Input extends Rule.Handler
+    {
+        private Input(Request request)
+        {
+            super(request);
+        }
+    }
 }

jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/CookiePatternRule.java

@@ -82,21 +82,18 @@ public void setValue(String value)
     @Override
     public Handler apply(Handler input) throws IOException
     {
-        // Check that cookie is not already set
+        // Check that the cookie is not already set.
         List<HttpCookie> cookies = Request.getCookies(input);
-        if (cookies != null)
+        for (HttpCookie cookie : cookies)
         {
-            for (HttpCookie cookie : cookies)
-            {
-                if (_name.equals(cookie.getName()) && _value.equals(cookie.getValue()))
-                    return null;
-            }
+            if (_name.equals(cookie.getName()) && _value.equals(cookie.getValue()))
+                return null;
         }
 
         return new Handler(input)
         {
             @Override
-            public boolean handle(Response response, Callback callback) throws Exception
+            protected boolean handle(Response response, Callback callback) throws Exception
             {
                 Response.addCookie(response, HttpCookie.from(_name, _value));
                 return super.handle(response, callback);

jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/HeaderPatternRule.java

@@ -65,8 +65,9 @@ public boolean isAdd()
     }
 
     /**
-     * Set true to add the response header, false to put the response header..
-     * @param add true to add the response header, false to put the response header.
+     * Use {@code true} to add the response header, {@code false} to put the response header.
+     *
+     * @param add {@code true} to add the response header, {@code false} to put the response header.
      */
     public void setAdd(boolean add)
     {
@@ -79,7 +80,7 @@ public Handler apply(Handler input) throws IOException
         return new Handler(input)
         {
             @Override
-            public boolean handle(Response response, Callback callback) throws Exception
+            protected boolean handle(Response response, Callback callback) throws Exception
             {
                 if (isAdd())
                     response.getHeaders().add(getHeaderName(), getHeaderValue());

jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/HeaderRegexRule.java

@@ -80,7 +80,7 @@ protected Handler apply(Handler input, Matcher matcher) throws IOException
         return new Handler(input)
         {
             @Override
-            public boolean handle(Response response, Callback callback) throws Exception
+            protected boolean handle(Response response, Callback callback) throws Exception
             {
                 if (isAdd())
                     response.getHeaders().add(getHeaderName(), matcher.replaceAll(getHeaderValue()));

jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/InvalidURIRule.java

@@ -97,7 +97,7 @@ private Handler apply(Handler input)
         return new Handler(input)
         {
             @Override
-            public boolean handle(Response response, Callback callback)
+            protected boolean handle(Response response, Callback callback)
             {
                 String message = getMessage();
                 if (StringUtil.isBlank(message))

jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectPatternRule.java

@@ -84,7 +84,7 @@ public Handler apply(Handler input) throws IOException
         return new Handler(input)
         {
             @Override
-            public boolean handle(Response response, Callback callback)
+            protected boolean handle(Response response, Callback callback)
             {
                 String location = getLocation();
                 response.setStatus(getStatusCode());

jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectRegexRule.java

@@ -82,7 +82,7 @@ protected Handler apply(Handler input, Matcher matcher) throws IOException
         return new Handler(input)
         {
             @Override
-            public boolean handle(Response response, Callback callback)
+            protected boolean handle(Response response, Callback callback)
             {
                 String target = matcher.replaceAll(getLocation());
                 response.setStatus(_statusCode);

jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRule.java

@@ -84,7 +84,7 @@ public Handler apply(Handler input) throws IOException
         return new Handler(input)
         {
             @Override
-            public boolean handle(Response response, Callback callback)
+            protected boolean handle(Response response, Callback callback)
             {
                 String message = getMessage();
                 if (StringUtil.isBlank(message))

jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RewriteHandler.java

@@ -100,6 +100,14 @@ public void addRule(Rule rule)
         _rules.addRule(rule);
     }
 
+    /**
+     * <p>Removes all the rules.</p>
+     */
+    public void clear()
+    {
+        _rules.clear();
+    }
+
     /**
      * @see RuleContainer#getOriginalPathAttribute()
      */
@@ -123,14 +131,32 @@ public boolean handle(Request request, Response response, Callback callback) thr
             return false;
 
         Rule.Handler input = new Rule.Handler(request);
-        Rule.Handler output = _rules.matchAndApply(input);
+        Rule.Handler result = getRuleContainer().matchAndApply(input);
 
         // No rule matched, call super with the original request.
-        if (output == null)
+        if (result == null)
             return super.handle(request, response, callback);
 
-        // At least one rule matched, call super with the result of the rule applications.
-        output.setHandler(getHandler());
-        return output.handle(output, response, callback);
+        // At least one rule matched, link the last Rule.Handler
+        // to invoke the child Handler of this RewriteHandler.
+        new LastRuleHandler(result, getHandler());
+        return input.handle(response, callback);
+    }
+
+    private static class LastRuleHandler extends Rule.Handler
+    {
+        private final Handler _handler;
+
+        private LastRuleHandler(Rule.Handler ruleHandler, Handler handler)
+        {
+            super(ruleHandler);
+            _handler = handler;
+        }
+
+        @Override
+        protected boolean handle(Response response, Callback callback) throws Exception
+        {
+            return _handler.handle(getWrapped(), response, callback);
+        }
     }
 }

jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/Rule.java

@@ -29,94 +29,96 @@ public abstract class Rule
     private boolean _terminating;
 
     /**
-     * <p>Tests whether the given {@code Request} should apply, and if so the rule logic is triggered.</p>
+     * <p>Tests whether the given input {@code Handler} (which wraps a
+     * {@code Request}) matches the rule, and if so returns an output
+     * {@code Handler} that applies the rule logic.</p>
+     * <p>If the input does not match, {@code null} is returned.</p>
      *
-     * @param input the input {@code Request} and {@code Handler}
-     * @return the possibly wrapped {@code Request} and {@code Handler}, or {@code null} if the rule did not match
-     * @throws IOException if applying the rule failed
+     * @param input the input {@code Handler} that wraps the {@code Request}
+     * @return an output {@code Handler} that wraps the input {@code Handler},
+     * or {@code null} if the rule does not match
+     * @throws IOException if applying the rule fails
      */
     public abstract Handler matchAndApply(Handler input) throws IOException;
 
     /**
-     * @return when {@code true}, rules after this one are not invoked
+     * @return whether rules after this one are not invoked
      */
     public boolean isTerminating()
     {
         return _terminating;
     }
 
+    /**
+     * @param value whether rules after this one are not invoked
+     */
     public void setTerminating(boolean value)
     {
         _terminating = value;
     }
 
-    /**
-     * Returns the handling and terminating flag values.
-     */
     @Override
     public String toString()
     {
         return "%s@%x[terminating=%b]".formatted(getClass().getSimpleName(), hashCode(), isTerminating());
     }
 
     /**
-     * <p>A {@link Request.Wrapper} that is also a {@link Handler},
-     * used to chain a sequence of {@link Rule}s together.
-     * The rule handler is initialized with the initial request, then it is
-     * passed to a chain of rules before the child {@code Handler} is
-     * passed in {@link #setHandler(Handler)}. Finally, the response
-     * and callback are provided in a call to {@link #handle(Request, Response, Callback)},
-     * which calls the {@link #handle(Response, Callback)}.</p>
+     * <p>A {@link Request.Wrapper} used to chain a sequence of {@link Rule}s together.</p>
+     * <p>The first {@link Rule.Handler} is initialized with the initial {@link Request},
+     * then it is passed to a chain of {@link Rule}s, which in turn chain {@link Rule.Handler}s
+     * together.
+     * At the end of the {@link Rule} applications, {@link Rule.Handler}s are chained so that
+     * so that the first rule produces the innermost {@code Handler} and the last rule produces
+     * the outermost {@code Handler} in this way: {@code RH3(RH2(RH1(Req)))}.</p>
+     * <p>After the {@link Rule} applications, the {@link Rule.Handler}s are then called in
+     * sequence, starting from the innermost and moving outwards with respect to the wrapping,
+     * until finally the {@link org.eclipse.jetty.server.Handler#handle(Request, Response, Callback)}
+     * method of the child {@code Handler} of {@link RewriteHandler} is invoked.</p>
      */
-    public static class Handler extends Request.Wrapper implements Request.Handler
+    public static class Handler extends Request.Wrapper
     {
-        private volatile Handler _handler;
+        private Rule.Handler _nextRuleHandler;
 
-        public Handler(Request request)
+        protected Handler(Request request)
         {
             super(request);
         }
 
-        @Override
-        public boolean handle(Request request, Response response, Callback callback) throws Exception
+        public Handler(Rule.Handler handler)
         {
-            return handle(response, callback);
+            super(handler);
+            handler._nextRuleHandler = this;
         }
 
         /**
-         * <p>Handles this wrapped request together with the passed response and
-         * callback, using the handler set in {@link #setHandler(Handler)}.
-         * This method should be extended if additional handling of the wrapped
-         * request is required.</p>
-         * @param response The response
-         * @param callback The callback
-         * @throws Exception If there is a problem handling
-         * @see #setHandler(Handler)
-         */
-        protected boolean handle(Response response, Callback callback) throws Exception
-        {
-            Handler handler = _handler;
-            return handler != null && handler.handle(this, response, callback);
-        }
-
-        /**
-         * <p>Wraps the given {@code Handler} within this instance and returns this instance.</p>
+         * <p>Handles this wrapped request together with the passed response and callback.</p>
+         * <p>This method should be overridden only if the rule applies to the response,
+         * or the rule completes the callback.
+         * By default this method forwards the handling to the next {@link Rule.Handler}.
+         * If a rule that overrides this method is non-{@link #isTerminating() terminating},
+         * it should call the {@code super} implementation to chain the rules.</p>
          *
-         * @param handler the {@code Handler} to wrap
+         * @param response the {@link Response}
+         * @param callback the {@link Callback}
+         * @throws Exception if there is a failure while handling the rules
          */
-        public void setHandler(Handler handler)
+        protected boolean handle(Response response, Callback callback) throws Exception
         {
-            _handler = handler;
+            return _nextRuleHandler.handle(response, callback);
         }
     }
 
+    /**
+     * <p>A {@link Rule.Handler} that wraps a {@link Request} to return a different {@link HttpURI}.</p>
+     */
     public static class HttpURIHandler extends Handler
     {
         private final HttpURI _uri;
 
-        public HttpURIHandler(Request request, HttpURI uri)
+        public HttpURIHandler(Rule.Handler handler, HttpURI uri)
         {
-            super(request);
+            super(handler);
             _uri = uri;
         }
 

jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RuleContainer.java

@@ -51,7 +51,7 @@ public List<Rule> getRules()
      */
     public void setRules(List<Rule> rules)
     {
-        _rules.clear();
+        clear();
         _rules.addAll(rules);
     }
 
@@ -71,6 +71,14 @@ public void addRule(Rule rule)
         _rules.add(rule);
     }
 
+    /**
+     * <p>Removes all the rules.</p>
+     */
+    public void clear()
+    {
+        _rules.clear();
+    }
+
     /**
      * @return the request attribute name used to store the request original path
      * @see #setOriginalPathAttribute(String)

jetty-core/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/HeaderPatternRuleTest.java

@@ -80,6 +80,7 @@ public void testHeaderWithNumberValues() throws Exception
             assertEquals(200, response.getStatus());
             assertEquals(value, response.get(name));
 
+            _rewriteHandler.clear();
             stop();
         }
     }

jetty-core/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/HeaderRegexRuleTest.java

@@ -81,6 +81,7 @@ public void testHeaderWithNumberValues() throws Exception
             assertEquals(200, response.getStatus());
             assertEquals(value, response.get(name));
 
+            _rewriteHandler.clear();
             stop();
         }
     }