feat(aws-control): file preview, rename, and filename search - #8236
Conversation
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All hunks are now accounted for: three drive features (preview, rename, search) plus a substantial security-fence apparatus (masked staging dir enforced at every agent spawn, a new First-Principles-Verdict: CONCERNS The 256 KB text preview drags a five-file spawn-blocking staging fence into core sandbox/security code, when the gateway could fetch its own presigned URL in memory. What this change shipsIntent: open, rename, and find drive files without downloading them — an ADDITION.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 833727e |
UX Review (Fable 5) — ✅ PASSUX-level review of The implementation is thorough — complete loading/empty/error/capped states, keyboard paths everywhere, careful error routing per row, honest copy. One residual gap worth noting: a rename from search results whose new name no longer matches the query makes the row silently vanish. Final review: UX-Verdict: PASS Every surface carries its states, errors land on the row that caused them, and labels keep their promises — a first-time user can search, preview, rename, and recover. Suggestions
[UX-REVIEWED] 833727e |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound feature slice, but search is an unbounded full-section walk per query, and the disk-staging fence may be heavier than the problem required. Watch
Suggestions
[DESIGN-REVIEWED] 833727e |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
f65801a to
2b73f92
Compare
b51f4cf to
ac9c082
Compare
ac9c082 to
c6b7a60
Compare
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
c6b7a60 to
4712439
Compare
bolichen97
left a comment
There was a problem hiding this comment.
Deep review (escalation: feat:) — approved.
Path traversal: clean. Preview reuses validate_key (rejects ../empty segments, 900-char cap, per-segment allowlist regex), section_key prepends a fixed prefix, and every AWS call is an argv list through _checked with no shell. Preview temp file is mkstemp and unlinked in finally.
Authz: unchanged. Both new GETs wear the existing _guarded (app-enabled + owner-dashboard) and _require_drive (account resolution + S3 consent gate).
Egress: preview text and search keys pass through redact_credentials + redact_exfiltration_urls, the same double pass as listing. Text renders in
{content} — React-escaped, no HTML sink.
Confinement, net improvement: PDF iframe is sandbox="" and new upload Content-Type is an allowlist denying text/html and image/svg+xml, leaving them at S3 opaque default. Tests pin .html, .htm, .svg, .txt, .js.
Skipping _publish_gate on preview is correct: it mints no bearer URL, unlike download.
CI is fully green on 4712439 — 60+ checks including CodeQL, Semgrep SAST, all 5 AI reviewers.
Two follow-ups (neither blocking): (1) /drive/{account}/preview accepts section=backup and section=library while download refuses backup and search refuses non-drive sections — preview will return the first 256 KB of a backup archive, owner-only and redacted but a consistency gap. (2) search_keys caps hits at SEARCH_MAX_RESULTS but not pages: a no-match query walks every 1000-object page sequentially; worth a page or wall-clock bound.
|
Approved (see review above) but merge conflicts developed — likely from the other PRs that landed on main in this batch. Could you rebase onto current main and push? The approval stands once the head is updated. |
|
/ai-review override gpt bc70b5f: Search hands keys back through the same egress redaction main's list_section already applies (storage.py _safe_name, pinned by test_names_are_run_through_the_egress_redactors): a credential-shaped key is deliberately unusable from the dashboard on every surface so the credential never reaches the browser — that is the security invariant, not corruption — and an opaque action-token scheme would be a design change for both surfaces, out of scope for this PR. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
Preview: images/video/audio/PDF render from the presigned download URL (CORS-exempt tags); text files read through a new gateway preview endpoint (256KB head, the bucket has no CORS for a browser fetch). Rename is a same-directory move through the existing move endpoint, so every move guarantee (no overwrite, live-share refusal) applies for free. Search walks the whole section server-side, case-insensitive, capped at 200 hits. Uploads now set Content-Type from the file name so new PDFs/videos render inline instead of downloading.
chenmingwei23
left a comment
There was a problem hiding this comment.
Approved: checks green at head, readiness passed, no blocking reviewer verdict outstanding.
Summary
Three everyday file-drive capabilities that the cloud drive was still missing after drag-and-drop landed (#7983): open a file without downloading it, rename it in place, and find it by name across the whole file drive rather than only the open folder.
Preview. A
PreviewDialogopens from the row / tile. Images, video, audio and PDF render inline through the existing presigned URL (those tags are exempt from CORS, so no proxy is needed); the PDF<iframe>carries an emptysandbox, because the branch is chosen by extension and a.pdfkey holding HTML must not run script or navigate the dashboard. Text-like files go through a newGET /drive/{account}/previewroute that reads the first 256 KB server-side — the browser cannot fetch object bodies from S3 directly because the bucket has no CORS policy — and the dialog says so when the object was truncated. The text passes through the same egress redactor as listing and search names, so a notes file holding an access key renders masked. Types we cannot render degrade to an honest "download to view" state instead of a broken frame. The media presign is the existing 60-second download grant; a clip longer than that 403s mid-play, so a media error re-mints the URL (another short grant, not a longer one) and resumes from the playback position; each successful load re-arms it so a long clip keeps playing across grants, and only two errors with no load between them fall back.put_filenow sets--content-typeon upload — but only for the inline-safe set the dialog embeds by URL (image/*minus SVG,video/*,audio/*,application/pdf). HTML, SVG and text stay on S3's opaque default on purpose: a storedtext/htmlwould make a shared or downloaded object render as a live document on the bucket origin, script included, while the same file opened in-app is inert bytes in the text preview.Rename. Inline rename on both the list rows and the grid tiles, at parity. It is a thin client over the
moveAPI from #7983 — same key-level lock coordination, same error codes — but the refusal wording is rename's own ("A file with this name already exists here", not "the destination folder…"), because the user pressed Rename and should not be told about a move they never made.Search. A toolbar field (300 ms debounce) backed by a new
GET /drive/{account}/searchroute: case-insensitive substring match on the section-relative key (folder path included), walked over the file-drive section with token pagination — the route refuses any other section (section_not_searchable): the library and backups keep their own listing surfaces — capped atSEARCH_MAX_RESULTS(200) hits with an early stop;cappedis set only when a match past the cap is seen, so exactly 200 hits is a complete result, not a truncated one. The response echoescappedand the cap itself, so the "showing the first N" notice interpolates the server's number and no locale spells it. Results sit under the same shared table head as the folder listing inside a horizontally scrolling container (narrow viewports scroll instead of pushing the actions off-screen), show the full relative key, and keep their actions behind the same labeled overflow menu the file rows use — Download, and a worded "Open containing folder" that navigates to the prefix and clears the search. Results pass through the same egress redactor as listings.Thirteen new i18n keys × 12 locales + en-XA. The aws-control feature spec documents both new routes and the content-type behaviour.
What was tested
npx tsc -b— 0 errors.npx vitest run src/apps/aws-control src/test/i18n src/i18n/style— 381/381 passing, including 10 new pinned tests (text preview through the proxy route with truncation notice, media preview via presigned URL, one-shot presign re-mint on media error then honest fallback, unsupported-type fallback, list and grid rename round-trip throughmove, rename-specific wording for share-active and name-collision refusals, grid tile hides its preview trigger while renaming, search rendering + server-echoed cap in the notice + go-to-folder navigation).pytest test/test_aws_control_routes.py test/test_aws_control_storage.py test/test_aws_control_app.py— 349 passing, including the P0 route-contract tests for the two new routes, the drive-only scope of/search, the 256 KB truncation boundary, credential redaction on preview text (the same egress floor listing and search names already pass through), search pagination/early-stop at the cap (and exactly-the-cap is not reported as capped), and the inline-safe content-type allowlist input_file(PDF/PNG/MP4 declared; HTML/SVG/TXT/JS stay opaque).aria-labelonvideo/audio; two documented exemptions forimg onErrorand the rename<tr>key handler).Demo
Text preview → image preview → inline rename (list updates live) → search for
quarterlyhitsdocs/deep/quarterly-report.mdwith download and go-to-folder actions.Notes
No linked issue: this is the next slice of the cloud-drive series (after #7825 and #7983), scoped in-chat with the PM.
UX review follow-ups
Every CONCERNS finding from the UX lane is implemented in this PR:
Search mode now withdraws the folder-scoped write controls (Upload, New folder, the section drop zone) together with the crumbs, so nothing can land in a folder the reader cannot see.
The grid/list toggle is hidden while searching (hits always render as a table).
The text preview reports
redactedalongsidetruncated; the dialog shows "Sensitive values are masked in this preview." whenever the egress redactor fired.The search box reads "Search file names…" (name-only search, never contents) in all 13 catalogs.
Search hits carry the same overflow menu as file rows (Download, Rename, Share, Delete) plus "Open containing folder"; rename and delete run in place against the hit's own path and re-run the search.
Dropping an OS file mid-search is swallowed (the section keeps an inert
dragover/droppreventDefault) instead of navigating the tab away; nothing is uploaded.A refinement keeps the previous hits on screen but dims them, marks them
aria-busy, and shows "Searching…" until the new walk returns.Not re-recorded here: the evidence GIF's blank first frame. Re-recording needs a live pod; the existing GIF stays as evidence and a re-capture is a follow-up.
Third UX pass (suggestions on the PASS verdict): the search-hit delete confirmation names the full relative key; "Open containing folder" moved above Delete with a separator; the search box reads "Search by file name or path…" since the match runs over the whole key.
Fourth UX pass: a download that fails from the preview header is now reported inside the dialog (the pane notice sits behind the scrim). Preview redaction now runs over the window plus an 8 KB look-ahead and trims at a whitespace boundary afterwards, so a secret straddling the 256 KB window is masked whole instead of reaching the browser as an unrecognised prefix.
Fifth UX pass: the download failure is keyed by object, so a preview shows only its own file's failure; the search box widened so the placeholder is not clipped; the empty search state echoes the query ("No files match “…”.") like the account search does.
Sixth UX pass: a search hit truncates its folder prefix (muted) and keeps the basename whole, with the full key on the title; changing the query closes any rename/delete editor left open on a row that is about to unmount. Preview decode + redaction now run off the event loop, like the search redaction.
Seventh UX pass: a
.pdfwhose stored Content-Type is notapplication/pdf(legacy octet-stream uploads) routes to the "cannot be previewed" fallback instead of a blank frame -- the download response now carriescontentTypefrom the HEAD it already makes; clearing the search skips the debounce; the rename editor has its own bad-name wording. Staging moved to a top-level data-home leaf (aws-control-staging) so no agent-writable ancestor sits between the mask and the root.Eighth UX pass: the search-table rename/delete strips get the folder rows' sticky-left viewport-width wrapper (Cancel/Rename stay on-screen on a narrow, horizontally scrolled viewport); "Open containing folder" marks the hit's row or grid card for a few seconds and scrolls it into view once the listing loads.
Rename completion is scoped to the row that started it: an editor opened on another row while a rename is in flight keeps its half-typed name on success, and a failure for the earlier row is reported in the page-level notice naming the file (
rename_failed_named) rather than under the wrong row or not at all.Preview staging is identity-pinned for the host with no sandbox mask (Windows): the gateway creates the destination itself with
O_EXCL, holds the handle open across the CLI call (no delete/rename of the file or any directory above it), re-checks device/inode/link count afterwards, and reads back through the handle rather than reopening the path.Ninth UX pass: the "Open containing folder" highlight window is counted from the moment the row mounts (inside the callback ref), not from the click, so a slow CLI-backed listing no longer spends the window before there is anything to mark; navigating to any other folder retires the marker.
Late failures always have a surface: a delete rejected after its confirmation strip is gone (view swapped to search, or moved to another row) is reported in the page-level notice naming the file (
delete_failed_named); a query change now closes in-place editors even with a commit in flight, and that commit's outcome takes the same page-level route.Tenth UX pass: a search hit that sits past the first listing page is paged in automatically until its row mounts (or the folder runs out), so the "Open containing folder" marker works in large folders, not only small ones.
Staging pin now covers the whole path: the staging root and the per-call directory are held open through a new
platform_compat.pin_directory(Windows: a handle withoutFILE_SHARE_DELETEthat refuses a reparse point at the name and blocks rename/delete of the directory and everything above it; POSIX:O_DIRECTORY|O_NOFOLLOW, used asdir_fdfor the create) before the destination is named, so a watcher cannot rename the directory away and plant a junction between the create and the CLI's open.Eleventh UX pass: opening a delete confirmation strip resets the shared delete mutation first, so a failed delete on one file no longer pre-renders "Delete failed" under a strip opened later on another file.
Advisory findings taken: a 0-byte object (416
InvalidRangeon the ranged GET) previews as empty instead of failing; the inline content-type round-trip test now covers theaudio/prefix too.Twelfth UX pass: the preview header shows the folder muted beside a nested key's name (two same-named search hits stay distinguishable once open); the redaction notice reads at body weight since it changes the meaning of what is shown. Advisory: the preview window is now a UTF-8 byte budget (multibyte text no longer carries the look-ahead past it); the inline delete error is keyed to the row whose delete it is, so a failure on A never renders under a strip opened on B.
Thirteenth UX pass:
preview_failednow says "Download the file to view it, or try again." so the copy and the Try again button beside it point the same way (13 catalogs).Fourteenth UX pass: "Open containing folder" moves keyboard focus onto the highlighted row as it mounts (
tabIndex={-1}+focus()), since the menu trigger unmounted with the search view and the ring alone is a cue assistive technology never announces.Mask materialisation is fail-closed all the way: a data home that cannot be resolved now refuses the spawn (
SandboxCeilingUnsealable) instead of skipping the staging mask and launching the agent with the directory visible.