Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion docs/docs/uploads.md
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,17 @@ Resolve `avatarUrl` to the upload id (a plain `avatarUrl: (user) => user.avatarI

Signed URLs default to `Content-Disposition: attachment`. The stored MIME type is whatever the client claimed, and an inline `image/svg+xml` served from your own origin is a stored cross-site-scripting vector, so inline rendering is opt-in. Pass `disposition: 'inline'` to `createWithSignedUrlDirective()` in the generated directive file when your profiles only admit types you trust to render.

Filesystem targets serve files through the api's serve route, which sends active content (HTML, SVG, XML, JavaScript and PDF) as an attachment even when the signed URL asks for inline. To preview PDFs in the browser, list them in the plugin's `inlineTypes` option:

```ts
await server.register(cedarUploadsPlugin, {
// ...
inlineTypes: ['application/pdf'],
})
```

Only list types you're comfortable rendering from your app's origin. S3 targets return presigned URLs to the bucket, so the disposition signed into the URL is always used there.

You can do the same thing in a service. Object-storage targets sign a URL; a database target has no URL, so read its bytes into a `data:` URI instead:

```ts
Expand Down Expand Up @@ -362,7 +373,7 @@ main()
| `/upload/webhook/s3` | POST | S3 event notifications via SNS (only with the `s3Webhook` option) |
| `/upload/health` | GET | Lists the configured targets |

`authenticate` resolves the requesting user from the request's auth header using the same decoder and `getCurrentUser` the GraphQL server uses. With it configured, the upload routes reject unauthenticated requests and tokens issued to someone else, so a leaked token can't be spent by anyone but its owner. The setup command wires it up when your app has auth. Without it, the token itself is the only identity on the route, which is only appropriate for apps with no auth at all. Other options: `prefix` (default `/upload`), `bodyLimit` (an outer ceiling, default 500 MB; the effective limit per request comes from the token), and `serveCacheControl`.
`authenticate` resolves the requesting user from the request's auth header using the same decoder and `getCurrentUser` the GraphQL server uses. With it configured, the upload routes reject unauthenticated requests and tokens issued to someone else, so a leaked token can't be spent by anyone but its owner. The setup command wires it up when your app has auth. Without it, the token itself is the only identity on the route, which is only appropriate for apps with no auth at all. Other options: `prefix` (default `/upload`), `bodyLimit` (an outer ceiling, default 500 MB; the effective limit per request comes from the token), `serveCacheControl`, and `inlineTypes` (see [Reading files back](#reading-files-back)).

The FS route rejects early: a `Content-Length` beyond what the token allows is refused before any body is read, and files are aborted mid-stream once they pass the profile's `maxFileSize`. Each file's row is created before its bytes are written, so a crash in between leaves a pending row the cleanup job can find rather than an orphaned file.

Expand Down
72 changes: 72 additions & 0 deletions packages/uploads/src/__tests__/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,78 @@ describe('cedarUploadsPlugin', () => {
expect(res.headers['content-disposition']).toMatch(/^attachment;/)
})

describe('with inlineTypes', () => {
let inlineApp: FastifyInstance

beforeEach(async () => {
inlineApp = Fastify()
await inlineApp.register(cedarUploadsPlugin, {
tokenSecret: SECRET,
targets,
db,
inlineTypes: ['Application/PDF'],
})
await inlineApp.ready()
})

afterEach(async () => {
await inlineApp.close()
})

const servePdf = async (
server: FastifyInstance,
disposition: 'attachment' | 'inline',
) => {
const upload = await storeFile(targets.local, {
db,
filename: 'report.pdf',
mimeType: 'application/pdf',
data: Buffer.from('%PDF'),
})

const url = await targets.local.getSignedReadUrl(upload.storageKey!, {
disposition,
})

return server.inject({ method: 'GET', url })
}

test('serves a listed active type inline when inline was signed', async () => {
const res = await servePdf(inlineApp, 'inline')

expect(res.statusCode).toBe(200)
expect(res.headers['content-disposition']).toMatch(/^inline;/)
Comment on lines +452 to +456

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 MIME parameters lack test coverage

The new tests cover mixed case in inlineTypes, but neither the option nor the stored MIME type includes parameters. Add cases with parameters on each side, plus a mixed-case stored type. Otherwise, breaking the promised parameter-insensitive matching could pass these tests and make PDF previews download instead.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not adding these. A stored type can't carry parameters: storeFile rejects a MIME type like application/pdf; version=1.7 as invalid, so a stored type with parameters can't reach the serve route. Case-insensitive matching is already covered through inlineTypes, and both sides go through the same normalize helper, so a stored-side case test would exercise the same code path again.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. storeFile rejects MIME types containing parameters, so a parameterized stored MIME value isn't reachable through that path. Since normalizeMimeType is shared by both inlineTypes and the serve-side value, the existing mixed-case inlineTypes test already covers the case-insensitive normalization behavior. I'll withdraw the request for those additional tests.

})

test('keeps attachment when the signed URL asks for it', async () => {
const res = await servePdf(inlineApp, 'attachment')

expect(res.headers['content-disposition']).toMatch(/^attachment;/)
})

test('still forces attachment for unlisted active types', async () => {
const upload = await storeFile(targets.local, {
db,
filename: 'sneaky.svg',
mimeType: 'image/svg+xml',
data: Buffer.from('<svg/>'),
})

const url = await targets.local.getSignedReadUrl(upload.storageKey!, {
disposition: 'inline',
})
const res = await inlineApp.inject({ method: 'GET', url })

expect(res.headers['content-disposition']).toMatch(/^attachment;/)
})

test('forces attachment for PDFs when they are not listed', async () => {
const res = await servePdf(app, 'inline')

expect(res.headers['content-disposition']).toMatch(/^attachment;/)
})
})

test('answers 404 for forged tokens, missing rows, and missing files', async () => {
const forged = await app.inject({
method: 'GET',
Expand Down
31 changes: 23 additions & 8 deletions packages/uploads/src/fastify/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,11 @@ const MULTIPART_ENVELOPE_ALLOWANCE = 64 * 1024

/**
* MIME types a browser executes or renders as a document when served
* inline from this origin. They are always served as attachments, even when
* the signed URL asked for inline, because the stored MIME type is
* inline from this origin. They are served as attachments, even when the
* signed URL asked for inline, because the stored MIME type is
* client-asserted and an inline document from the app's origin is stored
* cross-site scripting.
* cross-site scripting. The plugin's `inlineTypes` option exempts specific
* types.
*/
const ACTIVE_CONTENT_TYPES = new Set([
'text/html',
Expand Down Expand Up @@ -88,6 +89,14 @@ export interface UploadPluginOptions {
}
/** `Cache-Control` for served files. Defaults to `private, max-age=3600`. */
serveCacheControl?: string
/**
* Active-content MIME types the serve route may send inline when the
* signed URL asks for `inline`, e.g. `['application/pdf']` to preview PDFs
* in the browser. Every other active type is always served as an
* attachment. Only list types you're comfortable rendering from the app's
* origin, keeping in mind that the stored MIME type is client-asserted.
*/
inlineTypes?: string[]
}

function sendError(reply: FastifyReply, e: unknown) {
Expand All @@ -105,8 +114,8 @@ function headerValue(req: FastifyRequest, name: string): string | undefined {
return Array.isArray(value) ? value[0] : value
}

function isActiveContent(mimeType: string): boolean {
return ACTIVE_CONTENT_TYPES.has(mimeType.split(';')[0].trim().toLowerCase())
function normalizeMimeType(mimeType: string): string {
return mimeType.split(';')[0].trim().toLowerCase()
}

function contentDispositionHeader(
Expand Down Expand Up @@ -142,8 +151,11 @@ export async function cedarUploadsPlugin(
authenticate,
s3Webhook,
serveCacheControl = 'private, max-age=3600',
inlineTypes = [],
} = options

const allowedInlineTypes = new Set(inlineTypes.map(normalizeMimeType))

if (!tokenSecret) {
throw new UploadError(
'CONFIGURATION',
Expand Down Expand Up @@ -343,9 +355,12 @@ export async function cedarUploadsPlugin(
throw new UploadError('NOT_FOUND', 'File not found.', e)
}

const disposition: ContentDisposition = isActiveContent(upload.mimeType)
? 'attachment'
: requested
const mimeType = normalizeMimeType(upload.mimeType)
const disposition: ContentDisposition =
ACTIVE_CONTENT_TYPES.has(mimeType) &&
!allowedInlineTypes.has(mimeType)
? 'attachment'
: requested

return reply
.code(200)
Expand Down
Loading