Make sure we only skip web middlewares on /api/ and not on /api* routes
Closes #2444 (closed)
The problem
When we tried to unify our local development environment with the production in https://gitlab.com/gitlab-org/gitter/webapp/-/merge_requests/1652 we introduced the following wrapper, to exclude some middleware on /api/
routes:
// middleware wrapped in `skipForApi` is not used on API requests
const skipForApi = handler => (req, res, next) => {
const fullUrl = req.protocol + '://' + req.get('host') + req.originalUrl; // https://stackoverflow.com/a/10185427/606571
const apiBasePath = config.get('web:apiBasePath');
if (fullUrl.indexOf(apiBasePath) === 0) {
next();
} else {
handler(req, res, next);
}
};
One of those middlewares was configure-csrf
:
app.use(skipForApi(require('./middlewares/configure-csrf')));
Which is responsible for getting accessToken
from the session and putting it on the request
object:
return oauthService
.validateAccessTokenAndClient(sessionAccessToken)
.then(function(tokenInfo) {
if (!tokenInfo) {
return generateAccessToken();
}
req.accessToken = sessionAccessToken;
})
This req.accessToken
is then used by context-generator-request
to put add accessToken
to tropue context.
https://gitlab.com/gitlab-org/gitter/webapp/blob/develop/server/web/context-generator-request.js#L54
var contextHash = {
events: events,
accessToken: req.accessToken,
isNativeDesktopApp: isNative(req),
locale: req.i18n.locales[req.i18n.locale],
features: features
};
The real-time client needs this token on the context to connect, if the token doesn't exist, the connection attempt ends up with 401
error.
The root cause
The skipForApi
method was skipping a middleware if the URL started with https://gitter.im/api
(the value of web:apiBasePath
config property) which unfortunately applied to 126 production rooms:
TroupeReplicaSet:PRIMARY> db.troupes.find({lcUri: /^api.*/}).count()
126
Solution
There has already been discussion about these options on the original MR https://gitlab.com/gitlab-org/gitter/webapp/-/merge_requests/1652#note_244633712
Option A
Just add the tailing /
at the end of the API base URL.
const skipForApi = handler => (req, res, next) => {
const fullUrl = req.protocol + '://' + req.get('host') + req.originalUrl; // https://stackoverflow.com/a/10185427/606571
const apiBasePath = config.get('web:apiBasePath');
const ensureTrailingSlash = url => (url.endsWith('/') ? url : url + '/');
if (fullUrl.indexOf(ensureTrailingSlash(apiBasePath)) === 0) {
next();
} else {
handler(req, res, next);
}
};
I've tried it but it made the code convoluted and thanks to particulars of our local testing (when we don't hook up the tested express app to a port) we couldn't test it.
Option B - Chosen
Check whether the paths tarts with /api/
.
const skipForApi = handler => (req, res, next) => {
if (req.originalUrl.indexOf('/api/') === 0) {
next();
} else {
handler(req, res, next);
}
};
This would be now visually obvious and it's easy to write automated test for it this way.