refactor(resources): one canonical view per resource, mounted on source/grants/host - #6449
refactor(resources): one canonical view per resource, mounted on source/grants/host#6449waleedlatif1 wants to merge 3 commits into
Conversation
Nine findings, four of them mine from earlier in this branch.
**The R6 guarantee was not enforced.** `check-resource-views.ts` reported
`R6 … 0 (at baseline)` while a real violation sat in the tree:
`useParams<{ workspaceId: string }>()` in `add-connector-modal.tsx`. The pattern
demanded `(` immediately after the hook name, so the generic form — the dominant
idiom in this repo, quoted in two of the rule's own TSDoc blocks — was invisible.
Widened the pattern, watched it go 0 → 1, threaded `workspaceId` in as a prop
exactly as its sibling modal already documents, confirmed 0.
**An inline-image fan-out charged against a whole-file budget.** Every embedded
image spent a `content` token — 60/min, sized for downloading a file. A
thirty-image shared document spent half a reader's minute on one page view and
the second view inside that minute returned 429, which renders as broken images
with no stated reason. Added an `inline` scope denominated in what actually
happens (`INLINE_IMAGES_PER_VIEW * INLINE_VIEWS_PER_MINUTE`).
**The per-share ceiling equalled the per-IP budget it backstops.** Both 60/min
for `content`, so one visitor at full rate saturated the link and the aggregate
bound before the bucket it exists to protect. Now derived from the per-IP config
rather than written out, so `aggregate > per-IP` holds by construction. Dropped
the `execute` tier — no caller, no consumer of its type.
**My `.webm` fix reasoned from a false premise.** Routing `getContentType`
through `resolveEffectiveMimeType` was a no-op: `contentTypeMap` was consulted
first, so `DUAL_CONTAINER_MIME` never ran. The real defect was that
`contentTypeMap` duplicated 38 of `EXTENSION_TO_MIME`'s entries — drift between
two hand-maintained tables is what left `.mkv`/`.flac`/`.aac`/`.opus`/`.avi` at
`application/octet-stream`, unseekable. Collapsed to the three Google
pseudo-extensions no MIME table knows. Verified empirically before collapsing:
exactly five resolutions change, three are those pseudo-types, and `js`/`ts`
move to the WHATWG spellings while staying attachments either way. The TSDoc's
`.mp4` example was also false — staging already had that entry.
Also: one home for `REVALIDATE_CACHE_CONTROL` (four copies, three files);
`getContentType` no longer computed twice per serve branch; a dead default
parameter, a dead `binaryExtensions` export, and two TSDoc blocks that had
drifted above the wrong function; `toError` in place of the banned
`instanceof Error ? … : new Error(…)` in the three routes this branch touches.
New tests for `lib/public-shares`, which had none — 12 pinning the
aggregate-exceeds-per-IP invariant, the inline fan-out budget, bucket
separation and the 429 shape. Verified they fail against the original bug:
setting the multiple back to 1 turns two of them red.
Suite: 21113 passed. R6/R3c 0, 23 audits, lint and type-check clean.
Four conflicts, all from staging changing files this branch had moved. The one that mattered: staging added `assertOoxmlPreviewWithinLimits` — an OOXML zip-bomb guard — to the docx and xlsx preview paths. Both files moved into the file-view unit here, so the import came back as a conflict while the call sites auto-merged. Kept both; a security guard silently lost to a rename is exactly what these merges are for. `base-tags-modal` picked up staging's move of `FIELD_TYPE_LABELS` and `KNOWLEDGE_TAG_DISPLAY_NAME_MAX_LENGTH` into shared constants. Its new `getDocumentIcon` import pointed at the duplicate icons module this branch deleted as byte-identical to `@/components/icons/document-icons`; repointed there, which R3c would have required regardless. `bubble-menu-chrome.ts` was added by staging inside a renamed directory — placed at the new path. Suite: 21136 passed.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Too many files changed for review (627 files, 500 file limit). |
|
@cursor review |
PR SummaryHigh Risk Overview Public surfaces & chrome — Auth, 404/ Public file APIs — Workflow chat execution — For Misc — Workspace Reviewed by Cursor Bugbot for commit aed98d1. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit aed98d1. Configure here.
| if (!isMedia || isReadStart(rangeHeader)) { | ||
| const shareLimited = await enforcePerShareRateLimit('content', resolved.share.id) | ||
| if (shareLimited) return shareLimited | ||
| } |
There was a problem hiding this comment.
Range header bypasses share ceiling
Medium Severity
For media, enforcePerShareRateLimit runs only when isReadStart is true. Any Range that does not start at byte 0 — including bytes=1- or a suffix range — skips the aggregate ceiling entirely while still returning nearly the full object. That lets callers drain S3 egress across many IPs without ever hitting the per-share backstop this PR added.
Reviewed by Cursor Bugbot for commit aed98d1. Configure here.
| const disposition = | ||
| SAFE_INLINE_TYPES.has(safeContentType) || isMediaContentType(safeContentType) | ||
| ? 'inline' | ||
| : 'attachment' |
There was a problem hiding this comment.
Media forced inline for downloads
Medium Severity
getSecureFileHeaders now marks all audio/* and video/* responses as inline so players can render them. The public Download chip still hits the same /content URL with no attachment override. Browsers that honor Content-Disposition: inline over the anchor download attribute will play or navigate instead of saving the file.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit aed98d1. Configure here.
1 participant
Summary
Every resource — file, table, log, knowledge — now has exactly one view, and every consumer mounts that one. The workspace page and the mothership panel render the same component; neither imports the other's implementation.
apps/sim/resources/(pure TS, no React) replace the ad-hoc props:source(workspace vs share, discriminated onvia),grants(write/run/manage/settled),host(page/panel/public)@/app/workspace/[workspaceId]/{tables,knowledge,files,logs}— that coupling is what the axes exist to deletescripts/check-resource-views.tsenforces it: no wrappers, no imports past a unit barrel, no route/permission context inside a unit, noworkspaceId/canEdit/embeddedprops on a viewworkspaceId, and a kind whose seed isnever(table, knowledge, log) cannot be constructed anonymously at alltablesandknowledgewere severed from route context in place first (router →onNavigate, params →source, permission context →grants), so every semantic edit is reviewable against unmoved files and the move commits are import-only.Fixes found while reviewing this branch
0while a realuseParams<{ workspaceId }>()sat inside a canonical unit — the pattern required(immediately after the hook name, so the generic form (the dominant idiom here) was invisible. Widened, watched it go red, fixed the leak.inlinescope denominated in page views.aggregate > per-IPholds by construction..mkv/.flac/.aac/.opus/.aviatapplication/octet-stream— unseekable. Collapsed to the three Google pseudo-extensions; verified empirically that exactly five resolutions change and none flips a script or markup type inline.Intended behavior changes
Two, both stopping the panel writing to its host's URL:
sort/dir/table-viewinto/home's address bar. Both route pages keep their deep-linkable params unchanged?tab=traceonto the tables URLEverything else is byte-identical.
grants.settledexists socanEdit || isLoadinggating survives exactly rather than flickering on first paint.Type of Change
Testing
21136 tests passing.
check:resources:strict,check:api-validation,check:audits(23), type-check and lint green. New tests forlib/public-shares(had none) pinning the rate-limit invariants — verified they fail against the original bug.Not browser-verified — the two URL-ownership changes and the header chrome on each surface are what to click through.
Checklist