[go: nahoru, domu]

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

'/' route breaks strict routing #2281

Open
sgentle opened this issue Aug 5, 2014 · 32 comments
Open

'/' route breaks strict routing #2281

sgentle opened this issue Aug 5, 2014 · 32 comments
Assignees

Comments

@sgentle
Copy link
sgentle commented Aug 5, 2014

Hi there,

Currently, if you define a route like this:

route = require('express').Router({strict: true});
route.get('/', function(req, res) {
  res.send('hi');
});

And use() that in an express application like this:

app = require('express')();
app.use('/strict/', route);

You will receive a 200 when requesting /strict/ and /strict. I would expect only /strict/ to return a 200.

I've found an acceptable workaround by adding a check like this:

route.get('/', function(req, res, next) {
    if (req.originalUrl.slice(-1) != '/') return next();
    res.send('root with slash');
});
route.get('/', function(req, res) {
    res.send('root without slash');
});

But I think it would be less surprising if route.get('/') only worked for the path ending in / when strict routing is enabled, and perhaps route.get('') could be used for the no-slash case.

@sgnl
Copy link
sgnl commented Aug 5, 2014

try defining a route for '/strict' as well, I think that's the functionality of strict routing, you have to provide a route to handle for both.

app = require('express')();
app.use('/strict/', route);
app.use('/strict', route);

@gabeio
Copy link
Member
gabeio commented Aug 6, 2014

When the route gets passed to trim fix it adds the / to the /strict request... as it evaluates the path as '' when it arrives at trim fix and trimfix assures a leading slash.

@gabeio
Copy link
Member
gabeio commented Aug 6, 2014

https://github.com/strongloop/express/blob/master/lib/router/index.js#L247
the req.url is '' but becomes '/' after.

@Fishrock123 Fishrock123 added the Bug label Aug 6, 2014
@gabeio
Copy link
Member
gabeio commented Aug 9, 2014

It seems that settings in the router are not being passed to express... as such the strict setting in the router is being ignored, it apparently is expecting express's .enable("strict routing") for anything .use related.

@arjunrp
Copy link
arjunrp commented Sep 9, 2014

@sgentle Its working perfectly for me(https://gist.github.com/arjunrp/dba902fe3c1a0dba6f8d). Can u give a sample code?

@dougwilson
Copy link
Contributor

@arjunrp sample code is in the first post. all the code is one example, just split up by the explanation.

@arjunrp
Copy link
arjunrp commented Sep 9, 2014

@dougwilson I checked it out with the latest version,its working properly

@dougwilson
Copy link
Contributor

i just retried and it's still broken. here is the simplest example to demonstrate:

var express = require('express')
var app = express()
var router = express.Router({ strict: true })

router.get('/', function (req, res) {
  res.send('hi!')
})

app.use('/strict/', router)
app.listen(4000)

GET /strict should probably be 404, but it isn't. That's what @sgentle is reporting. The issue still exists in 4.9

@danieljuhl
Copy link

Another related case...

var express = require('express');
var app = express();
var router = express.Router({ strict: true });

router.use('', function (req, res) {
  res.send('hi');
});

app.use('/strict', router);
app.listen(4000);

GET /strict should give 200 and GET /strict/ should give 404, but both give a 200. Or is there another way to route a path to the routers root?

@dougwilson
Copy link
Contributor

@danieljuhl the issue is really a fundamental functionality of API guarantees within the router. The only work-around right now is to make both routers strict (your app with strict routing and the router strict) or use the work-around posted in the initial issue comment.

@danieljuhl
Copy link

@dougwilson I'm not sure, if I fully understand the work-around you're explaining. So far, I can't manage to get the result I'm looking for, no matter how many times I declare the app and the router strict. Can you provide a sample of how to accomplish this?

@dougwilson
Copy link
Contributor

@danieljuhl the work-around was posted above in #2281 (comment) by the original user :) For the code you posted, here is the work-around added to it:

var express = require('express');
var app = express();
var parseUrl = require('parseurl');
var router = express.Router({ strict: true });

router.use('', function (req, res, next) {
  if (parseUrl.original(req).pathname !== req.baseUrl) return next(); // skip this for strictness
  res.send('hi');
});

app.use('/strict', router);
app.listen(4000);

@danieljuhl
Copy link

@dougwilson Sorry, I read your comment, as if there were two work-arounds (the one you re-posted) AND another one using strict-routing on both application and router.

@Fishrock123 Fishrock123 removed the bug label Oct 6, 2014
@benmurrell
Copy link

+1, having the same issue. I have enabled strict routing at the app level and at the router level. Seems like this should still be classified as a bug?

@dougwilson
Copy link
Contributor

It's an extremely difficult fix, but can be done, just needs some effort for probably 5.0. I'll fix the labels :)

@dougwilson dougwilson self-assigned this Dec 8, 2014
@benmurrell
Copy link
var express = require('express');
var app = express();
app.set('strict routing', true);
var router = express.Router({ strict: true });

router.use('', function (req, res, next) {
  res.send('noslash');
});

router.use( '/', function(req, res) {
    res.send('slash');
});

app.use('/strict', router);
app.use('/strictslash/', router);

app.listen(4000);

For the code above, here are some test cases that can be used by whoever works on this:
GET /strict -> noslash
GET /strict/ -> noslash (expect slash)
GET /strict// -> noslash (expect 404)
GET /strictslash -> noslash (expect 404)
GET /strictslash/ -> noslash
GET /strictslash// -> noslash (expect 404 or slash - not sure)
GET /strictslash/// -> noslash (expect 404)

@blakeembrey
Copy link
Member

@dougwilson Just FYI, all these issues are fixed on latest path-to-regexp (I fixed it last July) and should be possible to backport. Also, in the latest path-to-regexp you no longer need to do the req.url || '/' hack.

Edit: Sorry, you're right. Forgot about .use('/test', route) and route.get('/') still working with /test. However, there's enough catches there that you hopefully can just do if (!options.strict) { req.url = req.url || '/'; }

@blakeembrey
Copy link
Member

@dougwilson Just wanted to clarify the previous message. It's actually all provided out of the box with the latest path-to-regexp version and those changes could be back-ported (maybe some odd incompatibility though). I realised that a / regexp that's not strict will actually match an empty string anyway. Same with the reverse definition. So it should pass all expected tests in strict and non-strict.

@dougwilson
Copy link
Contributor

The issue here is with Express and the way the router works, rather than anything with path-to-regexp :) Essentially what happens in the reported case is that req.url will never, ever be set to an empty string by Express, but it's not a possible valid value, ever. Instead if Express's router is about to set req.url to an empty string, it sets it to / instead. The sub router will not pick up the fact that there was no trailing slash is the issue. That can be fixed by Express by mucking about within req.originalUrl.

@blakeembrey
Copy link
Member

I've looked through the code and found that before, but why wouldn't you consider an empty string to be valid? It's factually correct (/test vs /test/) and would be matched still with the single slash regexp in the latest version. I just wanted to confirm that it's all currently possible without mucking with the path logic and you can fully rely on the regexp to be correct now.

@dougwilson
Copy link
Contributor

but why wouldn't you consider an empty string to be valid?

Because it's the contact of the req.url API from Node.js. The first line of an HTTP request is in the form <method> <path> HTTP/<version><CR><LF>. req.url refers to whatever the exact string that appears in the <path> portion and GET HTTP/1.1 (that is, GET<space><space>HTTP/1.1, because GitHub seems to ignore the double-space when rendered) is a syntax error (where the <path> would resolve to an empty string). As such, we will not violate the expectation that req.url cannot be an empty string.

@blakeembrey
Copy link
Member

Interesting, thanks. I wasn't sure if this would be treated differently with nested routing.

@dougwilson
Copy link
Contributor

Nope :) We want the nested stuff to think they are not even nested, which really helps with reusable code. serve-static does similar req.originalUrl "mucking" that I can pictures the router doing to be able to handle this case. Express may very well decide to pass something into path-to-regexp regular expressions that are not always exactly req.url, if it works :)

@xytis
Copy link
xytis commented Feb 22, 2016

Hi, I'm having a similar issue in 4.x branch:
https://github.com/expressjs/express/blob/4.x/lib/router/index.js#L466

Poked around the code base for reasons behind this:

path-to-regex considers 'strict' an option which means:
"Match ^path$ instead of ^path(?|/?)$"
While express expects 'strict' only influence trailing slash.

Even if I enforce 'strict' inheritance in this particular line -- I end up with middleware no longer working on child paths ('/path/sub/' no longer triggers middleware mounted on '/path/', since path-to-regex with 'strict' no longer matches).

@dougwilson
Copy link
Contributor

Hi @xytis, I'm not sure what you are reporting, I'm sorry. That line forces strict mode for a specific reason, and it is not meant to be changed. If you can please open a new GitHub issue (https://github.com/expressjs/express/issues/new) and describe your issue, and even better if you provide code we can use to reproduce the issue with the current version of Express, we would be happy to look into the issue (but please make a new issue).

@xytis
Copy link
xytis commented Feb 25, 2016

Code example:

app.enable('strict routing');
router = ...
app.use('/path/', router);

From documentation I would expect that my application will not serve "/path" endpoint, yet it gets forwarded to given router.

Line of code that I reference basically says "do not perform strict routing on internal layers". Which, by my understanding, is exactly what I expect in above example. I went deeper into path-to-regex of currently used version found the inconsistency in "strict" definition, which I mentioned earlier.

I am not sure if this requires a separate issue, since, by the looks of it, it will be fixed as soon as express updates it's dependancy of path-to-regex.

@dougwilson
Copy link
Contributor

.From documentation I would expect that my application will not serve "/path" endpoint, yet it gets forwarded to given router.

Unfortunately the documentation in this case is simply wrong. The strict routing feature in no way affects paths under app.use, and it is not intended to. This issue is definitely different from what you are describing.

I am not sure if this requires a separate issue, since, by the looks of it, it will be fixed as soon as express updates it's dependancy of path-to-regex.

It will not, because app.use is not affected by strict routing, and will always act in a non-strict manor.

There is a lot of off topic chatter in this thread, and some is issues like yours that are actually misunderstandings and some are of the original bug. The bug is only the code in the initial post. It is a bug because router.get is being used, which absolutely should be enforcing strict routing. Your code is not utilizing any methods that actually are affected by strict routing.

mmeisel added a commit to mmeisel/hashword-server that referenced this issue Mar 28, 2018
This is actually not working due to
expressjs/express#2281
but it's the right thing to do in theory...
@tilleps
Copy link
tilleps commented Jan 13, 2020

If you don't care about supporting empty paths, using the below middleware seems to enforce strict routing:

//  Results:
//
//  GET /strict -> (404)
//  GET /strict/ -> slash
//  GET /strict// -> (404)
//  GET /strictslash -> (404)
//  GET /strictslash/ -> slash
//  GET /strictslash// -> (404)
//  GET /strictslash/// -> (404)

const express = require("express");

const app = express();
app.set("strict routing", true);

const router = express.Router({ strict: true });

//  Middleware to enforce strict URLs
//  Note: must be applied to the router (and not app)
router.use(function strict(req, res, next) {
  if (req.app.get("strict routing")) {
    req.url = req.originalUrl.substring(req.baseUrl.length);
  }
  next();
});

//  This solution does not support empty paths like these:
//  router.use("", function (req, res, next) {
router.all("", function (req, res, next) {
  res.send("noslash");
});

//  Note: using .use() will not work, use methods instead (all, get, post, etc):
//  router.use("", function (req, res, next) {
router.all("/", function(req, res) {
    res.send("slash");
});

app.use("/strict", router);
app.use("/strictslash/", router);

app.listen(4000);

@dbauszus-glx
Copy link

@dougwilson Is there any more information in regard to 'mucking about' with the original url?

We really would like to use ourcompany.com as address and not ourcompany.com/

To be fair we use express mostly in a debug environment and the serverless routing support routes without trailing slashes. It would be great if the same links work in the express debug environment.

@dougwilson
Copy link
Contributor

Hi @dbauszus-glx sorry you are having issues. Your description, to me, sounds like a more specific bug instance. Would you be willing to open a new issue about this and include a reproduction case? I believe for your specific scenario ("We really would like to use ourcompany.com as address and not ourcompany.com/") it is the Node.hs HTTP server that is at issue here which is why it works serverless (it's not used) vs not (we use it to parse the HTTP requests).

@dbauszus-glx
Copy link

@dougwilson I will have to come back to this once a I get a chance to deploy an express instance. At the moment we use Express only for debugging which means I can only describe the problem with localhost which is probably not very helpful. I understand that req.url may not be empty and would expect app.get("", handler) to return localhost:3000/. I am only surprised why req.url is empty when I set app.get("/root", handler) and returns localhost:3000/root instead of localhost:3000/root

I will do a bit more reading on this and get back on this once I am better capable of explaining the issue. It's very low impact for now and I plan to use a micro router for my debug case in the future.

@skaziweb
Copy link
skaziweb commented Oct 22, 2020

I use middleware for add / and redirect all routes in new address.
I fink it's possible to use in strict mode.

app.use((req, res, next) => {
  const url = req.url;
  const fileRegExp = new RegExp(/\./);
  const isParams = Object.keys(req.params).length;
  const isQuery = Object.keys(req.query).length;
  if(url.length && fileRegExp.test(url) || isParams || isQuery){
    next();
  } else if(url.length && url.slice(-1) != '/') {
    res.redirect(301, `${url}/`);
  } else {
    next();
  }
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests