Skip to content

JS: Update MissingCsrfMiddleware after 'csurf' deprecation #11689

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

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Dec 14, 2022

Fixes #11682.

The csurf package has been deprecated, so it would be best not to use it in examples and recommendations. This PR replaces it with the csrf function from the lusca package.

Also updates the query to recognize tiny-csrf:

This is a tiny csrf library meant to replace what csurf used to do before it was deleted. It is almost a drop-in replacement.

Given the low number of users for that package, I am hesitant to actually recommend it in the qhelp, which is why I went with the more popular lusca package.

@asgerf asgerf added no-change-note-required This PR does not need a change note JS labels Dec 14, 2022
@github-actions
Copy link
Contributor

QHelp previews:

javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.qhelp

Missing CSRF middleware

Websites that rely on cookie-based authentication may be vulnerable to cross-site request forgery (CSRF). Specifically, a state-changing request should include a secret token so the request can't be forged by an attacker. Otherwise, unwanted requests can be submitted on behalf of a user who visits a malicious website.

This is typically mitigated by embedding a session-specific secret token in each request. This token is then checked as an additional authentication measure. A malicious website should have no way of guessing the correct token to embed in the request.

Recommendation

Use a middleware package such as lusca.csrf to protect against CSRF attacks.

Example

In the example below, the server authenticates users before performing the changeEmail POST action:

const app = require("express")(),
  cookieParser = require("cookie-parser"),
  bodyParser = require("body-parser"),
  session = require("express-session");

app.use(cookieParser());
app.use(bodyParser.urlencoded({ extended: false }));
app.use(session({ secret: process.env['SECRET'], cookie: { maxAge: 60000 } }));

// ...

app.post("/changeEmail", function(req, res) {
  const userId = req.session.id;
  const email = req.body["email"];
  // ... update email associated with userId
});

This is not secure. An attacker can submit a POST changeEmail request on behalf of a user who visited a malicious website. Since authentication happens without any action from the user, the changeEmail action would be executed, despite not being initiated by the user.

This vulnerability can be mitigated by installing a CSRF protecting middleware handler:

const app = require("express")(),
  cookieParser = require("cookie-parser"),
  bodyParser = require("body-parser"),
  session = require("express-session"),
  csrf = require('lusca').csrf;

app.use(cookieParser());
app.use(bodyParser.urlencoded({ extended: false }));
app.use(session({ secret: process.env['SECRET'], cookie: { maxAge: 60000 } }));
app.use(csrf());

// ...

app.post("/changeEmail", function(req, res) {
  const userId = req.session.id;
  const email = req.body["email"];
  // ... update email associated with userId
});

References

@asgerf asgerf marked this pull request as ready for review December 14, 2022 11:36
@asgerf asgerf requested a review from a team as a code owner December 14, 2022 11:36
@asgerf asgerf merged commit a92acf5 into github:main Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Javascript: Qhelp for Missing CSRF middleware recommends deprecated package
2 participants