Harden Docker API server key guidance#677
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances security by blocking common placeholder API keys, such as 'your_api_key_here' and 'your-api-key-here', and updating the Docker user guide to promote generating high-entropy keys dynamically. It also introduces corresponding unit tests. The feedback suggests parametrizing the new API server test to cover both placeholder variations.
| def test_documented_placeholder_key_never_authenticates(self): | ||
| config = PlatformConfig(enabled=True, extra={"key": "your_api_key_here"}) | ||
| adapter = APIServerAdapter(config) | ||
| mock_request = MagicMock() | ||
| mock_request.headers = {"Authorization": "Bearer your_api_key_here"} | ||
| result = adapter._check_auth(mock_request) | ||
| assert result is not None | ||
| assert result.status == 401 |
There was a problem hiding this comment.
To ensure comprehensive test coverage for both newly added placeholder variations (your_api_key_here and your-api-key-here), consider parametrizing this test case.
| def test_documented_placeholder_key_never_authenticates(self): | |
| config = PlatformConfig(enabled=True, extra={"key": "your_api_key_here"}) | |
| adapter = APIServerAdapter(config) | |
| mock_request = MagicMock() | |
| mock_request.headers = {"Authorization": "Bearer your_api_key_here"} | |
| result = adapter._check_auth(mock_request) | |
| assert result is not None | |
| assert result.status == 401 | |
| @pytest.mark.parametrize("placeholder", ["your_api_key_here", "your-api-key-here"]) | |
| def test_documented_placeholder_key_never_authenticates(self, placeholder): | |
| config = PlatformConfig(enabled=True, extra={"key": placeholder}) | |
| adapter = APIServerAdapter(config) | |
| mock_request = MagicMock() | |
| mock_request.headers = {"Authorization": f"Bearer {placeholder}"} | |
| result = adapter._check_auth(mock_request) | |
| assert result is not None | |
| assert result.status == 401 |
🔎 Lint report:
|
| Rule | Count |
|---|---|
invalid-argument-type |
6 |
unresolved-import |
1 |
unsupported-operator |
1 |
First entries
run_agent.py:7311: [invalid-argument-type] invalid-argument-type: Argument to function `build_anthropic_client` is incorrect: Expected `str`, found `str | dict[Unknown | str, Unknown | str | dict[str, str]] | Any | ... omitted 3 union elements`
tests/hermes_cli/test_secret_validation.py:3: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
gateway/platforms/qqbot/adapter.py:1182: [unsupported-operator] unsupported-operator: Operator `+` is not supported between objects of type `str` and `(str & ~AlwaysFalsy) | (list[Unknown] & ~AlwaysFalsy) | (Any & ~AlwaysFalsy)`
gateway/platforms/qqbot/adapter.py:1212: [invalid-argument-type] invalid-argument-type: Argument to function `QQAdapter._detect_message_type` is incorrect: Expected `list[Unknown]`, found `list[Unknown] | str | Any`
gateway/platforms/qqbot/adapter.py:1196: [invalid-argument-type] invalid-argument-type: Argument to function `QQAdapter._merge_quote_into` is incorrect: Expected `str`, found `str | Unknown | (list[Unknown] & ~AlwaysFalsy)`
gateway/platforms/qqbot/adapter.py:1216: [invalid-argument-type] invalid-argument-type: Argument is incorrect: Expected `list[str]`, found `list[Unknown] | str | Any`
run_agent.py:13568: [invalid-argument-type] invalid-argument-type: Argument to function `len` is incorrect: Expected `Sized`, found `(str & ~AlwaysFalsy) | (dict[Unknown | str, Unknown | str | dict[str, str]] & ~AlwaysFalsy) | (Any & ~AlwaysFalsy) | ... omitted 3 union elements`
run_agent.py:13565: [invalid-argument-type] invalid-argument-type: Argument to function `_is_oauth_token` is incorrect: Expected `str`, found `str | dict[Unknown | str, Unknown | str | dict[str, str]] | Any | ... omitted 3 union elements`
✅ Fixed issues (4):
| Rule | Count |
|---|---|
invalid-argument-type |
3 |
unresolved-reference |
1 |
First entries
run_agent.py:7311: [invalid-argument-type] invalid-argument-type: Argument to function `build_anthropic_client` is incorrect: Expected `str`, found `str | dict[Unknown, Unknown] | Any | ... omitted 3 union elements`
gateway/run.py:5544: [unresolved-reference] unresolved-reference: Name `team_id` used when not defined
run_agent.py:13565: [invalid-argument-type] invalid-argument-type: Argument to function `_is_oauth_token` is incorrect: Expected `str`, found `str | dict[Unknown, Unknown] | Any | ... omitted 3 union elements`
run_agent.py:13568: [invalid-argument-type] invalid-argument-type: Argument to function `len` is incorrect: Expected `Sized`, found `(str & ~AlwaysFalsy) | (dict[Unknown, Unknown] & ~AlwaysFalsy) | (Any & ~AlwaysFalsy) | ... omitted 3 union elements`
Unchanged: 4355 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
There was a problem hiding this comment.
Pull request overview
This PR hardens the Docker deployment guidance for the OpenAI-compatible API server by removing insecure copy/paste defaults (public placeholder bearer token and wildcard CORS) and ensuring runtime placeholder-secret detection aligns with documented examples.
Changes:
- Update Docker docs to generate a high-entropy API key outside the
docker runcommand and to use a trusted-origin CORS allowlist instead of'*'. - Extend
has_usable_secret(...)placeholder denylist to includeyour_api_key_here/your-api-key-hereso these values are never treated as valid secrets. - Add targeted unit/regression tests to ensure documented placeholders never authenticate and that a sufficiently long secret is accepted.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
website/docs/user-guide/docker.md |
Removes insecure placeholder token and wildcard CORS; documents generating a strong key and restricting CORS. |
hermes_cli/auth.py |
Adds documented placeholder strings to _PLACEHOLDER_SECRET_VALUES so has_usable_secret rejects them. |
tests/hermes_cli/test_secret_validation.py |
New unit tests asserting placeholder rejection (incl. ${...}) and acceptance of a long secret. |
tests/gateway/test_api_server.py |
Adds an auth regression test ensuring the documented placeholder key never authenticates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot, fix all failing checks. |
Auto-merge: checks failingThe following checks did not pass:
Please fix the failing checks before this PR can be merged. |
…tus normalization
…ver-security-issue' into badmade/create-patch-for-api-server-security-issue
Fixed all failing checks in commit
|
Auto-merge: no CI detectedNo CI check runs were found for commit |
Motivation
Description
website/docs/user-guide/docker.mdto remove the literalyour_api_key_hereexample and wildcard CORS, recommend generating a key withopenssl rand -hex 32, use an exported env var (e.g.HERMES_API_SERVER_KEY), and show a trusted-originAPI_SERVER_CORS_ORIGINSvalue.your_api_key_hereandyour-api-key-hereto_PLACEHOLDER_SECRET_VALUESinhermes_cli/auth.pysohas_usable_secret(...)treats those documented placeholders as unusable.tests/hermes_cli/test_secret_validation.pyto assert predictable placeholders (including${API_SERVER_KEY}) are rejected and that a generated-length secret is accepted.tests/gateway/test_api_server.py(TestAuth::test_documented_placeholder_key_never_authenticates) to ensureAPIServerAdapter._check_authdoes not authenticate the documented placeholder.Testing
scripts/run_tests.sh tests/hermes_cli/test_secret_validation.py tests/gateway/test_api_server.py::TestAuth, but the wrapper failed to installpytest-splitfrom PyPI due to a network/tunnel error, so the automated test run was blocked.python -m pytestin the local venv, butpytestis not installed in the environment so that run could not be executed.python -m py_compileonhermes_cli/auth.py,gateway/platforms/api_server.py, and the new/modified tests, which succeeded.rgandgit diffto confirm the insecure example and wildcard CORS were removed and placeholder values are added to the denylist.Codex Task