Skip to content

Harden Python query runner sandbox#7757

Merged
zachliu merged 1 commit into
getredash:masterfrom
shojiiii:fix/python-query-runner-security
Jun 25, 2026
Merged

Harden Python query runner sandbox#7757
zachliu merged 1 commit into
getredash:masterfrom
shojiiii:fix/python-query-runner-security

Conversation

@shojiiii

@shojiiii shojiiii commented Jun 19, 2026 •
edited by cubic-dev-ai Bot
Loading

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • Bug Fix

Description

Hardens the Python query runner in two areas:

  • Replaces raw attribute access and mutation hooks with RestrictedPython safer_getattr, guarded_setattr, and full_write_guard policies. This prevents private attribute traversal and mutation of exposed objects.
  • Enforces organization and data source permissions for execute_query, get_source_schema, and get_query_result.
    • execute_query requires non-view-only access and passes the current user to the nested query runner.
    • get_source_schema permits view-only access.
    • get_query_result validates the query organization and access to both the query and returned result data sources.

The RestrictedPython changes target the currently pinned RestrictedPython 8.1.

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Added regression tests for private attribute access, protected writes, organization isolation, inaccessible data sources, view-only behavior, and current-user propagation.

Test result: 23 passed in tests/query_runner/test_python.py. Ruff and Black checks pass.

Related Tickets & Documents

Related to #7684, specifically the RestrictedPython sandbox escape report. This PR does not close the issue because the issue tracks additional findings.

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

N/A

Review in cubic

cubic-dev-ai[bot]
cubic-dev-ai Bot reviewed Jun 19, 2026
View reviewed changes

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Re-trigger cubic

zachliu
zachliu requested changes Jun 23, 2026
View reviewed changes

@zachliu zachliu left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR! The sandbox fix is solid and the API choices are correct for our pinned RestrictedPython 8.1 (safer_getattr, guarded_setattr, full_write_guard all exist there).

One request: could you split this into two PRs? It bundles two distinct fixes:

  1. Sandbox escape (getattr/setattr -> guarded, custom_write -> full_write_guard): ~5 lines, critical, low-risk
  2. Broken access control (static -> instance methods, org/data-source has_access checks, passing the real user to run_query): larger behavioral change.

These are different CWE classes with different risk profiles. Splitting lets the critical sandbox fix merge fast without waiting on review of the access-control changes, and keeps revert boundaries clean. The tests already separate cleanly along the same line.

Minor note on the second one: _get_data_source relies on self._current_user; if it's ever None, user.org_id raises AttributeError which gets swallowed into a misleading "Wrong data source name/id" message. Worth confirming that path.

@shojiiii shojiiii mentioned this pull request Jun 24, 2026
Open
5 tasks
@shojiiii shojiiii force-pushed the fix/python-query-runner-security branch from e232319 to e044757 Compare June 24, 2026 05:59
@shojiiii

shojiiii commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@zachliu
Thanks for the review. I split the changes as requested:

I also addressed the _current_user is None note in #7759 by adding an explicit current-user guard, so that path no longer falls through to a misleading data source/query lookup error.

@shojiiii shojiiii requested a review from zachliu June 24, 2026 06:00
@zachliu zachliu changed the title Harden Python query runner sandbox and permissions Harden Python query runner sandbox Jun 24, 2026
zachliu
zachliu approved these changes Jun 25, 2026
View reviewed changes

@zachliu zachliu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪨 solid!

@zachliu zachliu merged commit ae936e2 into getredash:master Jun 25, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cubic-dev-ai cubic-dev-ai[bot] cubic-dev-ai[bot] left review comments
@zachliu zachliu zachliu approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@shojiiii @zachliu