remix icon indicating copy to clipboard operation
remix copied to clipboard

unstable_parseMultipartFormData nulls all non-file formdata fields

Open sidasmo opened this issue 2 years ago • 3 comments

What version of Remix are you using?

1.5.1

Steps to Reproduce

Installation

npx create-remix@latest ? Where would you like to create your app? ./remix-upload-test1.5.1 ? What type of app do you want to create?Just the basics ? Where do you want to deploy? Choose Remix if you're unsure; it's easy to change deployment targets. Remix App Server ? Do you want me to run npm install? (Y/n) y

Changes

change routes/index.ts to:

import type { ActionFunction } from '@remix-run/node'
import {
  redirect,
  unstable_createFileUploadHandler,
  unstable_parseMultipartFormData,
} from '@remix-run/node'
import { Form } from '@remix-run/react'

export const action: ActionFunction = async ({ request }) => {
  const uploadHandler = unstable_createFileUploadHandler({
    directory: './public/uploads',
    file: ({ filename }) => filename,
  })

  const formData = await unstable_parseMultipartFormData(request, uploadHandler)

  const title = formData.get('title')
  const fileId = formData.get('file')
  console.log('title', title, 'file: ', fileId)
  return redirect(``)
}

export default function Index() {
  return (
    <div>
      <h1>Upload Test</h1>
      <Form method='post' encType='multipart/form-data'>
        <label htmlFor='title'>Title</label>
        <input type='text' name='title' id='title' />

        <label htmlFor='file'>File</label>
        <input type='file' id='file' name='file' accept='application/pdf' />

        <button type='submit'>Submit</button>
      </Form>
    </div>
  )
}

Expected Behavior

I was expecting to get the title with formData.get('title') as i had sent it in the post request. but i got:

title:  null file:  NodeOnDiskFile [File] {
  lastModified: 0,
  webkitRelativePath: '',
  filepath: 'public/uploads/example.pdf',
  type: 'application/pdf',
  slicer: undefined,
  name: 'example.pdf'
}

Actual Behavior

Post The post request goes through with 200:

-----------------------------
Content-Disposition: form-data; name="title"

Test title
-----------------------------
Content-Disposition: form-data; name="file"; filename="example.pdf"
Content-Type:application/pdf

However, the console.log on title gives me null

sidasmo avatar Jun 07 '22 19:06 sidasmo

Can you test the answer provided here? https://github.com/remix-run/remix/issues/3238#issuecomment-1135147298

If it works, then we need to properly document this change

machour avatar Jun 08 '22 06:06 machour

thx very much, with the following changes to the uploadHandler it works:

const uploadHandler = unstable_composeUploadHandlers(
    unstable_createFileUploadHandler({
      directory: './public/uploads',
      file: ({ filename }) => filename,
    }),
    unstable_createMemoryUploadHandler()
  )

sidasmo avatar Jun 08 '22 18:06 sidasmo

facing this issue after writing a multi handler uploadHandler.

const uploadRamsHandler = unstable_composeUploadHandlers(
        unstable_createFileUploadHandler({
            maxPartSize: 25_000_000,
            directory: './private/rams/'
        }), 
        unstable_createMemoryUploadHandler()
    );

    const uploadAssociationsHandler = unstable_composeUploadHandlers(
        unstable_createFileUploadHandler({
            maxPartSize: 25_000_000,
            directory: './private/associations/'
        }), 
        unstable_createMemoryUploadHandler()
    );

    const uploadHandler: UploadHandler = async(args) => {
        if (args.name === "rams") {
            return uploadRamsHandler(args);
        } else if (args.name === "associations") {
            return uploadAssociationsHandler(args);
        }
    };

    const form = await unstable_parseMultipartFormData(request, uploadHandler);

Which then results in form.get("name") being null.

aaronduce avatar Oct 19 '22 10:10 aaronduce

I can confirm the issue persists also in version 1.7.5

akomm avatar Nov 25 '22 10:11 akomm

The problem is that the filter passed to the handler function is going through literally all parts, this includes regular string fields. This is a changed behavior that broke my previously functioning handler. While its not entirely wrong that all parts are treatet equally its very inconvenient because creating a uploadHandler for files, you want to handle files and pass through the parts representing simple fields.

Working currently with this one:

import {UploadHandler, UploadHandlerPart} from "@remix-run/node"

// https://github.com/remix-run/remix/issues/3409
type FilePart = Omit<UploadHandlerPart, "filename"> & {filename: string}

type MemoryUploadHandlerOptions = {
  maxPartSize: number
  accept: (part: FilePart) => boolean
}

export function createMemoryUploadHandler(
  opts: MemoryUploadHandlerOptions
): UploadHandler {
  const accept = opts.accept || (() => true)
  const maxPartSize = Math.max(1, opts.maxPartSize || 3_000_000)
  const textDecoder = new TextDecoder()

  async function getChunks(data: UploadHandlerPart["data"]) {
    const chunks: Uint8Array[] = []

    let partSize = 0
    for await (const chunk of data) {
      partSize += chunk.length

      if (maxPartSize < partSize) {
        return {chunks: null, partSize: 0}
      }

      chunks.push(chunk)
    }

    return {chunks, partSize}
  }

  async function handleFile(part: FilePart) {
    if (!accept(part)) {
      return null
    }

    const {chunks} = await getChunks(part.data)

    if (chunks == null) {
      return null
    }

    return new File(chunks, part.filename, {type: part.contentType})
  }

  async function handleString(part: UploadHandlerPart) {
    const {chunks, partSize} = await getChunks(part.data)

    if (!chunks) {
      return null
    }

    if (partSize === 0) {
      return ""
    }

    const data = new Uint8Array(partSize)

    let pointer = 0
    for (const chunk of chunks) {
      data.set(chunk, pointer)
      pointer += chunk.length
    }

    return textDecoder.decode(data)
  }

  return async part => {
    if (part.filename) {
      return handleFile({...part, filename: part.filename})
    }

    return handleString(part)
  }
}

Usage examples:

// 1.
const image = createMemoryUploadHandler({
  maxPartSize: 2_000_000,
  accept: ({contentType, filename}) => {
    if (contentType) {
      return ["image/jpeg", "image/png", "image/gif"].includes(
        contentType.toLowerCase()
      )
    }

    const ext = path.extname(filename).toLowerCase()
    return [".png", ".jpg", ".jpeg", ".gif"].includes(ext)
  }
})

// 2.
const zip = createMemoryUploadHandler({
  maxPartSize: 10_000_000,
  accept: ({contentType, filename}) => {
    if (contentType) {
      return ["application/zip", "application/x-zip-compressed"].includes(
        contentType
      )
    }
    const ext = path.extname(filename).toLowerCase()
    return ext === ".zip"
  }
})

Then use it with unstable_parseMultipartFormData as handler. This is not library level implementation, adjust as needed. You might want different maxPartSize for files and fields, etc.

akomm avatar Nov 29 '22 16:11 akomm