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:

https://gitlab.com/gitlab-org/gitter/webapp/blob/develop/server/web/middlewares/configure-csrf.js#L69

        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.