fix(executor): follow drive#operation downloadUri for large Shared-Drive downloads - #907
Open
TheSecMaven wants to merge 1 commit into
Open
Conversation
…ive downloads drive.files.download on a large file in a Shared Drive can respond with a drive#operation JSON envelope naming a downloadUri to fetch the bytes from, rather than the bytes themselves. The response router had no branch for this and treated the envelope as the final JSON output, so --output silently produced no file at all. Recognize the envelope (kind: drive#operation with a downloadUri or downloadUrl) and follow it with a second, restricted-host request before handing off to the existing binary-file writer. The follow-up request only ever targets googleapis.com/storage.googleapis.com/ googleusercontent.com hosts, and only attaches our bearer token to the exact API/storage hosts that expect one -- a signed URL already carries its own auth in the query string.
🦋 Changeset detectedLatest commit: 868a408 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A ~12MB native (non-Google) file living in a Shared Drive, downloaded with the
drive.files.downloadcommand and--output, silently produces no file. Repeatedattempts against the same file oscillated between:
500 backendError— likely transient on Google's side, not addressedby this PR (see below).
drive#operation:{ "kind": "drive#operation", "response": { "downloadUri": "https://www.googleapis.com/drive/v3/files/FAKE_FILE_ID_1?alt=media&source=downloadUrl", "partialDownloadAllowed": true } }downloadUri." The executor had no branch for this shape — it treated the envelopeas an ordinary JSON response, printed it, and never wrote anything to
--output, withno error at all.
Related: #789, #727 (also reports
files.downloadreturning500 backendError"onordinary files," and its own comment confirms
drive files get --params '{"fileId":"...","alt":"media"}' --output out.pngworks as a reliable workaround —worth documenting as the recommended path for plain binary fetches regardless of this
fix). #727 also has a second-platform report of the same root cause surfacing as
411 Length Requiredon arm64, which is consistent withfiles.downloadbeing a distinct,less-exercised code path from
files.get?alt=media.This reimplements and extends the approach from the earlier #805 (closed by the
72-hour stale-bot before review, not on the merits — the review comment on it was
positive) with additional negative-case tests.
Fix
extract_google_download_urirecognizes adrive#operationresponse (kind == "drive#operation") carrying adownloadUri/downloadUrlat any of the shapesactually observed, and rejects (errors, does not silently ignore) any such URI whose
host isn't Google's own — following an arbitrary redirect from a JSON response body
and attaching our auth to it would be a real SSRF/credential-leak vector otherwise.
drive.files.downloadreturns one of these envelopes and--outputwas given,the executor now follows the
downloadUriwith a second, restricted request andhands the real response off to the existing binary-file writer, instead of treating
the envelope as the final output.
one (
googleapis.com,www.googleapis.com,storage.googleapis.com); a signed URL(query string carries
GoogleAccessId/Signature/X-Goog-*) gets neither the quotaheader nor the bearer token, since it already carries its own auth.
On the 500s
Several of the repro attempts got a
500 backendErrorwith no envelope at all. I didnot add retry/backoff for this — it isn't clearly this client's bug (issue #727 shows
the same root cause surfacing as a different error,
411 Length Required, on adifferent architecture, which points at something inconsistent server-side or in how
this specific RPC constructs its request rather than at response handling). Happy to
add bounded retry-with-backoff on
files.downloadspecifically if a maintainer wantsit, but didn't want to bundle a guess at that into a response-parsing fix.
On the S3 suggestion
A workaround using an S3 (or S3-compatible) bucket as an intermediate staging target
for large/flaky binary downloads was floated while investigating this. Not used here —
this CLI has no existing AWS dependency, and the actual bug is that the client doesn't
follow a URI Google's own API is already handing it; adding a second cloud provider's
SDK to work around that would be solving the wrong layer. Noting it here in case it's
useful context for a different, unrelated feature request.
Tests
21 new unit tests: URI extraction from the operation envelope (including a same-shaped
plain-JSON file that must NOT be treated as an operation), host validation (Google
hosts allowed, look-alike/non-https/userinfo-bearing hosts rejected), the
googleusercontent.com-allowed-but-never-bearer-token asymmetry, signed-vs-unsignedURI detection, and header construction for both the fallback quota-project header and
the download follow-up request. Full crate suite:
cargo test -p google-workspace-cli— 715 passed, 0 failed.
cargo clippy --all-targetsandcargo fmt --checkboth cleanon the new code.