From 98ae4625df2a6939dfa06214a19e3ccfe6ca67a1 Mon Sep 17 00:00:00 2001
From: Dan Leyden <dan@52degreesnorth.com>
Date: Thu, 23 Jul 2020 09:14:50 +0100
Subject: [PATCH] Improve 'none' algorithm handling

when decoding a valid token with `algorithm: none`, unless explicitly
requesting not to verify the token, it could not be validated and raised
DecodeError with a message about insufficient segments.

This error message is misleading, because there are the correct number
of segments provided for that algorithm.

With this change, when a token with `algorithm: none` is provided:
* if the caller requests verification && does not specify `none` as an
  algorithm (default behaviour) it now raises `IncorrectAlgorithm`,
  which is a subclass of `DecodeError` to make the issue more clear.
  This is technically a minor change, but should not be breaking -
  it is returning a subclass, so the same rescues will still work. The
  message provided will be different.

* if the caller requests verification && specifies `none` as an allowed
  algorithm, it verifies the claims and decodes the token as it would
  for a valid, signed token.
  This is new behaviour supporting claims verification for 'none' which
  was not previously available and is only "accessed" through explicit
  settings

* if the caller explicitly requests no verification, the token is
  decoded without checking anything (no change in behaviour)
---
 lib/jwt/decode.rb    |  3 ++-
 lib/jwt/signature.rb |  2 ++
 spec/jwt_spec.rb     | 47 +++++++++++++++++++++++++++++++++++++-------
 3 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/lib/jwt/decode.rb b/lib/jwt/decode.rb
index 486acefe..9ee144fe 100644
--- a/lib/jwt/decode.rb
+++ b/lib/jwt/decode.rb
@@ -74,6 +74,7 @@ def verify_claims
     def validate_segment_count!
       return if segment_length == 3
       return if !@verify && segment_length == 2 # If no verifying required, the signature is not needed
+      return if segment_length == 2 && header['alg'] == 'none'
 
       raise(JWT::DecodeError, 'Not enough or too many segments')
     end
@@ -83,7 +84,7 @@ def segment_length
     end
 
     def decode_crypto
-      @signature = JWT::Base64.url_decode(@segments[2])
+      @signature = JWT::Base64.url_decode(@segments[2] || '')
     end
 
     def header
diff --git a/lib/jwt/signature.rb b/lib/jwt/signature.rb
index d6e208e5..f4a73282 100644
--- a/lib/jwt/signature.rb
+++ b/lib/jwt/signature.rb
@@ -38,6 +38,8 @@ def sign(algorithm, msg, key)
     end
 
     def verify(algorithm, key, signing_input, signature)
+      return true if algorithm == 'none'
+
       raise JWT::DecodeError, 'No verification key available' unless key
 
       algo = ALGOS.find do |alg|
diff --git a/spec/jwt_spec.rb b/spec/jwt_spec.rb
index e9b47359..e49f8dee 100644
--- a/spec/jwt_spec.rb
+++ b/spec/jwt_spec.rb
@@ -44,18 +44,51 @@
 
   context 'alg: NONE' do
     let(:alg) { 'none' }
+    let(:encoded_token) { data['NONE'] }
 
     it 'should generate a valid token' do
       token = JWT.encode payload, nil, alg
 
-      expect(token).to eq data['NONE']
+      expect(token).to eq encoded_token
     end
 
-    it 'should decode a valid token' do
-      jwt_payload, header = JWT.decode data['NONE'], nil, false
+    context 'decoding without verification' do
+      it 'should decode a valid token' do
+        jwt_payload, header = JWT.decode encoded_token, nil, false
 
-      expect(header['alg']).to eq alg
-      expect(jwt_payload).to eq payload
+        expect(header['alg']).to eq alg
+        expect(jwt_payload).to eq payload
+      end
+    end
+
+    context 'decoding with verification' do
+      context 'without specifying the none algorithm' do
+        it 'should fail to decode the token' do
+          expect do
+            JWT.decode encoded_token, nil, true
+          end.to raise_error JWT::IncorrectAlgorithm
+        end
+      end
+
+      context 'specifying the none algorithm' do
+        context 'when the claims are valid' do
+          it 'should decode the token' do
+            jwt_payload, header = JWT.decode encoded_token, nil, true, { algorithms: 'none' }
+
+            expect(header['alg']).to eq 'none'
+            expect(jwt_payload).to eq payload
+          end
+        end
+
+        context 'when the claims are invalid' do
+          let(:encoded_token) { JWT.encode({ exp: 0 }, nil, 'none') }
+          it 'should fail to decode the token' do
+            expect do
+              JWT.decode encoded_token, nil, true
+            end.to raise_error JWT::DecodeError
+          end
+        end
+      end
     end
   end
 
@@ -367,7 +400,6 @@
         iss_payload = payload.merge(iss: iss)
         JWT.encode iss_payload, data[:secret]
       end
-
       it 'if verify_iss is set to false (default option) should not raise JWT::InvalidIssuerError' do
         expect do
           JWT.decode token, data[:secret], true, iss: iss, algorithm: 'HS256'
@@ -384,7 +416,8 @@
 
   context 'a token with not enough segments' do
     it 'raises JWT::DecodeError' do
-      expect { JWT.decode('ThisIsNotAValidJWTToken.second', nil, true) }.to raise_error(JWT::DecodeError, 'Not enough or too many segments')
+      token = JWT.encode('ThisIsNotAValidJWTToken', 'secret').split('.').slice(1,2).join
+      expect { JWT.decode(token, nil, true) }.to raise_error(JWT::DecodeError, 'Not enough or too many segments')
     end
   end