Merge pull request #781 from njlr/bugfix/issue-780-safe-decode-invali…

…d-tokens

bugfix/issue-780-safe-decode-invalid-tokens

.github/workflows/build-suave.yml

@@ -21,6 +21,6 @@ jobs:
     - name: Setup .NET
       uses: actions/setup-dotnet@v3
       with:
-        dotnet-version: 7.0.x
+        dotnet-version: 7.0.200
     - name: Build
       run: ./build.sh

src/Suave.Tests/HttpAuthentication.fs

@@ -39,33 +39,34 @@ let authTests cfg =
       GET >=> path "/non-protected2" >=> okUser ]
 
   testList "basic authetication" [
-    testCase "add username to userstate for protectedPart only" <| fun _ ->
-
-      let req path reqMod = reqResp HttpMethod.GET path "" None None DecompressionMethods.None reqMod (fun res -> res.Content.ReadAsStringAsync().Result)
+    let req path reqMod = reqResp HttpMethod.GET path "" None None DecompressionMethods.None reqMod (fun res -> int res.StatusCode, res.Content.ReadAsStringAsync().Result)
 
-      let res = runWithConfig app |> req "/non-protected1" id
+    testCase "add username to userstate for protectedPart only" <| fun _ ->
+      let _, res = runWithConfig app |> req "/non-protected1" id
       Expect.equal res "no user" "access to /non-protected1 should result in no username"
 
-      let res = runWithConfig app |> req "/non-protected2" id
+      let _, res = runWithConfig app |> req "/non-protected2" id
       Expect.equal res "no user" "access to /non-protected2 should result in no username"
 
-      let res = runWithConfig app |> req "/protected" (fun reqmsg -> reqmsg.Headers.Authorization <- AuthenticationHeaderValue("Basic", basicCredentials); reqmsg)
+      let _, res = runWithConfig app |> req "/protected" (fun reqmsg -> reqmsg.Headers.Authorization <- AuthenticationHeaderValue("Basic", basicCredentials); reqmsg)
       Expect.equal res "hello foo" "should be username"
 
-      let res = runWithConfig app |> req "/non-protected1" id
+      let _, res = runWithConfig app |> req "/non-protected1" id
       Expect.equal res "no user" "access to /non-protected1 should result in no username (2)"
 
-      let res = runWithConfig app |> req "/non-protected2" id
+      let _, res = runWithConfig app |> req "/non-protected2" id
       Expect.equal res "no user" "access to /non-protected2 should result in no username (2)"
 
+    testCase "invalid Authorization token leads to challenge" <| fun _ ->
+      let code, _ = runWithConfig app |> req "/protected" (fun reqmsg -> reqmsg.Headers.Authorization <- AuthenticationHeaderValue("Basic", "totallyinvalid"); reqmsg)
+      Expect.equal code 401 "should be challenged"
+
     testCase "add username to userstate for protectedPart (async)" <| fun _ ->
 
       let authenticate credentials = async { return credentials = ("foo", "bar") }
       let app = GET >=> authenticateBasicAsync authenticate okUser
 
-      let req path reqMod = reqResp HttpMethod.GET path "" None None DecompressionMethods.None reqMod (fun res -> res.Content.ReadAsStringAsync().Result)
-
-      let res = runWithConfig app |> req "/protected" (fun reqmsg -> reqmsg.Headers.Authorization <- AuthenticationHeaderValue("Basic", basicCredentials); reqmsg)
+      let _, res = runWithConfig app |> req "/protected" (fun reqmsg -> reqmsg.Headers.Authorization <- AuthenticationHeaderValue("Basic", basicCredentials); reqmsg)
       Expect.equal res "hello foo" "should be username"
 
     ]

src/Suave/Authentication.fs

@@ -12,12 +12,22 @@ open Suave.Operators
 
 let UserNameKey = "userName"
 
-let internal parseAuthenticationToken (token : string) =
-  let parts = token.Split (' ')
-  let enc = parts.[1].Trim()
-  let decoded = ASCII.decodeBase64 enc
-  let indexOfColon = decoded.IndexOf(':')
-  (parts.[0].ToLower(), decoded.Substring(0,indexOfColon), decoded.Substring(indexOfColon+1))
+let internal tryParseBasicAuthenticationToken (rawHeader : string) =
+  match rawHeader.Split(' ') with
+  | [| basic; tokenBase64 |] when String.equalsOrdinalCI "basic" basic ->
+    match ASCII.tryDecodeBase64 tokenBase64 with
+    | Some token ->
+      match token.IndexOf(':') with
+      | -1 ->
+        None
+      | i ->
+        let username = token.Substring(0, i)
+        let password = token.Substring(i + 1)
+        Some (username, password)
+    | None ->
+      None
+  | _ ->
+    None
 
 let inline private addUserName username ctx =
   if ctx.userState.ContainsKey UserNameKey then
@@ -31,14 +41,15 @@ let authenticateBasicAsync f protectedPart ctx =
     let p = ctx.request
     match p.header "authorization" with
     | Choice1Of2 header ->
-      let (typ, username, password) = parseAuthenticationToken header
-      if (typ.Equals("basic")) then
+      match tryParseBasicAuthenticationToken header with
+      | Some (username, password) ->
           let! authenticated = f (username, password)
           if authenticated then
             return! protectedPart (addUserName username ctx)
           else
             return! challenge ctx
-      else return! challenge ctx
+      | None ->
+          return! challenge ctx
     | Choice2Of2 _ ->
       return! challenge ctx
   }

src/Suave/Utils/ASCII.fs

@@ -40,3 +40,14 @@ let encodeBase64 (s : string) =
 let decodeBase64 (s : string) =
   let bytes = Convert.FromBase64String s
   Encoding.ASCII.GetString bytes
+
+/// Try to convert the passed string `s`, which may be a valid Base64 encoding, to a
+/// CLR string, going through ASCII.
+let tryDecodeBase64 (s : string) =
+  try
+    let bytes = Convert.FromBase64String s
+    Encoding.ASCII.GetString bytes
+    |> Some
+  with
+  | :? FormatException ->
+    None