Harden Flask health endpoint registration#159
Conversation
Pigbibi
force-pushed
the
codex/harden-health-endpoint-registration-20260701
branch
from
d7bd10a to
40325da
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7bd10ad88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,31 @@ | |||
| from __future__ import annotations | |||
|
|
|||
| from flask import Flask | |||
There was a problem hiding this comment.
This new test module imports Flask at collection time, but the checked CI workflow installs only -e . numpy pandas pytest pytest-cov ruff before running PYTHONPATH=src python -m pytest ... tests (.github/workflows/ci.yml lines 27-28 and 76), and pyproject.toml still has an empty dependency list. In the normal CI environment this raises ModuleNotFoundError: No module named 'flask' before any tests run; either add Flask to the CI/test dependencies or guard/skip these tests when Flask is unavailable.
Useful? React with 👍 / 👎.
| return jsonify({"status": "ok", "timestamp": datetime.now(timezone.utc).isoformat()}) | ||
|
|
||
| existing_rules = {getattr(rule, "rule", "") for rule in self._app.url_map.iter_rules()} | ||
| if "/health" not in existing_rules: |
There was a problem hiding this comment.
This path-only check suppresses the GET health handler whenever any /health rule already exists, even if that rule only handles another method such as POST; in that app register_health_endpoint(app) still promises to add GET /health, but GET requests will return 405 because Flask allows separate rules for the same path with different methods. The same issue applies to the /healthz check below, so skip only when an existing rule actually handles GET/HEAD.
Useful? React with 👍 / 👎.
| if "/health" not in existing_rules: | ||
| self._app.add_url_rule( | ||
| "/health", | ||
| endpoint="qpk_health", |
There was a problem hiding this comment.
Using a fixed qpk_health endpoint still allows the same collision this change is trying to harden against: if the host Flask app already has any view registered with endpoint/function name qpk_health, or defines one after calling register_health_endpoint, Flask raises an endpoint-overwrite assertion even though /health itself may be free. Generate or probe for an unused endpoint name before registering the QPK health view.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation