🛡️ Sentinel: [HIGH] Remove insecure secret retrieval from environment variables#958
🛡️ Sentinel: [HIGH] Remove insecure secret retrieval from environment variables#958
Conversation
… variables - Update CurseForge API Key Retrieval to use `get_secret` from `core.secrets` - Update SendGrid API Key Retrieval to use `get_secret` from `core.secrets` - Updated sentinel journal Co-authored-by: anchapin <6326294+anchapin@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces direct environment variable access for API keys with a centralized secret retrieval helper and documents the associated security pattern in the Sentinel guide. Sequence diagram for secure secret retrieval in servicessequenceDiagram
participant App
participant CurseForgeService
participant EmailService
participant CoreSecrets as core_secrets
participant Env as environment
App->>CurseForgeService: import module
CurseForgeService->>CoreSecrets: get_secret(CURSEFORGE_API_KEY, "")
CoreSecrets-->>CurseForgeService: curseforge_api_key
App->>EmailService: get_email_service(api_key=None)
EmailService->>CoreSecrets: get_secret(SENDGRID_API_KEY)
CoreSecrets-->>EmailService: sendgrid_api_key
EmailService-->>App: EmailService_instance
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Loading
CURSEFORGE_API_KEYviaget_secretat module import time makes rotation and test overrides harder; consider resolving the secret in a factory or on-demand withinCurseForgeServicemethods instead of as a module-level constant. - For both
CURSEFORGE_API_KEYandSENDGRID_API_KEY, consider failing fast (e.g., raising a clear exception) when the secret is missing instead of silently defaulting to an empty or falsy value to avoid misconfigurations going unnoticed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Loading `CURSEFORGE_API_KEY` via `get_secret` at module import time makes rotation and test overrides harder; consider resolving the secret in a factory or on-demand within `CurseForgeService` methods instead of as a module-level constant.
- For both `CURSEFORGE_API_KEY` and `SENDGRID_API_KEY`, consider failing fast (e.g., raising a clear exception) when the secret is missing instead of silently defaulting to an empty or falsy value to avoid misconfigurations going unnoticed.
## Individual Comments
### Comment 1
<location path="backend/src/services/email_service.py" line_range="287" />
<code_context>
import os
- api_key = api_key or os.getenv("SENDGRID_API_KEY")
+ api_key = api_key or get_secret("SENDGRID_API_KEY")
if not api_key:
</code_context>
<issue_to_address>
**issue (bug_risk):** Validate that get_secret’s behavior on missing SENDGRID_API_KEY still allows the log-only fallback path.
Previously, a missing `SENDGRID_API_KEY` resulted in `None` and fell through to the `if not api_key:` block, triggering the log-only behavior. With `get_secret("SENDGRID_API_KEY")`, if the secret is missing and it either raises or returns a truthy sentinel, we may skip that fallback and fail earlier. Please confirm that `get_secret` returns a falsy value (e.g., `None`) when the secret is absent, or catch/handle its exceptions here to preserve the existing log-only behavior.
</issue_to_address>
### Comment 2
<location path=".jules/sentinel.md" line_range="43" />
<code_context>
+
+## 2025-04-05 - Remove insecure secret retrieval from environment variables
+**Vulnerability:** Retrieving API keys directly from `os.getenv` instead of using the centralized secrets management solution.
+**Learning:** Hardcoded environment variables don't support dynamic rotation, centralized security management (AWS Secrets Manager, Vault, etc), and potentially risk exposure if `.env` files are accidentally committed or logged. Using `core.secrets.get_secret()` allows transparent migration to secure vaults.
+**Prevention:** Always use `get_secret` from `core.secrets` when accessing secrets like API keys or credentials.
</code_context>
<issue_to_address>
**issue (typo):** Add a period after "etc" inside the parentheses for correct punctuation.
Change `etc)` to `etc.)` so the abbreviation is correctly punctuated within the parentheses.
```suggestion
**Learning:** Hardcoded environment variables don't support dynamic rotation, centralized security management (AWS Secrets Manager, Vault, etc.), and potentially risk exposure if `.env` files are accidentally committed or logged. Using `core.secrets.get_secret()` allows transparent migration to secure vaults.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| import os | ||
|
|
||
| api_key = api_key or os.getenv("SENDGRID_API_KEY") | ||
| api_key = api_key or get_secret("SENDGRID_API_KEY") |
There was a problem hiding this comment.
issue (bug_risk): Validate that get_secret’s behavior on missing SENDGRID_API_KEY still allows the log-only fallback path.
Previously, a missing SENDGRID_API_KEY resulted in None and fell through to the if not api_key: block, triggering the log-only behavior. With get_secret("SENDGRID_API_KEY"), if the secret is missing and it either raises or returns a truthy sentinel, we may skip that fallback and fail earlier. Please confirm that get_secret returns a falsy value (e.g., None) when the secret is absent, or catch/handle its exceptions here to preserve the existing log-only behavior.
|
|
||
| ## 2025-04-05 - Remove insecure secret retrieval from environment variables | ||
| **Vulnerability:** Retrieving API keys directly from `os.getenv` instead of using the centralized secrets management solution. | ||
| **Learning:** Hardcoded environment variables don't support dynamic rotation, centralized security management (AWS Secrets Manager, Vault, etc), and potentially risk exposure if `.env` files are accidentally committed or logged. Using `core.secrets.get_secret()` allows transparent migration to secure vaults. |
There was a problem hiding this comment.
issue (typo): Add a period after "etc" inside the parentheses for correct punctuation.
Change etc) to etc.) so the abbreviation is correctly punctuated within the parentheses.
| **Learning:** Hardcoded environment variables don't support dynamic rotation, centralized security management (AWS Secrets Manager, Vault, etc), and potentially risk exposure if `.env` files are accidentally committed or logged. Using `core.secrets.get_secret()` allows transparent migration to secure vaults. | |
| **Learning:** Hardcoded environment variables don't support dynamic rotation, centralized security management (AWS Secrets Manager, Vault, etc.), and potentially risk exposure if `.env` files are accidentally committed or logged. Using `core.secrets.get_secret()` allows transparent migration to secure vaults. |
There was a problem hiding this comment.
Pull request overview
This PR aims to route sensitive configuration (API keys) through the centralized secrets manager (core.secrets.get_secret) rather than direct os.getenv access, supporting future migration to vault-backed secret backends.
Changes:
- Updated SendGrid API key lookup in the email service to use
get_secret("SENDGRID_API_KEY"). - Updated CurseForge API key lookup to use
get_secret("CURSEFORGE_API_KEY", ""). - Added a Sentinel knowledge-base entry documenting the pattern change.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
backend/src/services/email_service.py |
Switches SendGrid API key retrieval to core.secrets.get_secret. |
backend/src/services/curseforge_service.py |
Switches CurseForge API key retrieval to core.secrets.get_secret. |
.jules/sentinel.md |
Documents the change as a Sentinel security learning/prevention entry. |
Comments suppressed due to low confidence (1)
backend/src/services/email_service.py:293
- The warning says emails will be "logged only", but the code assigns a dummy API key and still constructs a
SendGridEmailService. Ifsendgridis installed, this will initialize a real SendGrid client and attempt network sends (likely failing with 401), which is inconsistent with the intended behavior. Consider using a dedicated logging/no-op email service when the key is missing, or keep the service in logging-only mode instead of setting a dummy key.
if not api_key:
logger.warning("SendGrid API key not configured. Emails will be logged only.")
api_key = "dummy-key-for-development"
_email_service = SendGridEmailService(api_key=api_key, from_email=from_email)
| import os | ||
|
|
There was a problem hiding this comment.
import os inside get_email_service() is now unused after switching to get_secret(). Removing it will avoid lint/unused-import failures and keeps the function minimal.
| import os |
| import os | ||
| import httpx | ||
| from typing import Optional, Dict, Any | ||
| import logging | ||
| from core.secrets import get_secret |
There was a problem hiding this comment.
import os is unused in this module after replacing os.getenv() with get_secret() for the API key. Please remove it to prevent unused-import lint issues.
| # CurseForge API configuration | ||
| CURSEFORGE_API_BASE_URL = "https://api.curseforge.com/v1" | ||
| # Note: Requires API key in production - get from https://console.curseforge.com/ | ||
| CURSEFORGE_API_KEY = os.getenv("CURSEFORGE_API_KEY", "") | ||
| CURSEFORGE_API_KEY = get_secret("CURSEFORGE_API_KEY", "") | ||
|
|
There was a problem hiding this comment.
CURSEFORGE_API_KEY is now retrieved at import time via get_secret(). In core/secrets.py, backend initialization (e.g., Vault/Doppler) can raise before the internal try/except in SecretsManager.get_secret(), which would cause this module import to fail even though the CurseForge key is optional. Consider deferring secret lookup to CurseForgeService.__init__ (or first use), or updating SecretsManager.get_secret() to fail-soft during backend initialization when a default is provided.
| # CurseForge API configuration | ||
| CURSEFORGE_API_BASE_URL = "https://api.curseforge.com/v1" | ||
| # Note: Requires API key in production - get from https://console.curseforge.com/ | ||
| CURSEFORGE_API_KEY = os.getenv("CURSEFORGE_API_KEY", "") | ||
| CURSEFORGE_API_KEY = get_secret("CURSEFORGE_API_KEY", "") | ||
|
|
There was a problem hiding this comment.
PR description says secrets access was migrated from os.getenv to core.secrets.get_secret for API keys/credentials, but there are still API credentials being read directly from os.getenv (e.g. MODRINTH_TOKEN in backend/src/services/modrinth_service.py). Either expand this change to cover the remaining secret lookups, or adjust the PR description/sentinel entry to match the narrower scope.
| ## 2025-04-05 - Remove insecure secret retrieval from environment variables | ||
| **Vulnerability:** Retrieving API keys directly from `os.getenv` instead of using the centralized secrets management solution. | ||
| **Learning:** Hardcoded environment variables don't support dynamic rotation, centralized security management (AWS Secrets Manager, Vault, etc), and potentially risk exposure if `.env` files are accidentally committed or logged. Using `core.secrets.get_secret()` allows transparent migration to secure vaults. | ||
| **Prevention:** Always use `get_secret` from `core.secrets` when accessing secrets like API keys or credentials. |
There was a problem hiding this comment.
The wording "Hardcoded environment variables" is misleading—environment variables aren’t hardcoded in code. Consider rephrasing to something like "Retrieving secrets directly via os.getenv" / "direct environment variable access" to keep the vulnerability description accurate.
| ## 2025-04-05 - Remove insecure secret retrieval from environment variables | |
| **Vulnerability:** Retrieving API keys directly from `os.getenv` instead of using the centralized secrets management solution. | |
| **Learning:** Hardcoded environment variables don't support dynamic rotation, centralized security management (AWS Secrets Manager, Vault, etc), and potentially risk exposure if `.env` files are accidentally committed or logged. Using `core.secrets.get_secret()` allows transparent migration to secure vaults. | |
| **Prevention:** Always use `get_secret` from `core.secrets` when accessing secrets like API keys or credentials. | |
| ## 2025-04-05 - Remove insecure direct environment secret retrieval | |
| **Vulnerability:** Retrieving API keys directly from `os.getenv` instead of using the centralized secrets management solution. | |
| **Learning:** Direct environment variable access for secrets via `os.getenv` does not provide centralized rotation, auditing, or secret management (AWS Secrets Manager, Vault, etc.), and still risks exposure if `.env` files are accidentally committed or logged. Using `core.secrets.get_secret()` allows transparent migration to secure vaults. | |
| **Prevention:** Always use `get_secret` from `core.secrets` when accessing secrets like API keys or credentials, rather than retrieving them directly from environment variables. |
🚨 Severity: HIGH
💡 Vulnerability: Hardcoded environment variables don't support dynamic rotation, centralized security management (AWS Secrets Manager, Vault, etc), and potentially risk exposure if
.envfiles are accidentally committed or logged.🎯 Impact: Using
core.secrets.get_secret()allows transparent migration to secure vaults.🔧 Fix: Replaced
os.getenvwithget_secretfromcore.secretswhen accessing secrets like API keys or credentials.✅ Verification: Ran
pytestbackend tests to ensure no regressions.PR created automatically by Jules for task 5917544077159129643 started by @anchapin
Summary by Sourcery
Replace direct environment variable access for external service credentials with centralized secret retrieval to improve security.
Enhancements:
Documentation: