tests: resolve paths for windows #32

jonathanong · 2015-10-19T22:23:59Z

closes #31

felixfbecker · 2015-10-19T22:36:28Z

5 tests still failing where path.resolve() isn't used yet.

C:\Users\felix\git\send ((HEAD detached at upstream/31-windows))
λ npm test

> koa-send@2.0.1 test C:\Users\felix\git\send
> mocha --require should --reporter spec



  send(ctx, file)
    √ should set the Content-Type
    √ should set the Content-Length

  Error: boom
      at Object.<anonymous> (C:\Users\felix\git\send\test\index.js:449:33)
      at next (native)
      at Object.<anonymous> (C:\Users\felix\git\send\node_modules\koa-compose\index.js:29:12)
      at next (native)
      at onFulfilled (C:\Users\felix\git\send\node_modules\co\index.js:65:19)

    √ should cleanup on socket error
    with no .root
      when the path is absolute
        1) should 404
      when the path is relative
        √ should 200
      when the path contains ..
        √ should 403
    with .root
      when the path is absolute
        2) should 404
      when the path is relative and exists
        √ should serve the file
      when the path is relative and does not exist
        √ should 404
      when the path resolves above the root
        √ should 403
      when the path resolves within root
        √ should 403
    with .index
      when the index file is present
        √ should serve it
        √ should serve it
    when path is not a file
      √ should 404
      √ should return undefined
    when path is a directory
      √ should 404
    when path does not finish with slash and format is disabled
      √ should 404
      √ should 404
    when path does not finish with slash and format is enabled
      3) should 200
double callback!
    when path is malformed
      √ should 400
    when path is a file
      √ should return the path
      or .gz version when requested and if possible
        √ should return path
        √ should return .gz path (gzip option defaults to true)
        √ should return .gz path when gzip option is turned on
        √ should not return .gz path when gzip option is false
      and max age is specified
        √ should set max-age in seconds
        √ should truncate fractional values for max-age
    .hidden option
      when trying to get a hidden file
        4) should 404
      when trying to get a file from a hidden directory
        5) should 404
      when trying to get a hidden file and .hidden check is turned off
        √ should 200


  25 passing (236ms)
  5 failing

  1) send(ctx, file) with no .root when the path is absolute should 404:
     Error: expected 404 "Not Found", got 400 "Bad Request"
      at Test.assert (C:\Users\felix\git\send\node_modules\supertest\lib\test.js:205:15)
      at assert (C:\Users\felix\git\send\node_modules\supertest\lib\test.js:132:12)
      at C:\Users\felix\git\send\node_modules\supertest\lib\test.js:129:5
      at Test.Request.callback (C:\Users\felix\git\send\node_modules\superagent\lib\node\index.js:746:30)
      at Test.<anonymous> (C:\Users\felix\git\send\node_modules\superagent\lib\node\index.js:135:10)
      at IncomingMessage.<anonymous> (C:\Users\felix\git\send\node_modules\superagent\lib\node\index.js:938:12)
      at endReadableNT (_stream_readable.js:893:12)

  2) send(ctx, file) with .root when the path is absolute should 404:
     Error: expected 404 "Not Found", got 400 "Bad Request"
      at Test.assert (C:\Users\felix\git\send\node_modules\supertest\lib\test.js:205:15)
      at assert (C:\Users\felix\git\send\node_modules\supertest\lib\test.js:132:12)
      at C:\Users\felix\git\send\node_modules\supertest\lib\test.js:129:5
      at Test.Request.callback (C:\Users\felix\git\send\node_modules\superagent\lib\node\index.js:746:30)
      at Test.<anonymous> (C:\Users\felix\git\send\node_modules\superagent\lib\node\index.js:135:10)
      at IncomingMessage.<anonymous> (C:\Users\felix\git\send\node_modules\superagent\lib\node\index.js:938:12)
      at endReadableNT (_stream_readable.js:893:12)

  3) send(ctx, file) when path does not finish with slash and format is enabled should 200:
     Error: Parse Error
      at Error (native)
      at Socket.socketOnData (_http_client.js:305:20)
      at readableAddChunk (_stream_readable.js:146:16)
      at Socket.Readable.push (_stream_readable.js:110:10)
      at TCP.onread (net.js:523:20)

  4) send(ctx, file) .hidden option when trying to get a hidden file should 404:
     Error: expected 404 "Not Found", got 200 "OK"
      at Test.assert (C:\Users\felix\git\send\node_modules\supertest\lib\test.js:205:15)
      at assert (C:\Users\felix\git\send\node_modules\supertest\lib\test.js:132:12)
      at C:\Users\felix\git\send\node_modules\supertest\lib\test.js:129:5
      at Test.Request.callback (C:\Users\felix\git\send\node_modules\superagent\lib\node\index.js:746:30)
      at Test.<anonymous> (C:\Users\felix\git\send\node_modules\superagent\lib\node\index.js:135:10)
      at IncomingMessage.<anonymous> (C:\Users\felix\git\send\node_modules\superagent\lib\node\index.js:938:12)
      at endReadableNT (_stream_readable.js:893:12)

  5) send(ctx, file) .hidden option when trying to get a file from a hidden directory should 404:
     Error: expected 404 "Not Found", got 200 "OK"
      at Test.assert (C:\Users\felix\git\send\node_modules\supertest\lib\test.js:205:15)
      at assert (C:\Users\felix\git\send\node_modules\supertest\lib\test.js:132:12)
      at C:\Users\felix\git\send\node_modules\supertest\lib\test.js:129:5
      at Test.Request.callback (C:\Users\felix\git\send\node_modules\superagent\lib\node\index.js:746:30)
      at Test.<anonymous> (C:\Users\felix\git\send\node_modules\superagent\lib\node\index.js:135:10)
      at IncomingMessage.<anonymous> (C:\Users\felix\git\send\node_modules\superagent\lib\node\index.js:938:12)
      at endReadableNT (_stream_readable.js:893:12)

jonathanong · 2015-10-19T22:37:14Z

hmmm i have no idea... i don't have a windows machine!

felixfbecker · 2015-10-19T22:41:20Z

I will look into it

felixfbecker · 2015-10-19T23:26:47Z

Okay so I looked into the source code a bit. What's failing is that it should answer with a 404 when the path is absolute. However, I if the path is absolute, resolve-path will throw a 400 Bad Request (path-resolve/index.js:75, called in send/index.js:58). I cannot find any logic which actually catches this error and sets the status to 404. No idea why this apparently only happens on my machine though. Thoughts?

I also don't quite get why it should send a 404, does this mean it sends a 404 every time I try to send a file with an absolute path?

jonathanong · 2015-10-20T03:59:48Z

if it's 400, then something is "malformed", i.e. can't be decoded with decodeURIComponent()

felixfbecker · 2015-10-20T07:11:09Z

I stepped through it with a debugger, this is not the reason. decodeUriComponent() is working fine. Just to be clear, I'm talking about test case "should 404 with no root when the path is absolute". The error is thrown in resolve-path/index.js, line 75:

// path should never be absolute
if (pathIsAbsolute.posix(path) || pathIsAbsolute.win32(path)) {
  throw createError(400, 'Malicious Path')
}

Called here in send/index.js, line 58, without any try/catch:

path = resolvePath(root, path);

Which means this error bubbles up to the client with a 400 response.

What I don't understand is the desired behaviour. Why should it answer with a 404 when the developer-specified path is absolute? And where is the logic in the code for this? The number "404" doesn't even show up anywhere in the code. I'm confused...

jonathanong · 2015-10-21T04:35:05Z

hmmm i still don't understand what's wrong. :/

felixfbecker · 2015-10-21T07:05:27Z

Just... please explain to me how this feature (should answer with 404 if path is absolute) is supposed to work and I'll submit a PR

tejasmanohar · 2015-10-21T07:14:08Z

@felixfbecker Would also look into this bug... but I have removed all traces of Windows from my possessions, lol. Anyhow, I'd recommend setting up a Vagrant box or something so you can properly run tests (or to help compare desired functionality vs result on Windows).

felixfbecker · 2015-10-22T14:50:40Z

Merge #36 :)

haoxins · 2015-10-23T01:39:01Z

@jonathanong branch 31-windows not be merged to master?

jonathanong · 2015-10-23T01:52:42Z

i think he fixed it w/ #36

haoxins · 2015-10-23T02:02:08Z

#36 merged into koajs:31-windows
😢

jonathanong · 2015-10-23T03:28:30Z

OH

jonathanong · 2015-10-23T03:29:54Z

@felixfbecker do you mind rebasing? or @coderhaoxin want to merge?

haoxins · 2015-10-23T03:31:18Z

@jonathanong I'll rebase and merge later

jonathanong · 2015-10-23T03:31:36Z

👍

felixfbecker · 2015-10-23T05:29:19Z

I though this was the best approach since you opened this branch to fix the issue.

haoxins · 2015-10-23T07:33:23Z

merged

jonathanong mentioned this pull request Oct 19, 2015

Does this work with async/await? #26

Closed

jonathanong closed this Oct 22, 2015

jonathanong deleted the 31-windows branch October 22, 2015 22:52

jonathanong restored the 31-windows branch October 23, 2015 03:28

jonathanong reopened this Oct 23, 2015

jonathanong assigned haoxins Oct 23, 2015

haoxins added the enhancement label Oct 23, 2015

haoxins force-pushed the 31-windows branch from 1226c7d to 75cdbe5 Compare October 23, 2015 06:53

jonathanong and others added 3 commits October 23, 2015 15:01

tests: resolve paths for windows

b47fc03

Strip root correctly on Windows

d45affb

Split path by path.sep instead of slash

51248d2

haoxins force-pushed the 31-windows branch from 75cdbe5 to 51248d2 Compare October 23, 2015 07:08

haoxins merged commit 51248d2 into master Oct 23, 2015

haoxins deleted the 31-windows branch October 23, 2015 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: resolve paths for windows #32

tests: resolve paths for windows #32

jonathanong commented Oct 19, 2015

felixfbecker commented Oct 19, 2015

jonathanong commented Oct 19, 2015

felixfbecker commented Oct 19, 2015

felixfbecker commented Oct 19, 2015

jonathanong commented Oct 20, 2015

felixfbecker commented Oct 20, 2015

jonathanong commented Oct 21, 2015

felixfbecker commented Oct 21, 2015

tejasmanohar commented Oct 21, 2015

felixfbecker commented Oct 22, 2015

haoxins commented Oct 23, 2015

jonathanong commented Oct 23, 2015

haoxins commented Oct 23, 2015

jonathanong commented Oct 23, 2015

jonathanong commented Oct 23, 2015

haoxins commented Oct 23, 2015

jonathanong commented Oct 23, 2015

felixfbecker commented Oct 23, 2015

haoxins commented Oct 23, 2015

tests: resolve paths for windows #32

tests: resolve paths for windows #32

Conversation

jonathanong commented Oct 19, 2015

felixfbecker commented Oct 19, 2015

jonathanong commented Oct 19, 2015

felixfbecker commented Oct 19, 2015

felixfbecker commented Oct 19, 2015

jonathanong commented Oct 20, 2015

felixfbecker commented Oct 20, 2015

jonathanong commented Oct 21, 2015

felixfbecker commented Oct 21, 2015

tejasmanohar commented Oct 21, 2015

felixfbecker commented Oct 22, 2015

haoxins commented Oct 23, 2015

jonathanong commented Oct 23, 2015

haoxins commented Oct 23, 2015

jonathanong commented Oct 23, 2015

jonathanong commented Oct 23, 2015

haoxins commented Oct 23, 2015

jonathanong commented Oct 23, 2015

felixfbecker commented Oct 23, 2015

haoxins commented Oct 23, 2015