fix(warehouse-sources): catch untriaged bucket settings in CI - #81610
fix(warehouse-sources): catch untriaged bucket settings in CI#81610Piccirello wants to merge 2 commits into
Conversation
The owned-bucket allowlist in _posthog_owned_bucket_names (added by #81559) hardcoded four settings inline. A new bucket setting could land without anyone deciding whether it needs the same treatment, which is exactly how the original four-bucket list came to be incomplete: a repo sweep for every Django setting ending in "_BUCKET" turned up 20, not 4. - Split the check into an explicit registry (_POSTHOG_OWNED_BUCKET_SETTING_NAMES) and an exclusion list (_BUCKET_SETTINGS_NOT_READABLE_BY_THE_NODE_ROLE, each entry with a stated reason), and add a test that enumerates every "*_BUCKET" setting Django actually has and fails if one isn't in either list. - Tracing each of the 20 to its actual reader (not just which product's settings module defines it) found four more that ClickHouse reads or writes directly via credential-less `s3(...)` / `BACKUP ... TO S3(...)` - the same shape the original vulnerability exploited: CLICKHOUSE_BACKUPS_BUCKET, IDENTITY_MATCHING_S3_BUCKET, OBJECT_STORAGE_EXTERNAL_WEB_ANALYTICS_BUCKET, and QUERY_LOG_ARCHIVE_EXPORT_S3_BUCKET. These move into the owned list; none of them were reachable through a table's url_pattern before #81559 landed the owned-bucket check in the first place, since that check didn't include them either - so this closes a gap in the fix's coverage, not a currently-open one. - The other sixteen are excluded with the specific access path checked (a Python process's own boto3/aioboto3 client, a different IAM identity than the ClickHouse node role) or a note that no reader was found in this codebase at all.
|
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 1 must fix, 0 should fix, 0 consider. Published 1 finding (view the review). |
Prompt To Fix All With AI### Issue 1
products/warehouse_sources/backend/tests/test_util.py:127
**Exclusion reasons remain unchecked**
The test converts the exclusion dictionary to a set of keys without validating its values, so an empty reason still passes CI and undermines the stated requirement that every exclusion document its access-path analysis.
```suggestion
assert all(reason.strip() for reason in _BUCKET_SETTINGS_NOT_READABLE_BY_THE_NODE_ROLE.values()), (
"Every bucket excluded from node-role validation must include a reason"
)
triaged = set(_POSTHOG_OWNED_BUCKET_SETTING_NAMES) | set(_BUCKET_SETTINGS_NOT_READABLE_BY_THE_NODE_ROLE)
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(warehouse-sources): catch untriaged ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # by hand. What this catches is the drift that made the original check incomplete: a new | ||
| # "*_BUCKET" setting landing without anyone deciding whether the node role can read it. | ||
| existing_bucket_settings = {name for name in dir(settings) if name.isupper() and name.endswith("_BUCKET")} | ||
| triaged = set(_POSTHOG_OWNED_BUCKET_SETTING_NAMES) | set(_BUCKET_SETTINGS_NOT_READABLE_BY_THE_NODE_ROLE) |
There was a problem hiding this comment.
The test converts the exclusion dictionary to a set of keys without validating its values, so an empty reason still passes CI and undermines the stated requirement that every exclusion document its access-path analysis.
| triaged = set(_POSTHOG_OWNED_BUCKET_SETTING_NAMES) | set(_BUCKET_SETTINGS_NOT_READABLE_BY_THE_NODE_ROLE) | |
| assert all(reason.strip() for reason in _BUCKET_SETTINGS_NOT_READABLE_BY_THE_NODE_ROLE.values()), ( | |
| "Every bucket excluded from node-role validation must include a reason" | |
| ) | |
| triaged = set(_POSTHOG_OWNED_BUCKET_SETTING_NAMES) | set(_BUCKET_SETTINGS_NOT_READABLE_BY_THE_NODE_ROLE) |
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/warehouse_sources/backend/tests/test_util.py
Line: 127
Comment:
**Exclusion reasons remain unchecked**
The test converts the exclusion dictionary to a set of keys without validating its values, so an empty reason still passes CI and undermines the stated requirement that every exclusion document its access-path analysis.
```suggestion
assert all(reason.strip() for reason in _BUCKET_SETTINGS_NOT_READABLE_BY_THE_NODE_ROLE.values()), (
"Every bucket excluded from node-role validation must include a reason"
)
triaged = set(_POSTHOG_OWNED_BUCKET_SETTING_NAMES) | set(_BUCKET_SETTINGS_NOT_READABLE_BY_THE_NODE_ROLE)
```
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
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}
BUCKET_URL (posthog/settings/data_warehouse.py) is a third, independently-configurable setting naming the warehouse bucket - an s3:// URI, not a bare name, and not derived from DATAWAREHOUSE_BUCKET or BUCKET_PATH. s3_proxy.warehouse_bucket_host derives from it specifically "so it always names the bucket the Delta tables actually live in", and every warehouse-pipeline write path (data modeling, ducklake, external-data-schema sync) builds its storage path from it directly. It's also the setting most warehouse Delta tables actually read through, since ClickHouse pulls those tables via a credential-less s3()/deltaLake() call the same way the original vulnerability did. Because it's suffixed "_URL" rather than "_BUCKET", it was invisible to both _posthog_owned_bucket_names and the drift-detection test the previous commit added. In a deployment where it names a different bucket than DATAWAREHOUSE_BUCKET/BUCKET_PATH (a divergence the s3_proxy.py comment explicitly anticipates), that bucket was reachable through a table's url_pattern. - Add BUCKET_URL to _POSTHOG_OWNED_BUCKET_SETTING_NAMES, alongside BUCKET_PATH as the two names the drift test can't discover on its own and has to be told about by hand. - _posthog_owned_bucket_names now parses an "s3://bucket" value via its netloc instead of treating it as a bare name. - Added a regression test overriding BUCKET_URL independently of DATAWAREHOUSE_BUCKET/BUCKET_PATH to a distinct bucket and confirming a url_pattern targeting it is rejected.
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/warehouse_sources/backend/models/util.py |
91.7% | 675 |
🤖 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 31553827367 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
demo |
███████████░░░░░░░░░ 56.3% |
1,497 / 2,661 |
tasks |
██████████████░░░░░░ 70.5% |
37,026 / 52,507 |
signals |
█████████████████░░░ 82.6% |
27,454 / 33,231 |
cdp |
█████████████████░░░ 84.2% |
3,914 / 4,649 |
data_modeling |
█████████████████░░░ 86.5% |
8,459 / 9,782 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
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 |
exports |
██████████████████░░ 89.2% |
7,528 / 8,444 |
engineering_analytics |
██████████████████░░ 89.3% |
6,759 / 7,570 |
dashboards |
██████████████████░░ 89.4% |
6,189 / 6,920 |
alerts |
██████████████████░░ 90.3% |
4,507 / 4,992 |
mcp_analytics |
██████████████████░░ 90.4% |
3,968 / 4,388 |
conversations |
██████████████████░░ 90.5% |
18,390 / 20,315 |
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 |
early_access_features |
███████████████████░ 92.9% |
1,347 / 1,450 |
ai_observability |
███████████████████░ 92.9% |
17,217 / 18,527 |
web_analytics |
███████████████████░ 92.9% |
16,337 / 17,578 |
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 |
workflows |
███████████████████░ 94.3% |
7,969 / 8,454 |
endpoints |
███████████████████░ 94.3% |
8,814 / 9,349 |
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 |
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 |
warehouse_sources |
████████████████████ 97.5% |
390,105 / 400,084 |
data_catalog |
████████████████████ 97.9% |
2,677 / 2,734 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
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.
|
The run stopped after 4 min and this PR was not reviewed. Treat it as unreviewed, not as clean. Reason: Re-request a |
|
✅ Security review complete — 3 findings posted as a review, in 15 min. Re-request a |
![posthog-security-review-bot[bot]](https://avatars.githubusercontent.com/in/4551195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Agent-driven security review - findings inline.
low - tenant isolation (confidence 3/5)
The path-style bucket comparison runs on the raw, non-percent-decoded URL path, so %70osthog-... / bucket%2Fkey forms of an owned bucket are not recognised
Not introduced by this diff, but it is the check the new registry feeds. In _validate_url_pattern_is_not_posthog_storage (products/warehouse_sources/backend/models/util.py:696-712) the path-style bucket is taken straight from urlparse(url_pattern).path, which Python leaves percent-encoded, and compared verbatim against _posthog_owned_bucket_names(). Exercising a copy of the function:
https://s3.us-east-1.amazonaws.com/%70osthog-warehouse/x.parquet→ bucket read as%70osthog-warehouse→ allowed.https://s3.us-east-1.amazonaws.com/posthog-warehouse%2Fteam_2_model_x/**.parquet→ bucket read asposthog-warehouse%2Fteam_2_model_x→ allowed.
ClickHouse parses an s3() URL with Poco::URI, whose getPath() returns the percent-decoded path before the bucket/key split, so both forms resolve to the real bucket on the ClickHouse side. Related: a ? lands in urlparse's query component, so the _GLOB_METACHARACTERS guard never sees it (https://s3.../posthog-object?/x.csv reaches the guard as posthog-object) — Poco splits on ? too, so that one is a dead guard rather than a bypass.
Impact — the same class of miss the registry work is trying to close: a url_pattern that a human reader would call "our warehouse bucket" passes validation. It needs the same credential-less caller-controlled url_pattern path as the findings above (blocked today in TableSerializer) to become a cross-tenant read.
Fix — normalize before comparing, in both the virtual-hosted and path-style branches:
from urllib.parse import unquote
bucket = unquote(path).lstrip("/").split("/", 1)[0].lower()
if "%" in path.split("/", 2)[1]: # or simply reject a bucket segment containing '%'
return False, _NOT_OUR_STORAGEAgent-driven security review. The Python side is reproduced; the ClickHouse/Poco side is upstream behaviour that cannot be executed here, and there is no reachable credential-less url_pattern path today.
| "AGENT_BUNDLES_S3_BUCKET": "no reader found in this codebase; defaults to OBJECT_STORAGE_BUCKET", | ||
| "AI_BLOB_S3_BUCKET": "read via posthog.storage.object_storage (boto3), by ai_observability/backend/api/ai_blob.py", | ||
| "BATCH_EXPORTS_FILE_DOWNLOAD_BUCKET": "read via aioboto3 by the Temporal batch-exports workflow", | ||
| "BATCH_EXPORT_INTERNAL_STAGING_BUCKET": "read via aioboto3 by the Temporal batch-exports workflow", |
There was a problem hiding this comment.
medium - tenant isolation (confidence 4/5)
BATCH_EXPORT_INTERNAL_STAGING_BUCKET is excluded from the owned-bucket registry, but ClickHouse writes every team's staged export data to it with a credential-less s3() call in cloud
_BUCKET_SETTINGS_NOT_READABLE_BY_THE_NODE_ROLE excludes BATCH_EXPORT_INTERNAL_STAGING_BUCKET with the reason "read via aioboto3 by the Temporal batch-exports workflow". That covers only the read side. The write side is ClickHouse itself, which is the discriminator this PR uses everywhere else:
_get_clickhouse_s3_staging_folder_urlbuildshttps://{settings.BATCH_EXPORT_INTERNAL_STAGING_BUCKET}.s3.{region}.amazonaws.com/...—products/batch_exports/backend/temporal/pipeline/internal_stage.py:545.- That URL goes into
get_s3_function_call, which emitss3('<url>', 'ArrowStream')with no access key when credentials areNone—products/batch_exports/backend/temporal/sql/common.py:22, called atinternal_stage.py:423. - Credentials are
Nonein cloud:_get_s3_credentialsreturnsNoneunless_uses_object_storage_endpoint(), which is_is_local_dev_or_test() or not settings.CLOUD_DEPLOYMENT—internal_stage.py:86-108.
So on every batch export the ClickHouse node role writes staged event/person rows for every team into that bucket, i.e. exactly the "read or written by ClickHouse's own s3(...) with no explicit access key" shape that put CLICKHOUSE_BACKUPS_BUCKET and QUERY_LOG_ARCHIVE_EXPORT_S3_BUCKET into the owned list in this same diff. By the PR's own criterion this setting belongs in _POSTHOG_OWNED_BUCKET_SETTING_NAMES, not in the exclusion list — and the new test_every_bucket_setting_is_either_owned_or_excluded_with_a_reason will now certify the mistake as "triaged" forever.
Impact — the owned-bucket list is the backstop that stops a warehouse table's url_pattern from resolving to a bucket the node role can reach. With this setting excluded, https://<staging-bucket>.s3.<region>.amazonaws.com/batch-exports/** passes validate_warehouse_table_url_pattern (verified against a copy of the changed function), and that prefix holds other tenants' raw exported events and persons. It is not a live cross-tenant read today: the create path refuses a table without an access key (products/data_warehouse/backend/presentation/views/table.py:209-213) and a credential-less table's url_pattern cannot be changed (table.py:246-255), so the node-role read needs one of those two controls to break first. This is a hole in the second of the two, on a bucket with fleet-wide event data in it.
Fix — move the entry into the owned list:
_POSTHOG_OWNED_BUCKET_SETTING_NAMES = (
...
"BATCH_EXPORT_INTERNAL_STAGING_BUCKET",
...
)(BATCH_EXPORTS_FILE_DOWNLOAD_BUCKET really is Python-only — export_to_file_download_bucket_with_temporary_credentials writes it with assumed-role credentials — so that exclusion holds.)
Agent-driven security review. Not verified from source: whether the ClickHouse node role's IAM policy grants read (not just write) on the staging bucket, and whether cloud points BATCH_EXPORT_INTERNAL_STAGING_BUCKET at a bucket distinct from OBJECT_STORAGE_BUCKET, which is already listed.
There was a problem hiding this comment.
Yeah, I was just going to say the same thing: In production we don't use credentials here. So, I think we should exclude all batch export buckets too.
There was a problem hiding this comment.
Ultimately, not blocking though, not sure what you are doing with this setting. Just wanted to say we do use IAM-based role auth in batch exports too.
|
|
||
| # Every Django setting naming a bucket the ClickHouse node role can read, so a table's | ||
| # url_pattern must never resolve to one - see _posthog_owned_bucket_names below. | ||
| # test_bucket_settings_are_all_triaged in test_util.py enumerates every "*_BUCKET" setting that |
There was a problem hiding this comment.
low - tenant isolation (confidence 3/5)
The CI drift check only covers buckets named by a Django setting, so ClickHouse-readable buckets whose names come from the database (the duckling export targets) stay invisible to the registry
The comment says a newly added bucket "can't silently miss this check", qualified only by the *_BUCKET naming convention. There is a second, larger blind spot: _posthog_owned_bucket_names() can only enumerate Django settings, and not every bucket ClickHouse touches keylessly is named by one.
posthog/dags/events_backfill_to_duckling.py exports raw events and persons with INSERT INTO FUNCTION s3('https://{bucket}.s3.{region}.amazonaws.com/...', 'Parquet') and no credentials — get_s3_url_for_clickhouse at :947-953, the export SQL at :1561-1566 ("ClickHouse uses its EC2 instance role - no credentials needed / The duckling bucket policy allows the ClickHouse EC2 role"). The bucket name comes from the control plane or the stored DuckgresServer.bucket row (:255-299), never from settings, and the objects are Parquet — a format DataWarehouseTable.TableFormat supports directly.
Impact — a per-org managed-warehouse bucket that the node role can read is unreachable by this registry by construction, so no amount of setting-sweeping in CI will cover it; the guarantee the comment and the test advertise is narrower than it reads. Exploiting it needs the same credential-less caller-controlled url_pattern path that is currently blocked in TableSerializer (products/data_warehouse/backend/presentation/views/table.py:209-255), plus knowledge of the target org's bucket name.
Fix — say so in the comment (the registry covers settings-named buckets only), and treat the requirement that a caller-supplied url_pattern always carry a credential as the load-bearing control rather than the denylist — a positive rule covers DB-named buckets that no allowlist of settings can.
Agent-driven security review. Reachability depends on a credential-less table with a caller-controlled url_pattern, which no current API path allows; and on the duckling bucket policy actually granting the node role read, which is not visible from the repo.
| "AGENT_BUNDLES_S3_BUCKET": "no reader found in this codebase; defaults to OBJECT_STORAGE_BUCKET", | ||
| "AI_BLOB_S3_BUCKET": "read via posthog.storage.object_storage (boto3), by ai_observability/backend/api/ai_blob.py", | ||
| "BATCH_EXPORTS_FILE_DOWNLOAD_BUCKET": "read via aioboto3 by the Temporal batch-exports workflow", | ||
| "BATCH_EXPORT_INTERNAL_STAGING_BUCKET": "read via aioboto3 by the Temporal batch-exports workflow", |
There was a problem hiding this comment.
Yeah, I was just going to say the same thing: In production we don't use credentials here. So, I think we should exclude all batch export buckets too.
3 participants
Problem
_posthog_owned_bucket_names(added by #81559) hardcoded four bucket settings inline, with nothing to catch a new one landing without anyone deciding whether it needs the same treatment. That's exactly how the list came to be incomplete in the first place: a sweep of every Django setting ending in_BUCKETfound 20, not 4.Tracing each of the 20 to its actual reader (not just which product's
settings.pyfile defines it) found four more that ClickHouse itself reads or writes through credential-lesss3(...)/BACKUP ... TO S3(...)calls - the same shape the original vulnerability exploited. None of these were reachable through a warehouse table'surl_patternbefore #81559 either, since that fix's own bucket list didn't include them - so this closes a gap in the fix's coverage, not a currently open one.Changes
_posthog_owned_bucket_namesinto an explicit registry (_POSTHOG_OWNED_BUCKET_SETTING_NAMES) and an exclusion list (_BUCKET_SETTINGS_NOT_READABLE_BY_THE_NODE_ROLE, each entry with a stated reason), both inproducts/warehouse_sources/backend/models/util.py.test_every_bucket_setting_is_either_owned_or_excluded_with_a_reason, which enumerates every Django setting ending in_BUCKETand fails if one isn't in either list. Verified it actually catches drift: injecting a fakeSOME_NEW_THING_S3_BUCKETsetting viaoverride_settingsfails the test with a clear message pointing at which list to add it to.CLICKHOUSE_BACKUPS_BUCKET(ClickHouse's ownBACKUP ... TO S3(...), no access key in the query),IDENTITY_MATCHING_S3_BUCKET(the growth product's own comments confirm it's "mediated by ClickHouse...s3(...)"),OBJECT_STORAGE_EXTERNAL_WEB_ANALYTICS_BUCKET(posthog/hogql/database/schema/web_analytics_s3.pybuilds a credential-lesss3()call against it in production), andQUERY_LOG_ARCHIVE_EXPORT_S3_BUCKET(a Dagster job runsINSERT INTO FUNCTION s3(...)against it with no credentials).boto3/aioboto3client (a different IAM identity than the ClickHouse node role), or a note that no reader exists anywhere in this codebase.Note
The exclusion reasons are grounded in what the code actually does (which client reads the bucket, traced by hand for each of the 20), not in verified IAM policy - I have no access to the ClickHouse node role's actual permissions. Whoever owns that policy should treat this list as a starting point to confirm, not a final answer.
How did you test this code?
Added the registry-coverage test above, plus four new parameterized cases in
test_rejects_urls_that_address_posthog_storageexercising each newly-owned bucket in virtual-hosted and path-style URL forms. Ranproducts/warehouse_sources/backend/tests/test_util.py(32 passed) and the broaderproducts/data_warehouse/backend/tests/api/test_table.py+test_warehouse_table_file_upload.pysuite for regressions (82 passed). Also ranuv run mypy --cache-fine-grained .repo-wide (clean).Not independently verified: whether the ClickHouse node role's IAM policy actually grants it read access to the four newly-owned buckets, or whether it can reach any of the sixteen excluded ones despite the Python-level access path being different. Both are infra questions this PR can't answer from source alone.
Automatic notifications
Docs update
None - internal validation logic, no user-facing or documented behavior change.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Tom asked for a follow-up to #81559 to stop this class of "hardcoded list drifts" gap from recurring. Skills invoked:
/writing-code-comments,/writing-pr-descriptions.Building the exclusion list surfaced more than expected: my first pass judged each of the 20 settings by which product's
settings.pyfile defines it, which turned out to be the wrong signal -OBJECT_STORAGE_EXTERNAL_WEB_ANALYTICS_BUCKET,QUERY_LOG_ARCHIVE_EXPORT_S3_BUCKET,IDENTITY_MATCHING_S3_BUCKET, andCLICKHOUSE_BACKUPS_BUCKETall looked safely product-owned by that heuristic but turned out to be read directly by ClickHouse'ss3()/BACKUPfunctions once I traced the actual consumer code. Redid the pass checking the real discriminator (does ClickHouse itself touch this bucket without credentials) for all 20 before finalizing the two lists.