fix(SUPPORT-19): address GoogleDrivePicker code review issues

[maxtechera]

Summary

Follow-up to #963 addressing all code review issues:

• Critical: Validate event.origin against window.location.origin in handleMessage to prevent cross-origin message spoofing
• Critical: Add setInterval popup close detection to clean up the message event listener when user dismisses the OAuth popup without completing auth
• Major: Remove remaining || credentialData.googleAccessToken v1 fallback from GoogleDrive.ts init() (was missed in previous migration commit)
• Major: Add isReauthenticating loading state — button shows "Opening Google sign-in..." while popup is open
• Major: Reset isReauthRequired in both credential data useEffect handlers when a fresh (non-expired) token is detected

Test plan

• Expired v2 credential: "Refresh Access Token" button calls /oauth2-credential/refresh/:id and reloads credential
• Fully revoked token: refresh fails → "Re-authenticate with Google" button appears
• Re-authenticate flow: button shows loading state, popup opens, closes on success/error/dismiss
• After successful re-auth: isReauthRequired clears, picker becomes available
• Closing popup without auth: listener cleaned up, button re-enabled

[linear]

SUPPORT-19 Moonstruck: Re-authenticate Google Drive credential and re-sync product catalog document stores

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
answerai-docs	Building	Preview	Feb 19, 2026 8:21pm
the-answerai	Building	Preview	Feb 19, 2026 8:21pm

[claude]

PR Review: fix(SUPPORT-19): address GoogleDrivePicker code review issues

Summary: This is a focused follow-up to #963 that addresses five security, correctness, and UX issues in the Google Drive OAuth2 re-authentication flow. The diff touches two files: GoogleDrive.ts (1 line change) and GoogleDrivePicker.jsx (+36/-7). The changes are well-scoped and directly address the issues identified in the prior review.

---

Critical Issues

No new critical issues found. The two critical items listed in the PR description are correctly addressed:

• Origin validation (event.origin !== trustedOrigin) is now in place and correctly uses window.location.origin — this properly closes the cross-origin message spoofing vector.
• setInterval popup-close detection is implemented and hooks into the shared cleanup() helper — no listener leak on user-dismissed popups.

---

Major Issues

1. closedPoller can fire cleanup() after OAUTH2_SUCCESS/OAUTH2_ERROR has already called it

Location: packages/ui/src/ui-component/drive/GoogleDrivePicker.jsx — closedPoller block (~line 371)

The cleanup() function clears both the listener and the interval, but there is a potential race: if OAUTH2_SUCCESS arrives in the same JS tick as the popup closes (which is common — the OAuth callback closes the window immediately after postMessage), the sequence can be:

1. messageListener fires OAUTH2_SUCCESS → calls cleanup() → clearInterval(closedPoller) runs, setIsReauthenticating(false) runs.
2. One more interval tick fires before clearInterval takes effect → authWindow.closed is true → cleanup() called again → setIsReauthenticating(false) is a no-op but window.removeEventListener is called with null (since messageListener was set to the function reference, not null).

The real risk here: setIsReauthenticating(false) is called twice and getCredentialDataApi.request is NOT called the second time, so the state should still be consistent. However, to make this airtight consider nulling out both refs inside cleanup():

const cleanup = () => {
    if (messageListener) {
        window.removeEventListener('message', messageListener)
        messageListener = null   // prevent double-remove
    }
    if (closedPoller) {
        clearInterval(closedPoller)
        closedPoller = null      // prevent double-fire
    }
    setIsReauthenticating(false)
}

This is a minor correctness concern rather than a user-facing bug in practice, but worth hardening.

---

2. Unmounted component state update if auth popup completes after the picker unmounts

Location: packages/ui/src/ui-component/drive/GoogleDrivePicker.jsx — handleReauthenticate callback

The messageListener and closedPoller are captured in the handleReauthenticate useCallback closure but are not tied to any useEffect cleanup. If the GoogleDrivePicker component unmounts while the popup is still open (e.g., the user navigates away), the setIsReauthenticating, setIsTokenExpired, setIsReauthRequired calls will run on an unmounted component, producing React warnings and possible no-ops.

Consider tracking the listener/poller in useRef and cleaning them up in a useEffect return:

const messageListenerRef = useRef(null)
const closedPollerRef = useRef(null)

useEffect(() => {
    return () => {
        if (messageListenerRef.current) window.removeEventListener('message', messageListenerRef.current)
        if (closedPollerRef.current) clearInterval(closedPollerRef.current)
    }
}, [])

---

Minor Issues and Suggestions

3. GoogleDrive.ts fallback removal is correct but deserves a comment

Location: packages/components/nodes/documentloaders/GoogleDrive/GoogleDrive.ts, line 300

const accessToken = getCredentialParam('access_token', credentialData, nodeData)

The removal of || credentialData.googleAccessToken is the right call — the v1 field is no longer populated. However, this is a breaking change for any users who stored credentials before the v2 migration ran. The existing error thrown three lines below ('No access token found in credential') will surface this clearly, so the UX impact is acceptable. A brief inline comment explaining why the fallback was removed would help future maintainers:

// access_token is the v2 field set by refreshOAuth2Token; googleAccessToken (v1) is no longer supported
const accessToken = getCredentialParam('access_token', credentialData, nodeData)

4. isReauthRequired reset only on non-expired token — consider also resetting on credential ID change

Location: packages/ui/src/ui-component/drive/GoogleDrivePicker.jsx — useEffect on credentialId (~line 205)

The existing credentialId change effect already calls setIsReauthRequired(false) (line 216 in the current file), so this case is covered. The two new if (!isExpired) setIsReauthRequired(false) lines correctly add the missing "fresh token detected" path. No issue here — just confirming the coverage is complete.

5. Polling interval of 500ms is reasonable but undocumented

Location: packages/ui/src/ui-component/drive/GoogleDrivePicker.jsx, ~line 371

closedPoller = setInterval(() => {
    if (authWindow.closed) cleanup()
}, 500)

500ms is a standard choice and works well. A brief comment explaining the intent (detect user-dismissed popup) would improve readability for future maintainers — similar to what the PR description already says, just inline.

---

Positive Observations

• The introduction of a shared cleanup() helper is a clean refactor — it eliminates duplicated window.removeEventListener calls and ensures both the interval and listener are always torn down together.
• The origin check event.origin !== trustedOrigin is the correct, idiomatic approach and is placed as the very first guard in the listener, so spoofed messages are dropped before any state mutation.
• isReauthenticating state is set to false in all three exit paths (popup blocked, authorizationUrl missing, and catch block), avoiding stuck loading states. This is thorough.
• The button UX ('Opening Google sign-in...' while authenticating, disabled during the flow) gives clear feedback and prevents duplicate popup opens.
• Limiting setIsReauthRequired(false) to the !isExpired branch is correct — you should not clear the re-auth flag just because a credential was refreshed with an expired token still in state.

---

TheAnswer-Specific Checklist

• Multi-tenancy: Not applicable to this UI-only change.
• Authentication: enforceAbility is a server-side concern; no new routes introduced.
• Error handling: All catch blocks call showSnackbar with user-friendly messages.
• tags: ['AAI']: No new components added; existing component is not modified in this regard.
• Testing: The test plan items in the PR description are manual. There are no automated unit or integration tests added for the new state (isReauthenticating) or the cleanup() function. Consider adding tests for the popup-closed detection path and the double-cleanup guard if a test harness for this component exists.

---

Next Steps

1. Address the double-cleanup race (issue Answers Integration Beta v1 #1) by nulling messageListener and closedPoller inside cleanup() — low-effort, high-value hardening.
2. Add unmount cleanup (issue Tools Sandbox #2) via useRef + useEffect return to avoid React warnings when the component unmounts during an active auth flow.
3. The GoogleDrive.ts change is correct as-is; consider adding the inline comment for maintainability.
4. Once the above are addressed, this PR is ready to merge to staging.

Overall this is a well-executed, focused fix. The security issue (origin validation) and the resource-leak fix (popup close detection) are both implemented correctly. The main hardening opportunities are around making cleanup() idempotent and guarding against unmount-time state updates.

---

Reviewed by Claude Code (claude-sonnet-4-6)