chore: promote dev to master (3.3.2 — shared-drives review fixes)

.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.

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=

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=

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=

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=

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<DriveFile[]> {
         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

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, "\\'")
+}

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 }
 }

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 {

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('')
+    })
+})