feat: add read-only Google Drive source (drive:setup, drive:list, drive:get)#7
feat: add read-only Google Drive source (drive:setup, drive:list, drive:get)#7olavostauros wants to merge 5 commits into
Conversation
…st milestone) - drive:setup configures an rclone remote from a service account key; root_folder_id and scope drive.readonly are baked in at setup time so the folder scope is enforced server-side on every request - drive:list emits stable rclone lsjson output (path, name, size, MIME type, modified time, Drive file ID); supports --recursive/-r - drive:get downloads by path (rclone cat/copyto) or by Drive file ID (rclone backend copyid, --id flag); stdout and file destinations both supported - drive:_env validates env vars, key file existence, and remote configuration before any rclone call - drive:setup enforces 0600 permissions on the SA key file and rejects keys stored inside the project directory - rclone 1.74.2 added to mise [tools] Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
c0da-ricon
left a comment
There was a problem hiding this comment.
Adversarial pass: this is a useful direction for blobs, but I do not think this PR is safe or ready to merge. It adds a new external-service/auth integration and a new tool dependency, and the current implementation does not yet enforce the scoped-read-only contract it claims.
Blocking findings:
-
.mise/tasks/drive/setup:38-40uses GNUstat -c '%a'and even comments that no BSD fallback is needed because “CI is Linux.” This repo’s CI runs macOS too, and the macOS job is currently failing in the new Drive setup tests. I reproduced the same failure locally on macOS:mise run testfails 4 Drive setup tests (drive:setupsuccess, bad-perms, key-inside-repo, already-configured). Please make the permission check portable or gate it behind a helper with macOS coverage. -
.mise/tasks/drive/setup:62-69treats any existing rclone remote with the requested name as valid and exits without checking its type,root_folder_id,scope, or service-account file..mise/tasks/drive/_env:46-50has the same issue at runtime: it only checks that a remote named${GDRIVE_REMOTE}:exists. That meansGDRIVE_REMOTEcan point at an already-configured broad Google Drive remote, anddrive:list/drive:getwill use it while the PR still claims folder-scoped read-only access. For this milestone, setup/runtime need to verify the remote is actually the Drive remote this tool expects, or refuse to proceed and tell the user to reconfigure it. -
.mise/tasks/drive/get:15-26exposesdrive:get --idthroughrclone backend copyid, and the PR body correctly notes that this bypassesroot_folder_id. That conflicts with issue #4’s first-milestone requirement of “no broad Drive access.” Saying “IDs should come fromdrive:list” is not enforcement; the CLI accepts any raw Drive file ID that the service account can read. Please remove--idfor the first milestone, or add a real containment check before copying by ID. Path-based retrieval through the configured root is the safer initial shape. -
This still has no live Drive proof. The PR comment says live Google Drive API calls were not tested because no service account key was available. For an external-service/auth backend, mocked rclone argument tests are not enough to establish that
drive:setup,drive:list, anddrive:get <path>work against a shared folder with a service account. Before this is mergeable, please add a short reproduction log from a disposable Drive folder/service account, with any sensitive IDs redacted.
Validation I ran:
- Hosted CI: Ubuntu passes, macOS fails on the PR.
- Local macOS:
mise run testfails the same Drive setup area, 39/43 passing. shellcheck .mise/tasks/drive/_env .mise/tasks/drive/setup .mise/tasks/drive/list .mise/tasks/drive/getreports warnings, but I did not treat them as blockers:usage_keyis mise-provided, andDRIVE_TARGETis set by a sourced helper.rclone backend help driveconfirmscopyidis a Drive backend command and copies arbitrary IDs to a destination path, which is why the containment issue matters.
No fix-it PR from me here because the remaining changes are not just a mechanical patch: this needs a scoped-auth design decision plus a real Drive smoke. I would keep the direction, but narrow the first milestone to path-based read/list/download through a verified folder-scoped remote.
Addresses review on PR KnickKnackLabs#7 (issue KnickKnackLabs#4 milestone 1): - setup: detect SA key perms portably (stat -c with stat -f fallback) so the check works on macOS, not just Linux — was failing the macOS CI job. - _env + setup: verify the rclone remote is actually a folder-scoped, read-only Drive remote (type=drive, scope=drive.readonly, non-empty root_folder_id) before use, instead of trusting any remote with a matching name. Enforces "no broad Drive access" on every call. - get: drop --id / `rclone backend copyid`, which bypassed root_folder_id scoping. Path-based retrieval only for milestone 1. - tests: mock `rclone config show`, add remote-identity validation coverage and a --id-rejection test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two independent CI failures on PR KnickKnackLabs#7's merge ref: - drive:get accepted `--id <id>` instead of rejecting it: mise binds the unknown flag to the <key> positional, so the task ran rclone copyto (exit 0) where the test expects a non-zero exit. Reject option-style keys after _env runs, keeping path-based-only scoping (issue KnickKnackLabs#4 m1). - list (from merged KnickKnackLabs#8) expanded an empty MC_FLAGS array as "${MC_FLAGS[@]}", which is an unbound-variable error under `set -u` on macOS bash 3.2 (no-flag cases failed only on macOS). Use the ${arr[@]+"${arr[@]}"} idiom, safe when empty. All 51 tests pass locally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reopened & ready for re-review — all 4 findings addressedTechnical fixes (findings 1–3)All three technical findings from the original review were addressed in commits pushed after the review: 1. macOS CI — 2. Remote validation — Both
3. Live Drive proof (finding 4)Tested against a real GCP service account ( Test suite: |
Summary
drive:setup,drive:list,drive:gettasks backed by rclone + a Google service accountroot_folder_id) andscope drive.readonlyare baked into the rclone remote at setup time — enforced server-side on every requestdrive:getsupports path-based download (rclone cat/rclone copyto) within the configured folderdrive:setupenforces0600permissions on the SA key file and rejects keys stored inside the project directorydrive:_envguard validates env vars, key file existence, and remote configuration before any rclone callrclone 1.74.2added to mise[tools]All review findings addressed
All findings from c0da-ricon's review (2026-06-02) are resolved:
stat -cfails on BSDstat -f '%Lp'BSD fallbacksetup/_envverify type=drive, scope=drive.readonly, root_folder_id--idbypasses root_folder_id scoping--idremoved — path-based onlyCloses
Resolves first milestone of #4.
Test plan
mise run testpasses (51/51)blobs drive:setupwith a real service account key and shared folderblobs drive:listreturns JSON entries for files in the shared folderblobs drive:getdownloads a file to stdout and to a filedrive.readonlyscope (ACCESS_TOKEN_SCOPE_INSUFFICIENT)