From: Amish <3330468+amishmm@users.noreply.github.com>
Date: Wed, 22 May 2019 05:42:06 +0000 (+0000)
Subject: Bug 4912: same-name notes being appended instead of replaced (#393)
X-Git-Tag: SQUID_5_0_1~92
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=d665de37c2e6032068360b83b7ae372a77f5d0f4;p=thirdparty%2Fsquid.git

Bug 4912: same-name notes being appended instead of replaced (#393)

When auth helper replies with clt_conn_tag=foo (or any KV pair) and
then later in future replaces it with clt_conn_tag=bar. The new pair
was getting appended instead of getting replaced.

i.e. notes would contain two KV pairs:
clt_conn_tag=foo
clt_conn_tag=bar

This fixes it and also fixes:
https://bugs.squid-cache.org/show_bug.cgi?id=4912
---

diff --git a/src/Notes.cc b/src/Notes.cc
index d7cd274055..1e0232c17f 100644
--- a/src/Notes.cc
+++ b/src/Notes.cc
@@ -354,6 +354,16 @@ NotePairs::appendNewOnly(const NotePairs *src)
     }
 }
 
+void
+NotePairs::replaceOrAddOrAppend(const NotePairs *src, const NotePairs::Names &appendables)
+{
+    for (const auto e: src->entries) {
+        if (std::find(appendables.begin(), appendables.end(), e->name()) == appendables.end())
+            remove(e->name());
+    }
+    append(src);
+}
+
 void
 NotePairs::replaceOrAdd(const NotePairs *src)
 {
diff --git a/src/Notes.h b/src/Notes.h
index 66d38cfd36..f80031a4ae 100644
--- a/src/Notes.h
+++ b/src/Notes.h
@@ -191,6 +191,7 @@ public:
         SBuf theValue;
     };
     typedef std::vector<Entry::Pointer> Entries;      ///< The key/value pair entries
+    typedef std::vector<SBuf> Names;
 
     NotePairs() {}
     NotePairs &operator=(NotePairs const &) = delete;
@@ -199,6 +200,11 @@ public:
     /// Append the entries of the src NotePairs list to our list.
     void append(const NotePairs *src);
 
+    /// Replace existing list entries with the src NotePairs entries.
+    /// Do not replace but append entries named in the appendables
+    /// Entries which do not exist in the destination set are added.
+    void replaceOrAddOrAppend(const NotePairs *src, const Names &appendables);
+
     /// Replace existing list entries with the src NotePairs entries.
     /// Entries which do not exist in the destination set are added.
     void replaceOrAdd(const NotePairs *src);
diff --git a/src/auth/basic/UserRequest.cc b/src/auth/basic/UserRequest.cc
index 71bb717ab7..2b3fa53d78 100644
--- a/src/auth/basic/UserRequest.cc
+++ b/src/auth/basic/UserRequest.cc
@@ -168,7 +168,8 @@ Auth::Basic::UserRequest::HandleReply(void *data, const Helper::Reply &reply)
 
     // add new helper kv-pair notes to the credentials object
     // so that any transaction using those credentials can access them
-    r->auth_user_request->user()->notes.appendNewOnly(&reply.notes);
+    static const NotePairs::Names appendables = { SBuf("group"), SBuf("tag") };
+    r->auth_user_request->user()->notes.replaceOrAddOrAppend(&reply.notes, appendables);
 
     /* this is okay since we only play with the Auth::Basic::User child fields below
      * and do not pass the pointer itself anywhere */
diff --git a/src/auth/digest/UserRequest.cc b/src/auth/digest/UserRequest.cc
index 7aac197048..3d3151dcc4 100644
--- a/src/auth/digest/UserRequest.cc
+++ b/src/auth/digest/UserRequest.cc
@@ -327,7 +327,8 @@ Auth::Digest::UserRequest::HandleReply(void *data, const Helper::Reply &reply)
 
     // add new helper kv-pair notes to the credentials object
     // so that any transaction using those credentials can access them
-    auth_user_request->user()->notes.appendNewOnly(&reply.notes);
+    static const NotePairs::Names appendables = { SBuf("group"), SBuf("nonce"), SBuf("tag") };
+    auth_user_request->user()->notes.replaceOrAddOrAppend(&reply.notes, appendables);
     // remove any private credentials detail which got added.
     auth_user_request->user()->notes.remove("ha1");
 
diff --git a/src/auth/negotiate/UserRequest.cc b/src/auth/negotiate/UserRequest.cc
index 168d8cacd0..568fd33e5a 100644
--- a/src/auth/negotiate/UserRequest.cc
+++ b/src/auth/negotiate/UserRequest.cc
@@ -278,7 +278,8 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const Helper::Reply &reply
 
     // add new helper kv-pair notes to the credentials object
     // so that any transaction using those credentials can access them
-    auth_user_request->user()->notes.appendNewOnly(&reply.notes);
+    static const NotePairs::Names appendables = { SBuf("group"), SBuf("tag") };
+    auth_user_request->user()->notes.replaceOrAddOrAppend(&reply.notes, appendables);
     // remove any private credentials detail which got added.
     auth_user_request->user()->notes.remove("token");
 
diff --git a/src/auth/ntlm/UserRequest.cc b/src/auth/ntlm/UserRequest.cc
index 2edbeb59b7..0115d70f0a 100644
--- a/src/auth/ntlm/UserRequest.cc
+++ b/src/auth/ntlm/UserRequest.cc
@@ -272,7 +272,8 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const Helper::Reply &reply)
 
     // add new helper kv-pair notes to the credentials object
     // so that any transaction using those credentials can access them
-    auth_user_request->user()->notes.appendNewOnly(&reply.notes);
+    static const NotePairs::Names appendables = { SBuf("group"), SBuf("tag") };
+    auth_user_request->user()->notes.replaceOrAddOrAppend(&reply.notes, appendables);
     // remove any private credentials detail which got added.
     auth_user_request->user()->notes.remove("token");