On Fri, Mar 8, 2024 at 8:10 PM Christopher Schultz
<ch...@christopherschultz.net> wrote:
>
> Rémy,
>
> On 3/8/24 09:46, r...@apache.org wrote:
> > This is an automated email from the ASF dual-hosted git repository.
> >
> > remm pushed a commit to branch main
> > in repository https://gitbox.apache.org/repos/asf/tomcat.git
> >
> >
> > The following commit(s) were added to refs/heads/main by this push:
> > new 7a3bbc6e30 Add new valveSkip flag, also fix no rewrite rule match
> > 7a3bbc6e30 is described below
> >
> > commit 7a3bbc6e300ced35268fe1c46c90f6b5c752dc5c
> > Author: remm <r...@apache.org>
> > AuthorDate: Fri Mar 8 15:46:04 2024 +0100
> >
> > Add new valveSkip flag, also fix no rewrite rule match
> >
> > This allows skipping the next valve in the Catalina pipeline, for
> > conditional execution. This was inspired by BZ 60997, which the rewrite
> > valve can do (set the rewrite valve, then set the semaphore valve next;
> > if match then the semaphore is skipped).
> > Also notice major issue where rewrite is considered to have occurred
> > when the output is identical but equals return false due to type
> > differences.
> > ---
> > .../apache/catalina/valves/rewrite/RewriteRule.java | 14 ++++++++++++++
> > .../apache/catalina/valves/rewrite/RewriteValve.java | 20
> > ++++++++++++++++++--
> > .../catalina/valves/rewrite/TestRewriteValve.java | 18
> > ++++++++++++++++++
> > webapps/docs/changelog.xml | 9 +++++++++
> > webapps/docs/rewrite.xml | 11 +++++++++++
> > 5 files changed, 70 insertions(+), 2 deletions(-)
> >
> > diff --git a/java/org/apache/catalina/valves/rewrite/RewriteRule.java
> > b/java/org/apache/catalina/valves/rewrite/RewriteRule.java
> > index e4466d525b..269eedab7f 100644
> > --- a/java/org/apache/catalina/valves/rewrite/RewriteRule.java
> > +++ b/java/org/apache/catalina/valves/rewrite/RewriteRule.java
> > @@ -335,6 +335,11 @@ public class RewriteRule {
> > protected boolean type = false;
> > protected String typeValue = null;
> >
> > + /**
> > + * Allows skipping the next valve in the Catalina pipeline.
> > + */
> > + protected boolean valveSkip = false;
> > +
>
> Maybe "skipNextValve"? "valveSkip" isn't super descriptive.
>
> Same with accessor/mutator.
>
> Same for ValveSkip/VS flag name? Maybe SNV? SkipNextValve?
Maybe, but I'm not super motivated, the parser and params are a bit
the same philosophy as the mod_rewrite stuff so it's "startsWith" for
everything. It would need more changes because "skip" and
"skipNextValve" override, same for the abbreviated form.
Rémy
> -chris
>
> > public boolean isEscapeBackReferences() {
> > return escapeBackReferences;
> > }
> > @@ -560,4 +565,13 @@ public class RewriteRule {
> > public void setCookieHttpOnly(boolean cookieHttpOnly) {
> > this.cookieHttpOnly = cookieHttpOnly;
> > }
> > +
> > + public boolean isValveSkip() {
> > + return this.valveSkip;
> > + }
> > +
> > + public void setValveSkip(boolean valveSkip) {
> > + this.valveSkip = valveSkip;
> > + }
> > +
> > }
> > diff --git a/java/org/apache/catalina/valves/rewrite/RewriteValve.java
> > b/java/org/apache/catalina/valves/rewrite/RewriteValve.java
> > index 492d45e26d..d15210621d 100644
> > --- a/java/org/apache/catalina/valves/rewrite/RewriteValve.java
> > +++ b/java/org/apache/catalina/valves/rewrite/RewriteValve.java
> > @@ -38,6 +38,7 @@ import org.apache.catalina.Context;
> > import org.apache.catalina.Lifecycle;
> > import org.apache.catalina.LifecycleException;
> > import org.apache.catalina.Pipeline;
> > +import org.apache.catalina.Valve;
> > import org.apache.catalina.connector.Connector;
> > import org.apache.catalina.connector.Request;
> > import org.apache.catalina.connector.Response;
> > @@ -319,11 +320,12 @@ public class RewriteValve extends ValveBase {
> > boolean done = false;
> > boolean qsa = false;
> > boolean qsd = false;
> > + boolean valveSkip = false;
> > for (int i = 0; i < rules.length; i++) {
> > RewriteRule rule = rules[i];
> > CharSequence test = (rule.isHost()) ? host : urlDecoded;
> > CharSequence newtest = rule.evaluate(test, resolver);
> > - if (newtest != null && !test.equals(newtest.toString())) {
> > + if (newtest != null &&
> > !test.toString().equals(newtest.toString())) {
> > if (containerLog.isTraceEnabled()) {
> > containerLog.trace("Rewrote " + test + " as " +
> > newtest
> > + " with rule pattern " +
> > rule.getPatternString());
> > @@ -345,6 +347,10 @@ public class RewriteValve extends ValveBase {
> > qsd = true;
> > }
> >
> > + if (!valveSkip && newtest != null && rule.isValveSkip()) {
> > + valveSkip = true;
> > + }
> > +
> > // Final reply
> >
> > // - forbidden
> > @@ -543,7 +549,15 @@ public class RewriteValve extends ValveBase {
> > pipeline.getFirst().invoke(request, response);
> > }
> > } else {
> > - getNext().invoke(request, response);
> > + Valve next = getNext();
> > + if (valveSkip) {
> > + next = next.getNext();
> > + if (next == null) {
> > + // Ignore and invoke the next valve normally
> > + next = getNext();
> > + }
> > + }
> > + next.invoke(request, response);
> > }
> >
> > } finally {
> > @@ -795,6 +809,8 @@ public class RewriteValve extends ValveBase {
> > }
> > rule.setType(true);
> > rule.setTypeValue(flag);
> > + } else if (flag.startsWith("valveSkip") || flag.startsWith("VS")) {
> > + rule.setValveSkip(true);
> > } else {
> > throw new
> > IllegalArgumentException(sm.getString("rewriteValve.invalidFlags", line,
> > flag));
> > }
> > diff --git a/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java
> > b/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java
> > index 8702891ac3..464a960384 100644
> > --- a/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java
> > +++ b/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java
> > @@ -70,6 +70,11 @@ public class TestRewriteValve extends TomcatBaseTest {
> > doTestRewrite("RewriteRule ^(.*) $1", "/a/%255A", "/a/%255A");
> > }
> >
> > + @Test
> > + public void testNoopValveSkipRewrite() throws Exception {
> > + doTestRewrite("RewriteRule ^(.*) $1 [VS]", "/a/%255A", "/a/%255A",
> > null, null, true);
> > + }
> > +
> > @Test
> > public void testPathRewrite() throws Exception {
> > doTestRewrite("RewriteRule ^/b(.*) /a$1", "/b/%255A", "/a/%255A");
> > @@ -730,6 +735,11 @@ public class TestRewriteValve extends TomcatBaseTest {
> >
> > private void doTestRewrite(String config, String request, String
> > expectedURI, String expectedQueryString,
> > String expectedAttributeValue) throws Exception {
> > + doTestRewrite(config, request, expectedURI, expectedQueryString,
> > expectedAttributeValue, false);
> > + }
> > +
> > + private void doTestRewrite(String config, String request, String
> > expectedURI, String expectedQueryString,
> > + String expectedAttributeValue, boolean valveSkip) throws
> > Exception {
> >
> > Tomcat tomcat = getTomcatInstance();
> >
> > @@ -738,6 +748,14 @@ public class TestRewriteValve extends TomcatBaseTest {
> >
> > RewriteValve rewriteValve = new RewriteValve();
> > ctx.getPipeline().addValve(rewriteValve);
> > + if (valveSkip) {
> > + ctx.getPipeline().addValve(new ValveBase() {
> > + @Override
> > + public void invoke(Request request, Response response)
> > throws IOException, ServletException {
> > + throw new IllegalStateException();
> > + }
> > + });
> > + }
> >
> > rewriteValve.setConfiguration(config);
> >
> > diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
> > index bba9fe5235..19aab1f4eb 100644
> > --- a/webapps/docs/changelog.xml
> > +++ b/webapps/docs/changelog.xml
> > @@ -150,6 +150,15 @@
> > transformation of a class also triggers the loading of the same
> > class.
> > (markt)
> > </fix>
> > + <fix>
> > + The rewrite valve should not do a rewrite if the output is
> > identical
> > + to the input. (remm)
> > + </fix>
> > + <update>
> > + Add a new <code>valveSkip</code> (or <code>VS</code>) rule flag to
> > the
> > + rewrite valve to allow skipping over the next valve in the Catalina
> > + pipeline. (remm)
> > + </update>
> > </changelog>
> > </subsection>
> > <subsection name="Coyote">
> > diff --git a/webapps/docs/rewrite.xml b/webapps/docs/rewrite.xml
> > index 59c2d08624..72fc813c91 100644
> > --- a/webapps/docs/rewrite.xml
> > +++ b/webapps/docs/rewrite.xml
> > @@ -802,6 +802,17 @@ cannot use <code>$N</code> in the substitution string!
> > the <code>.phps</code> extension:
> > <source>RewriteRule ^(.+\.php)s$ $1
> > [T=application/x-httpd-php-source]</source>
> > </li>
> > +
> > + <li>'<strong><code>valveSkip|VS</code></strong>'
> > + (skip valve)<br />
> > + This flag can be used to setup conditional execution of valves.
> > + When the flag is set and the rule matches, the rewrite valve will
> > skip
> > + the next valve in the Catalina pipeline. If the rewrite valve is
> > the
> > + last of the pipeline, then the flag will be ignored and the
> > container
> > + basic valve will be invoked. If rewrite occurred, then the flag
> > will
> > + not have any effect.
> > + </li>
> > +
> > </ul>
> >
> > </subsection>
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> > For additional commands, e-mail: dev-h...@tomcat.apache.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
