cb6aa35 Merge pull request #171 from bnema/fix/oauth-popup-gtk-thread-safety · dumber

M internal/ui/coordinator/content.go +26 -10


@@ -1799,6 +1799,7 @@ func (c *ContentCoordinator) setupOAuthAutoClose(
1799	1799		var cancelSafetyTimerOnce sync.Once
1800	1800		var requestCloseOnce sync.Once
1801	1801		const oauthSafetyTimeout = 30 * time.Second
	1802	+	const oauthCloseDelay = 500 * time.Millisecond
1802	1803
1803	1804		startSafetyTimer := func() {
1804	1805		safetyTimerMu.Lock()

@@ -1807,10 +1808,22 @@ func (c *ContentCoordinator) setupOAuthAutoClose(
1807	1808		safetyTimer.Stop()
1808	1809		}
1809	1810		safetyTimer = time.AfterFunc(oauthSafetyTimeout, func() {
1810		-	if wv != nil && !wv.IsDestroyed() {
1811		-	log.Warn().Str("pane", string(paneID)).Msg("oauth safety timeout, closing stuck popup")
1812		-	wv.Close()
1813		-	}
	1811	+	cb := glib.SourceFunc(func(_ uintptr) bool {
	1812	+	if wv != nil && !wv.IsDestroyed() {
	1813	+	uri := wv.URI()
	1814	+	if shouldForceCloseOnSafetyTimeout(uri) {
	1815	+	log.Warn().Str("pane", string(paneID)).Msg("oauth safety timeout, closing stuck popup")
	1816	+	wv.Close()
	1817	+	return false
	1818	+	}
	1819	+	log.Debug().
	1820	+	Str("pane", string(paneID)).
	1821	+	Str("uri", logging.TruncateURL(uri, logURLMaxLen)).
	1822	+	Msg("oauth safety timeout reached during active auth flow, skipping forced close")
	1823	+	}
	1824	+	return false
	1825	+	})
	1826	+	glib.IdleAdd(&cb, 0)
1814	1827		})
1815	1828		}
1816	1829

@@ -1833,12 +1846,15 @@ func (c *ContentCoordinator) setupOAuthAutoClose(
1833	1846		Str("reason", reason).
1834	1847		Msg("oauth callback detected, closing")
1835	1848		requestCloseOnce.Do(func() {
1836		-	go func() {
1837		-	time.Sleep(500 * time.Millisecond)
1838		-	if wv != nil && !wv.IsDestroyed() {
1839		-	wv.Close()
1840		-	}
1841		-	}()
	1849	+	time.AfterFunc(oauthCloseDelay, func() {
	1850	+	cb := glib.SourceFunc(func(_ uintptr) bool {
	1851	+	if wv != nil && !wv.IsDestroyed() {
	1852	+	wv.Close()
	1853	+	}
	1854	+	return false
	1855	+	})
	1856	+	glib.IdleAdd(&cb, 0)
	1857	+	})
1842	1858		})
1843	1859		}
1844	1860

 	var cancelSafetyTimerOnce sync.Once
 	var requestCloseOnce sync.Once
 	const oauthSafetyTimeout = 30 * time.Second
+	const oauthCloseDelay = 500 * time.Millisecond
 	startSafetyTimer := func() {
 		safetyTimerMu.Lock()
 			safetyTimer.Stop()
 		}
 		safetyTimer = time.AfterFunc(oauthSafetyTimeout, func() {
-			if wv != nil && !wv.IsDestroyed() {
+			cb := glib.SourceFunc(func(_ uintptr) bool {
-				log.Warn().Str("pane", string(paneID)).Msg("oauth safety timeout, closing stuck popup")
+				if wv != nil && !wv.IsDestroyed() {
-				wv.Close()
+					uri := wv.URI()
-			}
+					if shouldForceCloseOnSafetyTimeout(uri) {
+						log.Warn().Str("pane", string(paneID)).Msg("oauth safety timeout, closing stuck popup")
+						wv.Close()
+						return false
+					}
+					log.Debug().
+						Str("pane", string(paneID)).
+						Str("uri", logging.TruncateURL(uri, logURLMaxLen)).
+						Msg("oauth safety timeout reached during active auth flow, skipping forced close")
+				}
+				return false
+			})
+			glib.IdleAdd(&cb, 0)
 		})
 	}
 			Str("reason", reason).
 			Msg("oauth callback detected, closing")
 		requestCloseOnce.Do(func() {
-			go func() {
+			time.AfterFunc(oauthCloseDelay, func() {
-				time.Sleep(500 * time.Millisecond)
+				cb := glib.SourceFunc(func(_ uintptr) bool {
-				if wv != nil && !wv.IsDestroyed() {
+					if wv != nil && !wv.IsDestroyed() {
-					wv.Close()
+						wv.Close()
-				}
+					}
-			}()
+					return false
+				})
+				glib.IdleAdd(&cb, 0)
+			})
 		})
 	}

M internal/ui/coordinator/oauth.go +10 -1


@@ -48,7 +48,6 @@ var oauthRequestPatterns = []string{
48	48		"client_id=",
49	49		"redirect_uri=",
50	50		"scope=",
51		-	"state=",
52	51		"nonce=",
53	52		}
54	53

@@ -106,6 +105,16 @@ func ShouldAutoClose(url string) bool {
106	105		return IsOAuthCallback(url)
107	106		}
108	107
	108	+	// shouldForceCloseOnSafetyTimeout determines if a popup should be force-closed
	109	+	// when the OAuth safety timeout is reached.
	110	+	//
	111	+	// Safety-timeout force close is disabled for stability. OAuth popups close on
	112	+	// callback detection or manual user action.
	113	+	func shouldForceCloseOnSafetyTimeout(url string) bool {
	114	+	_ = url
	115	+	return false
	116	+	}
	117	+
109	118		// IsOAuthSuccess checks if the callback indicates successful authentication.
110	119		func IsOAuthSuccess(url string) bool {
111	120		if url == "" {

 	"client_id=",
 	"redirect_uri=",
 	"scope=",
-	"state=",
 	"nonce=",
 }
 	return IsOAuthCallback(url)
 }
+// shouldForceCloseOnSafetyTimeout determines if a popup should be force-closed
+// when the OAuth safety timeout is reached.
+//
+// Safety-timeout force close is disabled for stability. OAuth popups close on
+// callback detection or manual user action.
+func shouldForceCloseOnSafetyTimeout(url string) bool {
+	_ = url
+	return false
+}
 // IsOAuthSuccess checks if the callback indicates successful authentication.
 func IsOAuthSuccess(url string) bool {
 	if url == "" {

M internal/ui/coordinator/oauth_test.go +24 -0


@@ -96,6 +96,30 @@ func TestHandlePopupOAuthClose_ErrorDoesNotScheduleParentRefresh(t *testing.T) {
96	96		assert.Nil(t, c.popupRefresh[parentPaneID], "oauth errors should not refresh parent pane")
97	97		}
98	98
	99	+	func TestIsOAuthURL_DoesNotTreatGenericStateParamAsOAuth(t *testing.T) {
	100	+	url := "https://github.com/apps/linear/installations/new?state=Ar4EJk1ao3eyEDgSbYOtG8Cr4"
	101	+
	102	+	assert.False(t, IsOAuthURL(url))
	103	+	}
	104	+
	105	+	func TestIsOAuthURL_DetectsOAuthAuthorizeRequest(t *testing.T) {
	106	+	url := "https://auth.openai.com/oauth/authorize?response_type=code&client_id=app_123&redirect_uri=http%3A%2F%2Flocalhost%3A1455%2Fauth%2Fcallback&scope=openid+profile+email+offline_access&state=abc"
	107	+
	108	+	assert.True(t, IsOAuthURL(url))
	109	+	}
	110	+
	111	+	func TestShouldForceCloseOnSafetyTimeout(t *testing.T) {
	112	+	t.Run("does not force close during in-progress auth challenge", func(t *testing.T) {
	113	+	url := "https://accounts.google.com/v3/signin/challenge/dp?TL=AHU8sQabc"
	114	+	assert.False(t, shouldForceCloseOnSafetyTimeout(url))
	115	+	})
	116	+
	117	+	t.Run("does not force close when callback is reached", func(t *testing.T) {
	118	+	url := "http://localhost:1455/auth/callback?code=abc123&state=xyz"
	119	+	assert.False(t, shouldForceCloseOnSafetyTimeout(url))
	120	+	})
	121	+	}
	122	+
99	123		func waitFor(t *testing.T, timeout time.Duration, condition func() bool) {
100	124		t.Helper()
101	125


@@ -96,6 +96,30 @@ func TestHandlePopupOAuthClose_ErrorDoesNotScheduleParentRefresh(t *testing.T) {
96	assert.Nil(t, c.popupRefresh[parentPaneID], "oauth errors should not refresh parent pane")	96		assert.Nil(t, c.popupRefresh[parentPaneID], "oauth errors should not refresh parent pane")
97	}	97		}
98		98
		99	+	func TestIsOAuthURL_DoesNotTreatGenericStateParamAsOAuth(t *testing.T) {
		100	+	url := "https://github.com/apps/linear/installations/new?state=Ar4EJk1ao3eyEDgSbYOtG8Cr4"
		101	+
		102	+	assert.False(t, IsOAuthURL(url))
		103	+	}
		104	+
		105	+	func TestIsOAuthURL_DetectsOAuthAuthorizeRequest(t *testing.T) {
		106	+	url := "https://auth.openai.com/oauth/authorize?response_type=code&client_id=app_123&redirect_uri=http%3A%2F%2Flocalhost%3A1455%2Fauth%2Fcallback&scope=openid+profile+email+offline_access&state=abc"
		107	+
		108	+	assert.True(t, IsOAuthURL(url))
		109	+	}
		110	+
		111	+	func TestShouldForceCloseOnSafetyTimeout(t *testing.T) {
		112	+	t.Run("does not force close during in-progress auth challenge", func(t *testing.T) {
		113	+	url := "https://accounts.google.com/v3/signin/challenge/dp?TL=AHU8sQabc"
		114	+	assert.False(t, shouldForceCloseOnSafetyTimeout(url))
		115	+	})
		116	+
		117	+	t.Run("does not force close when callback is reached", func(t *testing.T) {
		118	+	url := "http://localhost:1455/auth/callback?code=abc123&state=xyz"
		119	+	assert.False(t, shouldForceCloseOnSafetyTimeout(url))
		120	+	})
		121	+	}
		122	+
99	func waitFor(t *testing.T, timeout time.Duration, condition func() bool) {	123		func waitFor(t *testing.T, timeout time.Duration, condition func() bool) {
100	t.Helper()	124		t.Helper()
101		125