Skip to content

Conversation

@hib4
Copy link
Member

@hib4 hib4 commented May 9, 2025

PR Type

Enhancement, Documentation


Description

  • Add session cookie auth and CSRF protection

  • Implement application endpoints with validation

  • Support file uploads and Firebase storage

  • Enhance server CORS, cookies, and logging


Changes walkthrough 📝

Relevant files
Configuration changes
3 files
firebase.ts
Add Firebase storage bucket configuration                               
+1/-0     
index.ts
Enable CORS and maxInstances for API function                       
+4/-3     
package.json
Update dependencies and npm scripts                                           
+13/-7   
Enhancement
15 files
application_controller.ts
Implement application endpoints and validation                     
+889/-0 
auth_controller.ts
Add session cookies, CSRF, and logout logic                           
+261/-56
auth_middleware.ts
Validate session cookies instead of ID tokens                       
+34/-40 
csrf_middleware.ts
Introduce CSRF token protection middleware                             
+49/-0   
role_middleware.ts
Add restrictToRole authorization middleware                           
+35/-0   
application.ts
Create application-related API routes                                       
+22/-0   
auth.ts
Update auth routes for session login                                         
+5/-14   
index.ts
Mount application routes in router                                             
+2/-0     
ticket.ts
Remove auth middleware from ticket routes                               
+2/-11   
user.ts
Use camelCase conversion for user requests                             
+2/-2     
server.ts
Add cookie parsing, CSRF, session auth, logging                   
+46/-6   
application_types.ts
Add application state and question types                                 
+92/-0   
express.ts
Add TypedRequestBody interface                                                     
+3/-0     
fake_data_populator.ts
Extend fakery to generate application questions                   
+119/-4 
jwt.ts
Extract session cookies and UID utility                                   
+59/-0   
Miscellaneous
1 files
role.ts
Define RoleType enum                                                                         
+4/-0     
Documentation
1 files
application-endpoints.md
Document application PATCH endpoint                                           
+8/-0     
Additional files
1 files
.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.
  • @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: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Type Mismatch

    FileValidation.allowedTypes is declared as a single comma-separated string but treated like an array in validation logic, leading to incorrect MIME type checks.

    export interface FileValidation {
      required?: boolean;
      allowedTypes: string; // comma separated types e.g. image/jpeg,application/pdf
      maxSize: number; // in MB
    }
    Validation Logic

    The check if (fieldValue === undefined && fieldValue === "") uses && instead of ||, so empty or missing fields may bypass required validation.

    if (fieldValue === undefined && fieldValue === "") {
      continue;
    }
    CORS Misconfiguration

    allowedHeaders includes "X-XSRF-TOKEN" but CSRF middleware expects "x-csrf-token", causing valid CSRF-protected requests to be blocked.

      allowedHeaders: ["Content-Type", "Authorization", "X-XSRF-TOKEN"]
    }

    @github-actions
    Copy link

    github-actions bot commented May 9, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix storage bucket URL

    The storageBucket URL is malformed and missing the standard Firebase domain suffix.
    Update it to point to the correct AppSpot bucket.

    functions/src/config/firebase.ts [9]

    -storageBucket: "garuda-hacks-6-0.firebasestorage.app",
    +storageBucket: "garuda-hacks-6-0.appspot.com",
    Suggestion importance[1-10]: 9

    __

    Why: The bucket name "garuda-hacks-6-0.firebasestorage.app" is invalid and will break storage operations; switching to the correct AppSpot domain fixes a critical misconfiguration.

    High
    Correct empty value check

    The check for missing fieldValue uses &&, which is always false when one side is
    truthy. Change it to || to properly catch both undefined and empty string.

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

    -if (fieldValue === undefined && fieldValue === "") {
    +if (fieldValue === undefined || fieldValue === "") {
    Suggestion importance[1-10]: 8

    __

    Why: Using && here prevents catching either undefined or "", undermining validation logic; changing to || properly skips missing or empty inputs.

    Medium
    Fix undefined status field

    The response object references an undefined status variable, resulting in an
    incorrect payload. Replace it with a literal status code key.

    functions/src/controllers/auth_controller.ts [240-250]

     } catch (error) {
       functions.logger.error("Error when trying to logout", err.message);
       // force remove cookies
       res.clearCookie("__session", {
         httpOnly: true,
         sameSite: "strict",
         secure: process.env.NODE_ENV === "production",
       });
    -  res.status(500).json({ status, error: "Something went wrong." });
    +  res.status(500).json({ status: 500, error: "Something went wrong." });
     }
    Suggestion importance[1-10]: 8

    __

    Why: The catch block sends { status, error } referencing an undefined status variable, causing runtime errors; replacing it with status: 500 corrects the response payload.

    Medium
    General
    Add CSRF header to CORS

    The CSRF middleware checks the x-csrf-token header but CORS only allows
    X-XSRF-TOKEN. Add x-csrf-token to allowed headers to avoid preflight failures.

    functions/src/server.ts [11-21]

     const corsOptions: CorsOptions = {
       origin: [
         "http://localhost:3000",
         "http://localhost:3001",
         "http://localhost:5173",
         "https://garudahacks.com",
         "https://www.garudahacks.com",
       ],
       credentials: true,
    -  allowedHeaders: ["Content-Type", "Authorization", "X-XSRF-TOKEN"]
    +  allowedHeaders: ["Content-Type", "Authorization", "X-XSRF-TOKEN", "x-csrf-token"]
     }
    Suggestion importance[1-10]: 4

    __

    Why: The CSRF middleware relies on "x-csrf-token" but CORS only permits "X-XSRF-TOKEN", which may block valid requests; adding it ensures preflight passes.

    Low

    @hib4 hib4 merged commit 55cc96f into main 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.

    3 participants