Skip to content

Fix[mqb]: improve non-printable handling in negotiation/session logs - #1675

Merged
chrisbeard merged 1 commit into
bloomberg:mainfrom
chrisbeard:fix/sanitize-session-negotiation
Jul 31, 2026
Merged

Fix[mqb]: improve non-printable handling in negotiation/session logs#1675
chrisbeard merged 1 commit into
bloomberg:mainfrom
chrisbeard:fix/sanitize-session-negotiation

Conversation

@chrisbeard

Copy link
Copy Markdown
Contributor

Restrict negotiation messages (and derived fields) to printable characters in logs.

Additionally, add a BSLS_REVIEW check for negotiation messages containing non-printable characters in anticipation of enforcing that string fields are limited to printable ASCII characters.

@chrisbeard
chrisbeard requested a review from a team as a code owner July 31, 2026 17:19
Comment thread src/groups/bmq/bmqu/bmqu_stringutil.cpp Outdated
Comment thread src/groups/bmq/bmqu/bmqu_stringutil.t.cpp Outdated

/// Return `true` if every character in the specified string `str` is
/// printable, and `false` otherwise. An empty string is printable.
static bool isPrintable(const bslstl::StringRef& str);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were going to use bsl::string_view from now on.
Worth changing in the future

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All methods in this class using StringRef, I'll save that migration for another PR.

template <class TYPE>
bsl::string logSafe(const TYPE& obj)
{
bmqu::MemOutStream os;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might use local sequential allocator to ensure no heap allocations in most cases

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed convention in this file, most places don't use it for logging.

template <class TYPE>
bool isPrintable(const TYPE& obj)
{
bmqu::MemOutStream os;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Local sequential allocator might be used here too


// Detect non-printable characters in negotiation messages.
// TODO: fail negotiation if message contains non-printable characters
BSLS_REVIEW_OPT(isPrintable(negotiationContext->negotiationMessage()));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If negotiationMessage is printable, it automatically means that its client identity is also printable. This means that the following safe logs are not needed: logSafe(clientIdentity)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know if it's printable, that's why we have to sanitize until we reach the end goal of rejecting negotiation if not printable.

@chrisbeard
chrisbeard force-pushed the fix/sanitize-session-negotiation branch from 628056d to 9a8a574 Compare July 31, 2026 19:14
Restrict negotiation messages (and derived fields) to printable
characters in logs.

Additionally, add a BSLS_REVIEW check for negotiation messages
containing non-printable characters in anticipation of enforcing that
string fields are limited to printable ASCII characters.

Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
@chrisbeard
chrisbeard force-pushed the fix/sanitize-session-negotiation branch from 9a8a574 to 54316eb Compare July 31, 2026 19:23
@chrisbeard
chrisbeard requested a review from 678098 July 31, 2026 20:45
@chrisbeard
chrisbeard merged commit a495f02 into bloomberg:main Jul 31, 2026
53 checks passed
@chrisbeard
chrisbeard deleted the fix/sanitize-session-negotiation branch August 3, 2026 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants