fix: vcs.decodeToken can eat an error (#4336)

When the cookie contains valid base64 in json, with an empty token
field. We return a nil token and eat the innerError, which leads to nil
pointer exception down the line.

Author: simonswine

pkg/frontend/vcs/token.go

@@ -153,8 +153,11 @@ func decodeToken(value string, key []byte) (*oauth2.Token, error) {
 		// This may be a deprecated cookie. Deprecated cookies are base64 encoded deprecatedGitSessionTokenCookie objects.
 		token, innerErr := decodeDeprecatedToken(decoded, key)
 		if innerErr != nil {
-			// Deprecated fallback failed, return the original error.
-			return nil, err
+			// Deprecated fallback failed, return the original error if exists.
+			if err != nil {
+				return nil, err
+			}
+			return nil, innerErr
 		}
 		return token, nil
 	}

pkg/frontend/vcs/token_test.go

@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"net/http"
 	"net/url"
+	"strings"
 	"testing"
 	"time"
 
@@ -193,6 +194,26 @@ func Test_tokenFromRequest(t *testing.T) {
 		require.Error(t, err)
 		require.EqualError(t, err, wantErr)
 	})
+
+	t.Run("token with garbage should fail", func(t *testing.T) {
+		githubSessionSecret = []byte("16_byte_key_XXXX")
+
+		// a cookie with false metadata
+		cookieData, err := json.Marshal(map[string]interface{}{
+			"metadata": base64.StdEncoding.EncodeToString([]byte(strings.Repeat("x", 128))),
+			"expiry":   1234,
+		})
+		require.NoError(t, err)
+
+		req := connect.NewRequest(&vcsv1.GetFileRequest{})
+		req.Header().Add("Cookie", "pyroscope_git_session="+base64.RawStdEncoding.EncodeToString(cookieData))
+
+		token, err := tokenFromRequest(ctx, req)
+		require.Error(t, err)
+		require.EqualError(t, err, "cipher: message authentication failed")
+		require.Nil(t, token)
+
+	})
 }
 
 func Test_encodeTokenInCookie(t *testing.T) {