Skip to content

Conversation

@fisjac
Copy link
Contributor

@fisjac fisjac commented Nov 6, 2024

SUMMARY

When accessing a chart as a guest user, the query field is attached to the payload revealing the table names of db schema. This fix introduces a check for whether or not the user is a guest user, and removes the query field from the data payload.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@github-actions github-actions bot added the api Related to the REST API label Nov 6, 2024
@fisjac fisjac marked this pull request as ready for review November 6, 2024 17:20
@codecov
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.92%. Comparing base (76d897e) to head (b720666).
⚠️ Report is 2419 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #30858       +/-   ##
===========================================
+ Coverage   60.48%   83.92%   +23.43%     
===========================================
  Files        1931      536     -1395     
  Lines       76236    38924    -37312     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32667    -13447     
+ Misses      28017     6257    -21760     
+ Partials     2105        0     -2105     
Flag Coverage Δ
hive 48.86% <0.00%> (-0.30%) ⬇️
javascript ?
mysql 76.75% <100.00%> (?)
postgres 76.88% <100.00%> (?)
presto 53.34% <40.00%> (-0.46%) ⬇️
python 83.92% <100.00%> (+20.41%) ⬆️
sqlite 76.34% <100.00%> (?)
unit 60.88% <0.00%> (+3.24%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fisjac fisjac requested a review from dpgaspar November 6, 2024 17:28
@fisjac fisjac changed the title fix(security): removing query from /chart/data payload when accessing as guest user fix(payload data): removing query from /chart/data payload when accessing as guest user Nov 6, 2024
@fisjac fisjac changed the title fix(payload data): removing query from /chart/data payload when accessing as guest user fix(chart data): removing query from /chart/data payload when accessing as guest user Nov 6, 2024
Copy link
Member

Choose a reason for hiding this comment

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

nit, I think you could do:

if security_manager.is_guest_user():
    for query in queries:
        query.pop("query", None)

Copy link
Member

Choose a reason for hiding this comment

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

Great minds think alike! 😉

@michael-s-molina michael-s-molina added the review:checkpoint Last PR reviewed during the daily review standup label Nov 6, 2024
@fisjac fisjac force-pushed the query-paylod-guest-permission branch from a042a54 to b720666 Compare November 7, 2024 17:07
@fisjac fisjac merged commit dd39138 into apache:master Nov 7, 2024
@michael-s-molina michael-s-molina removed the review:checkpoint Last PR reviewed during the daily review standup label Nov 7, 2024
nyohasstium pushed a commit to Webgains/superset that referenced this pull request Jan 2, 2025
@sadpandajoe sadpandajoe added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Mar 24, 2025
sadpandajoe pushed a commit that referenced this pull request Mar 24, 2025
@mistercrunch mistercrunch added 🍒 4.1.3 Cherry-picked to 4.1.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 5.0.0 First shipped in 5.0.0 labels Jul 29, 2025
sha174n pushed a commit to sha174n/superset that referenced this pull request Jan 2, 2026
Confirmed that the vulnerability is fixed and identified the specific versions
and Pull Requests that addressed different aspects of the issue:

- **Strict IDOR (Accessing unauthorized charts):**
  - **Fixed in Version:** 3.0.0
  - **PR:** apache#25081 ("fix: Allow embedded guest user datasource access with dashboard context")
  - **Mechanism:** `raise_for_access` now validates that the chart belongs to the dashboard.

- **Payload Tampering (Modifying metrics/columns):**
  - **Fixed in Version:** 4.0.0
  - **PR:** apache#27484 ("fix: check if guest user modified query")
  - **Mechanism:** Introduced `query_context_modified` check.

- **Information Disclosure (Schema Leaks):**
  - **Fixed in Version:** 5.0.0
  - **PR:** apache#30858 ("fix(chart data): removing query from /chart/data payload when accessing as guest user")

Verified by running existing integration tests in `tests/integration_tests/security/guest_token_security_tests.py`
and temporarily adding a test case to simulate payload tampering, which successfully triggered a security exception.
sha174n pushed a commit to sha174n/superset that referenced this pull request Jan 2, 2026
- Confirmed strict IDOR fix (accessing unauthorized charts) was in 3.0.0 (PR apache#25081).
- Confirmed payload tampering fix (modifying metrics) was in 4.0.0 (PR apache#27484).
- Confirmed schema leak fix was in 5.0.0 (PR apache#30858).
- Verified via integration tests.
sha174n pushed a commit to sha174n/superset that referenced this pull request Jan 2, 2026
This commit confirms the fix for the reported Guest Token IDOR vulnerability
and payload tampering issues.

Findings:
- **Strict IDOR (Accessing unauthorized charts):**
  - **Fixed in Version:** 3.0.0
  - **PR:** apache#25081
  - **Verification:** Verified with standalone PoC `poc_idor.py`. Attempting to
    access a chart belonging to Dashboard B while authorized for Dashboard A
    raises a security exception.

- **Payload Tampering (Modifying metrics/columns):**
  - **Fixed in Version:** 4.0.0
  - **PR:** apache#27484
  - **Verification:** Verified with standalone PoC `poc_idor.py`. Attempting to
    modify metrics for an authorized chart raises a security exception.

- **Schema Leak:**
  - **Fixed in Version:** 5.0.0
  - **PR:** apache#30858

Verification method:
- Created and executed a standalone Proof-of-Concept script (`poc_idor.py`)
  that bootstraps the Superset app context, creates temporary dashboards/charts,
  issues a guest token, and attempts the exploits. Both exploits were blocked.
- Ran existing integration tests in `tests/integration_tests/security/guest_token_security_tests.py`.

No code changes required as fixes are already present.
cyber-jessie added a commit to CybercentreCanada/superset that referenced this pull request Jan 8, 2026
* chore: bump base image in Dockerfile with `ARG PY_VER=3.11.11-slim-bookworm` (apache#32780)

* chore: Revert "chore: bump base image in Dockerfile with `ARG PY_VER=3.11.11-slim-bookworm`" (apache#32782)

* fix(chart data): removing query from /chart/data payload when accessing as guest user (apache#30858)

(cherry picked from commit dd39138)

* fix: upgrade to 3.11.11-slim-bookworm to address critical vulnerabilities (apache#32240)

(cherry picked from commit ad05732)

* fix(model/helper): represent RLS filter clause in proper textual SQL string (apache#32406)

Signed-off-by: hainenber <[email protected]>
(cherry picked from commit ff0529c)

* fix: Log table retention policy (apache#32572)

(cherry picked from commit 89b6d7f)

* fix(welcome): perf on distinct recent activities (apache#32608)

(cherry picked from commit 832e028)

* fix(log): Update recent_activity by event name (apache#32681)

(cherry picked from commit 449f51a)

* fix: Signature of Celery pruner jobs (apache#32699)

(cherry picked from commit df06bdf)

* fix(logging): missing path in event data (apache#32708)

(cherry picked from commit cd5a943)

* fix(fe/dashboard-list): display modifier info for `Last modified` data (apache#32035)

Signed-off-by: hainenber <[email protected]>
(cherry picked from commit 88cf2d5)

* fix: make packages PEP 625 compliant (apache#32866)

Co-authored-by: Michael S. Molina <[email protected]>
(cherry picked from commit 6e02d19)

* all cccs changes

* fix: Downgrade to marshmallow<4 (apache#33216)

* fix(log): store navigation path to get correct logging path (apache#32795)

(cherry picked from commit 4a70065)

* fix(pivot-table): Revert "fix(Pivot Table): Fix column width to respect currency config (apache#31414)" (apache#32968)

(cherry picked from commit a36e636)

* fix: improve error type on parse error (apache#33048)

(cherry picked from commit ed0cd5e)

* fix(plugin-chart-echarts): remove erroneous upper bound value (apache#32473)

(cherry picked from commit 5766c36)

* fix(pinot): revert join and subquery flags (apache#32382)

(cherry picked from commit 822d72c)

* fix: loading examples from raw.githubusercontent.com fails with 429 errors (apache#33354)

(cherry picked from commit f045a73)

* chore: creating 4.1.3rc1 change log and updating frontend json

(cherry picked from commit 72cf9b6)

* chore(🦾): bump python sqlglot 26.1.3 -> 26.11.1 (apache#32745)

Co-authored-by: GitHub Action <[email protected]>
(cherry picked from commit 66c1a6a)

* chore(🦾): bump python h11 0.14.0 -> 0.16.0 (apache#33339)

Co-authored-by: GitHub Action <[email protected]>
(cherry picked from commit 8252686)

* docs: CVEs fixed on 4.1.2 (apache#33435)

(cherry picked from commit 8a8fb49)

* feat(api): Added uuid to list api calls (apache#32414)

(cherry picked from commit 8decc9e)

* fix(table-chart): time shift is not working (apache#33425)

(cherry picked from commit dc44748)

* fix(Sqllab):  Autocomplete got stuck in UI when open it too fast (apache#33522)

(cherry picked from commit b4e2406)

* chore: update Dockerfile - Upgrade to 3.11.12 (apache#33612)

(cherry picked from commit f0b6e87)

* chore: updating 4.1.3rc2 change log

* Select all Drag and Drop (#546)

* add a select all button for the dnd select

* remove cypress

* chore(deps): bump cryptography from 43.0.3 to 44.0.1 (apache#32236)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
(cherry picked from commit fa09d81)

* fix: Adds missing __init__ file to commands/logs (apache#33059)

(cherry picked from commit c1159c5)

* fix: Saved queries list break if one query can't be parsed (apache#34289)

(cherry picked from commit 1e5a4e9)

* chore: Adds 4.1.4RC1 data to CHANGELOG.md and UPDATING.md

* tag bump for select all drag and drop

* Fix package-lock.json

* Add db migration, bump Docker image base

* gevent for gunicorn

* remove threads and make worker-connections configurable

* Fix package-lock.json

* tag bump for cccs build

* Remove CCCS Dataset Explorer (#550)

* tag bump for CCCS build

---------

Signed-off-by: hainenber <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: gpchandran <[email protected]>
Co-authored-by: Joe Li <[email protected]>
Co-authored-by: Jack <[email protected]>
Co-authored-by: Đỗ Trọng Hải <[email protected]>
Co-authored-by: Michael S. Molina <[email protected]>
Co-authored-by: JUST.in DO IT <[email protected]>
Co-authored-by: Michael S. Molina <[email protected]>
Co-authored-by: Andreas Motl <[email protected]>
Co-authored-by: Ville Brofeldt <[email protected]>
Co-authored-by: Yuri <[email protected]>
Co-authored-by: Maxime Beauchemin <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: GitHub Action <[email protected]>
Co-authored-by: sha174n <[email protected]>
Co-authored-by: Paul Rhodes <[email protected]>
Co-authored-by: Rafael Benitez <[email protected]>
Co-authored-by: cccs-RyanK <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: cyber-jessie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Related to the REST API 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch 🍒 4.1.3 Cherry-picked to 4.1.3 🍒 4.1.4 🚢 5.0.0 First shipped in 5.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants