From 8f01954d7b2edf8e9e0e52ab5151ba6f0a7401a8 Mon Sep 17 00:00:00 2001 From: Alaeddin Date: Fri, 11 Sep 2026 10:38:16 +0100 Subject: [PATCH 1/3] style(storybook-config): format cloudDrives with the repo prettier config --- .../storybook-config/src/cloudDrives.test.ts | 68 +++++++++++-------- packages/storybook-config/src/cloudDrives.ts | 61 +++++++++-------- 2 files changed, 72 insertions(+), 57 deletions(-) diff --git a/packages/storybook-config/src/cloudDrives.test.ts b/packages/storybook-config/src/cloudDrives.test.ts index fe9501415..618f23ff4 100644 --- a/packages/storybook-config/src/cloudDrives.test.ts +++ b/packages/storybook-config/src/cloudDrives.test.ts @@ -3,38 +3,48 @@ import { describe, it, expect } from 'vitest' import { buildCloudDrives } from './cloudDrives' describe('buildCloudDrives', () => { - it('maps Google env vars into the googleDrive config', () => { - const cd = buildCloudDrives({ - VITE_GOOGLE_CLIENT_ID: 'gid', - VITE_GOOGLE_API_KEY: 'gkey', - VITE_GOOGLE_APP_ID: 'gapp', + it('maps Google env vars into the googleDrive config', () => { + const cd = buildCloudDrives({ + VITE_GOOGLE_CLIENT_ID: 'gid', + VITE_GOOGLE_API_KEY: 'gkey', + VITE_GOOGLE_APP_ID: 'gapp', + }) + expect(cd.googleDrive).toEqual({ + clientId: 'gid', + apiKey: 'gkey', + appId: 'gapp', + }) }) - expect(cd.googleDrive).toEqual({ clientId: 'gid', apiKey: 'gkey', appId: 'gapp' }) - }) - it('defaults every provider to empty strings when env is bare (so the auth screen still renders)', () => { - const cd = buildCloudDrives({}) - expect(cd.googleDrive).toEqual({ clientId: '', apiKey: '', appId: '' }) - expect(cd.oneDrive?.clientId).toBe('') - expect(cd.dropbox?.clientId).toBe('') - expect(cd.box?.clientId).toBe('') - }) + it('defaults every provider to empty strings when env is bare (so the auth screen still renders)', () => { + const cd = buildCloudDrives({}) + expect(cd.googleDrive).toEqual({ clientId: '', apiKey: '', appId: '' }) + expect(cd.oneDrive?.clientId).toBe('') + expect(cd.dropbox?.clientId).toBe('') + expect(cd.box?.clientId).toBe('') + }) - it('falls back redirectUri to the supplied origin when the env var is absent', () => { - const cd = buildCloudDrives({ VITE_DROPBOX_CLIENT_ID: 'dbx' }, 'http://localhost:53050') - expect(cd.dropbox).toEqual({ clientId: 'dbx', redirectUri: 'http://localhost:53050' }) - }) + it('falls back redirectUri to the supplied origin when the env var is absent', () => { + const cd = buildCloudDrives( + { VITE_DROPBOX_CLIENT_ID: 'dbx' }, + 'http://localhost:53050', + ) + expect(cd.dropbox).toEqual({ + clientId: 'dbx', + redirectUri: 'http://localhost:53050', + }) + }) - it('prefers an explicit redirectUri env var over the origin fallback', () => { - const cd = buildCloudDrives( - { VITE_ONEDRIVE_REDIRECT_URI: 'https://app.test/callback' }, - 'http://localhost:53050', - ) - expect(cd.oneDrive?.redirectUri).toBe('https://app.test/callback') - }) + it('prefers an explicit redirectUri env var over the origin fallback', () => { + const cd = buildCloudDrives( + { VITE_ONEDRIVE_REDIRECT_URI: 'https://app.test/callback' }, + 'http://localhost:53050', + ) + expect(cd.oneDrive?.redirectUri).toBe('https://app.test/callback') + }) - it('trims surrounding whitespace from values', () => { - const cd = buildCloudDrives({ VITE_BOX_CLIENT_ID: ' boxid ' }) - expect(cd.box?.clientId).toBe('boxid') - }) + it('trims surrounding whitespace from values', () => { + const cd = buildCloudDrives({ VITE_BOX_CLIENT_ID: ' boxid ' }) + expect(cd.box?.clientId).toBe('boxid') + }) }) diff --git a/packages/storybook-config/src/cloudDrives.ts b/packages/storybook-config/src/cloudDrives.ts index 4c848daf9..cd15566c1 100644 --- a/packages/storybook-config/src/cloudDrives.ts +++ b/packages/storybook-config/src/cloudDrives.ts @@ -18,10 +18,10 @@ // sign-in without any code change. export type CloudDrivesConfig = { - googleDrive?: { clientId: string; apiKey: string; appId: string } - oneDrive?: { clientId: string; redirectUri?: string } - dropbox?: { clientId: string; redirectUri?: string } - box?: { clientId: string; redirectUri?: string } + googleDrive?: { clientId: string; apiKey: string; appId: string } + oneDrive?: { clientId: string; redirectUri?: string } + dropbox?: { clientId: string; redirectUri?: string } + box?: { clientId: string; redirectUri?: string } } type EnvRecord = Record @@ -33,26 +33,29 @@ const read = (env: EnvRecord, key: string) => (env[key] ?? '').trim() * into a `cloudDrives` object. Every provider is always present so the adapter * reaches its real auth screen instead of the empty "not ready" panel. */ -export function buildCloudDrives(env: EnvRecord, origin = ''): CloudDrivesConfig { - return { - googleDrive: { - clientId: read(env, 'VITE_GOOGLE_CLIENT_ID'), - apiKey: read(env, 'VITE_GOOGLE_API_KEY'), - appId: read(env, 'VITE_GOOGLE_APP_ID'), - }, - oneDrive: { - clientId: read(env, 'VITE_ONEDRIVE_CLIENT_ID'), - redirectUri: read(env, 'VITE_ONEDRIVE_REDIRECT_URI') || origin, - }, - dropbox: { - clientId: read(env, 'VITE_DROPBOX_CLIENT_ID'), - redirectUri: read(env, 'VITE_DROPBOX_REDIRECT_URI') || origin, - }, - box: { - clientId: read(env, 'VITE_BOX_CLIENT_ID'), - redirectUri: read(env, 'VITE_BOX_REDIRECT_URI') || origin, - }, - } +export function buildCloudDrives( + env: EnvRecord, + origin = '', +): CloudDrivesConfig { + return { + googleDrive: { + clientId: read(env, 'VITE_GOOGLE_CLIENT_ID'), + apiKey: read(env, 'VITE_GOOGLE_API_KEY'), + appId: read(env, 'VITE_GOOGLE_APP_ID'), + }, + oneDrive: { + clientId: read(env, 'VITE_ONEDRIVE_CLIENT_ID'), + redirectUri: read(env, 'VITE_ONEDRIVE_REDIRECT_URI') || origin, + }, + dropbox: { + clientId: read(env, 'VITE_DROPBOX_CLIENT_ID'), + redirectUri: read(env, 'VITE_DROPBOX_REDIRECT_URI') || origin, + }, + box: { + clientId: read(env, 'VITE_BOX_CLIENT_ID'), + redirectUri: read(env, 'VITE_BOX_REDIRECT_URI') || origin, + }, + } } /** @@ -61,8 +64,10 @@ export function buildCloudDrives(env: EnvRecord, origin = ''): CloudDrivesConfig * e.g. under a non-Vite test runner. */ export function cloudDrivesFromEnv(): CloudDrivesConfig { - const env = (import.meta as unknown as { env?: EnvRecord }).env ?? {} - const origin = - typeof window !== 'undefined' && window.location ? window.location.origin : '' - return buildCloudDrives(env, origin) + const env = (import.meta as unknown as { env?: EnvRecord }).env ?? {} + const origin = + typeof window !== 'undefined' && window.location + ? window.location.origin + : '' + return buildCloudDrives(env, origin) } From b5dc9a38ba499535a4f05b3d1eaca4cf20c1e5e0 Mon Sep 17 00:00:00 2001 From: Alaeddin <15094821+BSalaeddin@users.noreply.github.com> Date: Fri, 11 Sep 2026 10:42:24 +0100 Subject: [PATCH 2/3] fix(core,server): escape drive query ids, degrade a failed drives.list, and lead the root listing with shared drives --- .changeset/google-drive-shared-drives.md | 18 +- apps/storybook-react/.env.example | 2 + apps/storybook-svelte/.env.example | 2 + apps/storybook-vanilla/.env.example | 2 + apps/storybook-vue/.env.example | 2 + .../core/src/drives/google-drive-plugin.ts | 67 +++++-- packages/core/src/drives/query-escape.ts | 17 ++ packages/core/src/drives/types.ts | 8 + packages/core/src/internal.ts | 1 + .../core/tests/drive-query-escape.test.ts | 41 ++++ .../core/tests/google-drive-plugin.test.ts | 177 +++++++++++++++++- packages/core/tests/internal-surface.test.ts | 1 + packages/server/src/drive-clients.ts | 24 ++- .../storybook-config/src/cloudDrives.test.ts | 27 ++- packages/storybook-config/src/cloudDrives.ts | 12 +- scripts/drive-sandbox/seed.mjs | 3 +- 16 files changed, 369 insertions(+), 35 deletions(-) create mode 100644 packages/core/src/drives/query-escape.ts create mode 100644 packages/core/tests/drive-query-escape.test.ts diff --git a/.changeset/google-drive-shared-drives.md b/.changeset/google-drive-shared-drives.md index b59d01596..756df3151 100644 --- a/.changeset/google-drive-shared-drives.md +++ b/.changeset/google-drive-shared-drives.md @@ -1,6 +1,7 @@ --- '@useupup/core': patch '@useupup/vanilla': patch +'@useupup/server': patch --- The Google Drive picker can browse shared drives and "Shared with me", behind a @@ -35,9 +36,20 @@ With `sharedDrives: true`: in both `loadFiles` and `loadMoreFiles`. Every other part of the picker treats it as an ordinary folder. -Both additions land on the root's first page only, never on a continuation page, -so they appear exactly once and pagination is unaffected in either the shared -drives or the "Shared with me" view. +Both additions lead the root's first page and are never repeated on a +continuation page, so they appear exactly once, stay above a second page of My +Drive files, and pagination is unaffected in either view. + +A `drives.list` failure — a 403 under a Workspace sharing policy, a 429, a 5xx — +degrades to no shared-drive rows rather than taking the whole root listing down +with it, and is reported on a new non-fatal `google-drive:shared-drives-error` +event. Drives collected before a mid-pagination failure are kept. + +Drive folder ids are now escaped into the `files.list` query instead of +interpolated raw, using the same `escapeDriveQueryValue` the server-mode drive +client uses. That escaper moved from `@useupup/server` into +`@useupup/core/internal`, and `@useupup/server` re-exports it, so the two halves +share ONE implementation that cannot drift. The flag defaults off. Nothing is sent, no `drives.list` is issued and no extra row appears when it is unset or false, so no existing picker changes shape. diff --git a/apps/storybook-react/.env.example b/apps/storybook-react/.env.example index cb662ac21..2c8879804 100644 --- a/apps/storybook-react/.env.example +++ b/apps/storybook-react/.env.example @@ -13,6 +13,8 @@ VITE_GOOGLE_CLIENT_ID= VITE_GOOGLE_API_KEY= VITE_GOOGLE_APP_ID= +# Optional: "true" lists shared drives + "Shared with me" in the picker root +VITE_GOOGLE_SHARED_DRIVES= # OneDrive — Azure app registration (Application/client ID). redirectUri defaults to window origin. VITE_ONEDRIVE_CLIENT_ID= diff --git a/apps/storybook-svelte/.env.example b/apps/storybook-svelte/.env.example index d500a6bab..532522f26 100644 --- a/apps/storybook-svelte/.env.example +++ b/apps/storybook-svelte/.env.example @@ -13,6 +13,8 @@ VITE_GOOGLE_CLIENT_ID= VITE_GOOGLE_API_KEY= VITE_GOOGLE_APP_ID= +# Optional: "true" lists shared drives + "Shared with me" in the picker root +VITE_GOOGLE_SHARED_DRIVES= # OneDrive — Azure app registration (Application/client ID). redirectUri defaults to window origin. VITE_ONEDRIVE_CLIENT_ID= diff --git a/apps/storybook-vanilla/.env.example b/apps/storybook-vanilla/.env.example index 1e56e6f64..4f284e3b3 100644 --- a/apps/storybook-vanilla/.env.example +++ b/apps/storybook-vanilla/.env.example @@ -13,6 +13,8 @@ VITE_GOOGLE_CLIENT_ID= VITE_GOOGLE_API_KEY= VITE_GOOGLE_APP_ID= +# Optional: "true" lists shared drives + "Shared with me" in the picker root +VITE_GOOGLE_SHARED_DRIVES= # OneDrive — Azure app registration (Application/client ID). redirectUri defaults to window origin. VITE_ONEDRIVE_CLIENT_ID= diff --git a/apps/storybook-vue/.env.example b/apps/storybook-vue/.env.example index 78af9e6ab..fdb16883b 100644 --- a/apps/storybook-vue/.env.example +++ b/apps/storybook-vue/.env.example @@ -13,6 +13,8 @@ VITE_GOOGLE_CLIENT_ID= VITE_GOOGLE_API_KEY= VITE_GOOGLE_APP_ID= +# Optional: "true" lists shared drives + "Shared with me" in the picker root +VITE_GOOGLE_SHARED_DRIVES= # OneDrive — Azure app registration (Application/client ID). redirectUri defaults to window origin. VITE_ONEDRIVE_CLIENT_ID= diff --git a/packages/core/src/drives/google-drive-plugin.ts b/packages/core/src/drives/google-drive-plugin.ts index 4ac718b50..f2b50eea6 100644 --- a/packages/core/src/drives/google-drive-plugin.ts +++ b/packages/core/src/drives/google-drive-plugin.ts @@ -3,6 +3,7 @@ import type { DrivePlugin } from './plugin' import type { GoogleDriveConfig } from './configs' import type { DriveFile, DriveState } from './types' import { storageGet, storageSet, storageDel } from './session-storage' +import { escapeDriveQueryValue } from './query-escape' import { UpupAuthError, UpupNetworkError } from '../errors' // ── Session storage keys ── @@ -162,11 +163,17 @@ export class GoogleDrivePlugin implements DrivePlugin { * The `files.list` query for one picker folder. Every id is a parent except * the synthetic "Shared with me" one, which is a query instead — Drive has no * folder whose children are the files other people shared with you. + * + * The id is escaped, not interpolated raw: a folder id reaches here from a + * Drive API response or a host-supplied argument, and an unescaped `'` ends + * the literal and lets the rest of the value become query syntax. Same + * escaper the server-mode drive client uses — one implementation in + * `query-escape.ts`, so the two halves cannot drift. */ private listQuery(parentId: string): string { return parentId === SHARED_WITH_ME_FOLDER_ID ? 'sharedWithMe = true and trashed = false' - : `'${parentId}' in parents and trashed = false` + : `'${escapeDriveQueryValue(parentId)}' in parents and trashed = false` } /** @@ -180,6 +187,14 @@ export class GoogleDrivePlugin implements DrivePlugin { * with no further special-casing. * * Requires no extra OAuth scope: `drive.readonly` covers `drives.list`. + * + * NEVER throws. `drives.list` can answer 403 under a Workspace sharing + * policy, or 429, or 5xx — and this runs inside the ROOT listing, so letting + * that escape would take a working My Drive listing down with it and leave + * the picker empty. A failure degrades to no shared-drive rows plus a + * `shared-drives-error` event, which is separate from the `error` event + * precisely because the browse did NOT fail. A partial result is kept: if + * page 3 of 5 fails, the drives already collected are still returned. */ private async listSharedDriveFolders(): Promise { const folders: DriveFile[] = [] @@ -196,13 +211,23 @@ export class GoogleDrivePlugin implements DrivePlugin { }) if (pageToken) params.set('pageToken', pageToken) - // oxlint-disable-next-line no-await-in-loop -- cursor pagination: each page's token comes from the previous response, so these cannot run in parallel - const res = await this.apiRequest( - `${DRIVES_URL}?${params.toString()}`, - { method: 'GET' }, - ) - // oxlint-disable-next-line no-await-in-loop -- same round trip as the request above - const data = (await res.json()) as GoogleDrivesListResponse + let data: GoogleDrivesListResponse + try { + // oxlint-disable-next-line no-await-in-loop -- cursor pagination: each page's token comes from the previous response, so these cannot run in parallel + const res = await this.apiRequest( + `${DRIVES_URL}?${params.toString()}`, + { method: 'GET' }, + ) + // oxlint-disable-next-line no-await-in-loop -- same round trip as the request above + data = (await res.json()) as GoogleDrivesListResponse + } catch (err) { + // upup-catch: reported on shared-drives-error and swallowed, per + // the contract above — the root listing must survive this. + this.emitter?.emit('google-drive:shared-drives-error', { + error: err instanceof Error ? err : new Error(String(err)), + }) + break + } for (const drive of data.drives ?? []) { folders.push( @@ -386,17 +411,21 @@ export class GoogleDrivePlugin implements DrivePlugin { ) const data = (await res.json()) as GoogleFilesListResponse - const files: DriveFile[] = (data.files ?? []).map(mapGoogleEntry) - - // The two doors out of My Drive, appended to the root page only — - // after its own children, and never on a continuation page, so they - // appear exactly once (#391). - if (this.config.sharedDrives && parentId === 'root') { - files.push( - ...(await this.listSharedDriveFolders()), - this.sharedWithMeFolder(), - ) - } + const ownFiles: DriveFile[] = (data.files ?? []).map(mapGoogleEntry) + + // The two doors out of My Drive, on the root page only and never on a + // continuation page, so they appear exactly once (#391). They go + // FIRST: a root with more than one page appends its later pages to + // the end of the list, which would bury them under My Drive files + // that arrived after them. + const files: DriveFile[] = + this.config.sharedDrives && parentId === 'root' + ? [ + ...(await this.listSharedDriveFolders()), + this.sharedWithMeFolder(), + ...ownFiles, + ] + : ownFiles const hasMore = !!data.nextPageToken const cursor = hasMore diff --git a/packages/core/src/drives/query-escape.ts b/packages/core/src/drives/query-escape.ts new file mode 100644 index 000000000..59ab1b229 --- /dev/null +++ b/packages/core/src/drives/query-escape.ts @@ -0,0 +1,17 @@ +/** + * Escape a value for use inside a Google Drive API query string literal + * (single-quoted). Backslashes must be escaped BEFORE quotes, or the backslash + * this function adds in front of a quote would itself be doubled and the quote + * would close the literal anyway — which is the query injection (audit S5). + * + * This lives in core because BOTH halves build Drive queries: the client-mode + * `GoogleDrivePlugin` and the server-mode drive client. It used to exist only in + * `@useupup/server`, so the browser plugin interpolated folder ids raw. + * `@useupup/server` imports this one rather than keeping a second copy. + * + * Twin: `scripts/drive-sandbox/seed.mjs` escapeGDriveQueryValue — that one runs + * outside the workspace graph and must be kept in sync by hand. + */ +export function escapeDriveQueryValue(value: string): string { + return value.replace(/\\/g, '\\\\').replace(/'/g, "\\'") +} diff --git a/packages/core/src/drives/types.ts b/packages/core/src/drives/types.ts index e4eaa9bc1..42d2545ef 100644 --- a/packages/core/src/drives/types.ts +++ b/packages/core/src/drives/types.ts @@ -52,5 +52,13 @@ export type DriveEventMap = { cursor?: string } error: { error: Error; action: string } + /** + * Google Drive only, and NON-FATAL (#391): the widened root listing could not + * enumerate the user's shared drives — a 403 from a Workspace sharing policy, + * a 429, a 5xx. The listing still resolves with whatever it has, so this is + * reported separately from `error`, which means "the browse failed". Subscribe + * with `core.on('google-drive:shared-drives-error', …)` to log or surface it. + */ + 'shared-drives-error': { error: Error } 'state-change': { state: DriveState } } diff --git a/packages/core/src/internal.ts b/packages/core/src/internal.ts index cc6ec9993..f54230e70 100644 --- a/packages/core/src/internal.ts +++ b/packages/core/src/internal.ts @@ -127,6 +127,7 @@ export type { UpupResolvedTheme, DeepPartial } from './theme/types' export type { DeepPartialSlots, InternalFlatClassNames } from './theme/slots' // ── Drive internals ───────────────────────────────────────────── +export { escapeDriveQueryValue } from './drives/query-escape' export { bindDriveEvents } from './drives/bind-drive-events' export type { DriveEventCallbacks } from './drives/bind-drive-events' export type { diff --git a/packages/core/tests/drive-query-escape.test.ts b/packages/core/tests/drive-query-escape.test.ts new file mode 100644 index 000000000..c39331213 --- /dev/null +++ b/packages/core/tests/drive-query-escape.test.ts @@ -0,0 +1,41 @@ +import { describe, it, expect } from 'vitest' +import { escapeDriveQueryValue } from '../src/drives/query-escape' + +// The backslash is built rather than written literally so no assertion below +// depends on counting escape characters in this file's own source. +const BS = String.fromCharCode(92) + +describe('escapeDriveQueryValue (core — shared by the browser plugin and the server drive client)', () => { + it('leaves a value with nothing to escape untouched', () => { + expect(escapeDriveQueryValue('Team Videos 2026')).toBe( + 'Team Videos 2026', + ) + }) + + it('escapes a single quote so it cannot close the query literal', () => { + expect(escapeDriveQueryValue("it's")).toBe(`it${BS}'s`) + }) + + it('escapes every quote, not just the first', () => { + expect(escapeDriveQueryValue("O'Brien's")).toBe(`O${BS}'Brien${BS}'s`) + }) + + it('escapes a backslash by doubling it', () => { + expect(escapeDriveQueryValue(`a${BS}b`)).toBe(`a${BS}${BS}b`) + }) + + it('escapes backslashes BEFORE quotes, so an injected backslash-quote cannot neutralise the escape', () => { + // A naive quote-first implementation turns this into a${BS}${BS}' — + // a literal backslash followed by an UNESCAPED quote, which ends the + // literal and hands the rest of the value to the query parser. + expect(escapeDriveQueryValue(`a${BS}'`)).toBe(`a${BS}${BS}${BS}'`) + }) + + it('handles a value that is only escape characters', () => { + expect(escapeDriveQueryValue(`${BS}${BS}`)).toBe(`${BS}${BS}${BS}${BS}`) + }) + + it('returns an empty string unchanged', () => { + expect(escapeDriveQueryValue('')).toBe('') + }) +}) diff --git a/packages/core/tests/google-drive-plugin.test.ts b/packages/core/tests/google-drive-plugin.test.ts index 1d6733360..184f97a6a 100644 --- a/packages/core/tests/google-drive-plugin.test.ts +++ b/packages/core/tests/google-drive-plugin.test.ts @@ -929,7 +929,7 @@ describe('GoogleDrivePlugin', () => { // ── On: the shared drives are reachable, not merely queryable ── - it('appends each shared drive to the root listing as a navigable folder, after the My Drive children', async () => { + it('puts each shared drive at the TOP of the root listing as a navigable folder, ahead of the My Drive children', async () => { vi.stubGlobal( 'fetch', mockFetchSequence([ @@ -955,18 +955,116 @@ describe('GoogleDrivePlugin', () => { const result = await plugin.loadFiles() expect(result.files.map(f => f.id)).toEqual([ - 'f1', 'drive-a', 'drive-b', SHARED_WITH_ME_FOLDER_ID, + 'f1', ]) - const teamA = result.files[1]! + const teamA = result.files[0]! expect(teamA.name).toBe('Team A') expect(teamA.isFolder).toBe(true) expect(teamA.mimeType).toBe('folder') expect(teamA.thumbnail).toBeUndefined() }) + it('keeps the My Drive listing when drives.list answers 403, degrading to no shared-drive rows instead of an empty picker, and reports it on a non-fatal event', async () => { + vi.stubGlobal( + 'fetch', + vi + .fn() + .mockResolvedValueOnce({ + ok: true, + status: 200, + json: vi.fn().mockResolvedValue({ + files: [ + { + id: 'f1', + name: 'own.txt', + mimeType: 'text/plain', + }, + ], + }), + text: vi.fn().mockResolvedValue(''), + }) + .mockResolvedValueOnce({ + ok: false, + status: 403, + json: vi.fn().mockResolvedValue({}), + text: vi + .fn() + .mockResolvedValue('sharing policy forbids this'), + }), + ) + configureWithSharedDrives(true) + events.length = 0 + + const result = await plugin.loadFiles() + + // The shared-drive rows are gone, the My Drive children survive, and + // Shared-with-me stays: it is a files.list query, so a drives.list + // failure says nothing about whether that door works. + expect(result.files.map(f => f.id)).toEqual([ + SHARED_WITH_ME_FOLDER_ID, + 'f1', + ]) + expect( + events.filter(e => e.event === 'google-drive:error'), + ).toHaveLength(0) + const degraded = events.filter( + e => e.event === 'google-drive:shared-drives-error', + ) + expect(degraded).toHaveLength(1) + expect( + (degraded[0]!.payload as { error: Error }).error.message, + ).toContain('403') + }) + + it('keeps the shared drives it already collected when a later drives.list page fails', async () => { + vi.stubGlobal( + 'fetch', + vi + .fn() + .mockResolvedValueOnce({ + ok: true, + status: 200, + json: vi.fn().mockResolvedValue({ files: [] }), + text: vi.fn().mockResolvedValue(''), + }) + .mockResolvedValueOnce({ + ok: true, + status: 200, + json: vi.fn().mockResolvedValue({ + drives: [{ id: 'drive-a', name: 'Team A' }], + nextPageToken: 'drives-page-2', + }), + text: vi.fn().mockResolvedValue(''), + }) + .mockResolvedValueOnce({ + ok: false, + status: 500, + json: vi.fn().mockResolvedValue({}), + text: vi.fn().mockResolvedValue('upstream exploded'), + }), + ) + configureWithSharedDrives(true) + events.length = 0 + + const result = await plugin.loadFiles() + + expect(result.files.map(f => f.id)).toEqual([ + 'drive-a', + SHARED_WITH_ME_FOLDER_ID, + ]) + expect( + events.filter( + e => e.event === 'google-drive:shared-drives-error', + ), + ).toHaveLength(1) + expect( + events.filter(e => e.event === 'google-drive:error'), + ).toHaveLength(0) + }) + it('asks drives.list for id and name a hundred at a time', async () => { const fetchMock = mockFetchSequence([{ files: [] }, { drives: [] }]) vi.stubGlobal('fetch', fetchMock) @@ -1085,6 +1183,79 @@ describe('GoogleDrivePlugin', () => { ) }) + it('keeps the shared-drive rows ahead of a second My Drive page, which the controller appends to the end of the list', async () => { + vi.stubGlobal( + 'fetch', + mockFetchSequence([ + { + files: [ + { + id: 'page1', + name: 'a.txt', + mimeType: 'text/plain', + }, + ], + nextPageToken: 'root-page-2', + }, + { drives: [{ id: 'drive-a', name: 'Team A' }] }, + ]), + ) + configureWithSharedDrives(true) + + const first = await plugin.loadFiles() + + // Page 1 leads with the doors out of My Drive; a continuation page + // lands after everything here, so those rows stay at the top. + expect(first.files.map(f => f.id)).toEqual([ + 'drive-a', + SHARED_WITH_ME_FOLDER_ID, + 'page1', + ]) + expect(first.hasMore).toBe(true) + }) + + // ── On: query-value escaping ── + + it('escapes a quote in a folder id so it cannot close the query literal and inject syntax', async () => { + const fetchMock = mockFetchSequence([{ files: [] }]) + vi.stubGlobal('fetch', fetchMock) + configureWithSharedDrives(true) + + await plugin.loadFiles("id' or name contains 'x") + + const q = firstRequestTo(fetchMock, FILES_ENDPOINT).get('q') + expect(q).toBe( + "'id\\' or name contains \\'x' in parents and trashed = false", + ) + }) + + it('escapes a backslash in a folder id before the quotes, so the escape cannot be escaped away', async () => { + const fetchMock = mockFetchSequence([{ files: [] }]) + vi.stubGlobal('fetch', fetchMock) + plugin.setAccessToken('valid-token', 3600) + + const backslash = String.fromCharCode(92) + await plugin.loadFiles(`a${backslash}'b`) + + const q = firstRequestTo(fetchMock, FILES_ENDPOINT).get('q') + expect(q).toBe( + `'a${backslash}${backslash}${backslash}'b' in parents and trashed = false`, + ) + }) + + it('escapes the folder id on the paginated call too', async () => { + const fetchMock = mockFetchSequence([{ files: [] }]) + vi.stubGlobal('fetch', fetchMock) + configureWithSharedDrives(true) + + await plugin.loadMoreFiles( + JSON.stringify({ folderId: "dri've", pageToken: 'p2' }), + ) + + const q = firstRequestTo(fetchMock, FILES_ENDPOINT).get('q') + expect(q).toBe("'dri\\'ve' in parents and trashed = false") + }) + // ── On: the Shared-with-me virtual folder ── it('queries sharedWithMe rather than a parent for the virtual Shared-with-me folder, because Drive has no such parent', async () => { diff --git a/packages/core/tests/internal-surface.test.ts b/packages/core/tests/internal-surface.test.ts index f46927a09..18d84b631 100644 --- a/packages/core/tests/internal-surface.test.ts +++ b/packages/core/tests/internal-surface.test.ts @@ -46,6 +46,7 @@ const EXPECTED_INTERNAL_VALUE_EXPORTS: string[] = [ 'createUploaderController', 'dataURLtoBlob', 'deriveFetchedFileName', + 'escapeDriveQueryValue', 'extensionFromMime', 'fileAppendParams', 'fileCanPreviewText', diff --git a/packages/server/src/drive-clients.ts b/packages/server/src/drive-clients.ts index 49baffefc..b6b9840a5 100644 --- a/packages/server/src/drive-clients.ts +++ b/packages/server/src/drive-clients.ts @@ -8,6 +8,7 @@ // from ./oauth (acyclic DAG). import { UpupNetworkError } from '@useupup/core' +import { escapeDriveQueryValue } from '@useupup/core/internal' import { type OAuthProvider } from './oauth' export type DriveFile = { @@ -79,12 +80,18 @@ async function driveFetch( return res } -/** Escape a value for use inside a Google Drive API query string literal (single-quoted). - * Backslashes must be escaped before quotes to prevent query injection (audit S5). - * Twin: scripts/drive-sandbox/seed.mjs escapeGDriveQueryValue — keep in sync. */ -export function escapeDriveQueryValue(value: string): string { - return value.replace(/\\/g, '\\\\').replace(/'/g, "\\'") -} +/** + * Escape a value for use inside a Google Drive API query string literal + * (single-quoted), preventing query injection (audit S5). + * + * The implementation moved to `@useupup/core/internal` so the client-mode + * `GoogleDrivePlugin` uses the SAME escaper instead of a second copy that could + * drift — it had none at all and interpolated folder ids raw. Re-exported here + * because this module is where drive-query callers look for it, and the query + * builders below call it. It is IMPORTED as well as re-exported: `export … from` + * alone re-exports without binding the name in this module's own scope. + */ +export { escapeDriveQueryValue } /** * Escape a user value for an OData string literal (Microsoft Graph). OData @@ -117,7 +124,10 @@ async function listGoogleDriveFiles( accessToken: string, opts: { folderId?: string; search?: string }, ): Promise { - const parent = opts.folderId ?? 'root' + // Both halves are escaped. `search` always was; `folderId` was not, and it + // arrives from the client the same way `search` does — so a quote in it + // closed the literal exactly as one in `search` would have. + const parent = escapeDriveQueryValue(opts.folderId ?? 'root') const q = opts.search ? `name contains '${escapeDriveQueryValue(opts.search)}' and trashed = false` : `'${parent}' in parents and trashed = false` diff --git a/packages/storybook-config/src/cloudDrives.test.ts b/packages/storybook-config/src/cloudDrives.test.ts index 618f23ff4..b7e9b002c 100644 --- a/packages/storybook-config/src/cloudDrives.test.ts +++ b/packages/storybook-config/src/cloudDrives.test.ts @@ -13,12 +13,37 @@ describe('buildCloudDrives', () => { clientId: 'gid', apiKey: 'gkey', appId: 'gapp', + sharedDrives: false, }) }) + it('leaves sharedDrives off unless the env var says exactly "true", mirroring the prop default', () => { + expect(buildCloudDrives({}).googleDrive?.sharedDrives).toBe(false) + expect( + buildCloudDrives({ VITE_GOOGLE_SHARED_DRIVES: 'false' }).googleDrive + ?.sharedDrives, + ).toBe(false) + expect( + buildCloudDrives({ VITE_GOOGLE_SHARED_DRIVES: '1' }).googleDrive + ?.sharedDrives, + ).toBe(false) + }) + + it('turns sharedDrives on when the env var is "true", so a story can browse shared drives', () => { + expect( + buildCloudDrives({ VITE_GOOGLE_SHARED_DRIVES: ' true ' }) + .googleDrive?.sharedDrives, + ).toBe(true) + }) + it('defaults every provider to empty strings when env is bare (so the auth screen still renders)', () => { const cd = buildCloudDrives({}) - expect(cd.googleDrive).toEqual({ clientId: '', apiKey: '', appId: '' }) + expect(cd.googleDrive).toEqual({ + clientId: '', + apiKey: '', + appId: '', + sharedDrives: false, + }) expect(cd.oneDrive?.clientId).toBe('') expect(cd.dropbox?.clientId).toBe('') expect(cd.box?.clientId).toBe('') diff --git a/packages/storybook-config/src/cloudDrives.ts b/packages/storybook-config/src/cloudDrives.ts index cd15566c1..e64b9c987 100644 --- a/packages/storybook-config/src/cloudDrives.ts +++ b/packages/storybook-config/src/cloudDrives.ts @@ -8,6 +8,7 @@ // Set them per app in an untracked env file (e.g. apps/storybook-react/.env.local; // see the matching .env.example): // VITE_GOOGLE_CLIENT_ID, VITE_GOOGLE_API_KEY, VITE_GOOGLE_APP_ID +// VITE_GOOGLE_SHARED_DRIVES ('true' to browse shared drives + Shared with me) // VITE_ONEDRIVE_CLIENT_ID, VITE_ONEDRIVE_REDIRECT_URI // VITE_DROPBOX_CLIENT_ID, VITE_DROPBOX_REDIRECT_URI // VITE_BOX_CLIENT_ID, VITE_BOX_REDIRECT_URI @@ -18,7 +19,13 @@ // sign-in without any code change. export type CloudDrivesConfig = { - googleDrive?: { clientId: string; apiKey: string; appId: string } + googleDrive?: { + clientId: string + apiKey: string + appId: string + /** Reach shared drives, not just My Drive (#391). Default false. */ + sharedDrives?: boolean + } oneDrive?: { clientId: string; redirectUri?: string } dropbox?: { clientId: string; redirectUri?: string } box?: { clientId: string; redirectUri?: string } @@ -42,6 +49,9 @@ export function buildCloudDrives( clientId: read(env, 'VITE_GOOGLE_CLIENT_ID'), apiKey: read(env, 'VITE_GOOGLE_API_KEY'), appId: read(env, 'VITE_GOOGLE_APP_ID'), + // Off unless the env says the exact string 'true', mirroring the + // prop's own default, so a story only widens the corpus when asked. + sharedDrives: read(env, 'VITE_GOOGLE_SHARED_DRIVES') === 'true', }, oneDrive: { clientId: read(env, 'VITE_ONEDRIVE_CLIENT_ID'), diff --git a/scripts/drive-sandbox/seed.mjs b/scripts/drive-sandbox/seed.mjs index ff9c87c03..8489825fb 100644 --- a/scripts/drive-sandbox/seed.mjs +++ b/scripts/drive-sandbox/seed.mjs @@ -58,7 +58,8 @@ function headerSafeJson(value) { } /** Escape a value for a Drive query single-quoted literal (backslashes first). - * Mirrors escapeDriveQueryValue in packages/server/src/drive-clients.ts. */ + * Mirrors escapeDriveQueryValue in packages/core/src/drives/query-escape.ts + * (re-exported by packages/server/src/drive-clients.ts). */ function escapeGDriveQueryValue(value) { return value.replace(/\\/g, '\\\\').replace(/'/g, "\\'") } From 5921da4d1c3094d76fbbd0f5deee7a1c5e7efbe9 Mon Sep 17 00:00:00 2001 From: Alaeddin <15094821+BSalaeddin@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:26:09 +0100 Subject: [PATCH 3/3] chore(changeset): give the review fixes their own changeset instead of editing the one 3.3.1 already consumed --- .../google-drive-shared-drives-review.md | 27 +++++++++ .changeset/google-drive-shared-drives.md | 55 ------------------- 2 files changed, 27 insertions(+), 55 deletions(-) create mode 100644 .changeset/google-drive-shared-drives-review.md delete mode 100644 .changeset/google-drive-shared-drives.md diff --git a/.changeset/google-drive-shared-drives-review.md b/.changeset/google-drive-shared-drives-review.md new file mode 100644 index 000000000..41b82d25f --- /dev/null +++ b/.changeset/google-drive-shared-drives-review.md @@ -0,0 +1,27 @@ +--- +'@useupup/core': patch +'@useupup/server': patch +--- + +Review fixes for the Google Drive shared-drives support that shipped in 3.3.1, +behind the same default-off `cloudDrives.googleDrive.sharedDrives` flag. Nothing +changes for a picker that leaves the flag unset. + +A `drives.list` failure — a 403 under a Workspace sharing policy, a 429, a 5xx — +now degrades to no shared-drive rows instead of taking the whole root listing +down with it. The call was awaited unguarded inside `loadFiles`, so for a user +with the flag on, one non-2xx from that endpoint threw away the My Drive +children the listing had already fetched and left the picker empty. The failure +is reported on a new non-fatal `google-drive:shared-drives-error` event, and +drives collected before a mid-pagination failure are kept. + +Drive folder ids are escaped into the `files.list` query instead of interpolated +raw, using the same `escapeDriveQueryValue` the server-mode drive client uses. +That escaper moved from `@useupup/server` into `@useupup/core/internal` and +`@useupup/server` re-exports it, so the two halves share ONE implementation that +cannot drift. + +The shared-drive rows and the virtual "Shared with me" row lead the root's first +page rather than trailing it, so they stay above a second page of My Drive files +instead of being pushed below one. They are still emitted on the first page only, +so they appear exactly once and pagination is unaffected in either view. diff --git a/.changeset/google-drive-shared-drives.md b/.changeset/google-drive-shared-drives.md deleted file mode 100644 index 756df3151..000000000 --- a/.changeset/google-drive-shared-drives.md +++ /dev/null @@ -1,55 +0,0 @@ ---- -'@useupup/core': patch -'@useupup/vanilla': patch -'@useupup/server': patch ---- - -The Google Drive picker can browse shared drives and "Shared with me", behind a -default-off `cloudDrives.googleDrive.sharedDrives` flag. - -`GoogleDrivePlugin.loadFiles` and `loadMoreFiles` sent a Drive v3 `files.list` -with no `corpora`, no `includeItemsFromAllDrives` and no `supportsAllDrives`, so -the API answered from the signed-in user's own My Drive corpus only. For a -business account that is most of the person's files: a file living in a shared -drive never appeared at any depth, and the picker's search box did not -compensate because it filters the children already loaded rather than issuing a -query. The requested scope was never the limit — `drive.readonly` covers shared -drives, and `drives.list`, already. - -With `sharedDrives: true`: - -- Both listing calls send `corpora=allDrives`, `includeItemsFromAllDrives=true` - and `supportsAllDrives=true`, and the single-file download sends - `supportsAllDrives=true` so a file the widened listing surfaced can actually be - fetched instead of answering 404. `corpora` and `includeItemsFromAllDrives` are - `files.list`-only and stay off the download. -- The ROOT listing appends the user's shared drives, from a paginated - `drives.list`, as navigable folder rows after the My Drive children. Those - params widen which files a query CAN return, but every listing is still - `'' in parents` and `'root'` resolves to My Drive root — so without - an entry to click, a shared drive stayed unreachable. A shared drive's root - folder id IS its drive id, so once one is listed the ordinary parent listing - walks it with no further special-casing. -- The root listing also carries one virtual "Shared with me" folder. Drive has no - parent whose children are the files others shared with you — it is the query - `sharedWithMe = true` — so that row uses a synthetic id the plugin branches on - in both `loadFiles` and `loadMoreFiles`. Every other part of the picker treats - it as an ordinary folder. - -Both additions lead the root's first page and are never repeated on a -continuation page, so they appear exactly once, stay above a second page of My -Drive files, and pagination is unaffected in either view. - -A `drives.list` failure — a 403 under a Workspace sharing policy, a 429, a 5xx — -degrades to no shared-drive rows rather than taking the whole root listing down -with it, and is reported on a new non-fatal `google-drive:shared-drives-error` -event. Drives collected before a mid-pagination failure are kept. - -Drive folder ids are now escaped into the `files.list` query instead of -interpolated raw, using the same `escapeDriveQueryValue` the server-mode drive -client uses. That escaper moved from `@useupup/server` into -`@useupup/core/internal`, and `@useupup/server` re-exports it, so the two halves -share ONE implementation that cannot drift. - -The flag defaults off. Nothing is sent, no `drives.list` is issued and no extra -row appears when it is unset or false, so no existing picker changes shape.