Skip to content

Commit 5a1af8f

Browse files
committed
fix(files, chat): contain the media work's blast radius
Each of these is a side-effect of a deliberate change reaching further than the change needed. The features stay; what they touched by accident is put back. - Config files download again. Resolving unknown extensions through the canonical MIME table is what makes byte-range serving work, but it also promoted .env/.log/.conf/.ini/.cfg from "unknown bytes" to text/plain, which is on the inline allowlist — a shared .env started rendering in the tab instead of downloading. Those five join FORCE_ATTACHMENT_EXTENSIONS, so the MIME resolution stays honest and the disposition stays what it was. Verified these are the only five of the 47 newly-typed extensions that flipped. - The aggregate per-share ceiling is charged once per read, not once per seek. It is shared by every visitor to a link, so counting each range request let one person scrubbing a shared video 429 everyone else. The audit row beside it already deduped seeks; both now use one `isReadStart` predicate rather than two copies of the same string test that can drift apart. - MediaPreview clears a decode failure when the bytes change. It is keyed on `file.id`, which is stable across content edits, so one transient error wedged the player until the viewer opened a different file. Adjusted during render, the same pattern the markdown image node view uses. - The chat-trigger attachment guard accepts `dataUrl`, the preferred inline field, not just the legacy `data`. `processChatFiles` reads `dataUrl || data`, so testing only `data` skipped the upload for every current client and dropped its attachments silently — the exact failure the guard was added to prevent. - The navbar sticks below the macOS traffic-light lane instead of the viewport. Every shell mounting it wears `.desktop-title-bar-page`, which reserves the lane with padding, but sticky positions against the scroll port — so the bar slid under the traffic lights on scroll. The variable is 0px off-desktop. - The wordmark opens in a new tab on the deployed chat and the public file page. Reusing the landing navbar turned those two from `sim.ai` in a new tab into same-origin `/` in the same tab, so clicking the logo mid-conversation navigated away and destroyed it. `/` is the better href — self-hosters get their own landing — so only the target moves back, behind one `brandInNewTab` prop for surfaces hosting something the viewer would lose.
1 parent 9a96054 commit 5a1af8f

11 files changed

Lines changed: 146 additions & 8 deletions

File tree

apps/sim/app/(interfaces)/chat/components/header/header.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ export function ChatHeader({ chatConfig }: ChatHeaderProps) {
3535
return (
3636
<Navbar
3737
logoOnly
38+
brandInNewTab
3839
name={title}
3940
meta={buildSharedByLabel(chatConfig?.sharedByName ?? null)}
4041
hideBrand={Boolean(brand.logoUrl)}

apps/sim/app/(landing)/components/navbar/components/navbar-shell/navbar-shell.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,15 @@ export function NavbarShell({ children }: NavbarShellProps) {
9494
return (
9595
<NavbarFrostContext value={frost}>
9696
<div ref={sentinelRef} aria-hidden='true' className='-mb-px h-px' />
97-
<header className='sticky top-0 z-50'>
97+
{/*
98+
Sticks below the macOS traffic-light lane, not to the viewport. Every shell
99+
that mounts this navbar wears `.desktop-title-bar-page`, which reserves the
100+
lane with `padding-top` — but sticky positions against the scroll port, so a
101+
plain `top-0` slides the bar under the traffic lights the moment the page
102+
scrolls. The variable is `0px` everywhere except the desktop app, so this is
103+
`top: 0` on the web.
104+
*/}
105+
<header className='sticky top-[var(--desktop-title-bar-height)] z-50'>
98106
<div
99107
aria-hidden='true'
100108
className={cn(

apps/sim/app/(landing)/components/navbar/navbar.tsx

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,16 @@ interface NavbarProps {
9292
* Download chip) — the minimal surfaces' equivalent of the marketing CTAs.
9393
*/
9494
actions?: ReactNode
95+
/**
96+
* Open the wordmark's home link in a new tab.
97+
*
98+
* For surfaces that host something the viewer would lose by navigating away —
99+
* a deployed chat's in-progress conversation, a shared file's scroll position
100+
* and unsaved viewer state. On those the wordmark is a credit, not navigation,
101+
* so it must not take the tab with it. Everywhere else it is ordinary
102+
* same-tab navigation.
103+
*/
104+
brandInNewTab?: boolean
95105
}
96106

97107
export function Navbar({
@@ -102,6 +112,7 @@ export function Navbar({
102112
meta,
103113
hideBrand = false,
104114
actions,
115+
brandInNewTab = false,
105116
}: NavbarProps) {
106117
/**
107118
* A named resource navbar (shared file / interface / chat) drops the centered
@@ -130,6 +141,7 @@ export function Navbar({
130141
aria-label='Sim home'
131142
itemProp='url'
132143
prefetch={false}
144+
{...(brandInNewTab ? { target: '_blank', rel: 'noopener noreferrer' } : {})}
133145
className='flex h-[30px] items-center'
134146
>
135147
<span itemProp='name' className='sr-only'>

apps/sim/app/api/files/public/[token]/content/route.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import { enforcePerIpRateLimit, enforcePerShareRateLimit } from '@/lib/public-sh
1212
import { resolveActiveShareByToken } from '@/lib/public-shares/share-manager'
1313
import { downloadFile, downloadFileStream, headObject } from '@/lib/uploads/core/storage-service'
1414
import { resolveServableImageBytes } from '@/lib/uploads/server/image-derivative'
15-
import { isMediaContentType } from '@/lib/uploads/utils/byte-range'
15+
import { isMediaContentType, isReadStart } from '@/lib/uploads/utils/byte-range'
1616
import {
1717
createByteRangeResponse,
1818
createErrorResponse,
@@ -74,9 +74,16 @@ export const GET = withRouteHandler(
7474
* bucket above, and after the auth gate so a caller holding the token but
7575
* failing the gate cannot drain the ceiling for everyone else. Both apply,
7676
* and this runs before any S3 egress is spent.
77+
*
78+
* A seek within an in-progress playback is charged nothing: the ceiling is
79+
* shared by every visitor to the link, so counting each of a player's range
80+
* requests would let one person scrubbing a video 429 everyone else. Same
81+
* rule as the audit row below, from the same predicate.
7782
*/
78-
const shareLimited = await enforcePerShareRateLimit('content', resolved.share.id)
79-
if (shareLimited) return shareLimited
83+
if (isReadStart(request.headers.get('range'))) {
84+
const shareLimited = await enforcePerShareRateLimit('content', resolved.share.id)
85+
if (shareLimited) return shareLimited
86+
}
8087

8188
const { file } = resolved
8289

@@ -104,7 +111,7 @@ export const GET = withRouteHandler(
104111
* starting at byte 0) is recorded, so the trail still shows every access
105112
* without a row for each scrub.
106113
*/
107-
if (!rangeHeader || rangeHeader.startsWith('bytes=0-')) {
114+
if (isReadStart(rangeHeader)) {
108115
recordAudit({
109116
workspaceId: file.workspaceId ?? null,
110117
actorId: null,

apps/sim/app/api/files/utils.test.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -490,6 +490,28 @@ describe('media disposition', () => {
490490
})
491491
expect(response.headers.get('Content-Disposition')).toContain('attachment')
492492
})
493+
494+
/**
495+
* Resolving unknown extensions through the canonical MIME table — which is what
496+
* makes byte-range serving work — promoted these from "unknown bytes" to
497+
* `text/plain`, which is on the inline allowlist. They downloaded before the
498+
* media work and must keep downloading after it.
499+
*/
500+
it.each(['app.env', 'server.log', 'nginx.conf', 'settings.ini', 'tool.cfg'])(
501+
'keeps %s downloading rather than rendering it in the tab',
502+
(filename) => {
503+
const response = createFileResponse({
504+
buffer: Buffer.from('SECRET=1'),
505+
contentType: getContentType(filename),
506+
filename,
507+
})
508+
expect(response.headers.get('Content-Disposition')).toContain('attachment')
509+
}
510+
)
511+
512+
it('still resolves those extensions to a real MIME type', () => {
513+
expect(getContentType('server.log')).toBe('text/plain')
514+
})
493515
})
494516

495517
describe('createByteRangeResponse', () => {

apps/sim/app/api/files/utils.ts

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,30 @@ const SAFE_INLINE_TYPES = new Set([
202202
'application/json',
203203
])
204204

205-
const FORCE_ATTACHMENT_EXTENSIONS = new Set(['html', 'htm', 'js', 'css', 'xml'])
205+
/**
206+
* Served as an opaque download regardless of what their bytes look like.
207+
*
208+
* `html`/`htm`/`js`/`css`/`xml` because rendering them inline is an execution
209+
* vector. The config-file group is here for a different reason: resolving an
210+
* extension through the canonical MIME table (rather than defaulting everything
211+
* unknown to `application/octet-stream`) is what makes byte-range serving work,
212+
* but it also promoted these five from "unknown bytes" to `text/plain`, which is
213+
* on the inline allowlist. A shared `.env` that used to download would otherwise
214+
* start rendering in the tab. Nothing about the media work needs that, so the
215+
* previous behaviour is kept explicitly.
216+
*/
217+
const FORCE_ATTACHMENT_EXTENSIONS = new Set([
218+
'html',
219+
'htm',
220+
'js',
221+
'css',
222+
'xml',
223+
'cfg',
224+
'conf',
225+
'env',
226+
'ini',
227+
'log',
228+
])
206229

207230
function getSecureFileHeaders(filename: string, originalContentType: string) {
208231
const extension = filename.split('.').pop()?.toLowerCase() || ''

apps/sim/app/api/workflows/[id]/execute/route.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1450,14 +1450,21 @@ async function handleExecutePost(
14501450
* same way the deployed chat route does, so the start block receives the
14511451
* `UserFile[]` it normalizes; anything else it silently drops.
14521452
*
1453-
* Already-uploaded files carry a storage `key` and no `data`, so a
1453+
* Already-uploaded files carry a storage `key` and no inline payload, so a
14541454
* re-submitted payload passes through untouched.
1455+
*
1456+
* `dataUrl` is the preferred inline field and `data` the legacy one — the
1457+
* same precedence `processChatFiles` itself applies. Testing only `data`
1458+
* would skip the upload for every current client and drop its attachments
1459+
* silently, which is the failure this pass exists to prevent.
14551460
*/
14561461
if (triggerType === 'chat') {
14571462
const chatFiles = (processedInput as { files?: unknown } | null | undefined)?.files
14581463
const pending =
14591464
Array.isArray(chatFiles) &&
1460-
chatFiles.some((file) => file && typeof file === 'object' && 'data' in file)
1465+
chatFiles.some(
1466+
(file) => file && typeof file === 'object' && ('dataUrl' in file || 'data' in file)
1467+
)
14611468
if (pending) {
14621469
const uploaded = await ChatFiles.processChatFiles(
14631470
chatFiles as Parameters<typeof ChatFiles.processChatFiles>[0],

apps/sim/app/f/[token]/public-file-view.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ export function PublicFileView({
6464
<DesktopTitleBarLane />
6565
<Navbar
6666
logoOnly
67+
brandInNewTab
6768
name={name}
6869
meta={buildSharedByLabel(ownerName)}
6970
hideBrand={Boolean(brand.logoUrl)}

apps/sim/components/resources/file-view/file-view.tsx

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -475,6 +475,19 @@ const MediaPreview = memo(function MediaPreview({
475475
? fileContentUrl(source, file.key, { version: file.updatedAt.getTime() })
476476
: null
477477

478+
/**
479+
* Clear a previous failure when the bytes change. Adjusted during render rather
480+
* than in an effect so the error state never paints for a frame over content
481+
* that has already been replaced. The parent keys this on `file.id`, which is
482+
* stable across content edits, so without this a single transient decode error
483+
* would wedge the player until the viewer opened a different file.
484+
*/
485+
const [prevSrc, setPrevSrc] = useState(src)
486+
if (prevSrc !== src) {
487+
setPrevSrc(src)
488+
setFailed(false)
489+
}
490+
478491
if (!src || failed) {
479492
return <PreviewError label={kind} error={`This ${kind} could not be played.`} />
480493
}

apps/sim/lib/uploads/utils/byte-range.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { describe, expect, it } from 'vitest'
55
import {
66
byteRangeLength,
77
contentRangeHeader,
8+
isReadStart,
89
parseByteRange,
910
unsatisfiableContentRangeHeader,
1011
} from '@/lib/uploads/utils/byte-range'
@@ -106,3 +107,30 @@ describe('header builders', () => {
106107
expect(byteRangeLength({ start: 0, end: 499 })).toBe(500)
107108
})
108109
})
110+
111+
describe('isReadStart', () => {
112+
it('treats an unranged request as opening a read', () => {
113+
expect(isReadStart(null)).toBe(true)
114+
expect(isReadStart(undefined)).toBe(true)
115+
expect(isReadStart('')).toBe(true)
116+
})
117+
118+
it('treats a range anchored at byte 0 as opening a read', () => {
119+
expect(isReadStart('bytes=0-')).toBe(true)
120+
expect(isReadStart('bytes=0-1023')).toBe(true)
121+
expect(isReadStart(' bytes=0- ')).toBe(true)
122+
})
123+
124+
/**
125+
* The audit trail records one row per playback and the aggregate per-share
126+
* ceiling charges one token per playback, both off this predicate. A seek is a
127+
* continuation of a read already accounted for — counting it would write a row
128+
* per drag, and let one viewer scrubbing a shared video spend the whole link's
129+
* minute budget on everyone else's behalf.
130+
*/
131+
it('treats a seek into the object as a continuation', () => {
132+
expect(isReadStart('bytes=1024-')).toBe(false)
133+
expect(isReadStart('bytes=500-999')).toBe(false)
134+
expect(isReadStart('bytes=-512')).toBe(false)
135+
})
136+
})

0 commit comments

Comments
 (0)