davsclaus commented on code in PR #27595:
URL: https://github.com/apache/camel/pull/27595#discussion_r4227329738
##########
components/camel-rest-postman/src/main/java/org/apache/camel/component/rest/postman/RestPostmanEndpoint.java:
##########
@@ -369,6 +373,15 @@ String buildDelegateUri(PostmanRequestBinding binding) {
if (!query.isEmpty()) {
uri = uri + "?" + query;
}
+ // getEndpoint resolves the property placeholders of a URI, functions
such as {{env:NAME}} included, so a
+ // placeholder that the collection's variables left unresolved must
not reach it
+ if (uri.contains("{{")) {
+ throw new IllegalArgumentException(
+ "Postman request " + binding.item().describe() + " still
contains a {{placeholder}} after variable"
+ + " substitution, in its URL,
Accept or Content-Type, which would be"
+ + " resolved from Camel
properties: " + URISupport.sanitizeUri(uri)
+ + ". Define it in the
collection, or supply it with the variables option.");
+ }
Review Comment:
The `{{` check runs on the URI after `queryParameters` has been
percent-encoded (`UnsafeUriCharactersEncoder` turns `{`/`}` into `%7B`/`%7D`),
but `createDelegateEndpoint` then hands the raw `binding.queryParameters()` to
`delegate.configureProperties(...)`, and `PropertyBindingSupport` resolves
property placeholders in String option values, functions included. The mapper
leaves query parameter *names* unencoded in `queryParameterMode=literal`
(`PostmanRequestMapper`) and for an `apikey` auth block with `in: query` under
`collectionAuth=header`. From reading the code, a cloud or HTTP collection with
a query key `{{someProperty}}`, or any collection with `{{env:SOME_SECRET}}` as
a key, would still be resolved and sent as the parameter name. Could the check
also cover the raw query parameters? A test with a cloud collection,
`queryParameterMode=literal` and a `{{leakProbe}}` query key (producer creation
fails, no request reaches the API) would pin it.
```suggestion
// getEndpoint resolves the property placeholders of a URI, and
configureProperties those of the option values
// (see determineEndpointParameters), functions such as {{env:NAME}}
included, so a placeholder that the
// collection's variables left unresolved must reach neither.
queryParameters is encoded in the URI, so it is
// checked as it is handed to configureProperties
String queryParameters = binding.queryParameters();
if (uri.contains("{{") || (queryParameters != null &&
queryParameters.contains("{{"))) {
throw new IllegalArgumentException(
"Postman request " + binding.item().describe() + " still
contains a {{placeholder}} after variable"
+ " substitution, in its URL,
query parameters, Accept or Content-Type,"
+ " which would be resolved
from Camel properties: "
+ URISupport.sanitizeUri(uri)
+ ". Define it in the
collection, or supply it with the variables option.");
}
```
##########
components/camel-rest-postman/src/main/docs/rest-postman-component.adoc:
##########
@@ -176,13 +177,28 @@ from("direct:start")
----
Postman environment files are not supported. Unresolved placeholders are left
as they are unless
-`failOnUnresolvedVariable=true`.
+`failOnUnresolvedVariable=true`. The exception is one left in the `Accept` or
`Content-Type` header of a request: these
Review Comment:
Nit: the producer also fails for a placeholder left in the URL host (it
stays raw in `host=` of the delegate URI and the new check rejects it, as its
message says), and with the change above in a query parameter name. Maybe "The
exception is one left in the host of the URL, a query parameter name, or the
`Accept` or `Content-Type` header of a request...". The catalog copy of the doc
needs regenerating with it.
--
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]
