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

Files are uploading as 'file' without its extension #170

Closed
GopalakrishnanK opened this Issue Jul 21, 2015 · 37 comments

Comments

Projects
None yet
@GopalakrishnanK
Copy link

GopalakrishnanK commented Jul 21, 2015

Hi,

i am using multer to upload my images. Uploading works fine but the problem is all my files are uploading with 'file' filetype ie, without its extension. can anyone tell me why.....?

My Code:

var uploadProfileImgs = multer({dest : './files/uploads/profile/'}).single('avatar');

app.post('/profile', function (req, res) {
  uploadProfileImgs(req, res, function (err) {
    if (err) {
      console.log(err.message);
      // An error occurred when uploading
      return
    }
    console.log('Everything went fine');
    // Everything went fine
  })
})

My files looks like below:
w

@ricardomoura

This comment has been minimized.

Copy link

ricardomoura commented Jul 21, 2015

same prob here! rename, and events like onFileUploadStart onFileUploadComplete got dropped.

@gabeio

This comment has been minimized.

Copy link
Member

gabeio commented Jul 21, 2015

@GopalakrishnanK

By default, Multer will rename the files so as to avoid naming conflicts. The renaming function can be customized according to your needs.

you have to define a function to rename the files otherwise multer renames them to random stuff like that on purpose so when you upload image1.jpg and image1.jpg they both can exist on the server at the same time...

Note: Multer will not append any file extension for you, your function should return a filename complete with an file extension.

Also the file extension is removed for safety reasons especially present within windows.


@ricardomoura his code is there he is not renaming anything... is probably the issue. if you are having issues with renaming please open a different issue thanks!

@ricardomoura

This comment has been minimized.

Copy link

ricardomoura commented Jul 21, 2015

my workaround:

var storage = multer.diskStorage({
  destination: function (req, file, cb) {
    cb(null, './uploads/')
  },
  filename: function (req, file, cb) {
    crypto.pseudoRandomBytes(16, function (err, raw) {
      cb(null, raw.toString('hex') + Date.now() + '.' + mime.extension(file.mimetype));
    });
  }
});
var upload = multer({ storage: storage });
@gabeio

This comment has been minimized.

Copy link
Member

gabeio commented Jul 21, 2015

@ricardomoura that is the correct way to rename your files.

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Jul 21, 2015

As @gabeio said this is the default behaviour from version 1.0.0 and forward.

@ricardomoura You can supply destination as a string if you want to. And if you don't want to include the mime library you could look at the extension of the original file using the builtin module path.

var path = require('path')
var multer = require('multer')

var storage = multer.diskStorage({
  destination: './uploads/',
  filename: function (req, file, cb) {
    crypto.pseudoRandomBytes(16, function (err, raw) {
      if (err) return cb(err)

      cb(null, raw.toString('hex') + path.extname(file.originalname))
    })
  }
})

var upload = multer({ storage: storage })

Attention Lookout when doing this thought as the client can send any file extension that they want, regardless of what the file actually contains. A rule of thumb is to never trust any data from the client.

@LinusU LinusU closed this Jul 21, 2015

@GopalakrishnanK

This comment has been minimized.

Copy link
Author

GopalakrishnanK commented Jul 22, 2015

Hey guys, thanks for your help. it worked.

e

@GopalakrishnanK

This comment has been minimized.

Copy link
Author

GopalakrishnanK commented Jul 22, 2015

anyway when i access the file without extension in url it opens without any problem. can anyone tell me whats behind that....?

r

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Jul 22, 2015

Because browsers doesn't use file extensions.

I would strongly recommend you not to expose user uploaded files directly to the user thought! What if someone used your image uploading form to upload an html files with inlined javascript code. That code would then run under your domain and get access to that domain's cookies etc.

What I would do is the following:

  1. Decide which file formats you want to allow.
  2. When a user uploads a file, check that the magic bytes corresponds correctly
    • e.g. a png image always starts with 89 50 4e 47
  3. Store this file without the file extension
  4. Use express to serve the images.
app.get('/image/:id', function (req, res) {
  // Validate that req.params.id is 16 bytes hex string
  // Get the stored image type for this image
  res.setHeader('Content-Type', storedMimeType)
  fs.createReadStream(path.join(UPLOAD_PATH, req.params.id)).pipe(res)
})
@GopalakrishnanK

This comment has been minimized.

Copy link
Author

GopalakrishnanK commented Jul 22, 2015

@LinusU thanks for your worthy suggesstion.

@tyagi15aug

This comment has been minimized.

Copy link

tyagi15aug commented Oct 7, 2015

Thanks @GopalakrishnanK , @LinusU , @ricardomoura for this post and your suggestions, it helped me a lot.

@leekyungmoon

This comment has been minimized.

Copy link

leekyungmoon commented Dec 7, 2015

@GopalakrishnanK
How your browser show us the picture?
What I got is the message saying 'Cannot GET /uploads/a539d27fd99be4274fa3e2eb22b520b21449498122507.png' && 'Cannot GET /uploads/a539d27fd99be4274fa3e2eb22b520b21449498122507'

@Natumsol

This comment has been minimized.

Copy link

Natumsol commented Jan 21, 2016

@LinusU greate suggestion~ thanks a lot~

@SarasArya

This comment has been minimized.

Copy link

SarasArya commented Feb 9, 2016

@GopalakrishnanK my file gets downloaded when I type localhost:3000/uploads/saras but gets shown in the browser if I type localhost:3000/uploads/saras.png This is in complete contrast to what @LinusU has said. @LinusU how do you explain this behavior

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Feb 10, 2016

This is in complete contrast to what @LinusU has said.

False.

You provide very little information on your setup so it's quite hard to have a look. But I think that different headers are being sent in the different cases. What server are you using?

@SarasArya

This comment has been minimized.

Copy link

SarasArya commented Feb 10, 2016

I am using a NodeJS server and save files as soon as they come using multer into the directory. How can I replicate and show you the issue?

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Feb 10, 2016

How can the file be accessible as both localhost:3000/uploads/saras and localhost:3000/uploads/saras.png then? The standard static server won't allow that

@BrentLeasure

This comment has been minimized.

Copy link

BrentLeasure commented Apr 9, 2016

I'm a little late to the party, but I have a question about file retrieval.

@LinusU, I'm using your code to serve the image to the front end, but I don't know what to do with it once I receive it. I'm using angular to display the image via ng-src, but the return data has a lot of jargon.

My code:

Angular:

$http.get("/getImage/", {params: {"filename" : $scope.recipe.image.filename, "mimetype" : $scope.recipe.image.mimetype}})
        .then(function(returnData){
            $scope.image = returnData.data;
        })

Node:

getImage = function(req, res){
    res.setHeader('Content-Type', req.query.mimetype)
    fs.createReadStream(path.join('./uploads/', req.query.filename)).pipe(res)
}

HTML:
<img ng-src="{{image}}">

The image doesn't display, and it gives a lot of crazy characters. At this point I'm not sure how to display it. I think the image is returning, but it's just in a format that doesn't work.

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Apr 10, 2016

@BrentLeasure I'm not familiar enough with $http to give a perfect answer here, but would it be possible to write something like this?

Angular:

$scope.imageUrl = `/getImage?filename=${scope.recipe.image.filename}&mimetype=${scope.recipe.image.mimetype}`

HTML:

<img ng-src="{{imageUrl}}" />
@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Apr 10, 2016

Also, if you use a magic bytes identifying library on the server side, you won't have to send the mimetype in the url which seems quite awkward and possible is an attack vector. (e.g. an attacker could upload something that looks like valid PNG, but that actual contains a <script> tag that could be executed by changing the content-type to text/html).

Then it could simply look like this:

Angular:

$scope.imageUrl = `/image/${scope.recipe.image.filename}`

HTML:

<img ng-src="{{imageUrl}}" />
@BrentLeasure

This comment has been minimized.

Copy link

BrentLeasure commented Apr 10, 2016

@LinusU I'm not 100% following your code, but let me give more information about what is returning to the front end.

I made a minor change to my original code (under the "angular" section, $scope.image.returndata.data.src is now just $scope.image.returndata.data)
When I console.log $scope.image, it prints out the following in the console:

screenshot

I don't know what is returning from the backend, and that is what I'm confused about. I'm guessing it's the image, but I don't know how to have it show up as an image, and not a bunch of jargon. Forgive me if this question has an easy answer, I'm just not used to transferring images.

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Apr 10, 2016

As far as I know, ng-src should resolve to the url of the image, not that data itself. Thus you shouldn't have to make any manual calls to the server yourself, but all you need to do is set the ng-src to the url that you where fetching with $http.

@BrentLeasure

This comment has been minimized.

Copy link

BrentLeasure commented Apr 10, 2016

@LinusU My data is on the backend though, and I'm using your previously stated code to serve the image back up.

Is there a way for me to display the image properly via the way I'm doing it?

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Apr 10, 2016

Yes I understand that, the only difference here is how you instruct the web browser to fetch the image, either via the src attribute, or via an xmlhttprequest.

You could probably fetch it via the Ajax APIs and the use URL.createObjectURL() to get an url that you can set the src to...

@Apurva1590

This comment has been minimized.

Copy link

Apurva1590 commented Jun 17, 2016

Below code is also working to the given problem :

`var storage = multer.diskStorage({
  destination: function (req, file, cb) {
    cb(null, './upload')
  },
  filename: function (req, file, cb) {
      cb(null, file.originalname);        
  }
})

var upload = multer({ storage: storage })`

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Jun 18, 2016

@Apurva1590 Keep in mind that that code is a potentially huge security risk. The client can send anything they want as the originalname.

What happens if the same name is given twice? What happens if the name ../index.js is given?

@sidonaldson

This comment has been minimized.

Copy link

sidonaldson commented Jul 22, 2016

This thread is incredibly useful, any chance some of this content could make the readme?

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Jul 22, 2016

I would love that! Any pull requests are very welcome 🙌

@Pokom

This comment has been minimized.

Copy link

Pokom commented Aug 4, 2016

@LinusU

I'm actually working through this exact use case, let me dumb down my current example into a barebones example to demo and I'll send it as a pull request since it seems like it is still particularly a hot topic for people.

@DonoA

This comment has been minimized.

Copy link

DonoA commented Aug 17, 2016

I made a few changes to the code posted above, I needed the file to have its current name as express's download function was not working to rename the downloaded file. I came up with this:

var upload = multer({ storage: multer.diskStorage({
    destination: (req, file, cb) => {
      now = Date.now().toString();
      require('fs').mkdir('uploads/'+now, err => {
        cb(null, 'uploads/'+now);
      });
    },
    filename: (req, file, cb) => {
      cb(null, file.originalname.split('/').pop().trim());
    }
  })
});

@LinusU:
Which brought me to the question: Why doesn't multer use a similar design to paperclip for rails? In paperclip the files are stored in folders by id with each folder containing the revisions of the file and inside those folders the files with extensions themselves. This would make system admin much easier and serving the documents much easier. Wouldn't it?

Also what is the status on the readme update? I might be interested in adding it if it hasn't been already.

@evinw

This comment has been minimized.

Copy link

evinw commented Aug 24, 2016

I used middleware

var multer = require('multer'); router.post('/upload', multer({dest: 'images/'}).array('upl', 10), function (req, res) { });

Then I used npm image-type
and added it in my route where I was displaying images like so...

var imageType = require('image-type');

Jade

img.img-responsive(src='/images/#{listing[0].Images[0].filename}')

And it worked for me. No need to add it on your file system

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Aug 25, 2016

Why doesn't multer use a similar design to paperclip for rails? In paperclip the files are stored in folders by id with each folder containing the revisions of the file and inside those folders the files with extensions themselves.

The file extension is set by the client and isn't checked in any way, thus it could lead to potential attack vectors if they are saved with the clients file extension.

@tankcong

This comment has been minimized.

Copy link

tankcong commented Aug 31, 2016

@LinusU So what if I check the file header first then store it with extension?

@DonoA

This comment has been minimized.

Copy link

DonoA commented Aug 31, 2016

@evinw I tried that but on Windows 10 it still downloaded the file as originally named. That's why I needed to store the file extensions. I suppose it's a windows thing.

@LinusU Couldn't you simply sterilize the file name before saving?

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Aug 31, 2016

Couldn't you simply sterilize the file name before saving?

That is what we are doing, we give it a random name? How would we sterilize in a way that prevents malicious extensions while still keeping the client sent extension?

@Ikhan

This comment has been minimized.

Copy link

Ikhan commented Dec 12, 2016

@LinusU Thanks for the explanation why storing files without extension worth it.

@folamy

This comment has been minimized.

Copy link

folamy commented Dec 14, 2016

Please, how can i implement this:
var storage = multer.diskStorage({
destination: function (req, file, cb) {
cb(null, './uploads/')
},
filename: function (req, file, cb) {
crypto.pseudoRandomBytes(16, function (err, raw) {
cb(null, raw.toString('hex') + Date.now() + '.' + mime.extension(file.mimetype));
});
}
});
var upload = multer({ storage: storage });

I had to use csrf/csurf in my app.post(), I can't pass upload to it. I had to stick to:
var multer = require('multer');
app.use(multer({dest: 'public/student/documents'}).single('passport'));
app.use(csrf());

but I need to rename the uploaded pictures

@bora89

This comment has been minimized.

Copy link

bora89 commented Mar 27, 2018

I gave up with saving an extension. So I went further with LinusU's code:

npm install --save read-chunk file-type

  app.get('/image/:id', function (req, res) {
    // Validate that req.params.id is 16 bytes hex string
    if (/^[0-9A-F]{32}$/i.test(req.params.id)) {
      const filePath = path.join(UPLOAD_PATH, req.params.id);
      const buffer = readChunk.sync(filePath, 0, 4100);
      // Get the stored image type for this image
      const storedMimeType = fileType(buffer);
      res.setHeader('Content-Type', storedMimeType.mime);
      fs.createReadStream(filePath).pipe(res);
    } else {
      res.send(400);
    }
  });

Now I do not need to save a file with an extension to use it. That approach is much more secure as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.