davsclaus commented on code in PR #27725:
URL: https://github.com/apache/camel/pull/27725#discussion_r4248952630
##########
components/camel-lra/src/main/java/org/apache/camel/service/lra/LRASagaService.java:
##########
@@ -84,17 +89,86 @@ public boolean isLongRunningActionHeaderSupported() {
return true;
}
+ /**
+ * Returns the coordinator of an LRA, which is joined, completed or
compensated by calling its URL. Only an LRA
+ * hosted by the configured coordinator, or by one of the allowed
coordinator URLs, is accepted.
+ */
@Override
public CompletableFuture<CamelSagaCoordinator> getSaga(String id) {
CompletableFuture<CamelSagaCoordinator> coordinator;
try {
- coordinator = CompletableFuture.completedFuture(new
LRASagaCoordinator(this, URI.create(id).toURL()));
+ URI lra = URI.create(id);
+ if (!isHostedByAllowedCoordinator(lra)) {
+ throw new IllegalArgumentException(
+ "The Long-Running-Action " + id + " is not hosted by
the LRA coordinator configured on the LRA"
+ + " saga service. If it is
a trusted coordinator, add its URL to"
+ + "
allowedCoordinatorUrls.");
+ }
+ coordinator = CompletableFuture.completedFuture(new
LRASagaCoordinator(this, lra.toURL()));
} catch (Exception ex) {
coordinator = CompletableFuture.failedFuture(ex);
}
return coordinator;
}
+ private boolean isHostedByAllowedCoordinator(URI lra) {
+ for (URI coordinator : allowedCoordinators()) {
+ if (isHostedBy(lra, coordinator)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * The URLs of the coordinators whose LRAs are accepted: the configured
coordinator, and the allowed coordinator
+ * URLs.
+ */
+ private List<URI> allowedCoordinators() {
+ List<URI> coordinators = new ArrayList<>();
+ coordinators.add(URI.create(new
LRAUrlBuilder().host(coordinatorUrl).path(coordinatorContextPath).build()));
+ coordinators.addAll(additionalCoordinators());
+ return coordinators;
+ }
+
+ private List<URI> additionalCoordinators() {
+ List<URI> coordinators = new ArrayList<>();
+ if (ObjectHelper.isNotEmpty(allowedCoordinatorUrls)) {
+ for (String url : allowedCoordinatorUrls.split(",")) {
+ if (!url.isBlank()) {
+ coordinators.add(URI.create(url.trim()));
+ }
+ }
+ }
+ return coordinators;
+ }
+
+ /**
+ * Whether the LRA URL is under the coordinator URL: same scheme, host and
port, no user info, and a path under the
+ * path of the coordinator.
+ */
+ private static boolean isHostedBy(URI lra, URI coordinator) {
+ if (lra.getScheme() == null || lra.getHost() == null ||
lra.getRawUserInfo() != null) {
+ return false;
+ }
+ String lraPath = lra.normalize().getRawPath();
Review Comment:
`normalize()` only resolves literal dot segments, so encoded or `;`-suffixed
ones pass the `startsWith` check. Running this method with the coordinator
`http://coordinator:8080/lra-coordinator`:
| LRA path | accepted |
|---|---|
| `/lra-coordinator/../admin/x` | no |
| `/lra-coordinator/%2e%2e/admin/x` (also `%2E%2E`) | yes |
| `/lra-coordinator/..;/admin/x` | yes |
| `/lra-coordinator/..%2fadmin/x` | yes |
Servlet containers strip `;` path parameters (so `..;` acts as `..`), and
many servers decode `%2e` before resolving the path, so such a URL can reach
another path on the coordinator's host and port, for example another service
behind the same ingress host. Rejecting a raw path that contains `;`, `%2e`,
`%2f` or `%5c` (case-insensitive), or checking the decoded, normalized path
instead, would close it; a few cases next to the `..` one in
`rejectsAPathOutsideTheCoordinatorContextPath` would cover it.
##########
components/camel-lra/src/main/java/org/apache/camel/service/lra/LRASagaService.java:
##########
@@ -139,6 +213,12 @@ protected void doStart() throws Exception {
if (localParticipantUrl == null) {
throw new IllegalStateException("localParticipantUrl must be
configured on the LRA saga service");
}
+ for (URI coordinator : additionalCoordinators()) {
+ if (!coordinator.isAbsolute() || coordinator.getHost() == null) {
Review Comment:
An entry whose host has an underscore is absolute but has no host here (see
the comment above), so it fails with "must hold absolute coordinator URLs",
which points the user the wrong way. Saying that no host could be read from the
URL would name the cause. Should this also accept only `http` and `https` URLs?
##########
components/camel-lra/src/main/java/org/apache/camel/service/lra/LRASagaService.java:
##########
@@ -84,17 +89,86 @@ public boolean isLongRunningActionHeaderSupported() {
return true;
}
+ /**
+ * Returns the coordinator of an LRA, which is joined, completed or
compensated by calling its URL. Only an LRA
+ * hosted by the configured coordinator, or by one of the allowed
coordinator URLs, is accepted.
+ */
@Override
public CompletableFuture<CamelSagaCoordinator> getSaga(String id) {
CompletableFuture<CamelSagaCoordinator> coordinator;
try {
- coordinator = CompletableFuture.completedFuture(new
LRASagaCoordinator(this, URI.create(id).toURL()));
+ URI lra = URI.create(id);
+ if (!isHostedByAllowedCoordinator(lra)) {
+ throw new IllegalArgumentException(
+ "The Long-Running-Action " + id + " is not hosted by
the LRA coordinator configured on the LRA"
+ + " saga service. If it is
a trusted coordinator, add its URL to"
+ + "
allowedCoordinatorUrls.");
+ }
+ coordinator = CompletableFuture.completedFuture(new
LRASagaCoordinator(this, lra.toURL()));
} catch (Exception ex) {
coordinator = CompletableFuture.failedFuture(ex);
}
return coordinator;
}
+ private boolean isHostedByAllowedCoordinator(URI lra) {
+ for (URI coordinator : allowedCoordinators()) {
+ if (isHostedBy(lra, coordinator)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * The URLs of the coordinators whose LRAs are accepted: the configured
coordinator, and the allowed coordinator
+ * URLs.
+ */
+ private List<URI> allowedCoordinators() {
+ List<URI> coordinators = new ArrayList<>();
+ coordinators.add(URI.create(new
LRAUrlBuilder().host(coordinatorUrl).path(coordinatorContextPath).build()));
+ coordinators.addAll(additionalCoordinators());
+ return coordinators;
+ }
+
+ private List<URI> additionalCoordinators() {
+ List<URI> coordinators = new ArrayList<>();
+ if (ObjectHelper.isNotEmpty(allowedCoordinatorUrls)) {
+ for (String url : allowedCoordinatorUrls.split(",")) {
+ if (!url.isBlank()) {
+ coordinators.add(URI.create(url.trim()));
+ }
+ }
+ }
+ return coordinators;
+ }
+
+ /**
+ * Whether the LRA URL is under the coordinator URL: same scheme, host and
port, no user info, and a path under the
+ * path of the coordinator.
+ */
+ private static boolean isHostedBy(URI lra, URI coordinator) {
+ if (lra.getScheme() == null || lra.getHost() == null ||
lra.getRawUserInfo() != null) {
Review Comment:
`java.net.URI` takes a host name with an underscore, such as a Docker name
like `lra_coordinator`, as a registry-based authority, so `getHost()` is `null`
for the LRA and for the configured coordinator. Every LRA is then rejected,
including the ones the service started itself:
`SagaProcessor.getCurrentSagaCoordinator()` calls `getSaga()` for nested saga
routes. Today `URI.create(id).toURL()` accepts such hosts, so those setups
would break after the upgrade. Comparing `getRawAuthority()` (without user
info, with the default port) when `getHost()` is null, or reading host and port
through `java.net.URL`, would keep them working; failing at startup with a
clear message when the configured coordinator URL has no host would at least
make it visible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
