Skip to content

🛡️ Sentinel: [HIGH] Remove insecure secret retrieval from environment variables #958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
anchapin wants to merge 1 commit into from
Closed

🛡️ Sentinel: [HIGH] Remove insecure secret retrieval from environment variables #958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .jules/sentinel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,8 @@
**Vulnerability:** The application used a hardcoded fallback secret key (`"dev-secret-key-do-not-use-in-production-change-me"`) for JWT token generation and validation if the `SECRET_KEY` environment variable was missing.
**Learning:** Hardcoded cryptographic secrets are a severe vulnerability (CWE-798). If the environment variable isn't set properly, the application falls back to an insecure, easily guessable default in production, compromising all tokens.
**Prevention:** Never use default fallback values for cryptographic secrets. When removing hardcoded cryptographic secrets to use environment variables, ensure the application fails securely (e.g., raises a `ValueError`) if the variable is unset, rather than falling back to an insecure default. For testing, set dummy values in `conftest.py` before application imports.

## 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.
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Apr 5, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
**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.

**Prevention:** Always use `get_secret` from `core.secrets` when accessing secrets like API keys or credentials.
Comment on lines +41 to +44
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
## 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.

Copilot uses AI. Check for mistakes.
3 changes: 2 additions & 1 deletion backend/src/services/curseforge_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@
import httpx
from typing import Optional, Dict, Any
import logging
from core.secrets import get_secret

logger = logging.getLogger(__name__)

# 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", "")

Comment on lines 16 to 20
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment on lines 16 to 20
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

class CurseForgeService:
Expand Down
3 changes: 2 additions & 1 deletion backend/src/services/email_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import logging
from typing import Optional, List, Dict, Any
from dataclasses import dataclass
from core.secrets import get_secret

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -283,7 +284,7 @@ def get_email_service(
if _email_service is None:
import os

Comment on lines 285 to 286
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
import os

Copilot uses AI. Check for mistakes.
api_key = api_key or os.getenv("SENDGRID_API_KEY")
api_key = api_key or get_secret("SENDGRID_API_KEY")
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Apr 5, 2026

Choose a reason for hiding this comment

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

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.


if not api_key:
logger.warning("SendGrid API key not configured. Emails will be logged only.")
Expand Down
Loading