Skip to content

Conversation

@LogicalGuy77
Copy link
Contributor

@LogicalGuy77 LogicalGuy77 commented Sep 16, 2025

User description

Removed the hardcoded Brevo API with its environment variable. Requesting maintainers to generate a new API key through the Brevo API dashboard and add the env variable in the Vercel's deployment. Fixes: #20

I will open up a follow up PR to move the email sending logic to server side.


PR Type

Bug fix, Enhancement


Description

  • Replace hardcoded Brevo API key with environment variable

  • Add error handling for missing API key configuration

  • Create environment variable example file

  • Improve security by removing exposed credentials


Diagram Walkthrough

flowchart LR
  A["Hardcoded API Key"] --> B["Environment Variable"]
  B --> C["Error Handling"]
  B --> D[".env.example"]
Loading

File Walkthrough

Relevant files
Bug fix
emailService.js
Replace hardcoded API key with environment variable           

utils/services/emailService.js

  • Replace hardcoded brevo_key with process.env.NEXT_PUBLIC_BREVO_API_KEY
  • Add validation check for missing API key
  • Include error logging and user notification for configuration issues
+8/-1     
Configuration changes
.env.example
Add environment variable example file                                       

.env.example

  • Add example environment variable for Brevo API key configuration
+1/-0     

@vercel
Copy link

vercel bot commented Sep 16, 2025

@LogicalGuy77 is attempting to deploy a commit to the lighthouse-storage Team on Vercel.

A member of the Team first needs to authorize it.

@codiumai-pr-agent-free
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

Client-side API key exposure:
While the PR removes the hardcoded API key (which is good), it uses the NEXT_PUBLIC_ prefix for the environment variable (NEXT_PUBLIC_BREVO_API_KEY). This prefix in Next.js makes the variable accessible on the client-side, meaning the API key will still be exposed to users in browser requests. API keys should be kept server-side only. The PR description mentions a follow-up PR to move email logic server-side, but until then, this implementation still has security concerns.

⚡ Recommended focus areas for review

Environment Variable Naming

The environment variable is defined as NEXT_PUBLIC_BREVO_API_KEY in the code but referenced as BREVO_API_KEY in the .env.example file. These should match to avoid configuration errors.

const brevo_key = process.env.NEXT_PUBLIC_BREVO_API_KEY;
Client-side Exposure

Using NEXT_PUBLIC_ prefix exposes this API key to the client-side, which is still a security risk. API keys should be used server-side only.

const brevo_key = process.env.NEXT_PUBLIC_BREVO_API_KEY;

@codiumai-pr-agent-free
Copy link

codiumai-pr-agent-free bot commented Sep 16, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Security
Prevent exposing secret API key

Remove the NEXT_PUBLIC_ prefix from the BREVO_API_KEY environment variable to
prevent exposing this secret key to the client-side browser.

utils/services/emailService.js [4]

-const brevo_key = process.env.NEXT_PUBLIC_BREVO_API_KEY;
+const brevo_key = process.env.BREVO_API_KEY;
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical security vulnerability where a secret API key is exposed to the client-side due to the NEXT_PUBLIC_ prefix.

High
Possible issue
Improve server-side error handling logic

Replace the client-side notify call with throw new Error for proper server-side
error handling, as this function should execute in a server environment.

utils/services/emailService.js [8-12]

 if (!brevo_key) {
-  console.error("Brevo API key is not configured");
-  notify("Email service configuration error", "error");
-  return;
+  console.error("Brevo API key is not configured.");
+  throw new Error("Email service is not configured.");
 }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly points out that the client-side notify function will not work in a server-side context (where this logic should reside) and proposes the correct error handling pattern of throwing an error.

High
  • Update

Copy link
Collaborator

@arpitB-dev arpitB-dev left a comment

Choose a reason for hiding this comment

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

Hey @LogicalGuy77 thanks for this, we are merging it to our main branch and adding to it, in terms of security we also have also ip check enabled on brevo

@arpitB-dev arpitB-dev merged commit 14d2e57 into lighthouse-web3:main Sep 16, 2025
1 check failed
@LogicalGuy77 LogicalGuy77 deleted the fix-env branch September 18, 2025 06:54
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.

Security: Hardcoded API Key

2 participants