This is an automated email from the ASF dual-hosted git repository.
Aias00 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/shenyu.git
The following commit(s) were added to refs/heads/master by this push:
new 10da866ef4 fix(rewrite): fail safe on an unusable rewrite rule handle
(#7853) (#7854)
10da866ef4 is described below
commit 10da866ef4e505de5b6c84bfff2469fe8765fd81
Author: build <[email protected]>
AuthorDate: Tue Oct 20 12:37:42 2026 +0800
fix(rewrite): fail safe on an unusable rewrite rule handle (#7853) (#7854)
* fix(rewrite): fail safe on an unusable rewrite rule handle
The rewrite plugin applies the rule handle with String#replaceAll, whose
replacement argument treats '\$' as a group reference. A replacement such as
"/new/\" without a capturing group, or a bare "\$\$", therefore throws on
every matching request and the request fails instead of being forwarded.
The placeholder branch can fail in the same way when the request path is
shorter than the pattern.
- validate the handle when it is published: a regex that cannot be compiled
or a replacement that cannot be applied is rejected with an error log
instead of being cached
- guard the rewrite step at runtime and forward the original uri when the
cached handle cannot be applied
* fix(common): include percentage in RewriteHandle equals and hashCode
toString() already reports the percentage but equals/hashCode ignored it,
so two handles that only differ by the rewritten traffic percentage compared
as equal.
---
.../common/dto/convert/rule/RewriteHandle.java | 3 +-
.../common/dto/convert/rule/RewriteHandleTest.java | 14 +++++++
.../shenyu/plugin/rewrite/RewritePlugin.java | 15 +++++--
.../rewrite/handler/RewritePluginDataHandler.java | 45 +++++++++++++++++++--
.../shenyu/plugin/rewrite/RewritePluginTest.java | 43 ++++++++++++++++++++
.../handler/RewritePluginDataHandlerTest.java | 47 ++++++++++++++++++++++
6 files changed, 159 insertions(+), 8 deletions(-)
diff --git
a/shenyu-common/src/main/java/org/apache/shenyu/common/dto/convert/rule/RewriteHandle.java
b/shenyu-common/src/main/java/org/apache/shenyu/common/dto/convert/rule/RewriteHandle.java
index c60b7f5679..8f231f4713 100644
---
a/shenyu-common/src/main/java/org/apache/shenyu/common/dto/convert/rule/RewriteHandle.java
+++
b/shenyu-common/src/main/java/org/apache/shenyu/common/dto/convert/rule/RewriteHandle.java
@@ -126,12 +126,13 @@ public class RewriteHandle {
}
RewriteHandle that = (RewriteHandle) o;
return Objects.equals(regex, that.regex) && Objects.equals(replace,
that.replace)
+ && Objects.equals(percentage, that.percentage)
&& Objects.equals(rewriteMetaData, that.rewriteMetaData);
}
@Override
public int hashCode() {
- return Objects.hash(regex, replace, rewriteMetaData);
+ return Objects.hash(regex, replace, percentage, rewriteMetaData);
}
@Override
diff --git
a/shenyu-common/src/test/java/org/apache/shenyu/common/dto/convert/rule/RewriteHandleTest.java
b/shenyu-common/src/test/java/org/apache/shenyu/common/dto/convert/rule/RewriteHandleTest.java
index 5ec74c0605..25f7e3b291 100644
---
a/shenyu-common/src/test/java/org/apache/shenyu/common/dto/convert/rule/RewriteHandleTest.java
+++
b/shenyu-common/src/test/java/org/apache/shenyu/common/dto/convert/rule/RewriteHandleTest.java
@@ -46,5 +46,19 @@ public class RewriteHandleTest {
assertThat(ImmutableSet.of(handle1, handle2), hasSize(1));
}
+
+ @Test
+ public void testEqualsIncludesPercentage() {
+ RewriteHandle handle1 = new RewriteHandle();
+ handle1.setRegex("regex");
+ handle1.setReplace("replace");
+ handle1.setPercentage(10);
+ RewriteHandle handle2 = new RewriteHandle();
+ handle2.setRegex("regex");
+ handle2.setReplace("replace");
+ handle2.setPercentage(20);
+
+ assertThat(ImmutableSet.of(handle1, handle2), hasSize(2));
+ }
}
diff --git
a/shenyu-plugin/shenyu-plugin-rewrite/src/main/java/org/apache/shenyu/plugin/rewrite/RewritePlugin.java
b/shenyu-plugin/shenyu-plugin-rewrite/src/main/java/org/apache/shenyu/plugin/rewrite/RewritePlugin.java
index 51c543b008..a334e92e76 100644
---
a/shenyu-plugin/shenyu-plugin-rewrite/src/main/java/org/apache/shenyu/plugin/rewrite/RewritePlugin.java
+++
b/shenyu-plugin/shenyu-plugin-rewrite/src/main/java/org/apache/shenyu/plugin/rewrite/RewritePlugin.java
@@ -74,10 +74,17 @@ public class RewritePlugin extends AbstractShenyuPlugin {
final Integer percentage =
Optional.ofNullable(rewriteHandle.getPercentage()).orElse(100);
if (StringUtils.isNoneBlank(rewriteHandle.getRegex(),
rewriteHandle.getReplace())
&& ThreadLocalRandom.current().nextInt(100) < percentage) {
- rewriteUri = rewriteHandle.getReplace().contains("{") &&
rewriteHandle.getRegex().contains("{")
- ? PathMatchUtils.replaceAll(rewriteHandle.getReplace(),
rewriteHandle.getRegex().substring(rewriteHandle.getRegex().indexOf("{")),
-
rewriteUri.substring(rewriteHandle.getRegex().indexOf("{")))
- : rewriteUri.replaceAll(rewriteHandle.getRegex(),
rewriteHandle.getReplace());
+ try {
+ rewriteUri = rewriteHandle.getReplace().contains("{") &&
rewriteHandle.getRegex().contains("{")
+ ?
PathMatchUtils.replaceAll(rewriteHandle.getReplace(),
rewriteHandle.getRegex().substring(rewriteHandle.getRegex().indexOf("{")),
+
rewriteUri.substring(rewriteHandle.getRegex().indexOf("{")))
+ : rewriteUri.replaceAll(rewriteHandle.getRegex(),
rewriteHandle.getReplace());
+ } catch (IllegalArgumentException | IndexOutOfBoundsException ex) {
+ // an unusable rule must not break the request, skip the
rewrite and keep the original uri.
+ LOG.error("failed to rewrite the uri [{}] with rule [{}],
regex: {}, replace: {}",
+ rewriteUri, rule.getId(), rewriteHandle.getRegex(),
rewriteHandle.getReplace(), ex);
+ return chain.execute(exchange);
+ }
Map<String, Object> attributes = exchange.getAttributes();
if
(Optional.ofNullable(rewriteHandle.getRewriteMetaData()).orElse(false)) {
// when the rewritten uri crosses plugins, this is necessary
diff --git
a/shenyu-plugin/shenyu-plugin-rewrite/src/main/java/org/apache/shenyu/plugin/rewrite/handler/RewritePluginDataHandler.java
b/shenyu-plugin/shenyu-plugin-rewrite/src/main/java/org/apache/shenyu/plugin/rewrite/handler/RewritePluginDataHandler.java
index d2cfc981cd..3d203b42bd 100644
---
a/shenyu-plugin/shenyu-plugin-rewrite/src/main/java/org/apache/shenyu/plugin/rewrite/handler/RewritePluginDataHandler.java
+++
b/shenyu-plugin/shenyu-plugin-rewrite/src/main/java/org/apache/shenyu/plugin/rewrite/handler/RewritePluginDataHandler.java
@@ -26,9 +26,13 @@ import org.apache.shenyu.plugin.base.cache.CommonHandleCache;
import org.apache.shenyu.plugin.base.handler.PluginDataHandler;
import org.apache.shenyu.plugin.base.utils.BeanHolder;
import org.apache.shenyu.plugin.base.utils.CacheKeyUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.util.Objects;
import java.util.Optional;
import java.util.function.Supplier;
+import java.util.regex.Pattern;
/**
* The type redirect plugin data subscriber.
@@ -37,14 +41,20 @@ public class RewritePluginDataHandler implements
PluginDataHandler {
public static final Supplier<CommonHandleCache<String, RewriteHandle>>
CACHED_HANDLE = new BeanHolder<>(CommonHandleCache::new);
+ private static final Logger LOG =
LoggerFactory.getLogger(RewritePluginDataHandler.class);
+
@Override
public void handlerRule(final RuleData ruleData) {
- Optional.ofNullable(ruleData.getHandle()).ifPresent(s -> {
- if (StringUtils.isBlank(s)) {
+ Optional.ofNullable(ruleData.getHandle()).ifPresent(handle -> {
+ if (StringUtils.isBlank(handle)) {
+ removeRule(ruleData);
+ return;
+ }
+ final RewriteHandle rewriteHandle =
GsonUtils.getInstance().fromJson(handle, RewriteHandle.class);
+ if (Objects.isNull(rewriteHandle) || !canRewrite(rewriteHandle,
ruleData)) {
removeRule(ruleData);
return;
}
- RewriteHandle rewriteHandle = GsonUtils.getInstance().fromJson(s,
RewriteHandle.class);
CACHED_HANDLE.get().cachedHandle(CacheKeyUtils.INST.getKey(ruleData),
rewriteHandle);
});
}
@@ -58,4 +68,33 @@ public class RewritePluginDataHandler implements
PluginDataHandler {
public String pluginNamed() {
return PluginEnum.REWRITE.getName();
}
+
+ /**
+ * Check whether the plugin can apply the rule handle, so that an unusable
rule is
+ * rejected when it is published instead of failing every matching request.
+ *
+ * @param rewriteHandle the parsed rule handle
+ * @param ruleData the rule that owns the handle
+ * @return true when the plugin can apply the handle
+ */
+ private static boolean canRewrite(final RewriteHandle rewriteHandle, final
RuleData ruleData) {
+ final String regex = rewriteHandle.getRegex();
+ final String replace = rewriteHandle.getReplace();
+ if (StringUtils.isAnyBlank(regex, replace)) {
+ // an incomplete expression means the plugin does not rewrite,
which is always safe.
+ return true;
+ }
+ if (replace.contains("{") && regex.contains("{")) {
+ // in placeholder mode both values are substituted literally, the
regex is never compiled.
+ return true;
+ }
+ try {
+ // the empty alternative makes the empty input match, which also
validates the replacement.
+ Pattern.compile(regex + "|").matcher("").replaceAll(replace);
+ return true;
+ } catch (IllegalArgumentException | IndexOutOfBoundsException ex) {
+ LOG.error("invalid rewrite rule [{}] is ignored, regex: {},
replace: {}", ruleData.getId(), regex, replace, ex);
+ return false;
+ }
+ }
}
diff --git
a/shenyu-plugin/shenyu-plugin-rewrite/src/test/java/org/apache/shenyu/plugin/rewrite/RewritePluginTest.java
b/shenyu-plugin/shenyu-plugin-rewrite/src/test/java/org/apache/shenyu/plugin/rewrite/RewritePluginTest.java
index d910bdd264..d8e06c4268 100644
---
a/shenyu-plugin/shenyu-plugin-rewrite/src/test/java/org/apache/shenyu/plugin/rewrite/RewritePluginTest.java
+++
b/shenyu-plugin/shenyu-plugin-rewrite/src/test/java/org/apache/shenyu/plugin/rewrite/RewritePluginTest.java
@@ -41,9 +41,11 @@ import reactor.core.publisher.Mono;
import reactor.test.StepVerifier;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
/**
@@ -138,6 +140,47 @@ public final class RewritePluginTest {
assertEquals("/new/{id}",
exchange.getAttributes().get(Constants.REWRITE_URI));
}
+ @Test
+ public void shouldNotThrowWhenReplacementReferencesMissingGroup() {
+ RuleData data = new RuleData();
+ data.setHandle("{\"regex\":\"test\",\"replace\":\"/new/$1\"}");
+ RewriteHandle rewriteHandle =
GsonUtils.getGson().fromJson(data.getHandle(), RewriteHandle.class);
+
RewritePluginDataHandler.CACHED_HANDLE.get().cachedHandle(CacheKeyUtils.INST.getKey(data),
rewriteHandle);
+ when(chain.execute(exchange)).thenReturn(Mono.empty());
+ SelectorData selectorData = mock(SelectorData.class);
+ StepVerifier.create(rewritePlugin.doExecute(exchange, chain,
selectorData, data)).expectSubscription().verifyComplete();
+ assertNull(exchange.getAttributes().get(Constants.REWRITE_URI));
+ verify(chain).execute(exchange);
+ }
+
+ @Test
+ public void shouldNotThrowWhenReplacementContainsBareDollar() {
+ RuleData data = new RuleData();
+ data.setHandle("{\"regex\":\"test\",\"replace\":\"/v1/$$\"}");
+ RewriteHandle rewriteHandle =
GsonUtils.getGson().fromJson(data.getHandle(), RewriteHandle.class);
+
RewritePluginDataHandler.CACHED_HANDLE.get().cachedHandle(CacheKeyUtils.INST.getKey(data),
rewriteHandle);
+ when(chain.execute(exchange)).thenReturn(Mono.empty());
+ SelectorData selectorData = mock(SelectorData.class);
+ StepVerifier.create(rewritePlugin.doExecute(exchange, chain,
selectorData, data)).expectSubscription().verifyComplete();
+ assertNull(exchange.getAttributes().get(Constants.REWRITE_URI));
+ verify(chain).execute(exchange);
+ }
+
+ @Test
+ public void shouldNotThrowWhenPathVariableRequestIsShorterThanPattern() {
+ RuleData data = new RuleData();
+
data.setHandle("{\"regex\":\"/shenyu/{id}\",\"replace\":\"/target/{id}\"}");
+ RewriteHandle rewriteHandle =
GsonUtils.getGson().fromJson(data.getHandle(), RewriteHandle.class);
+
RewritePluginDataHandler.CACHED_HANDLE.get().cachedHandle(CacheKeyUtils.INST.getKey(data),
rewriteHandle);
+ final ServerWebExchange shortExchange =
MockServerWebExchange.from(MockServerHttpRequest.get("/a").build());
+ shortExchange.getAttributes().put(Constants.CONTEXT, new
ShenyuContext());
+ when(chain.execute(shortExchange)).thenReturn(Mono.empty());
+ SelectorData selectorData = mock(SelectorData.class);
+ StepVerifier.create(rewritePlugin.doExecute(shortExchange, chain,
selectorData, data)).expectSubscription().verifyComplete();
+ assertNull(shortExchange.getAttributes().get(Constants.REWRITE_URI));
+ verify(chain).execute(shortExchange);
+ }
+
@Test
public void testSkip() {
final boolean result = rewritePlugin.skip(exchange);
diff --git
a/shenyu-plugin/shenyu-plugin-rewrite/src/test/java/org/apache/shenyu/plugin/rewrite/handler/RewritePluginDataHandlerTest.java
b/shenyu-plugin/shenyu-plugin-rewrite/src/test/java/org/apache/shenyu/plugin/rewrite/handler/RewritePluginDataHandlerTest.java
index c8d1041e09..f3c6d8ef43 100644
---
a/shenyu-plugin/shenyu-plugin-rewrite/src/test/java/org/apache/shenyu/plugin/rewrite/handler/RewritePluginDataHandlerTest.java
+++
b/shenyu-plugin/shenyu-plugin-rewrite/src/test/java/org/apache/shenyu/plugin/rewrite/handler/RewritePluginDataHandlerTest.java
@@ -112,4 +112,51 @@ public final class RewritePluginDataHandlerTest {
Assertions.assertNull(cached.getRegex());
Assertions.assertNull(cached.getReplace());
}
+
+ @Test
+ public void testInvalidRegexIsNotCached() {
+ ruleData.setSelectorId("invalid-regex-selector");
+ ruleData.setId("invalid-regex-rule");
+ ruleData.setHandle("{\"regex\":\"[a\",\"replace\":\"/new\"}");
+
+ rewritePluginDataHandler.handlerRule(ruleData);
+
+
Assertions.assertNull(RewritePluginDataHandler.CACHED_HANDLE.get().obtainHandle("invalid-regex-selector_invalid-regex-rule"));
+ }
+
+ @Test
+ public void testInvalidReplacementIsNotCached() {
+ ruleData.setSelectorId("invalid-replace-selector");
+ ruleData.setId("invalid-replace-rule");
+ ruleData.setHandle("{\"regex\":\"test\",\"replace\":\"/new/$1\"}");
+
+ rewritePluginDataHandler.handlerRule(ruleData);
+
+
Assertions.assertNull(RewritePluginDataHandler.CACHED_HANDLE.get().obtainHandle("invalid-replace-selector_invalid-replace-rule"));
+ }
+
+ @Test
+ public void testInvalidHandleEvictsPreviouslyCachedRule() {
+ ruleData.setSelectorId("stale-selector");
+ ruleData.setId("stale-rule");
+ ruleData.setHandle("{\"regex\":\"(test)\",\"replace\":\"/new/$1\"}");
+ rewritePluginDataHandler.handlerRule(ruleData);
+
Assertions.assertNotNull(RewritePluginDataHandler.CACHED_HANDLE.get().obtainHandle("stale-selector_stale-rule"));
+
+ ruleData.setHandle("{\"regex\":\"test\",\"replace\":\"/new/$1\"}");
+ rewritePluginDataHandler.handlerRule(ruleData);
+
+
Assertions.assertNull(RewritePluginDataHandler.CACHED_HANDLE.get().obtainHandle("stale-selector_stale-rule"));
+ }
+
+ @Test
+ public void testPlaceholderHandleIsCached() {
+ ruleData.setSelectorId("placeholder-selector");
+ ruleData.setId("placeholder-rule");
+
ruleData.setHandle("{\"regex\":\"/shenyu/{id}\",\"replace\":\"/target/{id}\"}");
+
+ rewritePluginDataHandler.handlerRule(ruleData);
+
+
Assertions.assertNotNull(RewritePluginDataHandler.CACHED_HANDLE.get().obtainHandle("placeholder-selector_placeholder-rule"));
+ }
}
