-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Comments
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.
|
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. |
https://github.com/strongloop/express/blob/master/lib/router/index.js#L247 |
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 |
@sgentle Its working perfectly for me(https://gist.github.com/arjunrp/dba902fe3c1a0dba6f8d). Can u give a sample code? |
@arjunrp sample code is in the first post. all the code is one example, just split up by the explanation. |
@dougwilson I checked it out with the latest version,its working properly |
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)
|
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);
|
@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. |
@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? |
@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); |
@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. |
+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? |
It's an extremely difficult fix, but can be done, just needs some effort for probably 5.0. I'll fix the labels :) |
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: |
@dougwilson Just FYI, all these issues are fixed on latest Edit: Sorry, you're right. Forgot about |
@dougwilson Just wanted to clarify the previous message. It's actually all provided out of the box with the latest |
The issue here is with Express and the way the |
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 ( |
Because it's the contact of the |
Interesting, thanks. I wasn't sure if this would be treated differently with nested routing. |
Nope :) We want the nested stuff to think they are not even nested, which really helps with reusable code. |
Hi, I'm having a similar issue in 4.x branch: Poked around the code base for reasons behind this: path-to-regex considers 'strict' an option which means: 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). |
Hi @xytis, I'm not sure what you are reporting, I'm sorry. That line forces |
Code example:
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. |
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.
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. |
This is actually not working due to expressjs/express#2281 but it's the right thing to do in theory...
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); |
@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. |
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). |
@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 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. |
I use middleware for add / and redirect all routes in new address. 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();
}
}); |
Hi there,
Currently, if you define a route like this:
And
use()
that in an express application like this: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:
But I think it would be less surprising if
route.get('/')
only worked for the path ending in/
when strict routing is enabled, and perhapsroute.get('')
could be used for the no-slash case.The text was updated successfully, but these errors were encountered: