🐛 Use constant-time comparison for management API shared key auth

The management API shared-key-auth middleware was using the standard = operator for key comparison, which is vulnerable to timing attacks. The RPC middleware already uses constant-time comparison via MessageDigest/isEqual.

This change:
- Makes constant-time-eq? public in app.http.middleware
- Updates app.http.management/shared-key-auth to use mw/constant-time-eq?
- Fixes an inconsistency where the nil-key branch returned a 2-arg function
- Adds comprehensive tests for the management shared-key-auth middleware

Closes #11426

AI-assisted-by: qwen3.7-plus

backend/src/app/http/management.clj

@@ -13,6 +13,7 @@
    [app.common.time :as ct]
    [app.config :as cf]
    [app.db :as db]
+   [app.http.middleware :as mw]
    [app.main :as-alias main]
    [app.rpc.commands.profile :as cmd.profile]
    [app.setup :as-alias setup]
@@ -57,11 +58,11 @@
        (if key
          (fn [request]
            (if-let [key' (yreq/get-header request "x-shared-key")]
-             (if (= key key')
+             (if (mw/constant-time-eq? key key')
                (handler request)
                {::yres/status 403})
              {::yres/status 403}))
-         (fn [_ _]
+         (fn [_]
            {::yres/status 403}))))})
 
 (defmethod ig/init-key ::routes

backend/src/app/http/middleware.clj

@@ -330,7 +330,7 @@
   {:name ::auth
    :compile (constantly wrap-auth)})
 
-(defn- constant-time-eq?
+(defn constant-time-eq?
   "Compare strings in constant time to prevent timing attacks."
   [^String a ^String b]
   (MessageDigest/isEqual (.getBytes a "UTF-8") (.getBytes b "UTF-8")))
@@ -350,7 +350,7 @@
                 (handler))
             {::yres/status 403}))
         {::yres/status 403}))
-    (fn [_ _]
+    (fn [_]
       {::yres/status 403})))
 
 (def shared-key-auth

backend/test/backend_tests/helpers.clj

@@ -627,3 +627,66 @@
             (parse-sse (slurp' input)))
       (finally
         (.close input)))))
+
+;; ---- Dummy Request Helpers
+
+(defrecord DummyRequest [headers cookies method body-stream
+                         remote-addr server-name server-port
+                         scheme protocol path query ssl-client-cert]
+  yrq/IRequestCookies
+  (get-cookie [_ name]
+    {:value (get cookies name)})
+
+  yrq/IRequest
+  (get-header [_ name]
+    (get headers name))
+  (method [_] method)
+  (body [_] body-stream)
+  (path [_] path)
+  (query [_] query)
+  (server-port [_] server-port)
+  (server-name [_] server-name)
+  (remote-addr [_] remote-addr)
+  (ssl-client-cert [_] ssl-client-cert)
+  (scheme [_] scheme)
+  (protocol [_] protocol))
+
+(defn make-dummy-request
+  "Constructs a DummyRequest from an options map. Every key is
+  optional; missing values fall back to sensible defaults. New
+  fields added to DummyRequest won't break existing call sites
+  as long as this constructor keeps its `:or` defaults in sync.
+
+  Recognized keys:
+    :headers         — map of header name → value
+    :cookies         — map of cookie name → value
+    :method          — HTTP method keyword (default :get)
+    :body-stream     — InputStream for the body (used directly)
+    :body-bytes      — bytes or string for the body; wrapped in a
+                       ByteArrayInputStream if :body-stream is not
+                       given
+    :remote-addr     — string (default \"127.0.0.1\")
+    :server-name     — string (default \"test\")
+    :server-port     — long   (default 0)
+    :scheme          — keyword (default :http)
+    :protocol        — string (default \"HTTP/1.1\")
+    :path            — string (default \"/test\")
+    :query           — string or nil (default nil)
+    :ssl-client-cert — X509Certificate or nil (default nil)"
+  [{:keys [headers cookies method body-stream body-bytes
+           remote-addr server-name server-port scheme protocol
+           path query ssl-client-cert]
+    :or   {headers {} cookies {} method :get
+           body-stream nil
+           remote-addr "127.0.0.1" server-name "test" server-port 0
+           scheme :http protocol "HTTP/1.1" path "/test" query nil
+           ssl-client-cert nil}}]
+  (let [body-stream (or body-stream
+                        (when body-bytes
+                          (java.io.ByteArrayInputStream.
+                           (if (string? body-bytes)
+                             (.getBytes ^String body-bytes "UTF-8")
+                             body-bytes))))]
+    (->DummyRequest headers cookies method body-stream
+                    remote-addr server-name server-port
+                    scheme protocol path query ssl-client-cert)))

backend/test/backend_tests/http_management_test.clj

@@ -78,3 +78,52 @@
 
       (let [subs' (-> response ::yres/body :subscription)]
         (t/is (= subs' subs))))))
+
+;; ---- Shared Key Auth Middleware Tests
+
+(t/deftest shared-key-auth-middleware
+  (let [;; The shared-key-auth middleware is private, so we access it via var
+        middleware-spec  @#'mgmt/shared-key-auth
+        compile-fn       (:compile middleware-spec)
+        make-middleware  (compile-fn nil nil)
+        handler          (fn [req] {::yres/status 200})
+        configured-key   "secret-management-key"]
+
+    ;; Test 1: Request with no x-shared-key header should be rejected (403)
+    (let [middleware (make-middleware handler configured-key)
+          response   (middleware (th/make-dummy-request {}))]
+      (t/is (= 403 (::yres/status response))))
+
+    ;; Test 2: Request with wrong key should be rejected (403)
+    (let [middleware (make-middleware handler configured-key)
+          response   (middleware (th/make-dummy-request {:headers {"x-shared-key" "wrong-key"}}))]
+      (t/is (= 403 (::yres/status response))))
+
+    ;; Test 3: Request with correct key should pass (200)
+    (let [middleware (make-middleware handler configured-key)
+          response   (middleware (th/make-dummy-request {:headers {"x-shared-key" configured-key}}))]
+      (t/is (= 200 (::yres/status response))))
+
+    ;; Test 4: When no key is configured, all requests should be rejected (403)
+    (let [middleware (make-middleware handler nil)
+          response   (middleware (th/make-dummy-request {:headers {"x-shared-key" "any-key"}}))]
+      (t/is (= 403 (::yres/status response))))
+
+    ;; Test 5: Keys differing only in the last character must still be rejected
+    (let [middleware (make-middleware handler "secret-key-12345")
+          response1  (middleware (th/make-dummy-request {:headers {"x-shared-key" "secret-key-1234X"}}))
+          response2  (middleware (th/make-dummy-request {:headers {"x-shared-key" "secret-key-12345"}}))]
+      (t/is (= 403 (::yres/status response1)))
+      (t/is (= 200 (::yres/status response2))))
+
+    ;; Test 6: Empty string in header must be rejected when configured key is non-empty
+    (let [middleware (make-middleware handler "secret-key")
+          response   (middleware (th/make-dummy-request {:headers {"x-shared-key" ""}}))]
+      (t/is (= 403 (::yres/status response))))
+
+    ;; Test 7: Empty string as configured key (truthy but empty) must reject all requests
+    (let [middleware (make-middleware handler "")
+          response1  (middleware (th/make-dummy-request {:headers {"x-shared-key" "any-key"}}))
+          response2  (middleware (th/make-dummy-request {:headers {"x-shared-key" ""}}))]
+      (t/is (= 403 (::yres/status response1)))
+      (t/is (= 200 (::yres/status response2))))))

backend/test/backend_tests/http_middleware_test.clj

@@ -30,78 +30,17 @@
 (t/use-fixtures :once th/state-init)
 (t/use-fixtures :each th/database-reset)
 
-(defrecord DummyRequest [headers cookies method body-stream
-                         remote-addr server-name server-port
-                         scheme protocol path query ssl-client-cert]
-  yreq/IRequestCookies
-  (get-cookie [_ name]
-    {:value (get cookies name)})
-
-  yreq/IRequest
-  (get-header [_ name]
-    (get headers name))
-  (method [_] method)
-  (body [_] body-stream)
-  (path [_] path)
-  (query [_] query)
-  (server-port [_] server-port)
-  (server-name [_] server-name)
-  (remote-addr [_] remote-addr)
-  (ssl-client-cert [_] ssl-client-cert)
-  (scheme [_] scheme)
-  (protocol [_] protocol))
-
-(defn- make-dummy-request
-  "Constructs a DummyRequest from an options map. Every key is
-  optional; missing values fall back to sensible defaults. New
-  fields added to DummyRequest won't break existing call sites
-  as long as this constructor keeps its `:or` defaults in sync.
-
-  Recognized keys:
-    :headers         — map of header name → value
-    :cookies         — map of cookie name → value
-    :method          — HTTP method keyword (default :get)
-    :body-stream     — InputStream for the body (used directly)
-    :body-bytes      — bytes or string for the body; wrapped in a
-                       ByteArrayInputStream if :body-stream is not
-                       given
-    :remote-addr     — string (default \"127.0.0.1\")
-    :server-name     — string (default \"test\")
-    :server-port     — long   (default 0)
-    :scheme          — keyword (default :http)
-    :protocol        — string (default \"HTTP/1.1\")
-    :path            — string (default \"/test\")
-    :query           — string or nil (default nil)
-    :ssl-client-cert — X509Certificate or nil (default nil)"
-  [{:keys [headers cookies method body-stream body-bytes
-           remote-addr server-name server-port scheme protocol
-           path query ssl-client-cert]
-    :or   {headers {} cookies {} method :get
-           body-stream nil
-           remote-addr "127.0.0.1" server-name "test" server-port 0
-           scheme :http protocol "HTTP/1.1" path "/test" query nil
-           ssl-client-cert nil}}]
-  (let [body-stream (or body-stream
-                        (when body-bytes
-                          (java.io.ByteArrayInputStream.
-                           (if (string? body-bytes)
-                             (.getBytes ^String body-bytes "UTF-8")
-                             body-bytes))))]
-    (->DummyRequest headers cookies method body-stream
-                    remote-addr server-name server-port
-                    scheme protocol path query ssl-client-cert)))
-
 (t/deftest auth-middleware-1
   (let [request (volatile! nil)
         handler (#'app.http.middleware/wrap-auth
                  (fn [req] (vreset! request req))
                  {})]
 
-    (handler (make-dummy-request {}))
+    (handler (th/make-dummy-request {}))
 
     (t/is (nil? (::http/auth-data @request)))
 
-    (handler (make-dummy-request {:headers {"authorization" "Token aaaa"}}))
+    (handler (th/make-dummy-request {:headers {"authorization" "Token aaaa"}}))
 
     (let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)]
       (t/is (= :token token-type))
@@ -114,10 +53,10 @@
                  (fn [req] (vreset! request req))
                  {})]
 
-    (handler (make-dummy-request {}))
+    (handler (th/make-dummy-request {}))
     (t/is (nil? (::http/auth-data @request)))
 
-    (handler (make-dummy-request {:headers {"authorization" "Bearer aaaa"}}))
+    (handler (th/make-dummy-request {:headers {"authorization" "Bearer aaaa"}}))
 
     (let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)]
       (t/is (= :bearer token-type))
@@ -130,10 +69,10 @@
                  (fn [req] (vreset! request req))
                  {})]
 
-    (handler (make-dummy-request {}))
+    (handler (th/make-dummy-request {}))
     (t/is (nil? (::http/auth-data @request)))
 
-    (handler (make-dummy-request {:cookies {"auth-token" "foobar"}}))
+    (handler (th/make-dummy-request {:cookies {"auth-token" "foobar"}}))
 
     (let [{:keys [token claims] token-type :type} (get @request ::http/auth-data)]
       (t/is (= :cookie token-type))
@@ -145,16 +84,16 @@
                  (fn [req] {::yres/status 200})
                  {:test1 "secret-key"})]
 
-    (let [response (handler (make-dummy-request {}))]
+    (let [response (handler (th/make-dummy-request {}))]
       (t/is (= 403 (::yres/status response))))
 
-    (let [response (handler (make-dummy-request {:headers {"x-shared-key" "secret-key2"}}))]
+    (let [response (handler (th/make-dummy-request {:headers {"x-shared-key" "secret-key2"}}))]
       (t/is (= 403 (::yres/status response))))
 
-    (let [response (handler (make-dummy-request {:headers {"x-shared-key" "secret-key"}}))]
+    (let [response (handler (th/make-dummy-request {:headers {"x-shared-key" "secret-key"}}))]
       (t/is (= 403 (::yres/status response))))
 
-    (let [response (handler (make-dummy-request {:headers {"x-shared-key" "test1 secret-key"}}))]
+    (let [response (handler (th/make-dummy-request {:headers {"x-shared-key" "test1 secret-key"}}))]
       (t/is (= 200 (::yres/status response))))))
 
 (t/deftest access-token-authz
@@ -265,7 +204,7 @@
                                                        :user-agent "user agent"})
                       (#'session/assign-token cfg))
 
-        response (handler (make-dummy-request {:cookies {"auth-token" (:token session)}}))
+        response (handler (th/make-dummy-request {:cookies {"auth-token" (:token session)}}))
 
         {:keys [token claims] token-type :type}
         (get response ::http/auth-data)]
@@ -292,7 +231,7 @@
         ;; value with a backslash followed by '}', which
         ;; clojure.data.json v0.5.x cannot handle.
         body    (.getBytes "{\"x\": \"\\}\"}" "UTF-8")
-        request (make-dummy-request
+        request (th/make-dummy-request
                  {:method  :post
                   :headers {"content-type" "application/json"}
                   :body-bytes body})
@@ -311,7 +250,7 @@
   ;; error.
   (let [handler (#'app.http.middleware/wrap-parse-request
                  (fn [_] (throw (RequestTooBigException. "too large"))))
-        request (make-dummy-request
+        request (th/make-dummy-request
                  {:method  :post
                   :headers {"content-type" "application/json"}
                   :body-bytes (.getBytes "{}" "UTF-8")})
@@ -329,7 +268,7 @@
   ;; should convert it to a 400 :malformed-json validation error.
   (let [handler (#'app.http.middleware/wrap-parse-request
                  (fn [_] (throw (java.io.EOFException. "stream closed"))))
-        request (make-dummy-request
+        request (th/make-dummy-request
                  {:method  :post
                   :headers {"content-type" "application/json"}
                   :body-bytes (.getBytes "{}" "UTF-8")})
@@ -352,7 +291,7 @@
                   (.initCause iae))
         handler (#'app.http.middleware/wrap-parse-request
                  (fn [_] (throw wrapped)))
-        request (make-dummy-request
+        request (th/make-dummy-request
                  {:method  :post
                   :headers {"content-type" "application/json"}
                   :body-bytes (.getBytes "{}" "UTF-8")})
@@ -370,7 +309,7 @@
   ;; :unexpected. This is the "true internal error" path.
   (let [handler  (#'app.http.middleware/wrap-parse-request
                   (fn [_] (throw (RuntimeException. "boom"))))
-        request  (make-dummy-request
+        request  (th/make-dummy-request
                   {:method  :post
                    :headers {"content-type" "application/json"}
                    :body-bytes (.getBytes "{}" "UTF-8")})
@@ -390,7 +329,7 @@
   ;; with :code :io-exception.
   (let [handler  (#'app.http.middleware/wrap-parse-request
                   (fn [_] (throw (java.io.IOException. "network gone"))))
-        request  (make-dummy-request
+        request  (th/make-dummy-request
                   {:method  :post
                    :headers {"content-type" "application/json"}
                    :body-bytes (.getBytes "{}" "UTF-8")})