Skip to content
This repository has been archived by the owner on Jan 25, 2020. It is now read-only.

Files with no extension are being processed by the directory traversal #73

Open
lmarkus opened this issue Dec 29, 2015 · 2 comments
Open

Comments

@lmarkus
Copy link
Contributor

lmarkus commented Dec 29, 2015

Referenced from krakenjs/kraken-js#440

The problem is on the express-enrouten module, specifically here: https://github.com/krakenjs/express-enrouten/blob/v1.2.x-release/lib/directory.js#L101-L102

In order to support requireable non-traditional file extensions, (eg: thing.coffee), the file extension is thrown away, and the file is checked using require.resolve().

There are three scenarios here:

  1. controllerFile.js: Extension thrown out, /path/to/controllerFile resolves as a module, since it can be matched to controllerFile.js. It will be processed.
  2. textFile.txt: Extension thrown out, path/to/textFile This does not resolve as a module, since require() can't match it to textFile, textFile.js, textFile.json nor a directory with an index.js file. It will be ignored.
  3. fileWithNoExtension: /path/to/fileWithNoExtension resolves as a module, because it can be matched to a valid existing file (just like the first case). It will be processed.

Case 3 causes issues with SVN-like version control systems, since CVS stores extensionless files in its hidden directories (eg: Entries, Repository, Root).

lmarkus pushed a commit to lmarkus/express-enrouten that referenced this issue Dec 31, 2015
Instead of just trying to resolve the module, try to load it.

Since this block is already surrounded by a try/catch statement it's a safe
place to attempt requiring a module. I don't think it's wasted effort, since
the whole point of validating the modules is to try to load them later.

Also, some very minor refactoring of the extension removal code to make it more readable.
lmarkus pushed a commit to lmarkus/express-enrouten that referenced this issue Dec 31, 2015
Instead of just trying to resolve the module, try to load it.

Since this block is already surrounded by a try/catch statement it's a safe
place to attempt requiring a module. I don't think it's wasted effort, since
the whole point of validating the modules is to try to load them later.

Also, some very minor refactoring of the extension removal code to make it more readable.
@jasisk
Copy link
Member

jasisk commented Jan 1, 2016

Hmm. I'm not so sure we should be taking on this responsibility—I think it should land on the app developer.

We'll be suppressing the error case of a documented behavior. "If I can resolve the file, I load it."

Imagine the common case: a bug in a controller. With this PR, it just won't be loaded. The dev will be none the wiser.

Rather than take on that responsibility, perhaps we should add blacklisting?

@lmarkus
Copy link
Contributor Author

lmarkus commented Jan 2, 2016

Good call on the buggy controller case. I just added debug statements, but I can see how that makes for a less enjoyable dev experience.

I started going down the configuration path, but this seemed like a simpler choice. I have a half-baked branch with configurable ignore lists that I can wrap up on Monday.

lmarkus pushed a commit to lmarkus/express-enrouten that referenced this issue Jan 2, 2016
Fixes krakenjs#73

This introduces a change to the directory configuration (It's now an {path:"path/to/controllers", ignore:['pattern1',...]} object instead of a path string.
However, I made it backwards compatible. It shows a deprecation message, but we could support both methods if desired
lmarkus pushed a commit to lmarkus/express-enrouten that referenced this issue Jan 2, 2016
Fixes krakenjs#73

This introduces a change to the directory configuration (It's now an {path:"path/to/controllers", ignore:['pattern1',...]} object instead of a path string.
However, I made it backwards compatible. It shows a deprecation message, but we could support both methods if desired
lmarkus pushed a commit to lmarkus/express-enrouten that referenced this issue Jan 2, 2016
Fixes krakenjs#73

This introduces a change to the directory configuration (It's now an {path:"path/to/controllers", ignore:['pattern1',...]} object instead of a path string.
However, I made it backwards compatible. It shows a deprecation message, but we could support both methods if desired
lmarkus pushed a commit to lmarkus/express-enrouten that referenced this issue Jan 2, 2016
Fixes krakenjs#73

This introduces a change to the directory configuration (It's now an {path:"path/to/controllers", ignore:['pattern1',...]} object instead of a path string.
However, I made it backwards compatible. It shows a deprecation message, but we could support both methods if desired
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants