yada
yada copied to clipboard
juxt
Implementation of `verify` multimethod for "Basic" auth scheme is buggy
Implementation of yada.security/verify multimethod for Basic auth scheme is buggy. It assumes that any Authorization header is destined for it and does not properly check if the header value starts with "Basic" or not. This leads to null pointer exception when multiple authentication schemes are defined for a resource, both of which rely on the same header.
Test code:
;; Modified from `test/yada/authentication_test.clj`.
(ns basicauth-bug-test
(:require
[clojure.test :refer :all :exclude [deftest run-tests]]
[clojure.string :as str]
[schema.core :as s]
[schema.test :refer [deftest]]
[yada.schema :as ys]
[yada.security :refer [authenticate verify]]))
;; Dummy Bearer scheme.
;; ------------------------------------------------------,
(defn verify-bearer
[token]
(when (= token "secret-token")
{:user "foo"}))
(defmethod verify "Bearer"
[ctx {:keys [verify]}]
(some-> (get-in ctx [:request :headers "authorization"])
(str/split #"\s+" 2)
(get 1)
(verify)))
;; ------------------------------------------------------'
(deftest authenticate_test
(testing "Testing with correct bearer token."
(let [ctx {:request {:headers {"authorization" "Bearer secret-token"}}
:resource
{:access-control
{:realms
{"Default" {:authentication-schemes
[{:scheme "Bearer"
:verify verify-bearer}
{:scheme "Basic"
:verify (constantly false)}]}}}}}
result (authenticate ctx)]
(is (= {"Default" {:user "foo"}}
(:authentication result)))))
(testing "Testing with incorrect bearer token."
(let [ctx {:request {:headers {"authorization" "Bearer not-secret-token"}}
:resource
{:access-control
{:realms
{"Default" {:authentication-schemes
[{:scheme "Bearer"
:verify verify-bearer}
{:scheme "Basic"
:verify (constantly false)}]}}}}}
result (authenticate ctx)]
(is (= nil
(:authentication result))))))
Testing the code in lein repl inside yada project dir yields:
user=> (require '[clojure.test :refer [run-tests]])
nil
user=> (load-file "basicauth_bug_test.clj")
#'basicauth-bug-test/authenticate_test
user=> (run-tests 'basicauth-bug-test)
Testing basicauth-bug-test
ERROR in (authenticate_test) (security.clj:17)
Uncaught exception, not in assertion.
expected: nil
actual: java.lang.NullPointerException: null
at yada.security$eval4782$fn__4784.invoke (security.clj:17)
clojure.lang.MultiFn.invoke (MultiFn.java:233)
clojure.core$partial$fn__5561.invoke (core.clj:2616)
clojure.core$some.invokeStatic (core.clj:2693)
clojure.core$some.invoke (core.clj:2684)
yada.security$authenticate$fn__4815.invoke (security.clj:96)
clojure.core.protocols$iter_reduce.invokeStatic (protocols.clj:49)
clojure.core.protocols$fn__7839.invokeStatic (protocols.clj:75)
clojure.core.protocols/fn (protocols.clj:75)
clojure.core.protocols$fn__7781$G__7776__7794.invoke (protocols.clj:13)
clojure.core$reduce.invokeStatic (core.clj:6748)
clojure.core$reduce.invoke (core.clj:6730)
yada.security$authenticate.invokeStatic (security.clj:66)
yada.security$authenticate.invoke (security.clj:63)
basicauth_bug_test$fn__4980$fn4981__4990$fn__4991.invoke (basicauth_bug_test.clj:51)
basicauth_bug_test$fn__4980$fn4981__4990.invoke (basicauth_bug_test.clj:25)
clojure.lang.AFn.applyToHelper (AFn.java:152)
clojure.lang.AFn.applyTo (AFn.java:144)
clojure.lang.AFunction$1.doInvoke (AFunction.java:29)
clojure.lang.RestFn.invoke (RestFn.java:397)
basicauth_bug_test$fn__4980.invokeStatic (basicauth_bug_test.clj:25)
basicauth_bug_test/fn (basicauth_bug_test.clj:25)
clojure.test$test_var$fn__9209.invoke (test.clj:716)
clojure.test$test_var.invokeStatic (test.clj:716)
clojure.test$test_var.invoke (test.clj:707)
clojure.test$test_vars$fn__9235$fn__9240.invoke (test.clj:734)
clojure.test$default_fixture.invokeStatic (test.clj:686)
clojure.test$default_fixture.invoke (test.clj:682)
clojure.test$test_vars$fn__9235.invoke (test.clj:734)
clojure.test$default_fixture.invokeStatic (test.clj:686)
clojure.test$default_fixture.invoke (test.clj:682)
clojure.test$test_vars.invokeStatic (test.clj:730)
clojure.test$test_all_vars.invokeStatic (test.clj:736)
clojure.test$test_ns.invokeStatic (test.clj:757)
clojure.test$test_ns.invoke (test.clj:742)
clojure.core$map$fn__5587.invoke (core.clj:2747)
clojure.lang.LazySeq.sval (LazySeq.java:40)
clojure.lang.LazySeq.seq (LazySeq.java:49)
clojure.lang.Cons.next (Cons.java:39)
clojure.lang.RT.boundedLength (RT.java:1785)
clojure.lang.RestFn.applyTo (RestFn.java:130)
clojure.core$apply.invokeStatic (core.clj:659)
clojure.test$run_tests.invokeStatic (test.clj:767)
clojure.test$run_tests.doInvoke (test.clj:767)
clojure.lang.RestFn.invoke (RestFn.java:408)
user$eval5010.invokeStatic (form-init9121862550941862138.clj:1)
user$eval5010.invoke (form-init9121862550941862138.clj:1)
clojure.lang.Compiler.eval (Compiler.java:7062)
clojure.lang.Compiler.eval (Compiler.java:7025)
clojure.core$eval.invokeStatic (core.clj:3206)
clojure.core$eval.invoke (core.clj:3202)
clojure.main$repl$read_eval_print__8572$fn__8575.invoke (main.clj:243)
clojure.main$repl$read_eval_print__8572.invoke (main.clj:243)
clojure.main$repl$fn__8581.invoke (main.clj:261)
clojure.main$repl.invokeStatic (main.clj:261)
clojure.main$repl.doInvoke (main.clj:177)
clojure.lang.RestFn.invoke (RestFn.java:1523)
clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__1142.invoke (interruptible_eval.clj:87)
clojure.lang.AFn.applyToHelper (AFn.java:152)
clojure.lang.AFn.applyTo (AFn.java:144)
clojure.core$apply.invokeStatic (core.clj:657)
clojure.core$with_bindings_STAR_.invokeStatic (core.clj:1965)
clojure.core$with_bindings_STAR_.doInvoke (core.clj:1965)
clojure.lang.RestFn.invoke (RestFn.java:425)
clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invokeStatic (interruptible_eval.clj:85)
clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invoke (interruptible_eval.clj:55)
clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn__1187$fn__1190.invoke (interruptible_eval.clj:222)
clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__1182.invoke (interruptible_eval.clj:190)
clojure.lang.AFn.run (AFn.java:22)
java.util.concurrent.ThreadPoolExecutor.runWorker (ThreadPoolExecutor.java:1135)
java.util.concurrent.ThreadPoolExecutor$Worker.run (ThreadPoolExecutor.java:635)
java.lang.Thread.run (Thread.java:844)
Ran 1 tests containing 2 assertions.
0 failures, 1 errors.
{:test 1, :pass 1, :fail 0, :error 1, :type :summary}
For reference, current buggy implementation of verify multimethod for Basic auth is:
(defmethod verify "Basic" [ctx {:keys [verify]}]
(let [auth (get-in ctx [:request :headers "authorization"])
cred (and auth (apply str (map char (base64/decode (.getBytes ^String (last (re-find #"^Basic (.*)$" auth)))))))]
(when cred
(let [[user password] (str/split (str cred) #":" 2)]
(verify [user password])))))
As we can see, the function tries to call .getBytes method of nil when re-find fails to find a proper match.
I suggest an alternate implementation:
(defmethod yada.security/verify "Basic"
[ctx {:keys [verify]}]
(let [auth (get-in ctx [:request :headers "authorization"])]
(when (and auth (str/starts-with? (str/lower-case auth) "basic"))
(let [cred-bytes (some-> auth
(str/split #"\s+" 2)
(get 1) ; `last` is intentionally avoided.
(#(.getBytes ^String %))
(base64/decode))
cred (apply str (map char cred-bytes))]
(when cred
(let [[user password] (str/split cred ":" 2)]
(verify [user password])))))))
