From 7f4abdaed21a6cdc2960c38d4f6aaf7d35d79f1d Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 19 Aug 2026 14:12:44 -0700 Subject: [PATCH] fix(browser): grant storage-access so requestStorageAccess() stops rejecting (#15481) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(browser): grant storage-access so cross-site frames can use their cookies AUTO_GRANTED_BROWSER_PERMISSIONS omitted storage-access, and the installed permission handlers deny anything absent from that set, so every document.requestStorageAccess() in the embedded browser was refused silently. Granting it unconditionally looks like a privacy hole, but that objection assumes Orca blocks third-party cookies. It does not: nothing blocks or partitions them and no Chromium switch touches cookie policy, so Electron's default applies. A cross-site frame therefore already reads and writes its unpartitioned cookies at the network layer, and denying the permission grants no protection - it only breaks sites that take the API's failure path. Chrome resolves requestStorageAccess() without a prompt under the same cookie policy; Electron has no such fast path and forwards to the embedder, so the embedder supplies it. The comment records the condition that would invalidate this. top-level-storage-access stays denied. requestStorageAccessFor() is a separate platform decision: Chromium consults Related Website Sets and has no third-party-cookie auto-grant, and Orca has no such data source, so granting it would invent a permissive answer to a restrictive question. A test pins it. Does not fix Google sign-in - that was #15216. Sign-in completes with this denial in place; this is an independent defect found while investigating it. * test(browser): pin the denial notice storage-access used to raise The suite proved the handler answers true but never pinned the symptom users actually reported — the "asked for storage-access, and Orca denied it" notice. The existing notified-list assertion runs before the storage-access request, so a regression that re-denied it would have left that list untouched. Assert the list after the new requests instead; it goes red pre-fix with 'storage-access' present. Drop the two vi.waitFor wrappers around the same requests. The request handler is synchronous for every non-media permission, so the wait bought nothing and imposed vi.waitFor's 1000ms default on a suite that allows 30s. The waitFor guarding the media path is genuinely async and stays. * refactor(browser): tighten the storage-access rationale and cover isolated partitions Trim the grant's comment to the facts that change a reader's decision. The mechanics of how the request reaches the embedder already live in the commit message; what belongs at the call site is why the answer is grant, and why the check handler must agree with the request handler — Electron builds neither Chrome's activation gate nor its auto-grant, so a disagreeing check pushes compliant sites onto the gesture path, where a rejection consumes the gesture. Move the top-level-storage-access note below the set. It sat after the last element with no trailing comma, so it read as a commented-out entry, and any permission appended at the natural insertion point landed above it. Cover the isolated-partition install path. createProfile and hydrateFromPersisted call installBrowserSessionPartitionPolicies separately from the default-partition path the persistence suite drives, and the two previous changes to this set each added a matching isolated test. Verified red without the grant. Rename the anti-detection case that claimed storage-access has a native denied state; it is granted in production now, and the case really pins pass-through. * docs(browser): correct the storage-access rationale The revisit trigger was backwards. If Orca ever blocked third-party cookies the grant would not become dangerous, it would become useless for cookies: Electron builds no HostContentSettingsMap, so no STORAGE_ACCESS content setting is ever written, and IsAllowedByStorageAccessGrant needs one. Point the tripwire at a cookie or storage-partitioning control instead, which is the change that would actually invalidate the reasoning. The premise was also narrower than the grant. Third-party storage partitioning is enabled by default and independent of cookie policy, and the same permission lifts it for localStorage, IndexedDB, CacheStorage and friends via StorageAccessHandle, which gates only on IsFullCookieAccessAllowed. So the frame does not "already have" everything this grants. It stays the right answer because Chrome grants the same permission under the same cookie policy, but the comment should not claim a narrower blast radius than the change has. * docs(browser): give the storage-access tripwire its consequence Say what goes wrong, not just when to look. If Orca ever blocks third-party cookies or gains a partitioning control, three separate gates stay shut in Electron - the network-service grant check, the frame's trusted status, and the STORAGE_ACCESS content setting that is never written - so the promise would resolve while access stayed blocked. Sites follow the documented pattern of reloading after a successful request, and on reload the check handler still reports granted, so no gesture is needed to ask again. That loops. Qualify the non-cookie clause: the no-arg call resolves undefined and touches only cookies. It is the dictionary form that returns a handle, and since the handler sees the permission name and never the call shape, one grant covers both. Also give "check must agree with request" its reason. Justify the isolated-partition test by the precedent it follows rather than by a call-site divergence the shared mock cannot actually distinguish. * docs(browser): scope the non-cookie clause to the handle A live probe pinned down what the grant actually widens. The frame's ambient window.localStorage and window.indexedDB stay partitioned before and after a successful request; the unpartitioned view is reachable only through the handle the dictionary form returns. Existing code in the frame is unaffected unless the site explicitly calls through that handle, so say handle-scoped rather than leaving a reader to assume the globals change. * docs(browser): correct the isolated-test justification and the tripwire scope "the isolated twin every other entry in this set already has" is false. Counting occurrences in browser-session-registry.test.ts: fullscreen, clipboard-read and clipboard-sanitized-write have none. Cite the pointerLock precedent the test actually mirrors, which is the case directly above it. Separate the two gates in the tripwire. The STORAGE_ACCESS content setting gates cookies; the handle path is gated on IsFullCookieAccessAllowed instead, and a live probe confirmed it works today. Saying "access stayed blocked" read as a claim that the handle is backed by nothing, which contradicted the sentence above it. Say cookie access, and say the handle survives. (cherry picked from commit a3b3254885d01113fc171eb307bbf6578ae055e0) --- src/main/browser/anti-detection.test.ts | 2 +- .../browser-session-permission-policy.ts | 16 ++++++++++++++- ...owser-session-registry.persistence.test.ts | 20 +++++++++++++++++++ .../browser/browser-session-registry.test.ts | 17 ++++++++++++++++ 4 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/main/browser/anti-detection.test.ts b/src/main/browser/anti-detection.test.ts index 1dfdd31b654..ddb904cb436 100644 --- a/src/main/browser/anti-detection.test.ts +++ b/src/main/browser/anti-detection.test.ts @@ -101,7 +101,7 @@ describe('ANTI_DETECTION_SCRIPT', () => { }) it.each(['geolocation', 'idle-detection', 'midi', 'storage-access'])( - 'preserves the native denied state for %s', + 'passes non-intercepted permission queries through to the native state for %s', async (name) => { const context = createContext({ nativeNotificationPermission: 'denied', diff --git a/src/main/browser/browser-session-permission-policy.ts b/src/main/browser/browser-session-permission-policy.ts index 895161db9e3..7dacf067dc7 100644 --- a/src/main/browser/browser-session-permission-policy.ts +++ b/src/main/browser/browser-session-permission-policy.ts @@ -12,8 +12,22 @@ const AUTO_GRANTED_BROWSER_PERMISSIONS = new Set([ 'persistent-storage', // Chromium still requires user activation, so this only removes Orca's // otherwise unactionable denial for immersive browser apps. - 'pointerLock' + 'pointerLock', + // Orca allows unpartitioned third-party cookies, so cross-site frames already send them; denying + // this only rejected the API and burned the caller's user gesture. Not cookies-only: the same + // permission also hands requestStorageAccess({localStorage: true}) a handle onto unpartitioned + // localStorage/IndexedDB, though ambient globals stay partitioned. Chrome grants the same + // permission under the same cookie policy, so this is parity. Electron has no auto-grant, so + // the embedder must answer, and check must agree with request or compliant sites fall back to + // the gesture path. Revisit if Orca ever gains a cookie or storage-partitioning control: the + // handle is gated separately and would survive, but Electron never writes the STORAGE_ACCESS + // content setting, so cookie access would stay blocked while the promise still resolved — and + // sites that reload on success would loop. + 'storage-access' ]) +// 'top-level-storage-access' is deliberately absent: Chromium gates requestStorageAccessFor() on +// Related Website Sets rather than cookie policy, and Orca has no such source, so the rationale +// above does not transfer to it. export function isAutoGrantedBrowserSessionPermission(permission: string): boolean { return AUTO_GRANTED_BROWSER_PERMISSIONS.has(permission) diff --git a/src/main/browser/browser-session-registry.persistence.test.ts b/src/main/browser/browser-session-registry.persistence.test.ts index f27a861a45c..193379239fc 100644 --- a/src/main/browser/browser-session-registry.persistence.test.ts +++ b/src/main/browser/browser-session-registry.persistence.test.ts @@ -547,6 +547,26 @@ describe('BrowserSessionRegistry persistence', () => { expect(checkHandler(null, 'persistent-storage', '')).toBe(true) expect(checkHandler(null, 'geolocation', '')).toBe(false) expect(checkHandler(null, 'media', '', { mediaType: 'video' })).toBe(true) + + // Why: this session allows unpartitioned third-party cookies, so a cross-site frame already has + // the access requestStorageAccess() would grant. Denying it protected nothing and only broke + // sites taking the API's failure path. Red before the grant landed. + requestHandler(guestWc, 'storage-access', permissionCallback) + expect(permissionCallback).toHaveBeenLastCalledWith(true) + expect(checkHandler(null, 'storage-access', '')).toBe(true) + + // Why: requestStorageAccessFor() is a different platform decision — Chromium consults Related + // Website Sets and has no third-party-cookie auto-grant, and Orca has no such data source. This + // pins the deliberate denial so a future blanket widening of the allow-set fails loudly. + requestHandler(guestWc, 'top-level-storage-access', permissionCallback) + expect(permissionCallback).toHaveBeenLastCalledWith(false) + expect(checkHandler(null, 'top-level-storage-access', '')).toBe(false) + + // Why: the reported symptom was a user-visible denial notice, so pin the notified list here — + // storage-access must no longer raise one, while the deliberate top-level denial still does. + expect( + browserManagerNotifyPermissionDeniedMock.mock.calls.map(([args]) => args.permission) + ).toEqual(['geolocation', 'top-level-storage-access']) expect(defaultSession.setDisplayMediaRequestHandler).toHaveBeenCalled() const displayMediaHandler = defaultSession.setDisplayMediaRequestHandler.mock.calls[0][0] const displayMediaCallback = vi.fn() diff --git a/src/main/browser/browser-session-registry.test.ts b/src/main/browser/browser-session-registry.test.ts index 9f32e4f379d..d91307d732f 100644 --- a/src/main/browser/browser-session-registry.test.ts +++ b/src/main/browser/browser-session-registry.test.ts @@ -249,6 +249,23 @@ describe('BrowserSessionRegistry', () => { expect(checkHandler(null, 'pointerLock', '', {})).toBe(true) }) + it('auto-grants storage-access for isolated partitions', () => { + // Why: mirrors the pointerLock precedent directly above — the default-partition suite does not + // reach this install path. + browserSessionRegistry.createProfile('isolated', 'Storage Access Test') + const mockSession = sessionFromPartitionMock.mock.results[0]?.value + const requestHandler = mockSession.setPermissionRequestHandler.mock.calls[0][0] + const checkHandler = mockSession.setPermissionCheckHandler.mock.calls[0][0] + const callback = vi.fn() + const guestWc = { id: 7, getURL: vi.fn(() => 'https://example.com/') } + + requestHandler(guestWc, 'storage-access', callback, {}) + + expect(callback).toHaveBeenCalledWith(true) + expect(checkHandler(null, 'storage-access', '', {})).toBe(true) + expect(checkHandler(null, 'top-level-storage-access', '', {})).toBe(false) + }) + it('routes media permission requests through macOS TCC for isolated partitions', async () => { // Why: verify the parallel fix to the default partition — isolated/imported // profiles must also defer media permission checks to macOS instead of