#75 Use `ctx.request.files` instead of `ctx.request.body.files`

[imkimchi]

I see a big security problem of putting file objects in ctx.request.body.files from https://github.com/dlau/koa-body/issues/75.

so I changed it to put file objects in ctx.request.files instead of ctx.request.body.files, because there's a couple issues at the current version.

1. if there's files field and multipart file included in request at the same time, there'd be a conflict.
2. if somebody specifies files field to request, somebody could view any contents on the server by specifying the path on files field. details in the issue.

[IlyaSemenov]

Shouldn't we also put ctx.request.body.fields directly into ctx.request.body in this case? This would allow the same handler to handle both simple POST and multipart POST with the same code.

This would make it work similar to Django, where there is always request.POST (ctx.request.body) and optionally request.FILES (ctx.request.files)

[katanacrimson]

@IlyaSemenov Definitely agree. It would also make it consistent no matter what the POST body type is. Having to try to adjust to the current unpredictable data structure is a serious headache, especially with typescript.

That being said, that's a breaking change which would be a major semver bump. IMO, though, it's very necessary and is probably the only appropriate fix for the problem at hand.

[IlyaSemenov]

For the time being, I am using this:

app.use(koaBody({ multipart: true })) // prepare ctx.body
app.use((ctx, next) => {
    if (ctx.is('multipart')) {
        const { fields, files } = ctx.request.body
        ctx.request.body = fields
        ctx.request.files = files
    }
    return next()
})

and to please the type checker, this (in root index.d.ts):

import { Request } from 'koa'

declare module 'koa' {
    interface Request {
        files?: Array<any>
    }
}

(not sure if there's better type than any for the case)

[katanacrimson]

@IlyaSemenov I like that approach, honestly.

Anyways I'm trying to throw together a PR that does just that, but as it turns out, the tests won't pass.

The tests can't pass.

supertest isn't setting res.files at all with what goes into ctx.request.files, even if you patch the node ctx.req object with a .files property.

From the tests:

    request(http.createServer(app.callback()))
      .post('/users')
      .type('multipart/form-data')
      .field('names', 'John')
      .field('names', 'Paul')
      .attach('firstField', 'package.json')
      .attach('secondField', 'index.js')
      .attach('secondField', 'package.json')
      .attach('thirdField', 'LICENSE')
      .attach('thirdField', 'README.md')
      .attach('thirdField', 'package.json')
      .expect(201)
      .end( (err, res) => {
        if (err) return done(err);
        res.body.names.should.be.an.Array().and.have.lengthOf(2);
        res.body.names[0].should.equal('John');
        res.body.names[1].should.equal('Paul');
        res.files.firstField.should.be.an.Object;
        res.files.firstField.name.should.equal('package.json');

Once we get to the req.files lines, it explodes. No matter what I try, res.files is undefined. It's pretty clear that res isn't koa's response object, and I can't be sure that it's node's either, because it looks more like that's what res.res is. Hmm.

[dlau]

Apologies for the lateness, yes this should go in.

How about a one-off test that bypasses supertest and manually constructs a request?

[MarkHerhold]

Closing in favor of #89, which this was the basis of.

Thank you @imkimchi !

[laurent22]

How does one find out that the files are under request.files and not request.body.files? I found it here (took me a while) but it doesn't appear to be documented anywhere?

[MarkHerhold]

@laurent22 added a quick note here https://github.com/dlau/koa-body/commit/8f95cfbaf4c4e50f1609b20dfe4a43502a82d53a . Feel free to PR more documentation.