[
https://issues.apache.org/jira/browse/CAMEL-25660?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Andrea Cosentino updated CAMEL-25660:
-------------------------------------
Description:
h3. Summary
With {{enableDeadLettering=true}} in PEEK_LOCK mode,
{{ServiceBusConsumer.ConsumerOnCompletion.onFailure}} dead-letters the message
with a reason and a description built from the route's exception. Two problems:
# The call is not guarded, so a rejected dead-letter request escapes
{{onFailure}} and the message is never dead-lettered at all.
# Neither field is bounded, so Camel does nothing to keep the message under the
size quota that Azure's own documentation warns about for exactly this practice.
h3. The unguarded call
{code:java}
try {
...
messageContext.deadLetter(deadLetterOptions);
...
} finally {
pendingExchanges.decrementAndGet();
}
{code}
There is no {{catch}}. If the broker rejects the request, the exception leaves
{{onFailure}} into the Azure SDK's processor callback, and nothing reports it
through the consumer's configured {{ExceptionHandler}}. The message stays
locked, the lock expires, Service Bus redelivers it, the route fails the same
way - and this repeats until {{maxDeliveryCount}} is reached, at which point
the broker dead-letters it itself with reason {{MaxDeliveryCountExceeded}}. The
outcome is that the diagnostics {{enableDeadLettering}} exists to provide are
exactly what gets lost, and the exchange is processed several more times than
intended.
{{onComplete}} has the same unguarded shape around
{{messageContext.complete()}}, which is worth deciding on at the same time.
h3. The unbounded fields
{code:java}
deadLetterOptions.setDeadLetterReason(
String.format("%s: %s", cause.getClass().getName(),
cause.getMessage()));
deadLetterOptions.setDeadLetterErrorDescription(Arrays.stream(cause.getStackTrace())
.map(StackTraceElement::toString)
.collect(Collectors.joining("\n")));
{code}
To be clear, the *content* here is what Azure asks for. The dead-letter queue
documentation says:
{quote}We recommend that you include the type of the exception in the
{{DeadLetterReason}} and the stack trace of the exception in the
{{DeadLetterDescription}} as it makes it easier to troubleshoot the cause of
the problem resulting in messages being dead-lettered. It might result in some
messages exceeding the 256 KB quota limit for the Standard tier of Azure
Service Bus.{quote}
So the component follows the recommendation but not the caveat: a full Camel
stack trace, with error-handler and processor frames, is routinely several
thousand characters, and {{cause.getMessage()}} is itself unbounded (a database
error quoting the statement, a validation report). Nothing truncates either
one, and {{com.azure.messaging.servicebus.models.DeadLetterOptions}} does no
client-side length validation (checked against azure-messaging-servicebus
7.17.16), so the broker is what rejects it - landing on problem 1 above.
h3. Suggested fix
Bound both fields, keeping the recommended content but capping it, and guard
the dead-letter call so a rejection is reported through the exception handler
rather than escaping {{onFailure}}. The head of a stack trace is the useful
part; the tail is error-handler frames.
h3. Evidence
* Verified by reading the component on main: no {{catch}} around
{{deadLetter}}, neither field bounded.
* Verified that {{DeadLetterOptions}} has no length validation in the SDK.
* The 256 KB Standard-tier caveat is quoted from Microsoft's dead-letter queue
documentation above.
* *Not* reproduced against a live namespace. The exact payload size at which
the broker rejects the request has not been measured, and the documentation
gives a message-size quota rather than a per-field limit.
h4. Correction to the original description
The first version of this issue claimed a documented 4096 limit on the
dead-letter reason and description. That came from the .NET
{{DeadLetterMessageAsync}} API reference and is *not* corroborated by the
dead-letter queue page or the Service Bus quotas page, so I am retracting it.
The documented constraint is the 256 KB message-size quota on the Standard
tier, quoted above. The unguarded call - the more serious half - is unaffected
either way, since it is verified in Camel's own code.
was:
h3. Summary
With {{enableDeadLettering=true}} in PEEK_LOCK mode,
{{ServiceBusConsumer.onFailure}} fills both dead-letter fields straight from
the route's exception, with no length limit:
{code:java}
deadLetterOptions.setDeadLetterReason(
String.format("%s: %s", cause.getClass().getName(),
cause.getMessage()));
deadLetterOptions.setDeadLetterErrorDescription(Arrays.stream(cause.getStackTrace())
.map(StackTraceElement::toString)
.collect(Collectors.joining("\n")));
messageContext.deadLetter(deadLetterOptions);
{code}
A full Camel stack trace, with the error-handler and processor frames, is
routinely several thousand characters. Microsoft's reference for
{{ServiceBusReceiver.DeadLetterMessageAsync}} lists 4096 as the ceiling for the
dead-letter reason or the error description, and exceeding it is one of the
documented error conditions for the call. So for an ordinary route failure the
description can be over the limit.
h3. Why it matters
The call is in a {{try}} whose only companion is a {{finally}} that decrements
{{pendingExchanges}} - there is no {{catch}}:
{code:java}
try {
...
messageContext.deadLetter(deadLetterOptions);
...
} finally {
pendingExchanges.decrementAndGet();
}
{code}
So if the broker rejects the dead-letter request, the exception leaves
{{onFailure}} and the message is *not* dead-lettered. Its lock then expires and
Service Bus redelivers it, the route fails the same way, and the same oversized
description is sent again - until {{maxDeliveryCount}} is reached and the
broker dead-letters it on its own terms. The net effect is that
{{enableDeadLettering}} does not deliver the diagnostics it is there for, and
the exchange is processed several more times than intended.
The reason field has the same shape: {{cause.getMessage()}} is unbounded, and
some messages are long (a database error quoting the statement, a validation
report).
h3. Suggested fix
Truncate both fields to what Service Bus accepts before calling {{deadLetter}},
and wrap the call so that a broker rejection is reported through the exception
handler instead of escaping {{onFailure}}. Keeping the head of the stack trace
is the useful part; the tail is error-handler frames.
h3. Evidence and what is NOT verified
* Verified by reading the component on main: both fields are unbounded, and
there is no {{catch}} around {{deadLetter}}.
* Verified that {{com.azure.messaging.servicebus.models.DeadLetterOptions}}
(azure-messaging-servicebus 7.17.16) does no client-side length validation, so
the broker is what rejects it.
* *Not* verified against a live namespace - this has not been reproduced, and
the 4096 figure comes from the SDK reference rather than the Service Bus quotas
page, which also leaves it unclear whether the limit counts characters or
bytes. Someone with a Service Bus namespace should confirm the threshold and
the resulting behaviour before the fix is sized.
Filed unassigned.
Found while sweeping for consumers that echo exception detail. Unlike that
class, this is not an information-disclosure concern: the text goes to the
operator's own dead-letter queue, not to an untrusted sender.
> camel-azure-servicebus - dead-lettering builds unbounded reason and
> description from the exception, and a failure to dead-letter is not caught
> ----------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: CAMEL-25660
> URL: https://issues.apache.org/jira/browse/CAMEL-25660
> Project: Camel
> Issue Type: Bug
> Components: camel-azure-servicebus
> Reporter: Andrea Cosentino
> Priority: Major
>
> h3. Summary
> With {{enableDeadLettering=true}} in PEEK_LOCK mode,
> {{ServiceBusConsumer.ConsumerOnCompletion.onFailure}} dead-letters the
> message with a reason and a description built from the route's exception. Two
> problems:
> # The call is not guarded, so a rejected dead-letter request escapes
> {{onFailure}} and the message is never dead-lettered at all.
> # Neither field is bounded, so Camel does nothing to keep the message under
> the size quota that Azure's own documentation warns about for exactly this
> practice.
> h3. The unguarded call
> {code:java}
> try {
> ...
> messageContext.deadLetter(deadLetterOptions);
> ...
> } finally {
> pendingExchanges.decrementAndGet();
> }
> {code}
> There is no {{catch}}. If the broker rejects the request, the exception
> leaves {{onFailure}} into the Azure SDK's processor callback, and nothing
> reports it through the consumer's configured {{ExceptionHandler}}. The
> message stays locked, the lock expires, Service Bus redelivers it, the route
> fails the same way - and this repeats until {{maxDeliveryCount}} is reached,
> at which point the broker dead-letters it itself with reason
> {{MaxDeliveryCountExceeded}}. The outcome is that the diagnostics
> {{enableDeadLettering}} exists to provide are exactly what gets lost, and the
> exchange is processed several more times than intended.
> {{onComplete}} has the same unguarded shape around
> {{messageContext.complete()}}, which is worth deciding on at the same time.
> h3. The unbounded fields
> {code:java}
> deadLetterOptions.setDeadLetterReason(
> String.format("%s: %s", cause.getClass().getName(),
> cause.getMessage()));
> deadLetterOptions.setDeadLetterErrorDescription(Arrays.stream(cause.getStackTrace())
> .map(StackTraceElement::toString)
> .collect(Collectors.joining("\n")));
> {code}
> To be clear, the *content* here is what Azure asks for. The dead-letter queue
> documentation says:
> {quote}We recommend that you include the type of the exception in the
> {{DeadLetterReason}} and the stack trace of the exception in the
> {{DeadLetterDescription}} as it makes it easier to troubleshoot the cause of
> the problem resulting in messages being dead-lettered. It might result in
> some messages exceeding the 256 KB quota limit for the Standard tier of Azure
> Service Bus.{quote}
> So the component follows the recommendation but not the caveat: a full Camel
> stack trace, with error-handler and processor frames, is routinely several
> thousand characters, and {{cause.getMessage()}} is itself unbounded (a
> database error quoting the statement, a validation report). Nothing truncates
> either one, and {{com.azure.messaging.servicebus.models.DeadLetterOptions}}
> does no client-side length validation (checked against
> azure-messaging-servicebus 7.17.16), so the broker is what rejects it -
> landing on problem 1 above.
> h3. Suggested fix
> Bound both fields, keeping the recommended content but capping it, and guard
> the dead-letter call so a rejection is reported through the exception handler
> rather than escaping {{onFailure}}. The head of a stack trace is the useful
> part; the tail is error-handler frames.
> h3. Evidence
> * Verified by reading the component on main: no {{catch}} around
> {{deadLetter}}, neither field bounded.
> * Verified that {{DeadLetterOptions}} has no length validation in the SDK.
> * The 256 KB Standard-tier caveat is quoted from Microsoft's dead-letter
> queue documentation above.
> * *Not* reproduced against a live namespace. The exact payload size at which
> the broker rejects the request has not been measured, and the documentation
> gives a message-size quota rather than a per-field limit.
> h4. Correction to the original description
> The first version of this issue claimed a documented 4096 limit on the
> dead-letter reason and description. That came from the .NET
> {{DeadLetterMessageAsync}} API reference and is *not* corroborated by the
> dead-letter queue page or the Service Bus quotas page, so I am retracting it.
> The documented constraint is the 256 KB message-size quota on the Standard
> tier, quoted above. The unguarded call - the more serious half - is
> unaffected either way, since it is verified in Camel's own code.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
