From dc2c28b9e1d01171c0dfce915d9d5b721d23ecf5 Mon Sep 17 00:00:00 2001
From: Joshua Rogers <MegaManSec@users.noreply.github.com>
Date: Mon, 8 Sep 2025 17:54:42 +0000
Subject: [PATCH] Improve how NotePairs::find() reports its results (#2175)

Replaced a problematic combination of a boolean return type and an SBuf
out-parameter with a safer and more readable optional SBuf return type.
---
 src/Notes.cc                   | 13 +++++++++----
 src/Notes.h                    |  6 +++---
 src/auth/UserRequest.cc        | 13 +++++++------
 src/auth/digest/UserRequest.cc |  5 ++---
 src/format/Format.cc           |  9 ++++-----
 5 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/src/Notes.cc b/src/Notes.cc
index 13a3368629..618b7fb2dd 100644
--- a/src/Notes.cc
+++ b/src/Notes.cc
@@ -277,10 +277,11 @@ Notes::printAsAnnotationAclParameters(std::ostream &os) const
     }
 }
 
-bool
-NotePairs::find(SBuf &resultNote, const char *noteKey, const char *sep) const
+std::optional<SBuf>
+NotePairs::find(const char *noteKey, const char *sep) const
 {
-    resultNote.clear();
+    SBuf resultNote;
+
     for (const auto &e: entries) {
         if (!e->name().cmp(noteKey)) {
             if (!resultNote.isEmpty())
@@ -288,7 +289,11 @@ NotePairs::find(SBuf &resultNote, const char *noteKey, const char *sep) const
             resultNote.append(e->value());
         }
     }
-    return resultNote.length();
+
+    if (resultNote.isEmpty())
+        return std::nullopt;
+
+    return resultNote;
 }
 
 void
diff --git a/src/Notes.h b/src/Notes.h
index f25c56bc36..2fcdf6ded0 100644
--- a/src/Notes.h
+++ b/src/Notes.h
@@ -17,6 +17,7 @@
 #include "SquidString.h"
 
 #include <iosfwd>
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -225,10 +226,9 @@ public:
     /// Entries which already exist in the destination set are ignored.
     void appendNewOnly(const NotePairs *src);
 
-    /// \param resultNote a comma separated list of notes with key 'noteKey'.
-    /// \returns true if there are entries with the given 'noteKey'.
+    /// \returns a comma separated list of notes with key 'noteKey', if any.
     /// Use findFirst() instead when a unique kv-pair is needed.
-    bool find(SBuf &resultNote, const char *noteKey, const char *sep = ",") const;
+    std::optional<SBuf> find(const char *noteKey, const char *sep = ",") const;
 
     /// \returns the first note value for this key or an empty string.
     const char *findFirst(const char *noteKey) const;
diff --git a/src/auth/UserRequest.cc b/src/auth/UserRequest.cc
index 3a20a67b5f..6a640ea710 100644
--- a/src/auth/UserRequest.cc
+++ b/src/auth/UserRequest.cc
@@ -25,6 +25,7 @@
 #include "HttpReply.h"
 #include "HttpRequest.h"
 #include "MemBuf.h"
+#include "sbuf/Stream.h"
 
 /* Generic Functions */
 
@@ -575,11 +576,11 @@ Auth::UserRequest::helperRequestKeyExtras(HttpRequest *request, AccessLogEntry::
 void
 Auth::UserRequest::denyMessageFromHelper(const char *proto, const Helper::Reply &reply)
 {
-    static SBuf messageNote;
-    if (!reply.notes.find(messageNote, "message")) {
-        messageNote.append(proto);
-        messageNote.append(" Authentication denied with no reason given");
+    auto messageNote = reply.notes.find("message");
+
+    if (!messageNote) {
+        messageNote = ToSBuf(proto, " Authentication denied with no reason given");
     }
-    setDenyMessage(messageNote.c_str());
-}
 
+    setDenyMessage(messageNote->c_str());
+}
diff --git a/src/auth/digest/UserRequest.cc b/src/auth/digest/UserRequest.cc
index b3c468ea9d..2ad3d24d1b 100644
--- a/src/auth/digest/UserRequest.cc
+++ b/src/auth/digest/UserRequest.cc
@@ -362,9 +362,8 @@ Auth::Digest::UserRequest::HandleReply(void *data, const Helper::Reply &reply)
         digest_request->user()->credentials(Auth::Failed);
         digest_request->flags.invalid_password = true;
 
-        SBuf msgNote;
-        if (reply.notes.find(msgNote, "message")) {
-            digest_request->setDenyMessage(msgNote.c_str());
+        if (auto msgNote = reply.notes.find("message")) {
+            digest_request->setDenyMessage(msgNote->c_str());
         } else if (reply.other().hasContent()) {
             // old helpers did send ERR result but a bare message string instead of message= key name.
             // TODO deprecate and remove old auth digest helper protocol
diff --git a/src/format/Format.cc b/src/format/Format.cc
index 711082379c..04efd69069 100644
--- a/src/format/Format.cc
+++ b/src/format/Format.cc
@@ -1390,19 +1390,18 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS
             tmp[1] = '\0';
             if (fmt->data.header.header && *fmt->data.header.header) {
                 const char *separator = tmp;
-                static SBuf note;
 #if USE_ADAPTATION
                 Adaptation::History::Pointer ah = al->request ? al->request->adaptHistory() : Adaptation::History::Pointer();
                 if (ah && ah->metaHeaders) {
-                    if (ah->metaHeaders->find(note, fmt->data.header.header, separator))
-                        sb.append(note);
+                    if (const auto note = ah->metaHeaders->find(fmt->data.header.header, separator))
+                        sb.append(*note);
                 }
 #endif
                 if (al->notes) {
-                    if (al->notes->find(note, fmt->data.header.header, separator)) {
+                    if (const auto note = al->notes->find(fmt->data.header.header, separator)) {
                         if (!sb.isEmpty())
                             sb.append(separator);
-                        sb.append(note);
+                        sb.append(*note);
                     }
                 }
                 out = sb.c_str();
-- 
2.47.3