expressjs / express

Fast, unopinionated, minimalist web framework for node.
https://expressjs.com
MIT License
65.33k stars 15.86k forks source link

Confusing behavior of applying middleware to paths instead of routers #5753

Closed tomasz13nocon closed 2 months ago

tomasz13nocon commented 3 months ago

It seems that middleware used on routers doesn't get applied to routers themselves, but to paths on which those routers are used. The docs say:

A router object is an isolated instance of middleware and routes.

But it doesn't really seem isolated.

Consider code like this:

const router = express.Router();
const routerProtected = express.Router();
const nestedRouter = express.Router();
const nestedRouterProtected = express.Router();

routerProtected.use(authenticateMiddleware);
nestedRouterProtected.use(authenticateMiddleware);

app.use("/api", router);
app.use("/api", routerProtected); // Everything under /api will now use authenticateMiddleware!
app.use("/api/nested", nestedRouter);
app.use("/api/nested", nestedRouterProtected);

What I expected to happen is for that authentication middleware to only run for methods declared on routers which directly used the middleware, i.e. routerProtected and nestedRouterProtected.

What actually happens is that once app.use("/api", routerProtected) runs, all methods declared from here on out under /api now have that middleware silently applied to them, including everything under nestedRouter, even though nestedRouter never used that middleware.

Is this intended behavior?

express version used: 5.0.0-beta.3

yajatkaul commented 3 months ago

sry i didnt completely understand the issue but cant u just use middlewares like router.get("/", protectRoute, route);

wesleytodd commented 2 months ago

Hey, I think this is a question more than a bug. To that point I am going to add that label as well. If I am wrong and you are reporting this as a bug please say so and we can fix it.

I think I am with @yajatkaul in that we need more information to help. My guess here is that the interpretation of the docs referenced is different than what was intended when they were written, but again I think we need more info to be sure. Can you add more example code and even better would be a link to the repo where you are having this issue with an example curl or request which illustrates the issue?

tomasz13nocon commented 2 months ago

cant u just use middlewares like router.get("/", protectRoute, route);

I wanted to apply that middleware to all handlers on a router. This way I would have to apply it individually to all routes. Is this the only way to do this? I though Routers were meant to group request handlers that need some common middleware.

@wesleytodd Here's perhaps a better example on StackBlitz: https://stackblitz.com/edit/stackblitz-starters-ufcosa?file=README.md The express stuff is in server.js

There are 2 routers. One with auth middleware, and one with logging middleware. Both routers are used on /api. They are supposed to group request handlers which need specific middleware. Requests on routerProtected should only use authMiddleware, and requests on routerLogging should only use loggingMiddleware. The problem is, once I use routerProtected on /api, every new handler or router on /api will use routerProtected's middleware. In the example, when you click "fetch /api/logged-data", authMiddleware runs even though it wasn't used for that router.

So basically, based on the documentation I've quoted above, I expected a router's middleware to only run for that router's handlers and nothing else. But instead when I use a router on some path, it applies all its middleware to other routers on that path.

If this is intended behavior, then I guess what I'm suggesting is to improve the docs to make this behavior clear. And I'm also asking if there is another way to group request handlers and middleware like that?

yajatkaul commented 2 months ago

i might be wrong here im kinda sleepy rn but i dont see u using the middleware anywere u just made them but u r not using them 😪 i dont know u r doing it in a pretty different way

const protectRoute = async (req,res,next) => {
    try{
        const token = req.cookies.jwt;
        if(!token){
            return res.status(401).json({error:"Unauthorized - No Token Provided"});
        }

        const decode = jwt.verify(token, process.env.JWT_SECRET);
        if(!decode){
            return res.status(401).json({error:"Unauthorized - Invalid Token"});
        }

        const user = await User.findById(decode.userId).select("-password");
        if(!user){
            return res.status(404).json({error:"User not found"});
        }
        req.user = user;

        next();

    }catch(err){
        console.log(err.message);
        res.status(500).json({err:"Internal Error"});
    }
}
const router = express.Router();

router.post("/send/:id", protectRoute,sendMessage);
router.get("/:id", protectRoute,getMessage);

i use it kinda like this

tomasz13nocon commented 2 months ago

I'm using it on the routers, not on the specific route handlers, so that it appplies to everything on the router. In my initial example it's the 5th and 6th lines. So instead of doing

router.get("something", middleware1, middleware2, handler);
router.get("something-else", middleware1, middleware2, handler2);

I do

router.use(middleware1, middleware2);
router.get("something", handler);
router.get("something-else", handler2);
wesleytodd commented 2 months ago

The req and res objects are passed through all middleware as is. This means that any modifications you make on them will persist across routers. My guess is the wording you quoted from the docs is just misleading in this case. I don't have much time to help do technical support here, but I can confirm this is intended behavior and not a bug.

Arvind-K1 commented 2 months ago

The middleware applied to routers in Express is tied to the paths those routers handle. This means that if we apply middleware to a router, it will affect the paths managed by that router but not the router itself.

However, in your current setup, both router and routerProtected handle the same base path /api. The same applies to nestedRouter and nestedRouterProtected with /api/nested.

If we intention is to protect only certain routes under /api and /api/nested, we should differentiate the base paths for the protected routes.

const express = require('express');
const app = express();
const router = express.Router();
const routerProtected = express.Router();
const nestedRouter = express.Router();
const nestedRouterProtected = express.Router();

const authenticateMiddleware = (req, res, next) => {
  // authentication logic 
  next();
};

// Define routes for router
router.get('/public-endpoint', (req, res) => {
  res.send('This is a public endpoint');
});

// Define routes for routerProtected
routerProtected.use(authenticateMiddleware);
routerProtected.get('/protected-endpoint', (req, res) => {
  res.send('This is a protected endpoint');
});

// Define routes for nestedRouter
nestedRouter.get('/public-nested-endpoint', (req, res) => {
  res.send('This is a public nested endpoint');
});

// Define routes for nestedRouterProtected
nestedRouterProtected.use(authenticateMiddleware);
nestedRouterProtected.get('/protected-nested-endpoint', (req, res) => {
  res.send('This is a protected nested endpoint');
});

// Apply routers to the app
app.use("/api", router); // Public routes under /api
app.use("/api/protected", routerProtected); // Protected routes under /api/protected
app.use("/api/nested", nestedRouter); // Public routes under /api/nested
app.use("/api/nested/protected", nestedRouterProtected); // Protected routes under /api/nested/protected

const port = 3000;
app.listen(port, () => {
  console.log(`Server running on port ${port}`);
});

In this setup:

This way, we can clearly separate protected and unprotected routes.

tomasz13nocon commented 2 months ago

Okay, thanks for confirming this as intended behavior.

In that case I would say the docs are a bit misleading. Specifically the Router section.

Here are my suggestions:

A router object is an isolated instance of middleware and routes. You can think of it as a “mini-application,” capable only of performing middleware and routing functions.

It should not say the router is "isolated" since it makes it sound like the router's middleware is applied only to its handlers.

// invoked for any requests passed to this router router.use((req, res, next) => {

True, but it is also invoked for any requests not passed to this router, which happen to be on the same path.

I suggest adding a sentence like this: Any middleware applied to a router will run for all requests on that router's path, even those that aren't part of the router.

Any thoughts on this? I can make a PR to the docs repo.

wesleytodd commented 2 months ago

but it is also invoked for any requests not passed to this router, which happen to be on the same path.

I think the miss here is that the request IS passed to this router. And that is because the path matched. I do agree we could drop the word isolated to help prevent this kind of miscommunication though. Nothing in an express app is "isolated" in that it uses the `re[q|s] objects as context objects passed from middleware to middleware. This has always been an unfortunate design, but it is one so deeply entrenched in this ecosystem that it will be long and hard to undo.

tomasz13nocon commented 2 months ago

@ritanshu747 well, that's incorrect. As we've discussed above, the actual and the intended behavior is the opposite of what you're describing.

I've opened a PR on the docs repo, so I'm closing this issue. Thanks again for the help.