Skip to content

Conversation

@heryandjaruma
Copy link
Collaborator

@heryandjaruma heryandjaruma commented May 23, 2025

User description

  • Handle case where existing user with email password is allowed.
  • Handle case where sign up with google replacing an existing user data.

PR Type

Bug fix, Enhancement


Description

  • Validate name and catch duplicate emails

  • Skip Firestore write for existing users

  • Structured error responses in registration flow

  • Explicit token error handling in login


Changes walkthrough 📝

Relevant files
Enhancement
auth_controller.ts
Refactor auth register and sessionLogin error handling     

functions/src/controllers/auth_controller.ts

  • Added name validation before user creation
  • Handled duplicate email errors in register
  • Avoided creating duplicate Firestore user documents
  • Refactored sessionLogin with token verification and errors
  • +133/-72

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @heryandjaruma heryandjaruma self-assigned this May 23, 2025
    @heryandjaruma heryandjaruma merged commit 3ef7596 into main May 23, 2025
    1 check passed
    @github-actions
    Copy link

    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

    Missing Error Response

    The Firestore check in sessionLogin catches errors and returns without sending an HTTP response, which can cause the request to hang. Ensure you send a proper error response before returning.

    } catch (error) {
      functions.logger.error("Error when trying to check if user existed for session login", error);
      return
    }
    Redundant User Fetch

    There's a second call to auth.getUserByEmail in the sessionLogin else branch even though the user was already fetched earlier, leading to unnecessary overhead. Consider reusing the existing user object.

    } else {
      user = await auth.getUserByEmail(decodedIdToken.email);
    }
    Uniform Error Handling

    Both the Firestore write and token generation error handlers in register return a generic "Something went wrong". Differentiating or propagating specific error details could improve debugging.

    } catch (error) {
      functions.logger.error("Error when checking existing user for registration:", error);
      res.status(500).json({
        status: 500,
        error: "Something went wrong",
      });
      return;
    }

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Send response on DB check failure

    Sending a response in this catch block prevents the request from hanging when the DB
    check fails. Include a 500 response and return to properly terminate the request.

    functions/src/controllers/auth_controller.ts [529-532]

     } catch (error) {
    -  functions.logger.error("Error when trying to check if user existed for session login", error);
    -  return
    +  functions.logger.error("Error when checking user existence for session login", error);
    +  res.status(500).json({ status: 500, error: "Something went wrong" });
    +  return;
     }
    Suggestion importance[1-10]: 8

    __

    Why: The catch block for checking user existence currently only logs errors and returns, causing requests to hang; adding a 500 response correctly handles failures.

    Medium
    Security
    Harden session cookie settings

    For production security, enable secure and set a sameSite policy on your session
    cookie to mitigate CSRF and ensure it's only sent over HTTPS.

    functions/src/controllers/auth_controller.ts [540-543]

     res.cookie("__session", cookies, {
       httpOnly: true,
    +  secure: process.env.NODE_ENV === "production",
    +  sameSite: "Strict",
       maxAge: SESSION_EXPIRY_SECONDS,
     });
    Suggestion importance[1-10]: 7

    __

    Why: Enabling the secure and sameSite cookie attributes improves session security by ensuring cookies are only sent over HTTPS and mitigating CSRF.

    Medium

    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