fix: rename MT5_API_KEY to MT5API_SECRET_KEY#16
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59a7a43c4e
ℹ️ 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".
There was a problem hiding this comment.
Clean rename — the env var, constant, config function, auth module, tests, and all user-facing docs are consistent. Two small items worth addressing:
1. Stale API_KEY references in specs/041-mt5-rest-api/quickstart.md
Two references to the old bare API_KEY name were missed (these lines are outside the diff):
- Line 558:
"Ensure API_KEY environment variable is set"→ should beMT5API_SECRET_KEY - Line 591:
nssm set MT5-API AppEnvironmentExtra API_KEY=your-key-here→ should beMT5API_SECRET_KEY=your-key-here
2. Pre-existing: timing-attack vulnerable key comparison in auth.py
Not introduced by this PR, but since auth.py is being touched: line 64 uses != for API key comparison, which is susceptible to timing side-channel attacks (CWE-208). Consider switching to hmac.compare_digest() for constant-time comparison — it's a one-line fix:
import hmac
if not hmac.compare_digest(api_key_header_value, expected_key):This could be a follow-up PR to keep this one focused on the rename.
|
Addressed the review feedback in
|
Summary
MT5_API_KEYtoMT5API_SECRET_KEYMT5_API_KEYas a backward-compatible fallback during the rename so upgrades do not disable auth unexpectedlyhmac.compare_digest()and fix the remaining quickstart references to the old env var nameTesting
/Users/dceoy/util/mt5api/.agents/skills/local-qa/scripts/qa.shgit push -u origin bugfix/update-skill