From a00200d8af356ebe996457c1e940535d170dfd58 Mon Sep 17 00:00:00 2001 From: Thomas Piccirello Date: Tue, 11 Aug 2026 16:21:53 -0700 Subject: [PATCH 1/3] fix(data-warehouse): gate url_pattern writes on node-role tables 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. --- .../backend/direct_clickhouse.py | 5 +- .../data_warehouse/backend/direct_mysql.py | 5 +- .../data_warehouse/backend/direct_postgres.py | 5 +- .../data_warehouse/backend/direct_redshift.py | 5 +- .../backend/direct_snowflake.py | 5 +- .../backend/presentation/views/table.py | 5 +- .../warehouse_sources/backend/models/table.py | 36 +++++++- .../data_imports/pipelines/pipeline_sync.py | 14 ++- .../backend/tests/test_table.py | 91 +++++++++++++++++++ 9 files changed, 162 insertions(+), 9 deletions(-) diff --git a/products/data_warehouse/backend/direct_clickhouse.py b/products/data_warehouse/backend/direct_clickhouse.py index 1dfb95e074fe..f52168a83f56 100644 --- a/products/data_warehouse/backend/direct_clickhouse.py +++ b/products/data_warehouse/backend/direct_clickhouse.py @@ -60,6 +60,8 @@ def upsert_direct_clickhouse_table( existing_table.options = options existing_table.deleted = False existing_table.deleted_at = None + # DIRECT_CLICKHOUSE_URL_PATTERN is a fixed sentinel, not request input, so this upsert is a + # trusted writer of a credential-less table's URL. existing_table.save( update_fields=[ "name", @@ -70,7 +72,8 @@ def upsert_direct_clickhouse_table( "deleted", "deleted_at", "updated_at", - ] + ], + internally_computed_url_pattern=True, ) return existing_table diff --git a/products/data_warehouse/backend/direct_mysql.py b/products/data_warehouse/backend/direct_mysql.py index 2f724f7b93ea..1188e25add2d 100644 --- a/products/data_warehouse/backend/direct_mysql.py +++ b/products/data_warehouse/backend/direct_mysql.py @@ -62,6 +62,8 @@ def upsert_direct_mysql_table( existing_table.options = options existing_table.deleted = False existing_table.deleted_at = None + # DIRECT_MYSQL_URL_PATTERN is a fixed sentinel, not request input, so this upsert is a + # trusted writer of a credential-less table's URL. existing_table.save( update_fields=[ "name", @@ -72,7 +74,8 @@ def upsert_direct_mysql_table( "deleted", "deleted_at", "updated_at", - ] + ], + internally_computed_url_pattern=True, ) return existing_table diff --git a/products/data_warehouse/backend/direct_postgres.py b/products/data_warehouse/backend/direct_postgres.py index 7469a9716143..4d03f198d3d1 100644 --- a/products/data_warehouse/backend/direct_postgres.py +++ b/products/data_warehouse/backend/direct_postgres.py @@ -70,6 +70,8 @@ def upsert_direct_postgres_table( existing_table.options = options existing_table.deleted = False existing_table.deleted_at = None + # DIRECT_POSTGRES_URL_PATTERN is a fixed sentinel, not request input, so this upsert is a + # trusted writer of a credential-less table's URL. existing_table.save( update_fields=[ "name", @@ -80,7 +82,8 @@ def upsert_direct_postgres_table( "deleted", "deleted_at", "updated_at", - ] + ], + internally_computed_url_pattern=True, ) return existing_table diff --git a/products/data_warehouse/backend/direct_redshift.py b/products/data_warehouse/backend/direct_redshift.py index 0e6035f6898c..9c31af5d0431 100644 --- a/products/data_warehouse/backend/direct_redshift.py +++ b/products/data_warehouse/backend/direct_redshift.py @@ -71,6 +71,8 @@ def upsert_direct_redshift_table( existing_table.options = options existing_table.deleted = False existing_table.deleted_at = None + # DIRECT_REDSHIFT_URL_PATTERN is a fixed sentinel, not request input, so this upsert is a + # trusted writer of a credential-less table's URL. existing_table.save( update_fields=[ "name", @@ -81,7 +83,8 @@ def upsert_direct_redshift_table( "deleted", "deleted_at", "updated_at", - ] + ], + internally_computed_url_pattern=True, ) return existing_table diff --git a/products/data_warehouse/backend/direct_snowflake.py b/products/data_warehouse/backend/direct_snowflake.py index 56ec4a7bd63e..7efc3ced9fec 100644 --- a/products/data_warehouse/backend/direct_snowflake.py +++ b/products/data_warehouse/backend/direct_snowflake.py @@ -67,6 +67,8 @@ def upsert_direct_snowflake_table( existing_table.options = options existing_table.deleted = False existing_table.deleted_at = None + # DIRECT_SNOWFLAKE_URL_PATTERN is a fixed sentinel, not request input, so this upsert is a + # trusted writer of a credential-less table's URL. existing_table.save( update_fields=[ "name", @@ -77,7 +79,8 @@ def upsert_direct_snowflake_table( "deleted", "deleted_at", "updated_at", - ] + ], + internally_computed_url_pattern=True, ) return existing_table diff --git a/products/data_warehouse/backend/presentation/views/table.py b/products/data_warehouse/backend/presentation/views/table.py index 8f7df4cabb08..403f167dd3aa 100644 --- a/products/data_warehouse/backend/presentation/views/table.py +++ b/products/data_warehouse/backend/presentation/views/table.py @@ -803,7 +803,10 @@ def file(self, request: request.Request, *args: Any, **kwargs: Any) -> response. # Try to determine columns from the file table.columns = table.get_columns() - table.save() + # team_id comes from routing and safe_filename is sanitized (no path separators), so + # the URL above is always scoped to this team's own managed/ prefix, never taken + # verbatim from request input the way the PATCH endpoint's url_pattern field is. + table.save(internally_computed_url_pattern=True) # Validate columns in background from posthog.tasks.warehouse import validate_data_warehouse_table_columns diff --git a/products/warehouse_sources/backend/models/table.py b/products/warehouse_sources/backend/models/table.py index bb1f42ab1598..7123b7a9ada2 100644 --- a/products/warehouse_sources/backend/models/table.py +++ b/products/warehouse_sources/backend/models/table.py @@ -2,10 +2,12 @@ import sys import time import subprocess +from collections.abc import Iterable from io import StringIO from typing import TYPE_CHECKING, Any, NotRequired, Optional, TypedDict, cast from uuid import UUID +from django.core.exceptions import ValidationError from django.db import models from django.db.models import Q from django.utils import timezone @@ -325,6 +327,36 @@ class TableFormat(models.TextChoices): class Meta: db_table = "posthog_datawarehousetable" + def save(self, *args: Any, internally_computed_url_pattern: bool = False, **kwargs: Any) -> None: + if not internally_computed_url_pattern: + self._reject_client_supplied_url_pattern_change(kwargs.get("update_fields")) + super().save(*args, **kwargs) + + def _reject_client_supplied_url_pattern_change(self, update_fields: Iterable[str] | None) -> None: + """Block a url_pattern change on a table with no credential, unless the caller declares the + new value was computed by PostHog's own code rather than taken from request input. + + A table with no credential is read by ClickHouse under the cluster's own S3 role rather than + a key the team supplied, so its url_pattern is only safe because PostHog chose it. Pipeline + syncs, saved-query materialization, and the direct-connection upsert helpers all legitimately + rewrite this field on such tables using values they compute themselves; those call + ``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: + return + if update_fields is not None and "url_pattern" not in update_fields: + return + prior = type(self).raw_objects.filter(pk=self.pk).values_list("credential_id", "url_pattern").first() + if prior is None: + return + prior_credential_id, prior_url_pattern = prior + if prior_credential_id is None and prior_url_pattern != self.url_pattern: + raise ValidationError( + "This table has no credential, so its URL is only safe because PostHog set it. " + "Add an access key and secret before pointing it at a different location." + ) + @property def name_chain(self) -> list[str]: return self.name.split(".") @@ -988,4 +1020,6 @@ def acreate_datawarehousetable(**kwargs): @database_sync_to_async def asave_datawarehousetable(table: DataWarehouseTable) -> None: - table.save() + # Saved-query materialization is the only caller: it computes url_pattern itself from the + # backing DataWarehouseSavedQuery rather than taking it from request input. + table.save(internally_computed_url_pattern=True) diff --git a/products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_sync.py b/products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_sync.py index f827ca736d45..0686dc30563e 100644 --- a/products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_sync.py +++ b/products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_sync.py @@ -227,8 +227,13 @@ def _validate_and_update(): # get_count() above can retry against a degraded ClickHouse cluster for minutes, long # enough for the pooled Postgres connection to be recycled underneath us. Retry once # on a fresh connection rather than let this escape as error-tracking noise. + # new_url_pattern above is derived from the job's own destination folder, not from + # request input, so this sync is a trusted writer of a credential-less table's URL. retry_on_db_connection_drop( - lambda: table.save(update_fields=["format", "url_pattern", "queryable_folder", "row_count"]) + lambda: table.save( + update_fields=["format", "url_pattern", "queryable_folder", "row_count"], + internally_computed_url_pattern=True, + ) ) if not table_created: @@ -389,8 +394,13 @@ def _register(): # get_count() above can retry against a degraded ClickHouse cluster for minutes, long # enough for the pooled Postgres connection to be recycled underneath us. Retry once # on a fresh connection rather than let this escape as error-tracking noise. + # new_url_pattern above is derived from the job's own destination folder, not from + # request input, so this sync is a trusted writer of a credential-less table's URL. retry_on_db_connection_drop( - lambda: table.save(update_fields=["format", "url_pattern", "queryable_folder", "row_count"]) + lambda: table.save( + update_fields=["format", "url_pattern", "queryable_folder", "row_count"], + internally_computed_url_pattern=True, + ) ) else: logger.debug(f"Creating CDC companion table: {companion_table_name}") diff --git a/products/warehouse_sources/backend/tests/test_table.py b/products/warehouse_sources/backend/tests/test_table.py index e4ad10a18e87..7dc69eab95cc 100644 --- a/products/warehouse_sources/backend/tests/test_table.py +++ b/products/warehouse_sources/backend/tests/test_table.py @@ -5,6 +5,7 @@ from posthog.test.base import BaseTest from unittest.mock import patch +from django.core.exceptions import ValidationError from django.test import SimpleTestCase from clickhouse_driver.errors import ServerException @@ -16,6 +17,7 @@ from posthog.exceptions import ClickHouseAtCapacity +from products.warehouse_sources.backend.models.credential import DataWarehouseCredential from products.warehouse_sources.backend.models.external_data_source import ExternalDataSource from products.warehouse_sources.backend.models.table import ( DataWarehouseTable, @@ -226,3 +228,92 @@ def test_uuid_column_typing( assert type(field) is expected_type assert field.is_nullable() + + +class TestUrlPatternChangeGuard(BaseTest): + # A table with no credential is read by ClickHouse under the node's own S3 role, so its + # url_pattern is only safe to trust because PostHog computed it. This guards that invariant at + # the model layer so any writer (not just the REST API) is refused by default. + def _credential_less_table( + self, url_pattern: str = "https://posthog-owned.example/team_1/x.csv" + ) -> DataWarehouseTable: + table = DataWarehouseTable(name="t", format="CSVWithNames", team=self.team, url_pattern=url_pattern) + table.save(internally_computed_url_pattern=True) + return table + + def _credentialed_table(self, url_pattern: str = "https://customer-bucket.example/x.csv") -> DataWarehouseTable: + credential = DataWarehouseCredential.objects.create( + team=self.team, access_key="access_key", access_secret="access_secret" + ) + table = DataWarehouseTable( + name="t", format="CSVWithNames", team=self.team, url_pattern=url_pattern, credential=credential + ) + table.save() + return table + + def test_creating_a_credential_less_table_does_not_require_the_flag(self) -> None: + table = DataWarehouseTable( + name="t", format="CSVWithNames", team=self.team, url_pattern="https://x.example/a.csv" + ) + table.save() + + table.refresh_from_db() + assert table.url_pattern == "https://x.example/a.csv" + + def test_changing_url_pattern_without_the_flag_is_rejected(self) -> None: + table = self._credential_less_table() + + table.url_pattern = "https://posthog-owned.example/team_2/y.csv" + with pytest.raises(ValidationError, match="no credential"): + table.save() + + table.refresh_from_db() + assert table.url_pattern == "https://posthog-owned.example/team_1/x.csv" + + def test_changing_url_pattern_with_the_flag_is_allowed(self) -> None: + table = self._credential_less_table() + + table.url_pattern = "https://posthog-owned.example/team_1/y.csv" + table.save(internally_computed_url_pattern=True) + + table.refresh_from_db() + assert table.url_pattern == "https://posthog-owned.example/team_1/y.csv" + + def test_changing_url_pattern_on_a_credentialed_table_does_not_require_the_flag(self) -> None: + table = self._credentialed_table() + + table.url_pattern = "https://customer-bucket.example/renamed.csv" + table.save() + + table.refresh_from_db() + assert table.url_pattern == "https://customer-bucket.example/renamed.csv" + + def test_resaving_the_same_url_pattern_does_not_require_the_flag(self) -> None: + table = self._credential_less_table() + + table.columns = {"id": {"clickhouse": "String", "hogql": "StringDatabaseField", "valid": True}} + table.save() + + table.refresh_from_db() + assert table.columns == {"id": {"clickhouse": "String", "hogql": "StringDatabaseField", "valid": True}} + + def test_update_fields_scoped_save_skips_the_check_when_url_pattern_is_excluded(self) -> None: + # Mirrors ExternalDataSchema._sync_teardown_kind: a save scoped away from url_pattern via + # update_fields can't have changed it, so the extra DB read to compare prior state is skipped. + table = self._credential_less_table() + + table.url_pattern = "https://posthog-owned.example/team_2/y.csv" # not persisted below + table.columns = {"id": {"clickhouse": "String", "hogql": "StringDatabaseField", "valid": True}} + table.save(update_fields=["columns"]) + + table.refresh_from_db() + assert table.url_pattern == "https://posthog-owned.example/team_1/x.csv" + assert table.columns == {"id": {"clickhouse": "String", "hogql": "StringDatabaseField", "valid": True}} + + def test_soft_delete_on_a_credential_less_table_does_not_trip_the_guard(self) -> None: + table = self._credential_less_table() + + table.soft_delete() + + table.refresh_from_db() + assert table.deleted is True From d2d10655f3da98a35c490d04646080a9903ea80f Mon Sep 17 00:00:00 2001 From: Thomas Piccirello Date: Tue, 11 Aug 2026 19:08:27 -0700 Subject: [PATCH 2/3] fix(data-warehouse): surface the url_pattern guard through clean() too 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. --- .../backend/logic/products/hedgebox/matrix.py | 6 ++- .../commands/seed_engineering_analytics.py | 6 ++- .../warehouse_sources/backend/models/table.py | 10 +++++ .../backend/tests/test_table.py | 38 +++++++++++++++++++ 4 files changed, 58 insertions(+), 2 deletions(-) diff --git a/products/demo/backend/logic/products/hedgebox/matrix.py b/products/demo/backend/logic/products/hedgebox/matrix.py index 8aebc609a928..522aa70ed24b 100644 --- a/products/demo/backend/logic/products/hedgebox/matrix.py +++ b/products/demo/backend/logic/products/hedgebox/matrix.py @@ -2433,7 +2433,11 @@ def _register_demo_data_warehouse_table( existing_table.deleted_at = None if existing_table.created_by_id is None: existing_table.created_by = user - existing_table.save() + # url_pattern is computed above from source_team_id/table_name, not request input, and + # credential is a real value from get_or_create_datawarehouse_credential (never None) - + # but the guard reads the row's prior DB state, so a stale credential-less row from + # before this function existed would still trip it without this declared explicitly. + existing_table.save(internally_computed_url_pattern=True) return DataWarehouseTable.objects.create( diff --git a/products/engineering_analytics/backend/management/commands/seed_engineering_analytics.py b/products/engineering_analytics/backend/management/commands/seed_engineering_analytics.py index 6bb8ebc08b69..7741d2e20279 100644 --- a/products/engineering_analytics/backend/management/commands/seed_engineering_analytics.py +++ b/products/engineering_analytics/backend/management/commands/seed_engineering_analytics.py @@ -1145,7 +1145,11 @@ def _upsert_schema_table( existing.options = {**(existing.options or {}), "csv_allow_double_quotes": True} existing.deleted = False existing.deleted_at = None - existing.save() + # url_pattern is computed above from team/table_name, not request input, and credential + # is a real value from get_or_create_datawarehouse_credential (never None) - but the + # guard reads the row's prior DB state, so a stale credential-less row would still trip + # it without this declared explicitly. + existing.save(internally_computed_url_pattern=True) table = existing else: table = DataWarehouseTable.objects.create( diff --git a/products/warehouse_sources/backend/models/table.py b/products/warehouse_sources/backend/models/table.py index 7123b7a9ada2..4b1a20640894 100644 --- a/products/warehouse_sources/backend/models/table.py +++ b/products/warehouse_sources/backend/models/table.py @@ -332,6 +332,16 @@ def save(self, *args: Any, internally_computed_url_pattern: bool = False, **kwar self._reject_client_supplied_url_pattern_change(kwargs.get("update_fields")) super().save(*args, **kwargs) + def clean(self) -> None: + # Django admin's changeform validates via full_clean() (form.is_valid() -> clean()) before + # ModelAdmin.save_model() ever calls save() - and unlike DRF's perform_update, save_model + # doesn't translate a save()-raised ValidationError into a form error, so it would surface + # as an unhandled 500. Running the same check here lets admin catch it as a normal field + # error. save()'s check stays the enforcement of record for every other caller (DRF, a + # management command, a future endpoint), since nothing but ModelForm calls full_clean(). + super().clean() + self._reject_client_supplied_url_pattern_change(update_fields=None) + def _reject_client_supplied_url_pattern_change(self, update_fields: Iterable[str] | None) -> None: """Block a url_pattern change on a table with no credential, unless the caller declares the new value was computed by PostHog's own code rather than taken from request input. diff --git a/products/warehouse_sources/backend/tests/test_table.py b/products/warehouse_sources/backend/tests/test_table.py index 7dc69eab95cc..a26079cca944 100644 --- a/products/warehouse_sources/backend/tests/test_table.py +++ b/products/warehouse_sources/backend/tests/test_table.py @@ -310,6 +310,44 @@ def test_update_fields_scoped_save_skips_the_check_when_url_pattern_is_excluded( assert table.url_pattern == "https://posthog-owned.example/team_1/x.csv" assert table.columns == {"id": {"clickhouse": "String", "hogql": "StringDatabaseField", "valid": True}} + def test_attaching_a_credential_in_the_same_call_still_requires_the_flag(self) -> None: + # The guard reads the row's prior DB state, not the value being assigned in this call, so + # attaching a real credential here doesn't retroactively make the prior state trusted - + # writers computing url_pattern from something other than request input (like the demo and + # seed-data table registration) still have to declare that explicitly. + table = self._credential_less_table() + credential = DataWarehouseCredential.objects.create( + team=self.team, access_key="access_key", access_secret="access_secret" + ) + + table.credential = credential + table.url_pattern = "https://posthog-owned.example/team_1/y.csv" + with pytest.raises(ValidationError, match="no credential"): + table.save() + + table.refresh_from_db() + assert table.url_pattern == "https://posthog-owned.example/team_1/x.csv" + + def test_full_clean_rejects_the_same_way_save_does(self) -> None: + # Django admin validates via full_clean() (form.is_valid() -> clean()) before ever calling + # save(), and ModelAdmin.save_model() doesn't translate a save()-raised ValidationError into + # a form error - so the same check has to be reachable from clean() too, for admin to show a + # normal field error instead of an unhandled 500. + table = self._credential_less_table() + + table.url_pattern = "https://posthog-owned.example/team_2/y.csv" + with pytest.raises(ValidationError, match="no credential"): + table.full_clean() + + def test_clean_allows_a_credentialed_table_to_change_url_pattern(self) -> None: + # Calls clean() directly rather than full_clean(): other required fields (row_count, + # size_in_s3_mib) are legitimately blank on a freshly built table and full_clean() would + # reject those regardless, which isn't what this test is checking. + table = self._credentialed_table() + + table.url_pattern = "https://customer-bucket.example/renamed.csv" + table.clean() # must not raise + def test_soft_delete_on_a_credential_less_table_does_not_trip_the_guard(self) -> None: table = self._credential_less_table() From 1db1c2f3a8c472474dafc753db8d466a01ec4775 Mon Sep 17 00:00:00 2001 From: Thomas Piccirello Date: Wed, 12 Aug 2026 09:18:37 -0700 Subject: [PATCH 3/3] fix(data-warehouse): disable Django admin add for DataWarehouseTable DataWarehouseTableAdmin leaves url_pattern and team editable while credential is readonly, and Django admin's add flow builds a brand-new instance from raw form input then calls save() - _state.adding is True, so the credential-less-URL guard's "compare against prior DB state" check has nothing to compare against and can't reject it. A staff account (compromised, delegated, or just unfamiliar with the invariant) could submit the add form directly with an attacker-chosen url_pattern and no credential, creating exactly the credential-less, node-role-readable table the rest of this PR series exists to prevent. Verified end to end: reverting the fix locally and submitting a fully valid add-form payload gets a 302 (row created), not a validation error. save()'s "compare against prior state" approach is fundamentally not extendable to creation - a new row has no prior row to diff against, so distinguishing a trusted create (pipeline sync, upload) from an untrusted one would need every legitimate creation site converted from Manager.create() to a two-step construct-then-save(flag=True), which none of the five direct-connection/pipeline call sites currently do. Disabling add on this specific admin surface is the narrower fix: DataWarehouseTable rows are meant to come from the product's own controlled creation paths (upload, pipeline sync, direct-connection upsert), never from staff hand-entering one in a raw admin form. --- .../admin/data_warehouse_table_admin.py | 10 ++++ .../tests/test_data_warehouse_table_admin.py | 57 +++++++++++++++++++ 2 files changed, 67 insertions(+) create mode 100644 products/warehouse_sources/backend/tests/test_data_warehouse_table_admin.py diff --git a/products/warehouse_sources/backend/admin/data_warehouse_table_admin.py b/products/warehouse_sources/backend/admin/data_warehouse_table_admin.py index 1f34b6e0e035..ebc0b433e47b 100644 --- a/products/warehouse_sources/backend/admin/data_warehouse_table_admin.py +++ b/products/warehouse_sources/backend/admin/data_warehouse_table_admin.py @@ -1,4 +1,5 @@ from django.contrib import admin +from django.http import HttpRequest from django.urls import reverse from django.utils.html import format_html @@ -24,6 +25,15 @@ class DataWarehouseTableAdmin(admin.ModelAdmin): readonly_fields = ("credential", "external_data_source") ordering = ("-created_at",) + def has_add_permission(self, request: HttpRequest) -> bool: + # A table created here would have no credential (this form has no way to set one - see + # readonly_fields) and an unrestricted url_pattern, which is exactly the combination + # DataWarehouseTable.clean()/save() exist to refuse on every other write path. Those checks + # can't cover creation (a brand-new row has no prior state to compare against), so the + # invariant depends entirely on the creator computing url_pattern itself rather than taking + # it from form input - true for upload/pipeline sync, never true for a raw admin add form. + return False + @admin.display(description="Team") def team_link(self, obj: DataWarehouseTable): return format_html( diff --git a/products/warehouse_sources/backend/tests/test_data_warehouse_table_admin.py b/products/warehouse_sources/backend/tests/test_data_warehouse_table_admin.py new file mode 100644 index 000000000000..6d1dee44085f --- /dev/null +++ b/products/warehouse_sources/backend/tests/test_data_warehouse_table_admin.py @@ -0,0 +1,57 @@ +from posthog.test.base import BaseTest + +from django.contrib.admin import AdminSite +from django.test import RequestFactory +from django.urls import reverse + +from posthog.admin import register_all_admin + +from products.warehouse_sources.backend.admin.data_warehouse_table_admin import DataWarehouseTableAdmin +from products.warehouse_sources.backend.models.table import DataWarehouseTable + + +class TestDataWarehouseTableAdmin(BaseTest): + def setUp(self) -> None: + super().setUp() + # is_superuser is a read-only alias for is_staff in this codebase (no separate superuser + # concept), and Django's default auth backend grants every permission to a superuser. + self.user.is_staff = True + self.user.save() + self.admin = DataWarehouseTableAdmin(DataWarehouseTable, AdminSite()) + + def test_has_add_permission_is_false(self) -> None: + # This form has no way to set a credential (see readonly_fields), so any table it created + # would carry the same credential-less-plus-attacker-chosen-url_pattern combination + # DataWarehouseTable.clean()/save() exist to refuse - and those checks can't cover creation, + # since a brand-new row has no prior state to compare against. Blocking add here is the + # closest equivalent for a surface that isn't the product's own controlled creation paths. + request = RequestFactory().get("/") + request.user = self.user + + assert self.admin.has_add_permission(request) is False + + def test_add_view_rejects_a_credential_less_table_pointed_at_another_teams_data(self) -> None: + register_all_admin() + self.client.force_login(self.user) + add_url = reverse(f"admin:{DataWarehouseTable._meta.app_label}_{DataWarehouseTable._meta.model_name}_add") + + # row_count/size_in_s3_mib/columns are filled in so this is otherwise a fully valid + # submission - without has_add_permission, it 302-redirects to the changelist and creates + # the row (verified by temporarily reverting the fix locally), so the 403 below is coming + # from the permission check this test exists to pin, not from incidental form invalidity. + response = self.client.post( + add_url, + { + "team": self.team.pk, + "name": "attacker_table", + "format": "CSVWithNames", + "url_pattern": "https://s3.us-east-1.amazonaws.com/ph-warehouse/file_uploads/team_999/*.csv", + "options": "{}", + "row_count": "0", + "size_in_s3_mib": "0", + "columns": "{}", + }, + ) + + assert response.status_code == 403 + assert not DataWarehouseTable.objects.filter(name="attacker_table").exists()