fix(data-warehouse): gate url_pattern writes on node-role tables - #81598
fix(data-warehouse): gate url_pattern writes on node-role tables#81598Piccirello wants to merge 2 commits into
Conversation
A table with no credential is read by ClickHouse under the cluster's own S3 role, so its url_pattern is only safe to trust because PostHog chose it. That invariant was enforced only in the DRF serializer (#81559), so any other writer of DataWarehouseTable - a future endpoint, an admin action, a management command - could reintroduce the same class of bug without anyone noticing. - DataWarehouseTable.save() now refuses to change url_pattern on a credential-less table unless the caller passes internally_computed_url_pattern=True, checked against the row's prior state so a same-value re-save is never blocked. - The pipeline sync activity, saved-query materialization, the direct- connection upsert helpers, and the legacy file-upload action all compute url_pattern themselves rather than taking it from request input, so each now declares that trust explicitly. - Every other caller is refused by default instead of relying on a check living only in the API layer.
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 0 should fix, 2 consider. Published 2 findings (view the review). |
|
Reviews (1): Last reviewed commit: "fix(data-warehouse): gate url_pattern wr..." | Re-trigger Greptile |
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/engineering_analytics/backend/management/commands/seed_engineering_analytics.py |
0.0% | 1152 |
products/warehouse_sources/backend/models/table.py |
90.5% | 362, 1035 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 31555939012 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
batch_exports |
████████░░░░░░░░░░░░ 39.5% |
8,852 / 22,431 |
warehouse_sources_queue |
█████████░░░░░░░░░░░ 45.9% |
90 / 196 |
demo |
███████████░░░░░░░░░ 56.3% |
1,497 / 2,661 |
data_tools |
█████████████░░░░░░░ 66.2% |
90 / 136 |
tasks |
██████████████░░░░░░ 70.5% |
37,026 / 52,507 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
signals |
█████████████████░░░ 82.6% |
27,456 / 33,231 |
apm |
█████████████████░░░ 83.8% |
1,220 / 1,455 |
cdp |
█████████████████░░░ 84.2% |
3,914 / 4,649 |
data_modeling |
█████████████████░░░ 86.5% |
8,459 / 9,782 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
wizard |
█████████████████░░░ 86.7% |
1,090 / 1,257 |
notebooks |
█████████████████░░░ 86.8% |
8,222 / 9,471 |
cohorts |
██████████████████░░ 87.6% |
6,482 / 7,400 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
managed_warehouse |
██████████████████░░ 88.1% |
6,057 / 6,876 |
data_warehouse |
██████████████████░░ 88.6% |
11,762 / 13,277 |
business_knowledge |
██████████████████░░ 89.0% |
4,384 / 4,928 |
exports |
██████████████████░░ 89.2% |
7,528 / 8,444 |
engineering_analytics |
██████████████████░░ 89.3% |
6,759 / 7,570 |
visual_review |
██████████████████░░ 89.5% |
5,870 / 6,558 |
dashboards |
██████████████████░░ 89.5% |
6,195 / 6,920 |
alerts |
██████████████████░░ 90.3% |
4,507 / 4,992 |
mcp_analytics |
██████████████████░░ 90.4% |
3,968 / 4,388 |
conversations |
██████████████████░░ 90.5% |
18,390 / 20,315 |
links |
██████████████████░░ 90.6% |
183 / 202 |
streamlit_apps |
██████████████████░░ 90.7% |
2,630 / 2,901 |
error_tracking |
██████████████████░░ 91.1% |
11,214 / 12,304 |
stamphog |
██████████████████░░ 91.3% |
4,505 / 4,936 |
canvas |
██████████████████░░ 92.0% |
2,596 / 2,823 |
slack_app |
██████████████████░░ 92.4% |
11,379 / 12,313 |
managed_migrations |
███████████████████░ 92.6% |
1,556 / 1,681 |
early_access_features |
███████████████████░ 92.9% |
1,347 / 1,450 |
notifications |
███████████████████░ 92.9% |
1,060 / 1,141 |
ai_observability |
███████████████████░ 92.9% |
17,217 / 18,527 |
web_analytics |
███████████████████░ 92.9% |
16,337 / 17,578 |
mcp_store |
███████████████████░ 93.0% |
6,983 / 7,512 |
surveys |
███████████████████░ 93.2% |
5,919 / 6,349 |
posthog_ai |
███████████████████░ 93.2% |
1,326 / 1,422 |
marketing_analytics |
███████████████████░ 93.4% |
16,364 / 17,518 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
approvals |
███████████████████░ 93.5% |
3,491 / 3,734 |
product_analytics |
███████████████████░ 93.6% |
7,115 / 7,602 |
legal_documents |
███████████████████░ 93.8% |
1,628 / 1,736 |
workflows |
███████████████████░ 94.3% |
7,969 / 8,454 |
endpoints |
███████████████████░ 94.3% |
8,814 / 9,349 |
tracing |
███████████████████░ 94.4% |
2,678 / 2,837 |
review_hog |
███████████████████░ 94.6% |
8,246 / 8,715 |
skills |
███████████████████░ 94.7% |
3,862 / 4,077 |
replay_vision |
███████████████████░ 95.6% |
19,114 / 20,004 |
logs |
███████████████████░ 95.6% |
11,834 / 12,382 |
messaging |
███████████████████░ 95.6% |
3,424 / 3,581 |
experiments |
███████████████████░ 95.8% |
28,704 / 29,965 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
data_quality |
███████████████████░ 96.4% |
747 / 775 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
customer_analytics |
███████████████████░ 96.5% |
12,361 / 12,805 |
feature_flags |
███████████████████░ 96.5% |
18,301 / 18,958 |
growth |
███████████████████░ 96.7% |
5,992 / 6,198 |
access_control |
███████████████████░ 96.7% |
873 / 903 |
warehouse_sources |
████████████████████ 97.5% |
390,187 / 400,167 |
data_catalog |
████████████████████ 97.9% |
2,676 / 2,734 |
analytics_platform |
████████████████████ 98.1% |
2,254 / 2,298 |
metrics |
████████████████████ 98.2% |
2,491 / 2,536 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
live_debugger |
████████████████████ 99.2% |
613 / 618 |
field_notes |
████████████████████ 99.4% |
158 / 159 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 2 issues
Files (8)
products/data_warehouse/backend/direct_clickhouse.pyproducts/data_warehouse/backend/direct_mysql.pyproducts/data_warehouse/backend/direct_postgres.pyproducts/data_warehouse/backend/direct_redshift.pyproducts/data_warehouse/backend/direct_snowflake.pyproducts/data_warehouse/backend/presentation/views/table.pyproducts/warehouse_sources/backend/models/table.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_sync.py
Two gaps in the prior commit's writer audit: - DataWarehouseTable is registered in Django admin with url_pattern as an editable field. Admin validates via full_clean() (form.is_valid() -> clean()) before ModelAdmin.save_model() ever calls save(), and save_model doesn't translate a save()-raised ValidationError into a form error the way DRF's perform_update does - so a staff edit that trips the guard surfaced as an unhandled 500 instead of a normal field error. Added clean(), calling the same check, so admin's existing full_clean() step catches it. save() remains the enforcement of record for every other caller, since nothing but ModelForm calls full_clean(). - matrix.py's demo-table registration and seed_engineering_analytics's schema-table upsert both attach a real credential and rewrite url_pattern in the same save() call, but the guard reads the row's prior DB state rather than the value being assigned - so a pre-existing credential-less row (predating either of these credential- attaching code paths) would still trip it. Both now declare internally_computed_url_pattern=True, matching the other four writers: each computes url_pattern from team/table_name, never from request input.
|
✅ Security review complete — 1 finding posted as a review, in 10 min. Re-request a |
![posthog-security-review-bot[bot]](https://avatars.githubusercontent.com/in/4551195?s=60&v=4){kind=small}
| ``save(internally_computed_url_pattern=True)`` to say so. Every other caller, present or | ||
| future, is refused by default rather than trusted to have remembered a check elsewhere. | ||
| """ | ||
| if self._state.adding: |
There was a problem hiding this comment.
medium - tenant isolation (confidence 4/5)
Django admin creation bypasses the credential-less URL guard
DataWarehouseTable._reject_client_supplied_url_pattern_change returns for every new instance. DataWarehouseTableAdmin leaves url_pattern and team editable while making credential read-only, so a staff account with the model's add permission can submit POST /admin/warehouse_sources/datawarehousetable/add/ with team=<its project>, format=CSVWithNames, and url_pattern=https://s3.us-east-1.amazonaws.com/ph-warehouse/file_uploads/team_42/*.csv. Both the admin form's full_clean() and the subsequent save() take this early return and persist the attacker-selected URL with credential_id = NULL. hogql_definition then passes that URL to ClickHouse without an access key or secret, so reads use the cluster's S3 role—the exact cross-tenant condition this guard is intended to prevent.
Impact — a compromised or delegated staff account that can add warehouse tables can expose another tenant's PostHog-hosted files by creating the table in a project it can query.
Fix — enforce the credential-less invariant on inserts as well as updates. Require credential-less creation sites to use an explicit trusted factory/flag (and update the server-computed pipeline/upload call sites), or disable DataWarehouseTableAdmin add permission / require a credential in its add form; do not blanket-return solely because _state.adding is true.
Agent-driven security review. Confidence is 4/5 because Django is not installed in the review runtime, so the admin POST could not be executed; the registered admin, editable/read-only fields, validation path, persisted state, and ClickHouse sink were verified statically.
1 participant
Problem
A future write path to
DataWarehouseTablecould reopen the cross-tenant S3 read that #81559 closed, because the credential requirement lived only in the DRF serializer.A table with no credential is read by ClickHouse under the cluster's own S3 role instead of a team-supplied key, so its
url_patternis only safe because PostHog chose it.#81559enforces that on the REST update path. Nothing enforced it anywhere else: a new endpoint, an admin action, or a management command could seturl_patternon a credential-less table and nothing would catch it.Changes
DataWarehouseTable.save()refuses to changeurl_patternon a credential-less table unless the caller passesinternally_computed_url_pattern=True. The check reads the row's prior state, so re-saving the same value never raises, and a save scoped away fromurl_patternviaupdate_fieldsskips the read entirely.url_patternon credential-less rows as normal operation: the pipeline sync activity, saved-query materialization, the direct-connection upsert helpers (Snowflake/ClickHouse/Redshift/MySQL/Postgres), and the legacy file-upload action. Each computes the value itself from trusted state (a job's destination folder, a fixed sentinel, or the caller's ownteam_idplus a sanitized filename), never from a raw request field, so each now passes the flag explicitly.I considered scoping the guard by table kind (e.g. skip it for
external_data_source-backed rows) instead of an explicit flag, but two of the four legitimate writers - pipeline sync and saved-query materialization - operate on exactly the credential-less, non-external_data_sourcerows the guard needs to protect. Table kind doesn't correlate with trust here; only the caller knows whether the value it's writing came from its own computation or from a request body.How did you test this code?
Added tests in
products/warehouse_sources/backend/tests/test_table.pycovering: a credential-less table'surl_patternchange is rejected without the flag and accepted with it, a credentialed table is unaffected, creating a new credential-less table needs no flag, a same-value re-save doesn't raise, anupdate_fields-scoped save that excludesurl_patternskips the check, andsoft_delete()doesn't trip it.Ran the full existing suites for every internal writer this touches to check for regressions:
products/data_warehouse/backend/test/test_direct_query_engines.py,test_direct_virtual_tables.py,products/warehouse_sources/backend/temporal/data_imports/pipelines/test_pipeline_sync.py,products/data_warehouse/backend/logic/data_load/test/test_create_table.py,products/data_warehouse/backend/tests/api/test_table.py,test_warehouse_table_file_upload.py, andproducts/warehouse_sources/backend/tests/test_util.py(81559's own suite) - all pass alongside the new tests. Also ranuv run mypy --cache-fine-grained .repo-wide (clean).Not run:
hogli build:openapidoesn't apply here - this PR changes model/business-logic behavior, not any serializer field, endpoint signature, or response schema.Automatic notifications
Docs update
None - internal model invariant, no user-facing or documented behavior changes.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Tom asked for a more holistic follow-up to #81559 after I traced through the original vulnerability report and confirmed it as a true positive: instead of only the REST serializer enforcing "credential-less tables can't change their read location," push the same invariant down to the model so any future writer inherits it by default. Skills invoked:
/writing-code-comments,/writing-pr-descriptions.The first design I considered was a blanket
save()guard with no opt-in, but investigating the codebase found three production Temporal paths (pipeline sync, direct-connection upserts, saved-query materialization) that legitimately rewriteurl_patternon credential-less rows - a blanket guard would have broken them. The explicitinternally_computed_url_patternflag is the result of that finding: it lets trusted internal writers declare themselves explicitly instead of the model trying to infer trust from row shape.