Skip to content

Commit bdbea2a

Browse files
t-burchpredic8claude
authored
Fix matchesPath to prevent base path conflicts (#3177)
* Fix matchesPath to honor configured path and prevent base path conflicts (#3173) * Keep /api-docs reachable for wsdl2openapi APIs Splitting the OpenAPI publisher paths out of basePaths made matchesPath honour the configured path, but Wsdl2OpenapiInterceptor registers its document path via addBasePaths and its key is built with openAPI=false (the api declares no specs). With a configured path such as /service, /api-docs no longer matched and returned 404. Add APIProxyKey.addApiDocsPaths() and let Wsdl2OpenapiInterceptor register PATH there, keeping only the api's own base path in basePaths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Make RuleKey equality value based AbstractRuleKey implements RuleKey but never overrode equals/hashCode, so ServiceProxyKey.equals started with an identity comparison via super and could never return true for two separately constructed keys. That made APIProxyKey's basePaths and expression comparison unreachable and left RuleManager.exists() always false, so duplicate APIs were never detected. Define equals/hashCode on AbstractRuleKey over port, ip, path, pathRegExp and usePathPattern. Subclasses keep adding their own discriminators on top. InternalProxyKey implements RuleKey directly and keeps identity semantics, since internal proxies are matched by name. APIProxyKey now also compares apiDocsPaths, and its hashCode no longer throws when no expression is configured. With exists() working, a duplicate API is dropped, which used to be a silent no-op. Log a warning naming the API and its key. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: thomas <bayer@predic8.de> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 3d1e427 commit bdbea2a

7 files changed

Lines changed: 174 additions & 14 deletions

File tree

‎core/src/main/java/com/predic8/membrane/core/interceptor/wsdl2openapi/Wsdl2OpenapiInterceptor.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -367,10 +367,13 @@ private Response soapFaultResponse(SoapFaultException fault) {
367367
* Makes the OpenAPI document reachable next to the API's own path. Only base paths are added:
368368
* the key's path itself is never rewritten, so this stays safe when init() runs again on the
369369
* same proxy (APIProxy rebuilds its key on each init, so the list cannot accumulate either).
370+
* PATH goes into the key's api docs paths so that it stays reachable even when the API has a
371+
* custom path configured.
370372
*/
371373
private void registerApiDocsPaths() {
372374
if (proxy.getKey() instanceof APIProxyKey apiKey) {
373-
apiKey.addBasePaths(new ArrayList<>(List.of(PATH, basePath)));
375+
apiKey.addApiDocsPaths(new ArrayList<>(List.of(PATH)));
376+
apiKey.addBasePaths(new ArrayList<>(List.of(basePath)));
374377
}
375378
}
376379

‎core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyKey.java‎

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,13 @@ public class APIProxyKey extends ServiceProxyKey {
3636

3737
private final ArrayList<String> basePaths = new ArrayList<>();
3838

39+
/**
40+
* Paths of the OpenAPIPublisherInterceptor (e.g. /api-docs). These stay reachable even when a
41+
* custom path is configured, but - unlike {@link #basePaths} - they must not override the
42+
* configured path when discriminating between several APIs sharing the same base path.
43+
*/
44+
private final ArrayList<String> apiDocsPaths = new ArrayList<>();
45+
3946
/**
4047
* For complex matches use SpEL
4148
*/
@@ -63,11 +70,11 @@ protected void init(ExchangeExpression exchangeExpression, boolean openAPI) {
6370
if (!openAPI)
6471
return;
6572

66-
// Add basePaths of OpenAPIPublisherInterceptor to accept them also
67-
basePaths.add(PATH); // new path
68-
basePaths.add(PATH_UI); // "
69-
basePaths.add("/api-doc"); // old to stay compatible
70-
basePaths.add("/api-doc/ui"); // "
73+
// Add paths of OpenAPIPublisherInterceptor to accept them also
74+
apiDocsPaths.add(PATH); // new path
75+
apiDocsPaths.add(PATH_UI); // "
76+
apiDocsPaths.add("/api-doc"); // old to stay compatible
77+
apiDocsPaths.add("/api-doc/ui"); // "
7178
}
7279

7380
@Override
@@ -80,10 +87,13 @@ public boolean complexMatch(Exchange exc) {
8087
return false;
8188
}
8289

83-
if (basePaths.isEmpty())
90+
if (basePaths.isEmpty() && apiDocsPaths.isEmpty())
8491
return true;
8592

8693
var uri = exc.getRequest().getUri();
94+
if (matchesApiDocsPath(uri))
95+
return true;
96+
8797
for (String basePath : basePaths) {
8898
if (!uri.startsWith(basePath))
8999
continue;
@@ -95,6 +105,14 @@ public boolean complexMatch(Exchange exc) {
95105
return false;
96106
}
97107

108+
private boolean matchesApiDocsPath(String path) {
109+
for (String apiDocsPath : apiDocsPaths) {
110+
if (path.startsWith(apiDocsPath))
111+
return true;
112+
}
113+
return false;
114+
}
115+
98116
private boolean testCondition(Exchange exc) {
99117
if (exchangeExpression == null)
100118
return true;
@@ -105,6 +123,15 @@ public void addBasePaths(ArrayList<String> paths) {
105123
basePaths.addAll(paths);
106124
}
107125

126+
/**
127+
* Registers additional paths under which an OpenAPI document is published. Use this instead of
128+
* {@link #addBasePaths(ArrayList)} for documentation paths: they stay reachable even when a
129+
* custom path is configured.
130+
*/
131+
public void addApiDocsPaths(ArrayList<String> paths) {
132+
apiDocsPaths.addAll(paths);
133+
}
134+
108135
public String getKeyId() {
109136
return (
110137
getMethod() + "-"
@@ -128,17 +155,18 @@ public boolean equals(Object obj) {
128155
if (obj instanceof APIProxyKey other) {
129156
if (!basePaths.equals(other.basePaths))
130157
return false;
158+
if (!apiDocsPaths.equals(other.apiDocsPaths))
159+
return false;
131160
return Objects.equals(exchangeExpression, other.exchangeExpression);
132161
}
133162
return false;
134163
}
135164

136165
@Override
137166
public boolean matchesPath(String path) {
138-
for (String basePath : basePaths) {
139-
if (path.startsWith(basePath))
140-
return true;
141-
}
167+
// OpenAPI docs stay reachable even when a custom path is configured, see issue #3173.
168+
if (matchesApiDocsPath(path))
169+
return true;
142170
try {
143171
matchTemplate(getPath(), path); // ignore result
144172
return true;
@@ -150,6 +178,6 @@ public boolean matchesPath(String path) {
150178

151179
@Override
152180
public int hashCode() {
153-
return super.hashCode() + Objects.hashCode( exchangeExpression.hashCode()) + basePaths.hashCode();
181+
return Objects.hash(super.hashCode(), basePaths, apiDocsPaths, exchangeExpression);
154182
}
155183
}

‎core/src/main/java/com/predic8/membrane/core/proxies/AbstractRuleKey.java‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import com.predic8.membrane.core.exchange.*;
1717
import org.slf4j.*;
1818

19+
import java.util.*;
1920
import java.util.regex.*;
2021

2122
public abstract class AbstractRuleKey implements RuleKey {
@@ -57,6 +58,28 @@ public AbstractRuleKey(RuleKey key) {
5758
}
5859
}
5960

61+
/**
62+
* Identity of a rule key is its port, ip and path. Subclasses add their own discriminators
63+
* (host, method, base paths, ...) on top and must call super.equals() first.
64+
*/
65+
@Override
66+
public boolean equals(Object obj) {
67+
if (this == obj)
68+
return true;
69+
if (!(obj instanceof AbstractRuleKey other))
70+
return false;
71+
return port == other.port
72+
&& pathRegExp == other.pathRegExp
73+
&& usePathPattern == other.usePathPattern
74+
&& Objects.equals(ip, other.ip)
75+
&& Objects.equals(path, other.path);
76+
}
77+
78+
@Override
79+
public int hashCode() {
80+
return Objects.hash(port, pathRegExp, usePathPattern, ip, path);
81+
}
82+
6083
public String getHost() {
6184
return "";
6285
}

‎core/src/main/java/com/predic8/membrane/core/proxies/RuleManager.java‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ public void addProxyAndOpenPortIfNew(SSLableProxy proxy) throws IOException {
6969
}
7070

7171
public synchronized void addProxyAndOpenPortIfNew(SSLableProxy proxy, RuleDefinitionSource source) throws IOException {
72-
if (exists(proxy.getKey()))
72+
if (skipDuplicate(proxy))
7373
return;
7474

7575
router.getTransport().openPort(proxy);
@@ -78,7 +78,7 @@ public synchronized void addProxyAndOpenPortIfNew(SSLableProxy proxy, RuleDefini
7878
}
7979

8080
public synchronized void addProxy(Proxy proxy, RuleDefinitionSource source) {
81-
if (exists(proxy.getKey()))
81+
if (skipDuplicate(proxy))
8282
return;
8383

8484
proxies.add(proxy);
@@ -138,6 +138,18 @@ public synchronized void openPorts() throws IOException {
138138
}
139139

140140

141+
/**
142+
* An API whose key is indistinguishable from one that is already registered can never be
143+
* reached, so it is dropped. Warn about it: silently ignoring it looks like a lost API.
144+
*/
145+
private boolean skipDuplicate(Proxy proxy) {
146+
if (!exists(proxy.getKey()))
147+
return false;
148+
log.warn("Ignoring API '{}': another API is already configured for {}. Give them different ports, hosts or paths.",
149+
proxy.getName(), proxy.getKey());
150+
return true;
151+
}
152+
141153
public boolean exists(RuleKey key) {
142154
return getRule(key) != null;
143155
}

‎core/src/test/java/com/predic8/membrane/core/RuleManagerTest.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,30 @@ public void tearDown() {
6363
router.stop();
6464
}
6565

66+
@Test
67+
@DisplayName("An API with a key that is already registered is not added")
68+
void duplicateKeyIsNotAdded() throws Exception {
69+
var duplicate = new ServiceProxy(new ServiceProxyKey("localhost", "*", ".*", 3014), "thomas-bayer.com", 80);
70+
duplicate.init(router);
71+
72+
manager.addProxyAndOpenPortIfNew(duplicate);
73+
74+
assertEquals(4, manager.getRules().size());
75+
assertSame(forwardBlz, manager.getRules().get(1));
76+
}
77+
78+
@Test
79+
@DisplayName("Internal proxies are told apart by identity, not by their key")
80+
void internalProxiesAreNotDeduplicated() {
81+
var second = new InternalProxy();
82+
second.setName("invoice");
83+
second.init(router);
84+
85+
manager.addProxy(second, RuleManager.RuleDefinitionSource.MANUAL);
86+
87+
assertEquals(5, manager.getRules().size());
88+
}
89+
6690
@Test
6791
void getRules() {
6892
assertEquals(4, manager.getRules().size());

‎core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyKeyComplexMatchTest.java‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,55 @@ void matchesPathAllowsApiDocsWhenPathIsConfigured() {
105105
assertFalse(key.matchesPath("/other"));
106106
}
107107

108+
@Test
109+
@DisplayName("matchesPath honours the configured path and does not swallow a shared base path (#3173)")
110+
void matchesPathDoesNotSwallowSharedBasePath() {
111+
// Two APIs share the rewrite base path /jfa/api/ but are told apart by their configured path.
112+
var key = new APIProxyKey("", "", 8443, "/jfa/api/housekeeping", "*", null, true) {{
113+
addBasePaths(new ArrayList<>(List.of("/jfa/api/")));
114+
}};
115+
116+
assertTrue(key.matchesPath("/jfa/api/housekeeping"));
117+
assertTrue(key.matchesPath("/jfa/api/housekeeping/foo"));
118+
119+
assertFalse(key.matchesPath("/jfa/api/echo"));
120+
assertFalse(key.matchesPath("/jfa/api/user"));
121+
122+
assertTrue(key.matchesPath("/api-docs"));
123+
}
124+
125+
@Test
126+
@DisplayName("Api docs paths stay reachable when the key was not built from OpenAPI specs, e.g. wsdl2openapi")
127+
void matchesPathAllowsRegisteredApiDocsPaths() {
128+
var key = new APIProxyKey("", "", 2000, "/service", "*", null, false) {{
129+
addApiDocsPaths(new ArrayList<>(List.of("/api-docs")));
130+
addBasePaths(new ArrayList<>(List.of("/service")));
131+
}};
132+
133+
assertTrue(key.matchesPath("/api-docs"));
134+
assertTrue(key.matchesPath("/api-docs/ui"));
135+
assertTrue(key.matchesPath("/service/get-city"));
136+
assertFalse(key.matchesPath("/other"));
137+
}
138+
139+
@Test
140+
@DisplayName("Keys differing only in their api docs paths are not equal")
141+
void keysWithDifferentApiDocsPathsAreNotEqual() {
142+
var openAPIKey = new APIProxyKey("", "", 80, "/cities", "*", null, true);
143+
144+
assertEquals(openAPIKey, new APIProxyKey("", "", 80, "/cities", "*", null, true));
145+
assertNotEquals(openAPIKey, new APIProxyKey("", "", 80, "/cities", "*", null, false));
146+
}
147+
148+
@Test
149+
@DisplayName("hashCode agrees with equals and works without an expression")
150+
void hashCodeWithoutExchangeExpression() {
151+
var key = new APIProxyKey("", "", 80, "/cities", "*", null, true);
152+
153+
assertEquals(key.hashCode(), new APIProxyKey("", "", 80, "/cities", "*", null, true).hashCode());
154+
assertNotEquals(key.hashCode(), new APIProxyKey("", "", 80, "/cities", "*", null, false).hashCode());
155+
}
156+
108157
private static Stream<Arguments> urls() {
109158
return Stream.of(
110159
of("/api-docs",true),

‎core/src/test/java/com/predic8/membrane/core/proxies/ServiceProxyKeyTest.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,27 @@
2121

2222
public class ServiceProxyKeyTest {
2323

24+
@Test
25+
@DisplayName("Keys with the same port, host, method and path are equal")
26+
void equalsAndHashCodeOnValues() {
27+
var key = new ServiceProxyKey("localhost", "GET", "/foo", 2000);
28+
var same = new ServiceProxyKey("localhost", "GET", "/foo", 2000);
29+
30+
assertEquals(key, same);
31+
assertEquals(key.hashCode(), same.hashCode());
32+
}
33+
34+
@Test
35+
@DisplayName("Port, host, method and path each discriminate")
36+
void notEqualOnDifferingValues() {
37+
var key = new ServiceProxyKey("localhost", "GET", "/foo", 2000);
38+
39+
assertNotEquals(key, new ServiceProxyKey("localhost", "GET", "/foo", 3000));
40+
assertNotEquals(key, new ServiceProxyKey("other", "GET", "/foo", 2000));
41+
assertNotEquals(key, new ServiceProxyKey("localhost", "POST", "/foo", 2000));
42+
assertNotEquals(key, new ServiceProxyKey("localhost", "GET", "/bar", 2000));
43+
}
44+
2445
@Test
2546
void createHostPatternSimple() {
2647
assertEquals("(\\Qmembrane\\E)", ServiceProxyKey.createHostPattern("membrane"));

0 commit comments

Comments
 (0)