Security hardening, reliability fixes, and expanded test coverage - #22
Open
baptiste-bonnaudet wants to merge 8 commits into
Open
Security hardening, reliability fixes, and expanded test coverage#22baptiste-bonnaudet wants to merge 8 commits into
baptiste-bonnaudet wants to merge 8 commits into
Conversation
User-controlled values (city, dates, air quality variables) were interpolated into request URLs with f-strings and no encoding, letting a crafted input inject extra query parameters into Open-Meteo calls. - Pass all query values through httpx params= so they are URL-encoded - Validate city (non-empty string, max length) before any network call - Validate dates as ISO YYYY-MM-DD and enforce start_date <= end_date - Enforce a server-side allowlist for air quality hourly variables - Stop logging full request URLs with raw user input
- Drop the unsafe wildcard-origin + allow_credentials CORS combination; credentials are only allowed when MCP_ALLOWED_ORIGINS restricts origins - Replace wildcard allowed headers with an explicit list - Default --host to 127.0.0.1 instead of 0.0.0.0 (bandit B104); Docker images still pass --host 0.0.0.0 explicitly - Extract build_arg_parser() so CLI defaults are testable
- Add a .dockerignore so .git, tests, and local tooling never enter the build context or published image - Create an unprivileged appuser in both images and drop root before CMD - Add permissions: contents: read to the Docker and PyPI workflows so the default GITHUB_TOKEN is no longer write-scoped Note: Docker builds not verified locally (daemon unavailable); the RUN install line keeps its previous semantics.
Raises overall coverage from 89% to 95%: run_server modes (stdio, sse, streamable-http, unknown), CLI port resolution from PORT env vs --port, SSE route registration, get_weather_details include_forecast branch, and generic exception handling in tool handlers.
- Explicit httpx timeout (10s read, 5s connect) so a stalled Open-Meteo endpoint cannot hang a tool call indefinitely - Retry transient connection failures at the transport level (2 retries) - Convert malformed JSON bodies into a clear ValueError instead of an unhandled JSONDecodeError bubbling out of response.json() - Guard get_closest_utc_index against empty hourly time lists
A trailing Z means UTC, but convert_time stamped such timestamps with from_timezone, shifting the converted result by the source offset. Timestamps carrying an explicit offset now also keep it instead of being overwritten. Also replace deprecated datetime.utcnow() with datetime.now(timezone.utc) in get_timezone_info, and drop redundant traceback logging and unused imports in server.py.
There was a problem hiding this comment.
Pull request overview
This PR hardens the server against upstream query-parameter injection, improves HTTP robustness (timeouts/retries/malformed JSON handling), corrects timezone conversion semantics (Zulu/offset handling), and expands test coverage around security, reliability, runners, and error paths.
Changes:
- Replace f-string URL construction with
httpxparams=plus input validation/allowlists; add explicit HTTP timeouts and transport retries. - Fix timezone conversion for
...Ztimestamps and make UTC timestamps timezone-aware (avoid deprecatedutcnow()). - Add extensive regression/security/reliability tests; tighten CORS defaults and localhost-by-default bind for HTTP modes; harden Docker/CI permissions and containers.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_weather_tools.py | Updates httpx mocks to accept params/kwargs after switching to params= usage. |
| tests/test_time_conversion.py | Adds regression tests for Z-suffix and offset-aware timezone conversion behavior. |
| tests/test_server_security.py | Adds tests covering CORS hardening and localhost-by-default binding. |
| tests/test_reliability.py | Adds tests for timeouts/retries/malformed JSON handling and empty-hourly guards. |
| tests/test_performance.py | Updates httpx mocks to accept params/kwargs to match new client usage. |
| tests/test_integration.py | Updates integration mocks to route based on params rather than URL interpolation. |
| tests/test_input_validation.py | Adds validation and injection-prevention tests for city/date/allowlisted AQ vars. |
| tests/test_error_paths.py | Adds coverage for runner modes, CLI port resolution, and generic exception paths. |
| tests/conftest.py | Updates shared httpx mocks to accept params/kwargs. |
| src/mcp_weather_server/utils.py | Adds a guard and clearer error for empty hourly time lists. |
| src/mcp_weather_server/tools/weather_service.py | Introduces input validation, params= requests, timeouts/retries, and JSON parsing helper. |
| src/mcp_weather_server/tools/tools_time.py | Fixes Z-suffix handling and uses timezone-aware UTC timestamps. |
| src/mcp_weather_server/tools/air_quality_service.py | Adds allowlisted hourly variables, uses shared HTTP client/config, and params= requests. |
| src/mcp_weather_server/server.py | Hardens CORS configuration, adds env-based allowed origins, and defaults HTTP bind to localhost. |
| README.md | Updates documented default bind host to 127.0.0.1. |
| Dockerfile.streamable-http | Drops root privileges and explicitly binds HTTP to 0.0.0.0 for container use. |
| Dockerfile | Drops root privileges and changes default container command (now stdio). |
| .gitignore | Ignores .venv, coverage artifacts, and local Claude settings file. |
| .github/workflows/publish-to-pypi.yml | Restricts workflow token permissions to contents: read. |
| .github/workflows/publish-docker.yml | Restricts workflow token permissions to contents: read. |
| .dockerignore | Excludes dev/test/CI artifacts from Docker build context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+24
to
+28
| def test_timeout_is_configured(self): | ||
| client = make_http_client() | ||
| assert client.timeout == REQUEST_TIMEOUT | ||
| assert REQUEST_TIMEOUT.read == 10.0 | ||
| assert REQUEST_TIMEOUT.connect == 5.0 |
Comment on lines
+30
to
+33
| def test_transport_retries_configured(self): | ||
| assert TRANSPORT_RETRIES > 0 | ||
| transport = httpx.AsyncHTTPTransport(retries=TRANSPORT_RETRIES) | ||
| assert transport._pool._retries == TRANSPORT_RETRIES |
Comment on lines
+64
to
+66
| invalid_vars = [v for v in hourly_vars if v not in self.ALLOWED_HOURLY_VARS] | ||
| if invalid_vars: | ||
| raise ValueError(f"Invalid air quality variables: {', '.join(sorted(invalid_vars))}") |
Comment on lines
+38
to
+41
| try: | ||
| return response.json() | ||
| except ValueError: | ||
| raise ValueError(f"Invalid JSON response from {api_name}") |
Comment on lines
+16
to
17
| # Run the server in stdio mode | ||
| CMD ["uv", "run", "python", "-m", "mcp_weather_server", "--mode", "stdio"] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A security audit of the server surfaced several issues; this PR fixes them in small, focused commits, each with tests. It also improves HTTP reliability and fixes a timezone conversion bug. No behavior changes for well-formed inputs, and the tool schemas are untouched.
Security
5070f55): city names, dates, and air quality variables were interpolated into Open-Meteo URLs with f-strings and no encoding, so a crafted input likeParis&count=100could inject extra query parameters upstream. All values now go through httpxparams=and are validated first (non-empty city with a length cap, ISOYYYY-MM-DDdates withstart <= end, allowlisted pollutant variables).c89bbb2): dropped the invalid and unsafeallow_origins=["*"]+allow_credentials=Truecombination. Credentials are now only allowed when origins are restricted via the newMCP_ALLOWED_ORIGINSenv var, wildcard headers were replaced with an explicit list, and HTTP modes bind to127.0.0.1by default instead of0.0.0.0(bandit B104). Docker still exposes0.0.0.0explicitly, so container behavior is unchanged.607e465): both images now run as an unprivileged user, a.dockerignorekeeps.gitand tests out of the build context, and the Docker/PyPI workflows getpermissions: contents: readso the defaultGITHUB_TOKENis no longer write-scoped.Reliability
de90b9d): explicit timeouts (10s read, 5s connect), 2 transport-level connect retries, clearValueErrors for malformed JSON bodies, and a guard for empty hourly data.63a9c3a):convert_timetreated...Ztimestamps as source-timezone local time instead of UTC, shifting results by the source offset; timestamps with an explicit offset also keep it now. Also replaces deprecateddatetime.utcnow().Tests
57d04a0covers server runners, CLI port resolution, theinclude_forecastbranch, and generic exception paths; the rest ship with their fixes).Test plan
pytest: 309 passed, 2 skippedbandit -r src/: 0 findings (was 2 medium)pip-audit: no known vulnerabilities