fix: move hardcoded Discord webhook URL to env var #83
No reviewers
Labels
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: cal/major-domo-database#83
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "fix/remove-hardcoded-webhook"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
dependencies.pywithDISCORD_WEBHOOK_URLenvironment variableFalseif the env var is unsetTest plan
DISCORD_WEBHOOK_URLset (should log warning, not crash)DISCORD_WEBHOOK_URLset🤖 Generated with Claude Code
AI Code Review
Files Reviewed
app/dependencies.py(modified)Findings
Correctness
osis already imported at line 5, so no new import is needed. TheDISCORD_WEBHOOK_URLmodule-level constant follows the same pattern already used forREDIS_HOST,REDIS_PORT,REDIS_DB, andCACHE_ENABLED— consistent with the file's existing conventions.if not webhook_url) is clean and correct. The function returnsFalseon missing config rather than crashing, which matches the existing error-handling pattern in the same function.Security
mainis the right call. The PR body notes the token must be rotated — this is mandatory. The token exists in git history and is fully compromised regardless of when this merges.Style & Conventions
REDIS_HOST,REDIS_PORT,REDIS_DB,CACHE_ENABLED) are grouped together near lines 26–29 at the top of the file.DISCORD_WEBHOOK_URLis declared mid-file just beforesend_webhook_message. Minor, but worth tidying.Deployment Gap —
docker-compose.ymlnot updated (MUST FIX)DISCORD_WEBHOOK_URLis not wired into theapiserviceenvironmentblock indocker-compose.yml(lines 21–36). Every other env var consumed by the API is listed there and pulled from.env. Without this, the variable will never reach the container in either the dev or prod compose stacks, and callers will always silently hit the warning path.Additionally,
.envdoes not include aDISCORD_WEBHOOK_URL=placeholder, so there is no documentation that the variable exists.Required before merging:
- DISCORD_WEBHOOK_URL=${DISCORD_WEBHOOK_URL}to theapienvironmentblock indocker-compose.yml.DISCORD_WEBHOOK_URL=(empty placeholder) to.envso the variable is discoverable.Without these two additions, the webhook silently stops working in all environments and the breakage only surfaces via log inspection.
Suggestions
DISCORD_WEBHOOK_URL = os.environ.get("DISCORD_WEBHOOK_URL")up to the env var block near line 26 alongside the other Redis/cache constants, for consistency.Verdict: REQUEST_CHANGES (posted as COMMENT — Gitea blocks self-review)
The core logic and security remediation are correct. The blocking issue is that
docker-compose.ymland.envwere not updated to carry the new variable, meaning no deployed container will receive it. Fix those two files and this is ready to merge.Automated review by Claude PR Reviewer
@ -503,6 +503,9 @@ def update_season_pitching_stats(player_ids, season, db_connection):raiseDISCORD_WEBHOOK_URL = os.environ.get("DISCORD_WEBHOOK_URL")All other env-var module-level constants (
REDIS_HOST,REDIS_PORT,REDIS_DB,CACHE_ENABLED) are grouped together near the top of the file around lines 26–29. Declaring this one mid-file is inconsistent with that pattern. Consider moving it there.@ -514,3 +517,3 @@bool: True if successful, False otherwise"""webhook_url = "https://discord.com/api/webhooks/1408811717424840876/7RXG_D5IqovA3Jwa9YOobUjVcVMuLc6cQyezABcWuXaHo5Fvz1en10M7J43o3OJ3bzGW"webhook_url = DISCORD_WEBHOOK_URLDISCORD_WEBHOOK_URLis not included in theapiserviceenvironmentblock indocker-compose.yml. Without- DISCORD_WEBHOOK_URL=${DISCORD_WEBHOOK_URL}in that file, thisos.environ.getwill always returnNonein every deployed environment (dev and prod) and the webhook will silently do nothing.AI Code Review
Files Reviewed
app/dependencies.py(modified)Findings
Correctness
os.environ.get("DISCORD_WEBHOOK_URL")at module level, then references that constant insend_webhook_message.if not webhook_url:guard handles bothNone(env var unset) and""(empty string), which is correct — both cases should short-circuit with a warning.osis already imported at line 5.loggeris already defined at module level (line 15). No new imports needed.update_season_pitching_stats, beforesend_webhook_message) is consistent with the existing pattern ofREDIS_HOST,REDIS_PORT,REDIS_DB, andCACHE_ENABLEDat lines 26–29.boolare unaffected.Security
7RXG_D5IqovA3Jwa9YOobUjVcVMuLc6cQyezABcWuXaHo5Fvz1en10M7J43o3OJ3bzGW) remains in git history. The PR body correctly calls out that the webhook should be rotated in Discord — that rotation is necessary regardless of this merge.DISCORD_WEBHOOK_URLmust be added todocker-compose.prod.ymland the dev server env for the function to work post-merge. This is an operational step, not a code issue.Style & Conventions
Suggestions
Verdict: APPROVED
This is a correct, well-scoped security fix. The hardcoded webhook token is removed, the graceful degradation (log warning + return False) is appropriate, and the implementation follows the file's established env var pattern. Rotate the webhook token in Discord after merging.
Automated review by Claude PR Reviewer
AI Code Review
Files Reviewed
app/dependencies.py(modified)docker-compose.yml(modified).env(modified)Findings
Correctness
send_webhook_message. The module-level constantDISCORD_WEBHOOK_URLis correctly read from the environment at import time, consistent with the pattern used by all other config constants in this file (REDIS_HOST,REDIS_PORT,API_TOKEN, etc.).if not webhook_url) correctly handles the unset case without crashing, returningFalseand logging a warning — matching the function's documented return contract.Security
.envplaceholder uses an empty value (DISCORD_WEBHOOK_URL=) which is the correct pattern for a development template: it documents the variable without supplying a real secret.Style & Conventions
DISCORD_WEBHOOK_URLis placed at the top of the module's configuration constants block (line 26), above Redis config — correct position as requested in the prior review.# Discord integrationsection comment is consistent with the# Redis configurationcomment style already present.—) consistent with other warning messages in the codebase.Suggestions
Verdict: APPROVED
All three items requested in the prior review have been addressed: the env var constant was moved to the top of the file,
docker-compose.ymlnow passesDISCORD_WEBHOOK_URLto the container, and.envhas the placeholder entry. The code is clean, follows existing conventions, and handles the degraded (unset) case gracefully. Ready to merge — remember to rotate the compromised webhook token before deploying.Automated review by Claude PR Reviewer
AI Code Review
Files Reviewed
app/dependencies.py(modified)docker-compose.yml(modified).env(modified)Findings
Correctness
DISCORD_WEBHOOK_URL = os.environ.get("DISCORD_WEBHOOK_URL")correctly follows the existing REDIS_HOST/PORT/DB pattern — evaluated at import time, consistent with all other env-var constants in this file.webhook_url = DISCORD_WEBHOOK_URL+if not webhook_url:guard handles bothNone(unset) and""(set to empty string), both falsy — correct graceful degradation.docker-compose.ymlupdated to pass${DISCORD_WEBHOOK_URL}from host into container — complete..envupdated with empty placeholder under# Integrationscomment — good documentation for developers setting up locally.Security
.envcontainsAPI_TOKEN=Tp3aO3jhYve5NJF1IqOmJTmkwith an actual value — out of scope for this PR.Style & Conventions
REDIS_HOST,REDIS_PORT, etc. constants. No issues.# Discord integrationcomment consistent with# Redis configurationstyle.Suggestions
DISCORD_WEBHOOK_URLmust be added todocker-compose.prod.ymlon the prod server (ssh akamai) for the webhook to function post-deploy. This is expected since prod secrets aren't in the repo.Verdict: APPROVED
Clean, complete fix. The implementation correctly removes the hardcoded credential and follows the existing env-var pattern. Graceful degradation (warn + return False) is the right behavior. All three changed files are consistent with each other.
Automated review by Claude PR Reviewer
All review feedback addressed. Approved.
Checkout
From your project repository, check out a new branch and test the changes.