Skip to content

Conversation

@junkisai
Copy link
Member

@junkisai junkisai commented Mar 18, 2025

Issue

  • resolve:

Why is this change needed?

Since there were multiple environment variables serving the same purpose, I wanted to consolidate them into one to avoid confusion.

Upon checking Vercel's Environments, I found that SUPABASE_AUTH_GITHUB_CLIENT_ID and SUPABASE_AUTH_GITHUB_SECRET were registered only for the Development environment, and their keys contained production environment values. Therefore, I decided that changing the keys referenced in config.toml to GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET would not be an issue.

What would you like reviewers to focus on?

Testing Verification

I confirmed whether GitHub authentication works on the staging branch.

2025-03-21.15.42.17.mov

ref. #922

What was done

🤖 Generated by PR Agent at da473e1

  • Updated GitHub authentication configuration to use unified environment variables.
  • Removed hardcoded GitHub configuration for better maintainability.
  • Updated config.toml to reference new environment variable keys.

Detailed Changes

Relevant files
Enhancement
config.ts
Removed hardcoded GitHub configuration object                       

frontend/apps/app/libs/github/config.ts

  • Removed hardcoded GitHub configuration object.
  • Retained the validation function for required environment variables.
  • +0/-8     
    config.toml
    Updated GitHub OAuth configuration in `config.toml`           

    frontend/packages/db/supabase/config.toml

  • Updated GitHub OAuth configuration to use new environment variables.
  • Replaced SUPABASE_AUTH_GITHUB_CLIENT_ID and
    SUPABASE_AUTH_GITHUB_SECRET with GITHUB_CLIENT_ID and
    GITHUB_CLIENT_SECRET.
  • +2/-2     

    Additional Notes


    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @changeset-bot
    Copy link

    changeset-bot bot commented Mar 18, 2025

    ⚠️ No Changeset found

    Latest commit: da473e1

    Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

    This PR includes no changesets

    When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

    Click here to learn what changesets are, and how to add one.

    Click here if you're a maintainer who wants to add a changeset to this PR

    @vercel
    Copy link

    vercel bot commented Mar 18, 2025

    The latest updates on your projects. Learn more about Vercel for Git ↗︎

    Name Status Preview Comments Updated (UTC)
    liam-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 21, 2025 6:14am
    liam-erd-sample ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 21, 2025 6:14am
    4 Skipped Deployments
    Name Status Preview Comments Updated (UTC)
    liam-docs ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2025 6:14am
    test-liam-app ⬜️ Ignored (Inspect) Mar 21, 2025 6:14am
    test-liam-docs ⬜️ Ignored (Inspect) Mar 21, 2025 6:14am
    test-liam-erd-sample ⬜️ Ignored (Inspect) Mar 21, 2025 6:14am

    @junkisai junkisai self-assigned this Mar 18, 2025
    @supabase
    Copy link

    supabase bot commented Mar 18, 2025

    Updates to Preview Branch (chore/env-file) ↗︎

    Deployments Status Updated
    Database Fri, 21 Mar 2025 05:13:52 UTC
    Services Fri, 21 Mar 2025 05:13:52 UTC
    APIs Fri, 21 Mar 2025 05:13:52 UTC

    Tasks are run on every commit but only new migration files are pushed.
    Close and reopen this PR if you want to apply changes from existing seed or migration files.

    Tasks Status Updated
    Configurations Fri, 21 Mar 2025 05:13:52 UTC
    Migrations Fri, 21 Mar 2025 05:13:53 UTC
    Seeding Fri, 21 Mar 2025 05:13:53 UTC
    Edge Functions Fri, 21 Mar 2025 05:13:53 UTC

    View logs for this Workflow Run ↗︎.
    Learn more about Supabase for Git ↗︎.

    @liam-migration-preview
    Copy link

    The schema changes provided involve modifications to two files related to GitHub integration: one in a TypeScript configuration file and the other in a Supabase configuration file. Below is a detailed review of these changes:

    1. File: frontend/apps/app/libs/github/config.ts

      • Changes Overview: The changes involve the removal of a GitHub configuration object that included sensitive credentials such as appId, privateKey, webhookSecret, clientId, and clientSecret. The code snippet shows that these attributes were directly referencing environment variables, which is a common practice for managing sensitive information.
      • Implications:
        • Security Improvements: The removal of direct exports for sensitive credentials is a positive step towards enhancing security. Hardcoding or directly exporting sensitive keys can expose them to unintended access, especially in a client-side context.
        • Environment Variable Management: While the export of the configuration object has been removed, it raises questions about how these credentials are now being handled. It is crucial to ensure that there is a robust mechanism in place to validate and load these environment variables effectively. The presence of a validateConfig function indicates that there is an intention to check for the existence of required variables, which is commendable.
        • Potential Impact on Functionality: If the application relies on these configurations for its operations, their removal could lead to runtime errors or failed integrations unless alternative methods for accessing these credentials are implemented.
    2. File: frontend/packages/db/supabase/config.toml

      • Changes Overview: This modification involves changing the way GitHub authentication credentials are referenced in the Supabase configuration. The previous references to environment variables prefixed by SUPABASE_AUTH_ have been simplified to GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET.
      • Implications:
        • Clarity and Consistency: Simplifying the variable names enhances clarity. It reduces the complexity of managing multiple prefixes and makes it easier to understand which environment variables are related to GitHub authentication.
        • Potential Issues with Deprecation: While this change may clarify the configuration, it is essential to ensure that any previous usages of the SUPABASE_AUTH_ prefixed variables are updated accordingly in the application. Failure to do so could lead to broken authentication workflows.
        • Deployment Considerations: The deployment process must be reviewed to ensure that the new environment variables are correctly set in all environments (development, testing, production). This change necessitates a thorough check to confirm that the application can access the newly defined environment variables without issues.

    Overall Assessment:
    The changes reflect a positive direction concerning security and configuration management. However, attention must be paid to ensure that the removal of the configuration object does not disrupt existing functionality. Additionally, the transition to new environment variable naming conventions requires careful validation to avoid potential integration problems. It is advisable to conduct comprehensive testing post-implementation to confirm that all aspects of GitHub integration function as intended without introducing vulnerabilities or breaking changes. Documentation should also be updated to reflect these changes for future reference and onboarding of new developers.

    Migration URL: https://liam-app-git-staging-route-06-core.vercel.app/app/projects/4/migrations/13

    @qodo-free-for-open-source-projects
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Missing Exports

    The PR removes the githubConfig export but doesn't provide a replacement. If this object was used elsewhere in the codebase, those references will break.

    export const validateConfig = (): { valid: boolean; missing: string[] } => {
      const requiredEnvVars = [
        'GITHUB_APP_ID',
        'GITHUB_PRIVATE_KEY',

    @qodo-free-for-open-source-projects
    Copy link
    Contributor

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    Copy link
    Contributor

    @MH4GF MH4GF left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thanks 👍🏻

    @MH4GF MH4GF added this pull request to the merge queue Mar 21, 2025
    Copy link
    Member

    @NoritakaIkeda NoritakaIkeda left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM!

    Comment on lines -1 to -7
    export const githubConfig = {
    appId: process.env.GITHUB_APP_ID,
    privateKey: process.env.GITHUB_PRIVATE_KEY,
    webhookSecret: process.env.GITHUB_WEBHOOK_SECRET,
    clientId: process.env.GITHUB_CLIENT_ID,
    clientSecret: process.env.GITHUB_CLIENT_SECRET,
    }
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    📝 I have confirmed that githubConfig is not referenced elsewhere.

    Comment on lines -230 to +231
    client_id = "env(SUPABASE_AUTH_GITHUB_CLIENT_ID)"
    secret = "env(SUPABASE_AUTH_GITHUB_SECRET)"
    client_id = "env(GITHUB_CLIENT_ID)"
    secret = "env(GITHUB_CLIENT_SECRET)"
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    📝 I have confirmed that SUPABASE_AUTH_GITHUB_CLIENT_ID and GITHUB_CLIENT_ID, as well as SUPABASE_AUTH_GITHUB_SECRET and GITHUB_CLIENT_SECRET, have the same values on Vercel.

    Merged via the queue into main with commit f2e133d Mar 21, 2025
    33 checks passed
    @MH4GF MH4GF deleted the chore/env-file branch March 21, 2025 06:53
    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.

    4 participants