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

fix empty filenames are not considered a field (#26) #38

Closed
wants to merge 1 commit into from
Closed

fix empty filenames are not considered a field (#26) #38

wants to merge 1 commit into from

Conversation

cyantree
Copy link

This PR normalizes empty filenames to be handled as undefined. This allows sending multipart requests from the browser containing fields with content types which then will be handled as fields and not as files.

Linting fails. I have fixed most of the issues locally however there are some missing dependencies regarding the benchmark tests. Should I add the fixes I made to this PR?

Checklist

@cyantree cyantree marked this pull request as draft November 29, 2021 23:13
@cyantree
Copy link
Author

I've marked it as draft because I haven't tested the new version with my usecase.

@kibertoad
Copy link
Member

You can ignore linting warnings for benchmarks, they will not fail the build, we'll fix them later.

@@ -9,3 +9,4 @@ Major changes since the last busboy release (0.31):
* Error on non-number limit rather than ignoring (#7)
* Dicer is now part of the busboy itself and not an external dependency (#14)
* Tests were converted to Mocha (#11, #12, #22, #23)
* Empty filenames of parts are handled as `undefined` (#26)
Copy link
Member

Choose a reason for hiding this comment

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

can you also add entry to MIGRATING.md, explaining the breaking change?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I can explain the breaking change.

'Content-Disposition: form-data; name="cont2"; filename=""',
'Content-Type: text/javascript',
'',
'console.log("hello world");',
Copy link
Member

Choose a reason for hiding this comment

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

is this intentional?

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean exactly? This tests whether it's possible to use a non-json text field with empty filename. Or do you think this test isn't necessary?

@kibertoad
Copy link
Member

Linting fixes unrelated to this PR are very welcome, but having them as a separate PR would be more convenient.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 30, 2021

Is this relevant to this PR?
simllll/busboy@c01a909

@kibertoad
Copy link
Member

@Uzlopak I'd say so, and going with a config options (with blob by default) sounds like a good solution.

@kibertoad kibertoad mentioned this pull request Nov 30, 2021
4 tasks
@kibertoad
Copy link
Member

@cyantree Please let me know if you'll need any help with this!

@cyantree
Copy link
Author

cyantree commented Dec 1, 2021

Is this relevant to this PR? simllll@c01a909

Yes, this is relevant. However I wouldn't recommend using blob as identifier as it can easily be used as a filename and therefore would lead to unexpected behaviour.

Regarding introducing a config option:
Yes, I can see this. But I wouldn't use blob as default there either because of the above reasons.
I think empty filenames should be sufficient and are usable with the browser api.
Another option would be using filenames that can't exist because of their structure. Maybe something like field://.

If you want to fix this quickly, then yes, it would be great if you could take over because atm I only can work at this when time allows it.

@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 1, 2021

I created a new PR with your work #52 . It also ensures that we are not breaking with original busboy.

@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 1, 2021

I created another PR #53 which i think handles the whole blob issue correctly.

@cyantree
Copy link
Author

cyantree commented Dec 1, 2021

Obsolete as of #53.

@cyantree cyantree closed this Dec 1, 2021
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 this pull request may close these issues.

3 participants