Skip to content

Conversation

@hib4
Copy link
Member

@hib4 hib4 commented May 15, 2025

PR Type

Bug fix, Enhancement


Description

  • Rename CSRF header to x-xsrf-token

  • Add timingSafeEqual for token comparison

  • Increase CSRF token length to 32 bytes

  • Introduce setCsrfCookie helper function


Changes walkthrough 📝

Relevant files
Enhancement
csrf_middleware.ts
Improve CSRF token validation and handling                             

functions/src/middlewares/csrf_middleware.ts

  • Renamed header check from x-csrf-token to x-xsrf-token
  • Replaced basic equality with timingSafeEqual comparison
  • Increased generated token size from 16 to 32 bytes
  • Added setCsrfCookie for secure cookie handling
  • +24/-7   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @hib4 hib4 self-assigned this May 15, 2025
    @github-actions
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    timingSafeEqual length mismatch

    crypto.timingSafeEqual throws if buffers have different lengths. Ensure both csrfCookie and csrfHeader buffers are the same length or guard against mismatches to prevent runtime errors.

    if (
      !crypto.timingSafeEqual(Buffer.from(csrfCookie), Buffer.from(csrfHeader))
    ) {
      functions.logger.log("CSRF validation rejected: Token mismatch");
      res
        .status(403)
        .json({ status: 403, error: "CSRF token validation failed" });
      return;
    }
    Error handling around buffer conversion

    Converting untrusted header values with Buffer.from could throw if the input is malformed. Consider wrapping timingSafeEqual and buffer creation in a try/catch to avoid crashing the middleware.

    if (
      !crypto.timingSafeEqual(Buffer.from(csrfCookie), Buffer.from(csrfHeader))
    ) {
      functions.logger.log("CSRF validation rejected: Token mismatch");
      res
        .status(403)
        .json({ status: 403, error: "CSRF token validation failed" });
      return;
    }
    Cookie configuration flexibility

    The setCsrfCookie helper always sets secure and sameSite flags. Verify whether secure should be conditional based on environment (e.g., disabled in local development) and if additional attributes (domain, path) are needed.

    export const setCsrfCookie = (res: Response, token: string): void => {
      res.cookie("CSRF-TOKEN", token, {
        httpOnly: true,
        secure: true,
        sameSite: "strict",
        path: "/",
        maxAge: 24 * 60 * 60 * 1000,
      });
    };

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Guard timingSafeEqual with length check

    Avoid runtime errors when token lengths differ by checking buffer lengths before
    using timingSafeEqual. Treat a length mismatch as a failed validation.

    functions/src/middlewares/csrf_middleware.ts [41-43]

    -if (
    -  !crypto.timingSafeEqual(Buffer.from(csrfCookie), Buffer.from(csrfHeader))
    -) {
    +const cookieBuf = Buffer.from(csrfCookie);
    +const headerBuf = Buffer.from(csrfHeader);
    +if (cookieBuf.length !== headerBuf.length || !crypto.timingSafeEqual(cookieBuf, headerBuf)) {
    Suggestion importance[1-10]: 7

    __

    Why: Pre-checking buffer lengths prevents runtime errors in crypto.timingSafeEqual and correctly treats mismatched lengths as validation failures.

    Medium
    Security
    Make CSRF cookie readable by JS

    Allow client-side scripts to read the CSRF token by disabling httpOnly.
    Double-submit cookie patterns require JavaScript access to the token for setting the
    header.

    functions/src/middlewares/csrf_middleware.ts [59-65]

     res.cookie("CSRF-TOKEN", token, {
    -  httpOnly: true,
    +  httpOnly: false,
       secure: true,
       sameSite: "strict",
       path: "/",
       maxAge: 24 * 60 * 60 * 1000,
     });
    Suggestion importance[1-10]: 7

    __

    Why: Disabling httpOnly is required for the double-submit pattern so client-side code can read the token and set the header, though it changes the security posture.

    Medium
    General
    Add header fallback support

    Provide a fallback to the previous header name for backward compatibility so that
    clients still sending x-csrf-token aren’t rejected. This ensures a smoother upgrade
    path without breaking existing consumers.

    functions/src/middlewares/csrf_middleware.ts [29]

    -const csrfHeader = req.header("x-xsrf-token");
    +const csrfHeader =
    +  req.header("x-xsrf-token") || req.header("x-csrf-token");
    Suggestion importance[1-10]: 6

    __

    Why: The fallback for "x-csrf-token" adds backward compatibility without breaking existing clients and is straightforward to implement.

    Low

    @hib4 hib4 merged commit ef66ebd into dev May 15, 2025
    1 check passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants