From 63d29d2ed4e0ccaaeb033ecd48a7a8c3721ea5cf Mon Sep 17 00:00:00 2001 From: zarzet Date: Sat, 29 Aug 2026 18:53:33 +0700 Subject: [PATCH] fix(security): validate extension callback state --- android/app/src/main/AndroidManifest.xml | 2 +- .../kotlin/com/zarz/spotiflac/MainActivity.kt | 20 ++-- go_backend/extension_runtime.go | 98 ++++++++++++++++++- go_backend/extension_runtime_auth.go | 44 +++++++-- .../extension_runtime_supplement_test.go | 26 ++++- go_backend/extension_signed_session.go | 51 +++++++--- go_backend/extension_signed_session_test.go | 9 +- ios/Runner/AppDelegate.swift | 25 +++-- ios/Runner/ExtensionCallbackParser.swift | 8 +- ios/RunnerTests/RunnerTests.swift | 4 +- 10 files changed, 236 insertions(+), 51 deletions(-) diff --git a/android/app/src/main/AndroidManifest.xml b/android/app/src/main/AndroidManifest.xml index d799627e..d50564ae 100644 --- a/android/app/src/main/AndroidManifest.xml +++ b/android/app/src/main/AndroidManifest.xml @@ -91,7 +91,7 @@ - + diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivity.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivity.kt index 080cd4d3..e62e4598 100644 --- a/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivity.kt +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivity.kt @@ -701,7 +701,8 @@ class MainActivity: FlutterFragmentActivity() { /** * Deliver Spotify (or other) OAuth authorization code to the extension runtime - * and run its token exchange (e.g. completeSpotifyLogin). State must be the extension id. + * and run its token exchange (e.g. completeSpotifyLogin). State is a one-time + * host nonce resolved to the owning extension by the Go backend. */ private fun handleExtensionOAuthIntent(intent: Intent?) { val uri = intent?.data ?: return @@ -729,14 +730,17 @@ class MainActivity: FlutterFragmentActivity() { if (code.isEmpty()) { return } - val extId = uri.getQueryParameter("state")?.trim().orEmpty() - if (extId.isEmpty()) { - android.util.Log.w("SpotiFLAC", "Extension OAuth redirect missing state (extension id)") + val callbackState = uri.getQueryParameter("state")?.trim().orEmpty() + if (callbackState.isEmpty()) { + android.util.Log.w("SpotiFLAC", "Extension callback missing state") return } intent.data = null + var callbackExtensionId = "" scope.launch(Dispatchers.IO) { try { + val extId = Gobackend.consumeExtensionCallbackState(callbackState) + callbackExtensionId = extId val json = if (isSessionGrant) { Gobackend.setExtensionSessionGrantByID(extId, code) Gobackend.invokeExtensionActionJSON(extId, "completeGrant") @@ -747,17 +751,17 @@ class MainActivity: FlutterFragmentActivity() { if (isSessionGrant) { requireSuccessfulExtensionAction(extId, "completeGrant", json) } - android.util.Log.i("SpotiFLAC", "Extension callback complete for $extId: $json") + android.util.Log.i("SpotiFLAC", "Extension callback completed") if (isSessionGrant) { withContext(Dispatchers.Main) { notifySessionGrantCompleted(extId, true) } } } catch (e: Exception) { - android.util.Log.w("SpotiFLAC", "Extension callback failed: ${e.message}") - if (isSessionGrant) { + android.util.Log.w("SpotiFLAC", "Extension callback failed (${e.javaClass.simpleName})") + if (isSessionGrant && callbackExtensionId.isNotEmpty()) { withContext(Dispatchers.Main) { - notifySessionGrantCompleted(extId, false) + notifySessionGrantCompleted(callbackExtensionId, false) } } } diff --git a/go_backend/extension_runtime.go b/go_backend/extension_runtime.go index e09d3987..b2a3a473 100644 --- a/go_backend/extension_runtime.go +++ b/go_backend/extension_runtime.go @@ -2,6 +2,8 @@ package gobackend import ( "context" + "crypto/rand" + "encoding/base64" "fmt" "net" "net/http" @@ -62,6 +64,7 @@ type PendingAuthRequest struct { ExtensionID string AuthURL string CallbackURL string + State string CreatedAt time.Time } @@ -71,9 +74,102 @@ const pendingAuthRequestTTL = 5 * time.Minute var ( pendingAuthRequests = make(map[string]*PendingAuthRequest) + pendingAuthStates = make(map[string]string) pendingAuthRequestsMu sync.RWMutex ) +func newExtensionCallbackState() (string, error) { + random := make([]byte, 32) + if _, err := rand.Read(random); err != nil { + return "", fmt.Errorf("generate callback state: %w", err) + } + return base64.RawURLEncoding.EncodeToString(random), nil +} + +func registerPendingAuthRequest(request *PendingAuthRequest) error { + if request == nil || strings.TrimSpace(request.ExtensionID) == "" { + return fmt.Errorf("extension id is required") + } + if request.State == "" { + state, err := newExtensionCallbackState() + if err != nil { + return err + } + request.State = state + } + if request.CreatedAt.IsZero() { + request.CreatedAt = time.Now() + } + + pendingAuthRequestsMu.Lock() + if owner := pendingAuthStates[request.State]; owner != "" && owner != request.ExtensionID { + ownerRequest := pendingAuthRequests[owner] + sameChallenge := ownerRequest != nil && + ownerRequest.State == request.State && + ownerRequest.AuthURL == request.AuthURL && + ownerRequest.CallbackURL == request.CallbackURL && + ownerRequest.CreatedAt.Equal(request.CreatedAt) + if !sameChallenge { + pendingAuthRequestsMu.Unlock() + return fmt.Errorf("callback state is already registered") + } + } + if previous := pendingAuthRequests[request.ExtensionID]; previous != nil && previous.State != request.State { + removePendingAuthRequestLocked(request.ExtensionID) + } + pendingAuthRequests[request.ExtensionID] = request + if pendingAuthStates[request.State] == "" { + pendingAuthStates[request.State] = request.ExtensionID + } + pendingAuthRequestsMu.Unlock() + return nil +} + +func removePendingAuthRequestLocked(extensionID string) { + request := pendingAuthRequests[extensionID] + delete(pendingAuthRequests, extensionID) + if request == nil || pendingAuthStates[request.State] != extensionID { + return + } + delete(pendingAuthStates, request.State) + for candidateID, candidate := range pendingAuthRequests { + if candidate != nil && candidate.State == request.State && + time.Since(candidate.CreatedAt) < pendingAuthRequestTTL { + pendingAuthStates[request.State] = candidateID + return + } + } +} + +func removePendingAuthStateLocked(state string) { + delete(pendingAuthStates, state) + for extensionID, request := range pendingAuthRequests { + if request != nil && request.State == state { + delete(pendingAuthRequests, extensionID) + } + } +} + +func ConsumeExtensionCallbackState(state string) (string, error) { + state = strings.TrimSpace(state) + if state == "" { + return "", fmt.Errorf("callback state is required") + } + + pendingAuthRequestsMu.Lock() + extensionID := pendingAuthStates[state] + request := pendingAuthRequests[extensionID] + if extensionID == "" || request == nil || request.State != state || + time.Since(request.CreatedAt) >= pendingAuthRequestTTL { + removePendingAuthStateLocked(state) + pendingAuthRequestsMu.Unlock() + return "", fmt.Errorf("callback state is invalid, expired, or already used") + } + removePendingAuthStateLocked(state) + pendingAuthRequestsMu.Unlock() + return extensionID, nil +} + func GetPendingAuthRequest(extensionID string) *PendingAuthRequest { pendingAuthRequestsMu.RLock() defer pendingAuthRequestsMu.RUnlock() @@ -83,7 +179,7 @@ func GetPendingAuthRequest(extensionID string) *PendingAuthRequest { func ClearPendingAuthRequest(extensionID string) { pendingAuthRequestsMu.Lock() defer pendingAuthRequestsMu.Unlock() - delete(pendingAuthRequests, extensionID) + removePendingAuthRequestLocked(extensionID) } func SetExtensionAuthCode(extensionID string, authCode string) { diff --git a/go_backend/extension_runtime_auth.go b/go_backend/extension_runtime_auth.go index a91f4efe..72cbb772 100644 --- a/go_backend/extension_runtime_auth.go +++ b/go_backend/extension_runtime_auth.go @@ -52,6 +52,17 @@ func summarizeURLForLog(urlStr string) string { return fmt.Sprintf("%s://%s%s", parsed.Scheme, parsed.Host, parsed.Path) } +func setOAuthState(urlStr, state string) (string, error) { + parsed, err := url.Parse(urlStr) + if err != nil { + return "", err + } + query := parsed.Query() + query.Set("state", state) + parsed.RawQuery = query.Encode() + return parsed.String(), nil +} + func (r *extensionRuntime) authOpenUrl(call goja.FunctionCall) goja.Value { if len(call.Arguments) < 1 { return r.jsError("auth URL is required") @@ -66,15 +77,23 @@ func (r *extensionRuntime) authOpenUrl(call goja.FunctionCall) goja.Value { if err := validateExtensionAuthURL(authURL); err != nil { return r.jsError("%s", err.Error()) } - - pendingAuthRequestsMu.Lock() - pendingAuthRequests[r.extensionID] = &PendingAuthRequest{ + callbackState, err := newExtensionCallbackState() + if err != nil { + return r.jsError("%s", err.Error()) + } + authURL, err = setOAuthState(authURL, callbackState) + if err != nil { + return r.jsError("invalid auth URL: %v", err) + } + if err := registerPendingAuthRequest(&PendingAuthRequest{ ExtensionID: r.extensionID, AuthURL: authURL, CallbackURL: callbackURL, + State: callbackState, CreatedAt: time.Now(), + }); err != nil { + return r.jsError("%s", err.Error()) } - pendingAuthRequestsMu.Unlock() extensionAuthStateMu.Lock() state, exists := extensionAuthState[r.extensionID] @@ -148,9 +167,7 @@ func (r *extensionRuntime) authClear(call goja.FunctionCall) goja.Value { delete(extensionAuthState, r.extensionID) extensionAuthStateMu.Unlock() - pendingAuthRequestsMu.Lock() - delete(pendingAuthRequests, r.extensionID) - pendingAuthRequestsMu.Unlock() + ClearPendingAuthRequest(r.extensionID) GoLog("[Extension:%s] Auth state cleared\n", r.extensionID) return r.vm.ToValue(true) @@ -336,18 +353,25 @@ func (r *extensionRuntime) authStartOAuthWithPKCE(call goja.FunctionCall) goja.V for k, v := range extraParams { query.Set(k, fmt.Sprintf("%v", v)) } + callbackState, err := newExtensionCallbackState() + if err != nil { + return r.jsError("failed to generate OAuth state: %v", err) + } + // Host-generated state always wins over extension-supplied extraParams. + query.Set("state", callbackState) parsedURL.RawQuery = query.Encode() fullAuthURL := parsedURL.String() - pendingAuthRequestsMu.Lock() - pendingAuthRequests[r.extensionID] = &PendingAuthRequest{ + if err := registerPendingAuthRequest(&PendingAuthRequest{ ExtensionID: r.extensionID, AuthURL: fullAuthURL, CallbackURL: redirectURI, + State: callbackState, CreatedAt: time.Now(), + }); err != nil { + return r.jsError("failed to register OAuth callback: %v", err) } - pendingAuthRequestsMu.Unlock() GoLog("[Extension:%s] PKCE OAuth started: %s\n", r.extensionID, summarizeURLForLog(fullAuthURL)) diff --git a/go_backend/extension_runtime_supplement_test.go b/go_backend/extension_runtime_supplement_test.go index e0fadb7f..7200d2ff 100644 --- a/go_backend/extension_runtime_supplement_test.go +++ b/go_backend/extension_runtime_supplement_test.go @@ -79,9 +79,33 @@ func TestExtensionRuntimeAuthAndPolyfills(t *testing.T) { if openResult["success"] != true { t.Fatalf("authOpenUrl = %#v", openResult) } - if pending := GetPendingAuthRequest("auth-ext"); pending == nil || pending.AuthURL == "" { + pending := GetPendingAuthRequest("auth-ext") + if pending == nil || pending.AuthURL == "" || pending.State == "" || !strings.Contains(pending.AuthURL, "state=") { t.Fatalf("pending auth = %#v", pending) } + if extensionID, err := ConsumeExtensionCallbackState(pending.State); err != nil || extensionID != "auth-ext" { + t.Fatalf("consume callback state = %q/%v", extensionID, err) + } + if _, err := ConsumeExtensionCallbackState(pending.State); err == nil { + t.Fatal("callback state replay should be rejected") + } + collisionState := "shared-callback-state" + collisionCreatedAt := time.Now() + if err := registerPendingAuthRequest(&PendingAuthRequest{ + ExtensionID: "auth-ext", + State: collisionState, + CreatedAt: collisionCreatedAt, + }); err != nil { + t.Fatalf("register first callback state: %v", err) + } + if err := registerPendingAuthRequest(&PendingAuthRequest{ + ExtensionID: "other-ext", + State: collisionState, + CreatedAt: collisionCreatedAt.Add(time.Second), + }); err == nil { + t.Fatal("callback state collision should be rejected") + } + ClearPendingAuthRequest("auth-ext") if code := runtime.authGetCode(goja.FunctionCall{}); !goja.IsUndefined(code) { t.Fatalf("expected undefined code, got %v", code) } diff --git a/go_backend/extension_signed_session.go b/go_backend/extension_signed_session.go index 5d4ccb67..1c647bb4 100644 --- a/go_backend/extension_signed_session.go +++ b/go_backend/extension_signed_session.go @@ -112,6 +112,7 @@ type signedSessionCoordinator struct { authURL string callbackURL string + callbackState string challengeCreatedAt time.Time pendingExtensionIDs map[string]struct{} completedGrantHash string @@ -172,16 +173,18 @@ func (c *signedSessionCoordinator) clearChallenge() { } c.authURL = "" c.callbackURL = "" + c.callbackState = "" c.challengeCreatedAt = time.Time{} c.pendingExtensionIDs = nil } -func (c *signedSessionCoordinator) rememberChallenge(extensionID, authURL, callbackURL string) { +func (c *signedSessionCoordinator) rememberChallenge(extensionID, authURL, callbackURL, callbackState string) { if c.pendingExtensionIDs == nil { c.pendingExtensionIDs = make(map[string]struct{}) } c.authURL = authURL c.callbackURL = callbackURL + c.callbackState = callbackState c.challengeCreatedAt = time.Now() c.pendingExtensionIDs[extensionID] = struct{}{} } @@ -1156,14 +1159,15 @@ func (r *extensionRuntime) startSignedSessionVerificationLocked( reason string, ) (string, error) { if coordinator.activeChallenge() { - pendingAuthRequestsMu.Lock() - pendingAuthRequests[r.extensionID] = &PendingAuthRequest{ + if err := registerPendingAuthRequest(&PendingAuthRequest{ ExtensionID: r.extensionID, AuthURL: coordinator.authURL, CallbackURL: coordinator.callbackURL, + State: coordinator.callbackState, CreatedAt: coordinator.challengeCreatedAt, + }); err != nil { + return "", err } - pendingAuthRequestsMu.Unlock() coordinator.pendingExtensionIDs[r.extensionID] = struct{}{} return coordinator.authURL, nil } @@ -1177,6 +1181,7 @@ func (r *extensionRuntime) startSignedSessionVerificationLocked( r.extensionID, pending.AuthURL, pending.CallbackURL, + pending.State, ) return pending.AuthURL, nil } @@ -1277,25 +1282,47 @@ func (r *extensionRuntime) startSignedSessionVerificationLocked( if authURL == "" && boot.ChallengeURL != "" { authURL = boot.ChallengeURL } + // Preserve a server-provided state when present. Otherwise add a fresh + // host-generated nonce so the callback is bound to this one challenge. + callbackState, err := newExtensionCallbackState() + if err != nil { + return "", fmt.Errorf("prepare signed-session callback state: %w", err) + } + if parsedAuthURL, parseErr := url.Parse(authURL); parseErr == nil { + if serverState := strings.TrimSpace(parsedAuthURL.Query().Get("state")); serverState != "" { + callbackState = serverState + } else if authURL != "" { + authURL, err = setOAuthState(authURL, callbackState) + if err != nil { + return "", fmt.Errorf("prepare signed-session verification URL: %w", err) + } + } + } + callbackURL, err := setOAuthState(config.CallbackURL, callbackState) + if err != nil { + return "", fmt.Errorf("prepare signed-session callback: %w", err) + } if authURL == "" && boot.ChallengeID != "" { - authURL = r.buildSignedSessionChallengeURL(config, boot.ChallengeID) + authURL = r.buildSignedSessionChallengeURL(config, boot.ChallengeID, callbackState) } if authURL == "" { return "", fmt.Errorf("signed-session bootstrap did not return a session or verification challenge") } - pendingAuthRequestsMu.Lock() - pendingAuthRequests[r.extensionID] = &PendingAuthRequest{ + request := &PendingAuthRequest{ ExtensionID: r.extensionID, AuthURL: authURL, - CallbackURL: config.CallbackURL, + CallbackURL: callbackURL, + State: callbackState, CreatedAt: time.Now(), } - pendingAuthRequestsMu.Unlock() - coordinator.rememberChallenge(r.extensionID, authURL, config.CallbackURL) + if err := registerPendingAuthRequest(request); err != nil { + return "", err + } + coordinator.rememberChallenge(r.extensionID, authURL, callbackURL, callbackState) return authURL, nil } -func (r *extensionRuntime) buildSignedSessionChallengeURL(config SignedSessionConfig, challengeID string) string { +func (r *extensionRuntime) buildSignedSessionChallengeURL(config SignedSessionConfig, challengeID, callbackState string) string { challengeURL, err := signedSessionURL(config, config.Endpoints.Challenge) if err != nil { return "" @@ -1310,7 +1337,7 @@ func (r *extensionRuntime) buildSignedSessionChallengeURL(config SignedSessionCo } q := callback.Query() q.Set("cb_version", "v2grant") - q.Set("state", r.extensionID) + q.Set("state", callbackState) callback.RawQuery = q.Encode() query := parsed.Query() diff --git a/go_backend/extension_signed_session_test.go b/go_backend/extension_signed_session_test.go index 13f62abe..ba1f32aa 100644 --- a/go_backend/extension_signed_session_test.go +++ b/go_backend/extension_signed_session_test.go @@ -10,6 +10,7 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "path/filepath" goruntime "runtime" @@ -913,7 +914,8 @@ func TestSignedSessionFetchUnauthenticatedTriggersVerification(t *testing.T) { if result["needsVerification"] != true { t.Fatalf("expected needsVerification=true, got %+v", result) } - if result["auth_url"] != "https://auth.example.com/login?state=abc" { + authURL, err := url.Parse(result["auth_url"].(string)) + if err != nil || authURL.Scheme != "https" || authURL.Host != "auth.example.com" || authURL.Path != "/login" || authURL.Query().Get("state") == "" { t.Fatalf("unexpected auth_url: %+v", result) } } @@ -1153,7 +1155,8 @@ func TestSignedSessionFetchCanonicalVerifyDoesNotClearSession(t *testing.T) { runtime.vm.ToValue("/tracks/search"), }} result := runtime.signedSessionFetch(call).Export().(map[string]any) - if result["needsVerification"] != true || result["auth_url"] != "https://auth.example.com/verify" { + authURL, err := url.Parse(result["auth_url"].(string)) + if result["needsVerification"] != true || err != nil || authURL.Path != "/verify" || authURL.Query().Get("state") == "" { t.Fatalf("canonical VERIFY_REQUIRED did not open verification: %+v", result) } if calls != 2 { @@ -2004,7 +2007,7 @@ func TestBuildSignedSessionChallengeURL(t *testing.T) { }) runtime := newSignedSessionTestRuntime(t, "tidal-ext", nil) - got := runtime.buildSignedSessionChallengeURL(config, "chal-123") + got := runtime.buildSignedSessionChallengeURL(config, "chal-123", "state-123") if !strings.HasPrefix(got, "https://auth.example.com/challenge?") { t.Fatalf("unexpected base URL: %q", got) diff --git a/ios/Runner/AppDelegate.swift b/ios/Runner/AppDelegate.swift index b5dda825..c1cf73f1 100644 --- a/ios/Runner/AppDelegate.swift +++ b/ios/Runner/AppDelegate.swift @@ -104,25 +104,32 @@ import Gobackend } /// Extension return URLs: - /// - OAuth: spotiflac://callback?code=...&state= - /// - Signed session: spotiflac://session-grant?grant=...&state= + /// - OAuth: spotiflac://callback?code=...&state= + /// - Signed session: spotiflac://session-grant?grant=...&state= @discardableResult private func handleExtensionOAuthRedirect(url: URL) -> Bool { guard let route = ExtensionCallbackParser.parse(url) else { return false } streamQueue.async { var err: NSError? var response: String? + guard let extensionId = GobackendConsumeExtensionCallbackState( + route.state, + &err + ), err == nil else { + NSLog("SpotiFLAC Mobile: Rejected invalid or expired extension callback") + return + } if route.isSessionGrant { - GobackendSetExtensionSessionGrantByID(route.extensionId, route.code) + GobackendSetExtensionSessionGrantByID(extensionId, route.code) response = GobackendInvokeExtensionActionJSON( - route.extensionId, + extensionId, "completeGrant", &err ) } else { - GobackendSetExtensionAuthCodeByID(route.extensionId, route.code) + GobackendSetExtensionAuthCodeByID(extensionId, route.code) response = GobackendInvokeExtensionActionJSON( - route.extensionId, + extensionId, "completeSpotifyLogin", &err ) @@ -130,7 +137,7 @@ import Gobackend if err == nil && route.isSessionGrant { do { try self.requireSuccessfulExtensionAction( - extensionId: route.extensionId, + extensionId: extensionId, actionName: "completeGrant", response: response ) @@ -140,11 +147,11 @@ import Gobackend } if let err = err { NSLog( - "SpotiFLAC: Extension callback complete failed: \(err.localizedDescription)") + "SpotiFLAC Mobile: Extension callback failed (code \(err.code))") } else if route.isSessionGrant { DispatchQueue.main.async { [weak self] in self?.notifySessionGrantCompleted( - extensionId: route.extensionId + extensionId: extensionId ) } } diff --git a/ios/Runner/ExtensionCallbackParser.swift b/ios/Runner/ExtensionCallbackParser.swift index 11e06e0e..b57428df 100644 --- a/ios/Runner/ExtensionCallbackParser.swift +++ b/ios/Runner/ExtensionCallbackParser.swift @@ -2,7 +2,7 @@ import Foundation struct ExtensionCallbackRoute: Equatable { let code: String - let extensionId: String + let state: String let isSessionGrant: Bool } @@ -36,17 +36,17 @@ enum ExtensionCallbackParser { ?? queryItems.first { $0.name == "code" }?.value? .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" - let extensionId = + let state = queryItems.first { $0.name == "state" }?.value? .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" - guard !code.isEmpty, !extensionId.isEmpty else { + guard !code.isEmpty, !state.isEmpty else { return nil } return ExtensionCallbackRoute( code: code, - extensionId: extensionId, + state: state, isSessionGrant: isSessionGrant ) } diff --git a/ios/RunnerTests/RunnerTests.swift b/ios/RunnerTests/RunnerTests.swift index 6822204a..c60759e3 100644 --- a/ios/RunnerTests/RunnerTests.swift +++ b/ios/RunnerTests/RunnerTests.swift @@ -12,7 +12,7 @@ class RunnerTests: XCTestCase { route, ExtensionCallbackRoute( code: "auth-code", - extensionId: "spotify-web", + state: "spotify-web", isSessionGrant: false ) ) @@ -30,7 +30,7 @@ class RunnerTests: XCTestCase { route, ExtensionCallbackRoute( code: "session-token", - extensionId: "provider", + state: "provider", isSessionGrant: true ) )