krakenjs / lusca

Application security for express apps.
Other
1.78k stars 123 forks source link

Add bypass csrf validation for post first applications #59

Closed shaunwarman closed 8 years ago

shaunwarman commented 9 years ago

Certain applications (e.g. merchant integration) may have an initial request of POST. If this occurs, we need to bypass the csrf validation, but still generate a token for future requests.

Setting this bypass flag will require some simple no-op middleware on a per route basis.

aredridel commented 9 years ago

Oh, I like this! There's really no reason to have a rule that the first request to an app must be a GET.

Please don't bump versions in a PR -- that decision is made prepping for a release, not in the initial change (and also: this is a feature addition, it's a minor, not patch change)

shaunwarman commented 9 years ago

Thanks! Updated.

jasisk commented 9 years ago

I'm quite hesitant to make this change.

For one, using the req object for configuration feels ... icky. In many cases of req stuffing, it's merely providing data to a final route handler—in some cases, to provide some midstream conditional (not a big fan of this pattern). In this case, it's neither. It requires fine grained continuation control (though, in most cases with regards to lusca, is a non-issue given meddleware), and merely to act as a toggle. req as configuration is a slippery slope.

Second, while I know your use-case (discussed OOB) and can agree on the validity, csrf bypass is one of those things that is proposed as a solution 100 times and is proven to be the wrong solution for 99 of them. CSRF bypass is the ultimate hammer.

I'm more inclined to propose an application-level solution, particularly since you'd have to define the bypass logic in app code somewhere.

Consider:

var assert = require('assert');
var csrf = require('lusca').csrf;

module.exports = function csrfBypassGen(csrfOpts, opts) {
  var exemptions, middleware;
  opts || (opts = {});

  exemptions = opts.exemptions;
  if (typeof exemptions === 'string') {
    exemptions = [exemptions];
  }

  assert(Array.isArray(exemptions), 'opts.exemptions expects string or Array.');

  middleware = csrf(csrfOpts);

  return function isCsrfRequired(req, res, next) {
    // consider your routing here ... may be better with regexp or case-normalization.
    var shouldNotBypass = exemptions.every(function (exemption) {
      return req.originalUrl.indexOf(exemption);
    });

    if (!shouldNotBypass) {
      return next();
    }

    middleware(req, res, next);
  };
};
aredridel commented 9 years ago

Yeah. The downside of that approach is that it doesn't play nice with apps that can be mounted at various URLs.

jasisk commented 9 years ago

Yeah. The downside of that approach is that it doesn't play nice with apps that can be mounted at various URLs.

In those cases, use req.path which is normalized to remove the mountpath.

aredridel commented 9 years ago

Oh! I was not aware req.path normalized!

aredridel commented 9 years ago

That does get interesting. The wrapping lusca.csrf is an interesting approach that I like, but it won't work in this case -- the problem is that half of lusca.csrf still needs to run. Just not all of it.

jasisk commented 9 years ago

the problem is that half of lusca.csrf still needs to run.

If the POST redirects to a GET, the token will be generated. Is there a specific reason to avoid the redirect?

aredridel commented 9 years ago

Cookies are lost in Safari and IE in that case, so you can't pass information to the redirected request except in the URL.

shaunwarman commented 9 years ago

Yes, for third party cookies issues in our case. There is no way to keep state during redirect (header or cookie) and I think if anyone has a POST as initial request, they are most likely an iframe or in context mini-browser that will hit this issue.

jasisk commented 9 years ago

Cookies are lost in Safari and IE in that case

Gotcha. Let me look at a couple of things but—otherwise—I'm more inclined to recommend we expose the seeding mechanism so you can "seed" the secret long before you ever use it. Should solve this problem assuming the cookie is held in a POST (render), future GET.

In other words, similar code as above, except in the bypass case, conditionally seed the token generation (if one doesn't exist) instead of just calling next().

tlivings commented 9 years ago

Seconded on exposing seeding mechanism.