diff --git a/backend/src/app/http/management.clj b/backend/src/app/http/management.clj index f72c4b6d14..114dd0a8c0 100644 --- a/backend/src/app/http/management.clj +++ b/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 diff --git a/backend/src/app/http/middleware.clj b/backend/src/app/http/middleware.clj index 6cb8e6b8b7..2f8793194e 100644 --- a/backend/src/app/http/middleware.clj +++ b/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 diff --git a/backend/test/backend_tests/helpers.clj b/backend/test/backend_tests/helpers.clj index 0f582497d9..25aa8b20bc 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/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))) diff --git a/backend/test/backend_tests/http_management_test.clj b/backend/test/backend_tests/http_management_test.clj index ba114a673a..fb6f96c527 100644 --- a/backend/test/backend_tests/http_management_test.clj +++ b/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)))))) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index bca962d3fc..1843f6d53d 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/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")})