Skip to content

Conversation

@hib4
Copy link
Member

@hib4 hib4 commented May 11, 2025

PR Type

Bug fix, Enhancement


Description

  • Pin Firebase Admin to supported version

  • Use env vars for project ID and bucket

  • Refactor initialization and logging format

  • Standardize formatting and ESLint rules


Changes walkthrough 📝

Relevant files
Configuration changes
2 files
firebase.ts
Use environment variables in Firebase config                         
+3/-2     
.eslintrc.js
Consolidate ESLint plugins and rule ordering                         
+8/-8     
Formatting
4 files
index.ts
Reformat onRequest invocation arguments                                   
+7/-4     
application.ts
Add semicolons and format route definitions                           
+6/-5     
server.ts
Standardize imports, commas, and logging format                   
+16/-10 
fake_data_populator.ts
Add trailing commas in object literals                                     
+27/-27 
Dependencies
1 files
package.json
Downgrade firebase-admin dependency version                           
+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 11, 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
    🔒 Security concerns

    Sensitive information logging:
    The new logging middleware captures and logs full req.headers, req.cookies, req.body, and authorizationHeader, which can expose session cookies and authorization tokens in logs. Consider redacting or filtering sensitive fields before logging.

    ⚡ Recommended focus areas for review

    Env Var Validation

    The code now reads PROJECT_ID and STORAGE_BUCKET from environment variables but lacks validation or fallbacks. Ensure these vars are present and properly handled to avoid runtime errors.

    projectId: process.env.PROJECT_ID,
    credential: admin.credential.cert(serviceAccount as admin.ServiceAccount),
    storageBucket: process.env.STORAGE_BUCKET,
    Sensitive Logging

    Logging full req.headers, req.cookies, and req.body including authorization and session cookies may expose sensitive data in logs. Consider sanitizing or redacting sensitive fields.

      body: undefined,
    };
    
    const contentType = req.headers["content-type"] || "";
    if (!contentType.includes("multipart/form-data")) {
      logData.body = req.body;
    }
    
    const timestamp = new Date().toISOString();
    functions.logger.info(
      `[${timestamp}] Incoming Request Details: ${JSON.stringify(
        logData,
        null,
        2
      )}`
    );

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Security
    Redact sensitive log fields

    Avoid logging raw authentication tokens and session cookies by redacting them prior
    to logging. Mask or omit these fields to prevent sensitive data exposure in logs.

    functions/src/server.ts [41-59]

    -authorizationHeader: req.headers.authorization || "Not Present",
    -sessionCookie: req.cookies.__session || "Not Present",
    -...
    +const safeLogData = {
    +  ...logData,
    +  authorizationHeader: logData.authorizationHeader !== "Not Present" ? "[REDACTED]" : "Not Present",
    +  sessionCookie: logData.sessionCookie !== "Not Present" ? "[REDACTED]" : "Not Present",
    +};
     functions.logger.info(
       `[${timestamp}] Incoming Request Details: ${JSON.stringify(
    -    logData,
    +    safeLogData,
         null,
         2
       )}`
     );
    Suggestion importance[1-10]: 9

    __

    Why: Logging raw authentication headers and session cookies exposes sensitive data, so masking these fields greatly improves security.

    High
    Possible issue
    Validate Firebase config vars

    Validate that the PROJECT_ID and STORAGE_BUCKET env vars are set before calling
    admin.initializeApp to prevent runtime errors if they're missing. Throw a
    descriptive error or provide defaults. This ensures the app won't initialize with
    undefined config.

    functions/src/config/firebase.ts [8-10]

    -projectId: process.env.PROJECT_ID,
    -storageBucket: process.env.STORAGE_BUCKET,
    +const { PROJECT_ID, STORAGE_BUCKET } = process.env;
    +if (!PROJECT_ID || !STORAGE_BUCKET) {
    +  throw new Error("Environment variables PROJECT_ID and STORAGE_BUCKET must be defined");
    +}
    +admin.initializeApp({
    +  projectId: PROJECT_ID,
    +  credential: admin.credential.cert(serviceAccount as admin.ServiceAccount),
    +  storageBucket: STORAGE_BUCKET,
    +});
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly ensures that PROJECT_ID and STORAGE_BUCKET are defined before initializing Firebase to prevent runtime errors, improving robustness.

    Medium

    @hib4 hib4 merged commit f3e33c3 into dev May 11, 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