Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only the last csrfToken generated will be valid #11

Open
chr15m opened this issue Jan 22, 2024 · 3 comments
Open

Only the last csrfToken generated will be valid #11

chr15m opened this issue Jan 22, 2024 · 3 comments

Comments

@chr15m
Copy link

chr15m commented Jan 22, 2024

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
Copy link
Owner

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
Copy link
Author

chr15m commented Feb 2, 2024

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants