gnodet commented on PR #29055:
URL: https://github.com/apache/camel/pull/29055#issuecomment-6599554597
<!-- hermes-watermark:v1 -->
The backport is **incomplete**: the cherry-pick carried `requireHttps`
configuration wiring from `main` but omitted the underlying HTTPS delivery
implementation (SSLContext support, scheme-branching in `send()`). The result
is dead security code and tests that won't compile.
---
### 🔴 Compile-time failures
**Finding 3 — Test constructors called with 11 args, but constructor has 10
parameters**
`AS2ServerConnectionTest.aBareHostEntryMatchesTheDefaultPort` (and
`theWarningNamesTheAuthorityAsAnAllowListEntry`) pass 11 arguments to
`AS2ServerConnection`:
```java
new AS2ServerConnection("1.1", "Camel", "sender.example.com",
null, null, "user", "password", null, ALLOWED, null, false)
```
But the constructor added by this PR has exactly 10 parameters
(`as2Version`, `userAgent`, `senderFQDN`, `signingCertificateChain`,
`signingPrivateKey`, `userName`, `password`, `accessToken`, `allowedHosts`,
`requireHttps`). The extra `null` (intended for an SSLContext that exists on
`main` but was not backported) has no matching parameter. **Will not compile.**
**Finding 4 — Tests call a 4-arg `deliver()` overload that doesn't exist**
`httpIsRefusedWhenHttpsIsRequired` calls
`deliver("http://partner.example/receipts";, ALLOWED, null, true)` and
`httpsIsAcceptedWhenHttpsIsRequired` calls
`deliver("https://localhost:1/receipts";, "localhost:1", sslContext, true)` —
both with 4 arguments. The PR only adds 2-arg and 3-arg overloads; the 4-arg
`deliver(String, String, SSLContext, boolean)` was not backported. **Will not
compile.**
---
### 🔴 Dead security control
**Finding 1 — `requireHttps` field assigned but never read in `send()`**
`requireHttps` is declared as a field and assigned in the constructor, but
the `send()` method in `AS2AsynchronousMDNManager` never checks it. The field
is a dead store — there is no `if (requireHttps &&
!url.startsWith("https://";))` guard or equivalent. Operators who set
`asyncMdnRequireHttps=true` believing they are enforcing HTTPS will get no
enforcement.
**Finding 2 — `requireHttps` wiring present end-to-end, enforcement missing**
The configuration plumbing is complete (`AS2Configuration`,
`AS2ConsumerConfiguration`, endpoint DSL, generated configurers) but the
enforcement point is absent. This is the root cause of Finding 1: the feature
was not fully backported.
**Finding 5 — Documentation describes behaviour that doesn't exist**
The updated documentation says *"a bare host matches only the default port
for the scheme (80 for http, 443 for https)"*, but `send()` hardcodes `int
defaultPort = 80` with no scheme-branching logic. The `443` case is simply not
implemented.
**Finding 6 — Javadoc on getter/setter promises enforcement that doesn't
exist**
`AS2Configuration.setAsyncMdnRequireHttps()` Javadoc states *"an
asynchronous MDN whose delivery address does not use https is refused"*. It is
not refused. This creates a false sense of security for users relying on the
documentation.
---
### What's missing from the backport
The following pieces exist on `main` but were not cherry-picked:
- `AS2AsynchronousMDNManager` constructor accepting `SSLContext`
- `AS2ServerConnection` 11-parameter constructor accepting `SSLContext`
- `createSocket(SSLContext)` / scheme-branching in `send()`
- 4-arg `deliver(String, String, SSLContext, boolean)` test helper
- The compile-time guard that makes `requireHttps=true` actually refuse
non-HTTPS URLs
Until these are included, `requireHttps` is inert and the tests will not
build.
--
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]
