Skip to content

Conversation

@hib4
Copy link
Member

@hib4 hib4 commented May 9, 2025

PR Type

Enhancement


Description

  • Standardize formatting and JSON payloads

  • Enhance file upload with type and size checks

  • Improve session and logout cookie handling

  • Refine CSRF and role middleware logic


Changes walkthrough 📝

Relevant files
Enhancement
application_controller.ts
Refactor application controller formatting and uploads     

functions/src/controllers/application_controller.ts

  • standardized import formatting and semicolons
  • improved patchApplication error handling
  • enhanced file upload with Busboy checks
  • refactored data construction and validation logic
  • +239/-167
    auth_controller.ts
    Refactor authentication controller formatting                       

    functions/src/controllers/auth_controller.ts

  • standardized JSON and response punctuation
  • improved session cookie and CSRF handling
  • added logout unauthorized user check
  • refined sessionLogin and sessionCheck logic
  • +100/-75
    csrf_middleware.ts
    Enhance CSRF middleware handling                                                 

    functions/src/middlewares/csrf_middleware.ts

  • convert to RequestHandler type
  • added logging for CSRF tokens
  • standardized next/return calls
  • +24/-10 
    role_middleware.ts
    Refactor role middleware request handling                               

    functions/src/middlewares/role_middleware.ts

  • refactored restrictToRole signature multiline
  • fixed cookie extraction from request
  • standardized JSON error responses
  • +17/-8   
    Formatting
    auth_middleware.ts
    Cleanup authentication middleware imports                               

    functions/src/middlewares/auth_middleware.ts

    • removed unused FirebaseError import
    • reformatted import spacing
    +0/-1     
    Configuration changes
    .eslintrc.js
    Update ESLint no-explicit-any rule                                             

    functions/.eslintrc.js

    +1/-1     

    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 9, 2025
    @github-actions
    Copy link

    github-actions bot commented May 9, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Empty value check

    The condition if (fieldValue === undefined && fieldValue === "") uses && instead of ||, so undefined or empty values won't be skipped correctly.

    if (fieldValue === undefined && fieldValue === "") {
      continue;
    }
    Undefined status variable

    In the logout error handler you call res.status(500).json({ status, error: ... }), but status is not defined, which will cause a runtime exception.

    res.status(500).json({ status, error: "Something went wrong." });
    File size message

    The error message divides MAX_FILE_SIZE by 1024*1024, but MAX_FILE_SIZE is already in MB, resulting in an incorrect size displayed.

    file.on("limit", () => {
      fileSizeExceeded = true;
      res.writeHead(413, {
        Connection: "close",
        "Content-Type": "application/json",
      });
      res.end(
        JSON.stringify({
          error: "File too large",

    @github-actions
    Copy link

    github-actions bot commented May 9, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix empty-field skipping logic

    The check to skip empty or missing values uses AND, which will never be true. It
    should use OR to correctly skip when the field is undefined or empty.

    functions/src/controllers/application_controller.ts [232-234]

    -if (fieldValue === undefined && fieldValue === "") {
    +if (fieldValue === undefined || fieldValue === "") {
       continue;
     }
    Suggestion importance[1-10]: 9

    __

    Why: The check fieldValue === undefined && fieldValue === "" is always false; switching to OR fixes a critical bug in request validation.

    High
    Add return after error response

    After sending an error response, you must return to stop further processing.
    Otherwise, the success response will still be sent.

    functions/src/controllers/auth_controller.ts [374-379]

     if (!decodedSessionCookie) {
       functions.logger.error("Could not find session cookie");
       res
         .status(400)
         .json({ status: 400, error: "Could not find session cookie" });
    +  return;
     }
    Suggestion importance[1-10]: 6

    __

    Why: Without a return after the error response, execution continues, causing unintended behavior in sessionCheck.

    Low
    Return after 404 response

    You need to return immediately after sending a 404 response to prevent the handler
    from continuing and causing errors.

    functions/src/controllers/application_controller.ts [822-828]

     if (!docRef.exists) {
       res.status(404).json({
         status: 404,
         error: "Not found",
         message: `Cannot find this user`,
       });
    +  return;
     }
    Suggestion importance[1-10]: 6

    __

    Why: Adding a return after sending the 404 prevents further execution and potential runtime errors.

    Low
    General
    Reject promise on file limit

    After closing the response on size limit, reject the busboy promise so the upstream
    logic exits cleanly instead of hanging.

    functions/src/controllers/application_controller.ts [572-591]

     file.on("limit", () => {
       fileSizeExceeded = true;
       res.writeHead(413, {
         Connection: "close",
         "Content-Type": "application/json",
       });
       res.end(
         JSON.stringify({
           error: "File too large",
           details: [
             {
               field_id: questionId,
    -          message: `File size exceeds maximum limit of ${
    -            MAX_FILE_SIZE / (1024 * 1024)
    -          }MB`,
    +          message: `File size exceeds maximum limit of ${MAX_FILE_SIZE}MB`,
             },
           ],
         })
       );
    +  reject(new Error("File size exceeds maximum limit"));
     });
    Suggestion importance[1-10]: 7

    __

    Why: Rejecting the busboy promise ensures that the upstream await does not hang after res.end, improving error handling in the file upload flow.

    Medium

    @hib4 hib4 merged commit 0ba6ff5 into dev May 9, 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