fix: validate outbound URL at the OpenGraph fetch sink - #28
Merged
Conversation
…caller CodeQL flagged py/full-ssrf (critical) at _fetch_open_graph_metadata's urllib.request.Request(url, ...): the URL is user-controlled and the function had no validation of its own. The one existing caller does call _validate_reference_url() (which runs validate_outbound_url()) before reaching this function, but CodeQL's dataflow analysis doesn't credit that as a sanitizer for this sink, and relying solely on the caller is fragile against a future call site skipping it. Re-validate directly at the network call. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Iranman
added a commit
that referenced
this pull request
Jul 30, 2026
* fix: close non-app backend path flows and sanitize errors (SEC-002 wave 2) Independently traced and closed all 25 open CodeQL alerts in routes_submissions.py, routes_setup.py, routes_lidarr.py, and backend/transaction_engine.py (scope agreed with repo owner; app.py, index.html, and frontend files remain out of scope for a future wave). Genuine fixes: - routes_submissions.py _abs_resolved(): performed zero containment before Path.expanduser().resolve(); reachable from an authenticated GET query parameter (/api/submissions/target?path=...) with no other validation in the chain -- an arbitrary directory-read/audio-tag-parse primitive anywhere in the container filesystem. Fixed by requiring the resolved path fall under MUSIC_ROOT or DOWNLOADS_ROOT, reusing the existing _path_is_under()/_SUBMISSION_ALLOWED_ROOTS already present in the file (CodeQL #128). - routes_submissions.py: sanitized 3 genuinely broad `except Exception` handlers (submission_target, submission_reference_url's metadata fetch, validate_musicbrainz_release) that returned raw exception text; deliberate ValueError/KeyError paths with fixed safe messages are unchanged (CodeQL #323, #34, #36). - routes_setup.py: sanitized 6 broad-except response leaks (_write_env_file x2 call sites, AI/MusicBrainz/AcoustID/Plex connectivity tests) plus 2 unreported instances of the identical pattern found while reviewing the same functions (fpcalc check, dormant _check_path helper) (CodeQL #24, #25, #26, #27, #322). - routes_setup.py setup_test_ai(): "openai.com" in base_url matched any URL merely containing that substring; replaced with a real parsed- hostname check (CodeQL #102, py/incomplete-url-substring-sanitization). - routes_lidarr.py wanted_lidarr(): the true unbounded str(exc) source is app.py's _acq_fetch_lidarr_wanted() (out of scope this wave); fixed at this file's boundary instead, only echoing the known-safe "not configured" message (CodeQL #19). Proven false positives (14 total, dismissed on GitHub, each with an individual justification and a passing behavioral test -- see docs/TECHNICAL_DEBT.md SEC-002 for full per-alert detail): - #23, #28, #30, #31, #32, #33, #321: ValueError/KeyError raised only with fixed, deliberately-crafted, non-sensitive messages (env-var key names, numeric IDs the caller supplied, or OutboundPolicyError's own safe text), never a wrapped raw system exception. - #20, #21: routes_lidarr._http_error_message() already redacts every exception type except a RuntimeError carrying one of two fixed config-status strings from _lidarr_config_error(). - #129, #130, #131, #132, #133: TransactionStore._path() requires a txn_ prefix and rejects /, \, and NUL before any of its 5 flagged sink lines; it is the only path-construction site in the file. Also corrects docs/TECHNICAL_DEBT.md SEC-002's prior description of PR #52's #332/#334/#335 (previously described only as "fixed"; records that CodeQL's rescan still flagged them post-fix and they required a separate post-merge dismissal, with the reason each fix wasn't recognized by CodeQL's static model). * docs: update SEC-002 remediation history Corrects the PR #52 record (#332/#334/#335 required a post-merge dismissal, not just a code fix -- CodeQL's static model didn't recognize either mitigation) and records wave 2's full accounting: starting/in-scope counts, alert numbers, root-cause groups, fixes, dismissals, remaining 287-alert count (all in app.py/frontend), and the next recommended wave. * fix: check Lidarr config status directly instead of matching error text CodeQL flagged a new alert (#407) against the previous wanted_lidarr() fix: "not configured" in error.lower() distinguishes safe-to-echo messages from _acq_fetch_lidarr_wanted()'s own unbounded str(exc) by pattern-matching the exception TEXT, which isn't airtight -- a coincidentally-worded exception could slip through. Fixed by checking this file's own type-safe _lidarr_config_error() before ever calling _acq_fetch_lidarr_wanted(), removing the text-matching entirely.
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
1 participant
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
Fixes CodeQL alert #18 (
py/full-ssrf, critical): https://github.com/Iranman/beets-web-manager/security/code-scanning/18_fetch_open_graph_metadata(url)(routes_submissions.py) makes an outboundurllib.requestcall with a user-supplied URL and had no validation of its own. Its one call site (/api/submissions/reference-url) does validate first via_validate_reference_url()->validate_outbound_url(), so this wasn't currently exploitable, but CodeQL's static analysis doesn't credit a separate caller-side check as a sanitizer for this sink, and depending solely on every future caller remembering to validate first is fragile. Added a directvalidate_outbound_url(url)call at the top of the function, right before the network request it guards.Test plan
python -m py_compile app.py routes_submissions.py backend/security.pytests.test_submissions_page,tests.test_discogs_fallback,tests.test_outbound_security,tests.test_submission_folder_resolution(49 tests) passpython scripts/security_secret_scan.py,python scripts/validate_compose_security.pycleanunittest discoverrun