Use single source of truth for equality checks (#1)

Enforced a single point of token equality check for all functions. Added a test case to demonstrate why the verifyToken can not just be used.
justinas · Aug 27, 2020 · 0bc5e56 · 0bc5e56
1 parent ee7691f
commit 0bc5e56
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 6 deletions.
diff --git a/token.go b/token.go
@@ -72,9 +72,10 @@ func VerifyToken(realToken, sentToken string) bool {
 	if len(s) == 2*tokenLength {
 		s = unmaskToken(s)
 	}
-	return subtle.ConstantTimeCompare(r, s) == 1 && len(r) > 0 && len(s) > 0
+	return tokensEqual(r, s)
 }
 
+// verifyToken expects the realToken to be unmasked and the sentToken to be masked
 func verifyToken(realToken, sentToken []byte) bool {
 	realN := len(realToken)
 	sentN := len(sentToken)
@@ -83,15 +84,16 @@ func verifyToken(realToken, sentToken []byte) bool {
 	// sentN == 2*tokenLength means the token is masked.
 
 	if realN == tokenLength && sentN == 2*tokenLength {
-		return verifyMasked(realToken, sentToken)
+		return tokensEqual(realToken, unmaskToken(sentToken))
 	}
 	return false
 }
 
-// Verifies the masked token
-func verifyMasked(realToken, sentToken []byte) bool {
-	sentPlain := unmaskToken(sentToken)
-	return subtle.ConstantTimeCompare(realToken, sentPlain) == 1
+// tokensEqual expects both tokens to be unmasked
+func tokensEqual(realToken, sentToken []byte) bool {
+	return len(realToken) == tokenLength &&
+		len(sentToken) == tokenLength &&
+		subtle.ConstantTimeCompare(realToken, sentToken) == 1
 }
 
 func checkForPRNG() {

diff --git a/token_test.go b/token_test.go
@@ -2,6 +2,7 @@ package nosurf
 
 import (
 	"crypto/rand"
+	"encoding/base64"
 	"testing"
 )
 
@@ -83,3 +84,47 @@ func TestVerifyTokenBase64Invalid(t *testing.T) {
 		}
 	}
 }
+
+func TestVerifyTokenUnMasked(t *testing.T) {
+	for i, tc := range []struct {
+		real  string
+		send  string
+		valid bool
+	}{
+		{
+			real:  "qwertyuiopasdfghjklzxcvbnm123456",
+			send:  "qwertyuiopasdfghjklzxcvbnm123456",
+			valid: true,
+		},
+		{
+			real: "qwertyuiopasdfghjklzxcvbnm123456",
+			send: "qwertyuiopasdfghjklzxcvbnm123456" +
+				"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
+				"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00",
+			valid: true,
+		},
+		{
+			real: "qwertyuiopasdfghjklzxcvbnm123456" +
+				"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
+				"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00",
+			send: "qwertyuiopasdfghjklzxcvbnm123456" +
+				"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
+				"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00",
+			valid: true,
+		},
+		{
+			real: "qwertyuiopasdfghjklzxcvbnm123456" +
+				"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
+				"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00",
+			send:  "qwertyuiopasdfghjklzxcvbnm123456",
+			valid: true,
+		},
+	} {
+		if VerifyToken(
+			base64.StdEncoding.EncodeToString([]byte(tc.real)),
+			base64.StdEncoding.EncodeToString([]byte(tc.send)),
+		) != tc.valid {
+			t.Errorf("Verify token returned wrong result for case %d: %+v", i, tc)
+		}
+	}
+}