Skip to content

Require explicit FORGE_API_KEY at API startup#23

Open
SaltProphet wants to merge 1 commit intomainfrom
remove-api-key-fallback-and-validate-startup
Open

Require explicit FORGE_API_KEY at API startup#23
SaltProphet wants to merge 1 commit intomainfrom
remove-api-key-fallback-and-validate-startup

Conversation

@SaltProphet
Copy link
Owner

Motivation

  • Prevent accidental use of an insecure default API key and ensure the service is only usable when an operator explicitly configures credentials.
  • Fail fast on startup when API key configuration is missing so deployments do not expose an unprotected API.
  • Allow an explicit, opt-in local development mode only when FORGE_DEV_MODE is intentionally set.

Description

  • Removed the previous insecure fallback and changed API key handling so API_KEY is initialized to None and populated during startup validation in startup_event in api.py.
  • Implemented startup validation that reads FORGE_API_KEY, sets API_KEY when present, and raises a clear RuntimeError when the variable is missing or empty; when FORGE_DEV_MODE is explicitly enabled a non-production DEV_API_KEY is used with a printed warning.
  • Added a readiness guard in the auth dependency get_api_key that returns 503 while API_KEY is not configured to prevent accepting requests before startup succeeds.
  • Updated README.md and DEPLOYMENT.md with required FORGE_API_KEY setup instructions and a clear note about the explicit FORGE_DEV_MODE opt-in for local development; added new unit tests at tests/unit/test_api_startup_security.py covering the new behaviors.

Testing

  • Ran the focused unit tests with pytest -q -o addopts='' tests/unit/test_api_startup_security.py, which passed (4 passed).
  • An initial attempt to run the single test file with the repository default pytest configuration failed due to coverage-related pytest.ini addopts, so the targeted command above was used to run the new tests in isolation.

Codex Task

Copilot AI review requested due to automatic review settings February 21, 2026 21:46
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens security for the FastAPI service by removing the insecure default API key behavior and requiring operators to explicitly configure FORGE_API_KEY (with an opt-in dev-mode fallback) before the API is considered ready.

Changes:

  • Change API key initialization to None and enforce startup-time validation of FORGE_API_KEY (or explicit FORGE_DEV_MODE) in startup_event.
  • Add an auth readiness guard to return 503 until the API key is configured.
  • Add unit tests and update docs to reflect the new required environment variable behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
api.py Enforces explicit API key configuration at startup; blocks auth until configured.
tests/unit/test_api_startup_security.py Adds tests covering startup validation and readiness behavior.
README.md Documents required FORGE_API_KEY / optional FORGE_DEV_MODE configuration.
DEPLOYMENT.md Adds deployment guidance for required API environment variables.

Comment on lines 59 to +65
async def get_api_key(api_key: str = Security(api_key_header)):
"""Validate API key."""
if not API_KEY:
raise HTTPException(
status_code=503,
detail="API key is not configured. Service is not ready."
)
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

APIKeyHeader(..., auto_error=False) can yield None when the header is missing, but get_api_key currently types api_key as str and doesn’t explicitly handle the None case. Update the signature to accept Optional[str] (or APIKey | None) and return a clear 403/401 for missing credentials before comparing to API_KEY.

Copilot uses AI. Check for mistakes.
Comment on lines +7 to +8


Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

Other unit tests in tests/unit/ are consistently marked with @pytest.mark.unit (e.g., tests/unit/test_utils.py). Consider marking this module/tests as unit as well (e.g., module-level pytestmark = pytest.mark.unit) so marker-based test selection remains consistent.

Suggested change
pytestmark = pytest.mark.unit

Copilot uses AI. Check for mistakes.
Comment on lines +132 to +139
### API Security Configuration (Required for `api.py`)

When running the FastAPI service (`python api.py`), you **must** set `FORGE_API_KEY` explicitly.

```bash
export FORGE_API_KEY="replace-with-a-strong-random-key"
python api.py
```
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

This section documents how to configure the server-side API key, but it doesn’t say how clients must present it (header name / request format). Please mention that requests must include the X-API-Key header set to the same value as FORGE_API_KEY (and how dev mode affects that), otherwise users won’t know how to authenticate.

Copilot uses AI. Check for mistakes.
Comment on lines +78 to +85
### API Environment Variables (Required)

If you deploy or run the FastAPI service (`api.py`), configure the API key before startup:

```bash
export FORGE_API_KEY="replace-with-a-strong-random-key"
python api.py
```
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

Like the README change, this deployment section explains setting FORGE_API_KEY but doesn’t mention how clients authenticate. Add a note that callers must send X-API-Key: <FORGE_API_KEY> with API requests so operators can validate deployments end-to-end.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants