Skip to content

fix(security): remediate Snyk Code Python findings (command injection, open redirect, raw JSON) - #35

Open
devin-ai-integration[bot] wants to merge 1 commit into
masterfrom
devin/1788763709-snyk-python-sast
Open

devin-ai-integration[bot] wants to merge 1 commit into
masterfrom
devin/1788763709-snyk-python-sast

Conversation

@devin-ai-integration

Copy link
Copy Markdown

SUMMARY

Root-cause fixes for Snyk Code (SAST) findings in the Python codebase:

  • python/CommandInjectionscripts/cypress_run.py: the Cypress command was built as an f-string (spec path, browser from env, group id) and run via subprocess.Popen(cmd, shell=True). It is now an argv list passed without a shell (xvfb-run and its --server-args are split explicitly); the dry-run/RUN log output uses shlex.join.
  • python/OpenRedirectsuperset/views/core.py (explore redirect to datasource.default_endpoint) and superset/views/users/api.py (/api/v1/me/avatar redirect to Slack avatar URL). Added is_safe_redirect_target() in superset/utils/urls.py which allows only relative paths or absolute http(s) URLs, and rejects protocol-relative (//, \\) and non-web schemes (javascript:, data: …). Both redirect sites now gate on it.
  • python/XSSsuperset/views/api.py /api/v1/query/ returned a raw json.dumps string, which Flask serves as text/html. Wrapped in json_success() so it is returned as application/json.

Deliberately skipped (with rationale):

  • python/OpenRedirect in superset/views/redirect.py:73 — already validated by is_safe_redirect_url() + DANGEROUS_SCHEMES block; finding is a false positive.
  • python/XSS in superset/security/api.py, superset/async_events/api.py, superset/views/core.py (generate_json/json_error_response), superset/views/auth.py, superset/views/datasource/views.py — these already respond via self.response()/json_error_response()/json_success() (JSON application/json) or Jinja render_app_template, which autoescapes; no HTML reflection exists.
  • python/PT (path traversal) in superset/app.py:119,126send_static_file delegates to Flask's send_from_directory, which already normalizes and confines to the static folder (raises NotFound otherwise); the override only changes the 404→204 for hot-update files.
  • python/Jinja2AutoEscape in scripts/erd/erd.py, RELEASING/generate_email.py, superset/cli/… — these render non-HTML text templates (DOT/Markdown/plain-text email) from repo-controlled inputs; enabling HTML autoescape would corrupt output.
  • python/HardcodedPassword/HardcodedSecret — all reported locations are test fixtures, example configs (superset/config.py placeholder defaults) or docs; no real secrets.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

  • ruff check / ruff format --check on changed files: pass.
  • python scripts/cypress_run.py --parallelism-id 0 --dry-run prints the expected argv-based command.
  • New parametrized unit test test_is_safe_redirect_target in tests/unit_tests/utils/urls_tests.py; run pytest tests/unit_tests/utils/urls_tests.py.
  • curl -X POST /api/v1/query/ should now return Content-Type: application/json.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
  • Introduces new feature or API
  • Removes existing feature or API

Link to Devin session: https://app.devin.ai/sessions/6722f19ed59a48bda02a0275375426c9
Open in Devin Desktop: https://app.devin.ai/desktop/session/6722f19ed59a48bda02a0275375426c9?variant=devin
Requested by: @shayanshafii

…redirect, raw JSON response)

Co-Authored-By: shayan <shayan@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant