Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

\u2028 in filename kills header parsing #664

Closed
tuomassalo opened this issue Dec 9, 2020 · 6 comments · Fixed by #670
Closed

\u2028 in filename kills header parsing #664

tuomassalo opened this issue Dec 9, 2020 · 6 comments · Fixed by #670

Comments

@tuomassalo
Copy link

Support plan

  • which support plan is this issue covered by? (e.g. Community, Sponsor, or
    Enterprise): community
  • is this issue currently blocking your project? (yes/no): no
  • is this issue affecting a production system? (yes/no): yes

Context

  • node version: v12.18.3
  • module (formidable) version: 1.2.2, 2.0.0-canary.20200402.2
  • environment (e.g. node, browser, native, OS): node
  • used with (i.e. popular names of modules): none
  • any other relevant information:

What are you trying to achieve or the steps to reproduce?

Some user-agents seem to allow sending filenames with the infamous \u2028 LINE SEPARATOR. It seems that the header parser dies with Request aborted on such filenames, since the character is parsed as a line feed. The value passed to _getFileName is cut at the \u2028.

I tried to modify test/fixture/js/special-chars-in-filename.js and *.http files to add a test but ran out of time. To test, try copy-pasting a literal \u2028 (not the string but the actual character) to one of the *.http files.

What was the result you got?

The header parser dies with Request aborted.

What result did you expect?

The filename is reported even if it contains \u2028.

@tuomassalo tuomassalo added the bug Something isn't working label Dec 9, 2020
@GrosSacASac GrosSacASac added area: file naming and removed bug Something isn't working labels Dec 11, 2020
@GrosSacASac GrosSacASac reopened this Jan 7, 2021
@GrosSacASac
Copy link
Contributor

How did you manage to put that character in a filename ?

@tuomassalo
Copy link
Author

@GrosSacASac, well - I didn't, but I noticed the character in some legitimate files uploaded by a client. I noticed them since I used this library to migrate a big number of files from a legacy system. This legacy system had saved the upload filenames, so I believe at least some clients (OS/browser combinations) might be able to produce such stuff.

Unfortunately, I think they were uploaded about two years ago, so I didn't have detailed logs about e.g. user-agent.

So, quite an edge case.

@tuomassalo
Copy link
Author

At least on macos, one can create a file with this char in the name with e.g.:

echo foo > $(perl -C -wle 'print "foo\x{2028}bar.txt"')

Checking:

ls | hexdump -C
00000000  66 6f 6f e2 80 a8 62 61  72 2e 74 78 74 0a        |foo...bar.txt.|
0000000e

NB: e2 80 a8 is the UTF-8 representation of \u2028.

@GrosSacASac
Copy link
Contributor

I did some additional testing and decided it is not worth it to handle this case because

  • The user should already handle file.name === null
  • There are maybe other characters that will give null as a result

From my findings: You got Request aborted. because it threw an error somewhere. But \u2028 can be handled gracefully.

I am going to update the examples a bit to better handle common errors.

Code I used to manually test

import "../patchNode/fetch.js";
import FormData from 'form-data';
var form = new FormData();

form.append('file', `Hello`, {
  filename: String.raw`

.txt`, // put here \u2028 (github removes it)
  contentType: 'text/plain',
});

form.append('my_field', 'my value');
fetch(`http://localhost:3000/api/upload`, {
    method: `POST`,
    body: form,
    headers: form.getHeaders(),
}).then(response => {
    if(!response.ok) {
        throw response.statusText;
    }
    return response.text();
}).then(console.log).catch(console.error);

and

    form.on("fileBegin",(formName, file) => {
      if (file.name === null) {
        file.name = "invalid-characters"
      }
    })

@GrosSacASac
Copy link
Contributor

@tunnckoCore
Copy link
Member

@tunnckoCore How to handle this lint error https://github.com/node-formidable/formidable/runs/1882614195 ?

I think it was

// eslint-disable-next-line no-param-reassign
file.name = 'dsadasd';

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants