Only the last csrfToken generated will be valid

[chr15m]

If any token other than the last call to req.csrfToken() is passed it fails. In a situation where multiple clients are loading the page and each getting a token this means most form submissions will fail, because only the client last loading the page will have a valid csrf token.

To replicate:

1. Run your example server.
2. Verify you can submit correctly.
3. Reload the page (so csrfToken is set again).
4. Without submitting, open a new tab (so a new csrfToken is generated).
5. Now go back to the first tab and submit.
6. Observe the error as the csrf check things the first token generated is now invalid.

Maybe I'm missing something obvious but it seems to me this renders the library as not fit for purpose.

[valexandersaulys]

Taking a look at the source code, I don't see this occurring.

The csrfToken is generated on a per-request basis but its compared within the request. There's nothing storing the state server-side that would be reset.

I wrote this test to mimic what you're describing and it seems fine to me.

  it("allows for cookie reuse", () => {
    // get the page once
    const reqOne = mockRequest({
      method: "GET",
      signedCookies: {},
      body: {},
      headers: {
        referer: "http://localhost:3001"
      }
    });
    const resOne = mockResponse({
      cookie: sinon.stub()
    });
    const nextOne = sinon.stub();
    this.csrfMiddlware(reqOne, resOne, nextOne);
    assert.isFunction(reqOne.csrfToken);
    const csrfTokenOne = reqOne.csrfToken();
    assert.isNotNull(csrfTokenOne);

    // get the page a second time
    const reqTwo = mockRequest({
      method: "GET",
      signedCookies: {},
      body: {},
      headers: {
        referer: "http://localhost:3001"
      }
    });
    const resTwo = mockResponse({
      cookie: sinon.stub()
    });
    const nextTwo = sinon.stub();
    this.csrfMiddlware(reqTwo, resTwo, nextTwo);
    assert.isFunction(reqTwo.csrfToken);
    const csrfTokenTwo = reqTwo.csrfToken();
    assert.isNotNull(csrfTokenTwo);

    // submit the first page request
    const reqThree = mockRequest({
      method: "POST",
      signedCookies: {
        csrfToken: encryptCookie(csrfTokenOne, this.secret)
      },
      body: {
        _csrf: csrfTokenOne
      },
      headers: {
        referer: "http://localhost:3001"
      }
    });
    const resThree = mockResponse({
      cookie: sinon.stub()
    });
    const nextThree = sinon.stub();
    assert.isNotFunction(reqThree.csrfToken);
    this.csrfMiddlware(reqThree, resThree, nextThree);
    assert.isTrue(nextThree.calledOnce);
  });

[chr15m]

Have you actually tried running your demo server and accessing it from two tabs and observing that one of the tabs is unable to log in and throws a CSRF error?

[valexandersaulys]

Ok I see what you mean. Given the test passes, this smells like something to do with how browsers operate and thus is related to the configuration (which may be why you mentioned it in a separate thread).

There will have to be some investigation into modifying those cookie parameters. I'm open to suggestions on how they should be set.

Unfortunately my time is limited and I don't deal with javascript on a regular basis. I'll get to this when I can.

[IanKulin]

Since this code works by storing an encrypted version of the CSRF token in a cookie so they can be compared later, it would need a whole different strategy to deal with this edge case of routes being POSTed to from different tabs in the same browser - since those tabs will share the same cookie. The most recent token generated for this cookie will always be the only valid one to tiny-csrf.

Here's a couple of possible strategies:

1. Keep a server store of tokens and just use the cookie for identifying the session. It would need to allow for multiple tokens per session, and they would have to be removed as they were used.

2. Implement a FIFO queue of stored encrypted tokens in the cookie - there will be some limit to how many you can fit in there, but this would keep it stateless, and again, we would remove them as they get used.

I wouldn't say this issue renders the library "not fit for purpose", just not fit for the purpose of submitting overlapping POSTs from the same browser. This is a use case that will occur in the wild, but in most cases the server response is going to be to message the user and refresh the form, so the recovery path from this is not a catastrophic one.

It might be worth documenting in the readme in case it's a deal breaker for some users.

[chr15m]

This is a use case that will occur in the wild, but in most cases the server response is going to be to message the user and refresh the form, so the recovery path from this is not a catastrophic one.

I think most users would find this surprising. If you're working on a client side web application that has any level of complexity (e.g. background fetch() posting updates to the server) it's also surprising behaviour.

The correct solution is to use HMAC tokens with a server secret so no matter when req.csrfToken() is called, the token returned is always valid when the server sees it later. Only validating the last token produced is not a good implementation.

I would recommend using a library like https://github.com/Psifi-Solutions/csrf-csrf which doesn't have this issue.