res.download building a wrong path when root options is set

[Fischerfredl]

Hello everyone!

I just ran into a bug in res.download which is also an inconsistency between res.download and res.sendFile.

The bug

res.download breaks for relative paths when specifying a root in the options argument. Express tries to access the following path: {options.root}/{process.cwd()}/{path}. Expected is {options.root}/{path}

Example

In the example below I try to access the file /dev/null. The following Error is raised when navigating to /download

Error: ENOENT: no such file or directory, stat '/dev/home/user/my-project/null'

const express = require('express')                                                                            
                                                                                                              
const app = express()                                                                                         
  .get('/', (req, res) => {                                                                                   
    res.send(`                                                                                                
<ul>                                                                                                          
  <li><a href="/sendFile">res.sendFile</a></li>                                                               
  <li><a href="/download">res.download</a></li>                                                               
</ul>                                                                                                         
`)                                                                                                            
  })                                                                                                          
  .get('/sendFile', (req, res) => {                                                                           
    res.sendFile('null', { root: '/dev' })                                                                    
  })                                                                                                          
  .get('/download', (req, res) => {                                                                           
    res.download('null', 'null', { root: '/dev' })                                                            
  })                                                                                                          
  .listen(() => {                                                                                             
    console.log(`Listening on http://localhost:${app.address().port}`)                                        
  })   

res.sendFile works as expected.

Expectation

My assumption that res.download should behave like res.sendFile is based on the following line of the documentation

The optional options argument passes through to the underlying res.sendFile() call, and takes the exact same parameters.

Cause

Problematic is the following line in res.download: /lib/response.js:575:

  // Resolve the full path for sendFile
  var fullPath = resolve(path);

The resolve method is imported from the 'path' module: https://nodejs.org/api/path.html#pathresolvepaths This is where the current working directory gets added. Anyway res.sendFile won't allow relative paths at all without a root specified. So I suggest to leave out the path.resolve call at all for a breaking release. For a non breaking fix one could only call path.resolve if root is not set in options.

Research

I have only looked briefly but have not found this fixed in the 5.x branch.

[dougwilson]

Hello, and thank you. It looks like this is primarily an issue with the documentation; res.download should document it's arguments separately.

As for accepting a root option for res.download, you are correct, it doesn't (and never has). We can add that as an enhancement, though 👍 .

[Fischerfredl]

Thank you for your response. I just have two questions out of curiosity:

removed

Enhancement or bug

Currently the library concatenates two absolute paths which I think is always not intended. To prevent this in the future one would either have to

• explicitly warn of providing the root option in the documentation
• implicitly strip the root property from the option object
• support the root property properly

Even if res.download does not accept a root option, providing it still leads to unexpected behavior. What do you think, is it enough to document the arguments res.download does accept?

[dougwilson]

Hi @Fischerfredl I have to go to sleep (3am in my time zone), so I cannot respond to your questions right away, but just wanted to say I did remove a part regarding security, as if you believe there is a vulnerability here, please submit it through our security procedure so we can evaluate it. The best I can comment is that from the initial report, the changes made were acceptable to the original researcher, but if you know of additional issues, please report it to us using the security procedure so we can evaluate 👍

[dougwilson]

Also, I did take the time to fix the res.download documentation on expressjs.com today if you would like to review and make any pull requests if you think there is still something missing. As far as the root being prepended, it looks like this is also a bug in the old res.sendfile (which is what res.dowload is based off) when you provide both an absolute path and a root option.

So it seems like yes, there is a bug here indeed, as well as a feature request.

[Fischerfredl]

Wow, thanks for the quick reply. I am sorry if the wording of my comment triggered any alarm at your side and woke you up.

Also thanks for the quick action. The documentation looks good to me!

[sidwebworks]

@dougwilson Hey man, thanks for all the hardwork. I wanted to contribute to help with the issues too. I started trying to resolve this bug, still I am not as experienced as you are so you got any suggestions where should I start from?

[sidwebworks]

Okay yeah I think did repro the same issue, @Fischerfredl is this what you meant?

with res.download = ENOENT: no such file or directory, stat '/files/home/projects/node-83jh6f/my-file.txt'
with res.sendFile = ENOENT: no such file or directory, stat '/files/my-file.txt'

Here is the example: https://stackblitz.com/edit/node-83jh6f?file=index.js