From 77fab743390994928347bcf09713f5f43b0b1ab0 Mon Sep 17 00:00:00 2001 From: Paulo Valente Date: Thu, 11 Jul 2019 15:36:10 -0300 Subject: [PATCH 1/7] fix: resolve Base.decode64 {:ok, :error} bleedtrough --- lib/joken.ex | 4 +++- test/joken_test.exs | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/lib/joken.ex b/lib/joken.ex index a3d4da1..4da5b8b 100644 --- a/lib/joken.ex +++ b/lib/joken.ex @@ -129,10 +129,12 @@ defmodule Joken do @spec peek_header(bearer_token) :: {:ok, claims} | {:error, error_reason} def peek_header(token) when is_binary(token) do with {:ok, %{"protected" => protected}} <- expand(token), - {:ok, decoded_str} <- Base.url_decode64(protected, padding: false), + {:decode64, {:ok, decoded_str}} <- + {:decode64, Base.url_decode64(protected, padding: false)}, header <- JOSE.json_module().decode(decoded_str) do {:ok, header} else + {:decode64, _error} -> {:error, :improper_token} error -> error end end diff --git a/test/joken_test.exs b/test/joken_test.exs index 2109fa2..fc5a5d9 100644 --- a/test/joken_test.exs +++ b/test/joken_test.exs @@ -161,4 +161,39 @@ defmodule JokenTest do assert token = Joken.generate_and_sign!(%{}, %{"some" => custom_claim}, signer) assert Joken.peek_claims(token) == {:ok, %{"some" => custom_claim}} end + + test "verify_and_validate, peek_header and peek_claims give proper error upon improper token, instead of raising" do + # This test ensures that peek_header and peek_claims use Base.url_decode64 properly + + defmodule HeaderHook do + use Joken.Hooks + + @impl true + def before_verify(_options, {token, signer}) do + with {:ok, _} <- Joken.peek_header(token) do + {:cont, {token, signer}} + else + error -> {:halt, error} + end + end + end + + defmodule ClaimsHook do + use Joken.Hooks + + @impl true + def before_verify(_options, {token, signer}) do + with {:ok, _} <- Joken.peek_header(token) do + {:cont, {token, signer}} + else + error -> {:halt, error} + end + end + end + + signer = Joken.Signer.create("HS256", "secret") + + assert {:error, :improper_token} = Joken.verify("a.a.a", signer, [{HeaderHook, []}]) + assert {:error, :improper_token} = Joken.verify("a.a.a", signer, [{ClaimsHook, []}]) + end end From fa68c605b6ae0bb2a59484abc4a5fb8de0f8e920 Mon Sep 17 00:00:00 2001 From: Paulo Valente Date: Thu, 11 Jul 2019 15:36:24 -0300 Subject: [PATCH 2/7] docs: update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5657ab..f2d491f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Changed ### Fixed +- (@polvalente) Fix issue where Base.decode64 :error return could bleed trough as {:ok, :error} when using pre_verification hooks + ## [2.1.0] - 2019-05-27 ### Added From ad76cc55fb1d6c04efb6fb30c3ff49e5a26c8852 Mon Sep 17 00:00:00 2001 From: Paulo Valente Date: Thu, 11 Jul 2019 15:43:34 -0300 Subject: [PATCH 3/7] docs: update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f2d491f..931f9f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ ### Changed ### Fixed -- (@polvalente) Fix issue where Base.decode64 :error return could bleed trough as {:ok, :error} when using pre_verification hooks +- (@polvalente) Fix issue where Base.decode64 :error return could bleed trough as {:ok, :error} (#237) ## [2.1.0] - 2019-05-27 From 32d43b9bb6be4ec7518c67d215e29b9878ce79af Mon Sep 17 00:00:00 2001 From: Paulo Valente Date: Thu, 11 Jul 2019 15:48:45 -0300 Subject: [PATCH 4/7] fix: copy-pasted code is now corrected --- lib/joken.ex | 4 +++- test/joken_test.exs | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/joken.ex b/lib/joken.ex index 4da5b8b..e3d129c 100644 --- a/lib/joken.ex +++ b/lib/joken.ex @@ -150,10 +150,12 @@ defmodule Joken do @spec peek_claims(bearer_token) :: {:ok, claims} | {:error, error_reason} def peek_claims(token) when is_binary(token) do with {:ok, %{"payload" => payload}} <- expand(token), - {:ok, decoded_str} <- Base.url_decode64(payload, padding: false), + {:decode64, {:ok, decoded_str}} <- + {:decode64, Base.url_decode64(payload, padding: false)}, claims <- JOSE.json_module().decode(decoded_str) do {:ok, claims} else + {:decode64, _error} -> {:error, :improper_token} error -> error end end diff --git a/test/joken_test.exs b/test/joken_test.exs index fc5a5d9..1c67c0d 100644 --- a/test/joken_test.exs +++ b/test/joken_test.exs @@ -183,7 +183,7 @@ defmodule JokenTest do @impl true def before_verify(_options, {token, signer}) do - with {:ok, _} <- Joken.peek_header(token) do + with {:ok, _} <- Joken.peek_claims(token) do {:cont, {token, signer}} else error -> {:halt, error} From 764ee62d2501b0c15f79673969266529ae2cfeb0 Mon Sep 17 00:00:00 2001 From: Paulo Valente Date: Thu, 11 Jul 2019 15:57:31 -0300 Subject: [PATCH 5/7] refactor: simplify tests to deal with root cause --- CHANGELOG.md | 2 +- test/joken_test.exs | 35 +++-------------------------------- 2 files changed, 4 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 931f9f9..1f0492b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ ### Changed ### Fixed -- (@polvalente) Fix issue where Base.decode64 :error return could bleed trough as {:ok, :error} (#237) +- (@polvalente) Fix issue where Base.decode64 made peek_claims and peek_header return out of spec (#237) ## [2.1.0] - 2019-05-27 diff --git a/test/joken_test.exs b/test/joken_test.exs index 1c67c0d..587b63d 100644 --- a/test/joken_test.exs +++ b/test/joken_test.exs @@ -162,38 +162,9 @@ defmodule JokenTest do assert Joken.peek_claims(token) == {:ok, %{"some" => custom_claim}} end - test "verify_and_validate, peek_header and peek_claims give proper error upon improper token, instead of raising" do + test "peek_header and peek_claims give proper error upon improper token, instead of returning out of spec :error" do # This test ensures that peek_header and peek_claims use Base.url_decode64 properly - - defmodule HeaderHook do - use Joken.Hooks - - @impl true - def before_verify(_options, {token, signer}) do - with {:ok, _} <- Joken.peek_header(token) do - {:cont, {token, signer}} - else - error -> {:halt, error} - end - end - end - - defmodule ClaimsHook do - use Joken.Hooks - - @impl true - def before_verify(_options, {token, signer}) do - with {:ok, _} <- Joken.peek_claims(token) do - {:cont, {token, signer}} - else - error -> {:halt, error} - end - end - end - - signer = Joken.Signer.create("HS256", "secret") - - assert {:error, :improper_token} = Joken.verify("a.a.a", signer, [{HeaderHook, []}]) - assert {:error, :improper_token} = Joken.verify("a.a.a", signer, [{ClaimsHook, []}]) + assert {:error, :improper_token} = Joken.peek_claims(".a.") + assert {:error, :improper_token} = Joken.peek_header("a..") end end From 3b97715561d7944aa6b0c19166708d01da068a22 Mon Sep 17 00:00:00 2001 From: Paulo Valente Date: Thu, 11 Jul 2019 16:10:48 -0300 Subject: [PATCH 6/7] refactor: rename error return to be the same as 'Joken.expand's --- lib/joken.ex | 4 ++-- test/joken_test.exs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/joken.ex b/lib/joken.ex index e3d129c..f74cde1 100644 --- a/lib/joken.ex +++ b/lib/joken.ex @@ -134,7 +134,7 @@ defmodule Joken do header <- JOSE.json_module().decode(decoded_str) do {:ok, header} else - {:decode64, _error} -> {:error, :improper_token} + {:decode64, _error} -> {:error, :malformed_token} error -> error end end @@ -155,7 +155,7 @@ defmodule Joken do claims <- JOSE.json_module().decode(decoded_str) do {:ok, claims} else - {:decode64, _error} -> {:error, :improper_token} + {:decode64, _error} -> {:error, :malformed_token} error -> error end end diff --git a/test/joken_test.exs b/test/joken_test.exs index 587b63d..686ec1d 100644 --- a/test/joken_test.exs +++ b/test/joken_test.exs @@ -164,7 +164,7 @@ defmodule JokenTest do test "peek_header and peek_claims give proper error upon improper token, instead of returning out of spec :error" do # This test ensures that peek_header and peek_claims use Base.url_decode64 properly - assert {:error, :improper_token} = Joken.peek_claims(".a.") - assert {:error, :improper_token} = Joken.peek_header("a..") + assert {:error, :malformed_token} = Joken.peek_claims(".a.") + assert {:error, :malformed_token} = Joken.peek_header("a..") end end From 66997bd2fee3dc6130694787359d00ff7cadb575 Mon Sep 17 00:00:00 2001 From: Paulo Valente Date: Thu, 11 Jul 2019 16:13:46 -0300 Subject: [PATCH 7/7] refactor: rename correctly to token_malformed --- lib/joken.ex | 4 ++-- test/joken_test.exs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/joken.ex b/lib/joken.ex index f74cde1..f819b25 100644 --- a/lib/joken.ex +++ b/lib/joken.ex @@ -134,7 +134,7 @@ defmodule Joken do header <- JOSE.json_module().decode(decoded_str) do {:ok, header} else - {:decode64, _error} -> {:error, :malformed_token} + {:decode64, _error} -> {:error, :token_malformed} error -> error end end @@ -155,7 +155,7 @@ defmodule Joken do claims <- JOSE.json_module().decode(decoded_str) do {:ok, claims} else - {:decode64, _error} -> {:error, :malformed_token} + {:decode64, _error} -> {:error, :token_malformed} error -> error end end diff --git a/test/joken_test.exs b/test/joken_test.exs index 686ec1d..5aa5b86 100644 --- a/test/joken_test.exs +++ b/test/joken_test.exs @@ -164,7 +164,7 @@ defmodule JokenTest do test "peek_header and peek_claims give proper error upon improper token, instead of returning out of spec :error" do # This test ensures that peek_header and peek_claims use Base.url_decode64 properly - assert {:error, :malformed_token} = Joken.peek_claims(".a.") - assert {:error, :malformed_token} = Joken.peek_header("a..") + assert {:error, :token_malformed} = Joken.peek_claims(".a.") + assert {:error, :token_malformed} = Joken.peek_header("a..") end end