headers-support-utf8

[Doc999tor]

This PR adds support of busboy's defParamCharset option for supporting UTF8 characters in request headers such as filename
Default: 'latin1' for backward compatibility
Not required

Resolves #1104, #1194

Example of usage:

multer({
	storage,
	defParamCharset: 'utf8',
})

[Doc999tor]

TypeScript definitions update - DefinitelyTyped/DefinitelyTyped#65578

[Doc999tor]

@starnayuta
I understand that but the v1.4.5 was marked as lts, so I don't believe it will be changed as part of 1.4.5; only as a new version, until it will become lts as well

[Doc999tor]

@LinusU Are there any blockers for merging this PR? Can we merge it?

[Doc999tor]

@UlisesGascon, maybe you could assist with this PR?

[threshold862543]

Would be nice to merge this.

[PRR24]

It has been over a year, please kindly merge it.

[bjohansebas]

Hi @Doc999tor, you can add tests to verify the behavior

[luiscabus]

Hi @Doc999tor, you can add tests to verify the behavior

it just works, get another task to add tests for this, the current behavior isnt even well tested anyways

[Darkhogg]

@bjohansebas This change is simply "pass this option down to busboy", so a test for it should basically just be "busbuy has received the argument", right?
@luiscabus If you need help getting that done I can lend you a hand, I'm a bit blocked by this PR and would love to see it merged and released.

[bjohansebas]

Well, I really have no idea if it's actually possible to test this, but since it fixes something in the package, ideally, we should have a test for it.

[bjohansebas]

@Doc999tor Could you please do a rebase? And an example of tests could be:

describe('UTF-8 filenames', function () {
  var uploadDir, upload

  beforeEach(function (done) {
    temp.mkdir(function (err, path) {
      if (err) return done(err)

      var storage = multer.diskStorage({
        destination: path,
        filename: function (req, file, cb) {
          cb(null, file.originalname)
        }
      })

      uploadDir = path
      upload = multer({ storage: storage, defParamCharset: 'utf-8' })
      
      done()
    })
  })

  afterEach(function (done) {
    rimraf(uploadDir, done)
  })

  it('should handle UTF-8 filenames', function (done) {
    var req = new stream.PassThrough()
    var boundary = 'AaB03x'
    var body = [
      '--' + boundary,
      'Content-Disposition: form-data; name="small0"; filename="ÖoϪ.dat"',
      'Content-Type: text/plain',
      '',
      'test with UTF-8 filename',
      '--' + boundary + '--'
    ].join('\r\n')

    req.headers = {
      'content-type': 'multipart/form-data; boundary=' + boundary,
      'content-length': body.length
    }

    req.end(body)

    upload.single('small0')(req, null, function (err) {
      assert.ifError(err)

      assert.strictEqual(req.file.originalname, 'ÖoϪ.dat')
      assert.strictEqual(req.file.fieldname, 'small0')

      done()
    })
  })
})

[Doc999tor]

I will do, thanks for the test example

[bjohansebas]

Great, if you do it I can close #1327

[ShubhamOulkar]

Just a remainder

filename="ÖoϪ.dat"'

Non-ASCII characters (like Ö or Ϫ) can be misinterpreted if the stream isn’t properly encoded. To avoid ambiguity and ensure cross-platform compatibility, use the filename* syntax with UTF-8 percent-encoding as recommended in RFC 5987, Section 3.2.2.

Also, see this test case in the Multer codebase before making changes to how filenames are handled.

[unc0ded]

Non-ASCII characters (like Ö or Ϫ) can be misinterpreted if the stream isn’t properly encoded. To avoid ambiguity and ensure cross-platform compatibility, use the filename* syntax with UTF-8 percent-encoding as recommended in RFC 5987, Section 3.2.2.

@ShubhamOulkar Although, if I understand correctly, RFC 7578, Section 4.2 forbids the usage of this syntax.

[planted-mreinstein]

Why is this not merged yet? I appreciate the desire for good test coverage but this is very clearly broken, and it's not a very complex change, just passing through a parameter to busboy.

[Doc999tor]

@bjohansebas rebased and added tests, sorry for taking too long 🙏

• should use latin1 as default charset for non-extended parameters
• should use custom charset when defParamCharset is specified
• should not affect extended parameters with explicit charset
• should handle different charsets correctly
• should work with memory storage
• should work with array uploads

[coveralls]

coverage: 98.496% (+0.01%) from 98.485%
when pulling 6937df5 on Doc999tor:master
into b6e4b1f on expressjs:main.

[unc0ded]

Interesting coincidence, I had just completed working on a new PR with your changes and a few tests 😆

[unc0ded]

This default shouldn't need to be passed explicitly, busboy uses latin1 anyway when the defParamCharset passed is undefined.

[Doc999tor]

Yes, I know - I added it in case something will change in busboy default settings, multer will still behave consistently
@LinusU @bjohansebas @UlisesGascon what do you think? I can change it if it's unnecessary precaution

[planted-mreinstein]

It feels like the default should be utf-8 but that would also technically be a breaking API change for multer. Thank you for maintaining consistency. :)

[Doc999tor]

@UlisesGascon @unc0ded @bjohansebas @LinusU Can one of you review the PR?
@unc0ded Is my comment ok from your perspective?

[unc0ded]

@UlisesGascon @unc0ded @bjohansebas @LinusU Can one of you review the PR? @unc0ded Is my comment ok from your perspective?

I think busboy won't change the default without a major version bump anyway, so setting a default here may be overkill. But I leave it upto the maintainers here to take a call on that.

[faileon]

Non-english parts of the world are in pain for ~3 years thanks to this. I just love littering my codebase with code like Buffer.from(file.originalname, 'latin1').toString('utf8')

[planted-mreinstein]

Please merge and publish this.

[ducfilan]

It should be merged!

[alex-Arc]

Is there any to-do's left here?
Anything I can do to push this forward?

[adi-bradwell]

This is so important!! please merge it! 🙏