Don't use req.path for getting the full path

[RichardS]

I've been scratching my head around why ACE authentication wouldn't work with some routes. It turns out that if you have a router set up in Express 4.0 then req.path returns only the path relative to the router's base if the middleware is called as part of the router. Here's a small example:

const testRouter = new require('express').Router();
app.use('/test', testRouter);
testRouter.get('/my-path', addon.authenticate(), function(req, res) {
  // Will fail with query hash mismatch
});

This situation breaks canonicalizeUri as it relies on req.path which in this case req.path returns /my-path as per the Express docs. A better option is probably to use req.originalUrl and remove the query string. I'm happy to submit a PR for this but I wanted to open this ticket for some initial discussion on that solution, it might have implications that I haven't considered.

[Seb Ruiz]

@rw_richard great catch! We'd love a PR, I think that this sounds like the right fix.

[RichardS]

@sebr Great! I'll send in a PR

[Derek Overlock]

I think originalUrl should be used instead of baseUrl. baseUrl only returns the mount path for the router, it doesn't include the path used for the actual endpoint.

For example:

// server.js
app.use('/addon', atlassianConnectAddonRouter);

// atlassian-connect-addon-router.js
router.get('/connect', jwtVerificationMiddleware, function (request, response) {
    console.log(request.baseUrl); // returns '/addon' and not '/addon/connect', causing QSH validation errors
});

I think a better approach would be to use originalUrl with the query string removed.

This was a breaking change (at least to the code I was running to validate JWTs), happy to submit a PR to use originalUrl instead.

[RichardS]

Damn it! originalUrl was the one I intended to use (As I mentioned in the description) but I must have gotten them mixed up when implementing. I'll get a PR fixing that in ASAP, terribly sorry about the screwup!

EDIT: Done! I opened PR #6