fix(livestream): recheck current project membership - #108135
pauldambra wants to merge 7 commits into
Conversation
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
|
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 |
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review 🦔 PostHog Review reviewed this pull requestNothing worth raising this time. Enjoy the moment:
|
🤖 CI report
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe change adds a livestream authorization endpoint that validates tokens and current access. The Go service calls the endpoint before handling stats, events, and notifications requests. It also rechecks access during open event and notification streams and closes them when access fails. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Revoked users can receive a queued notification or event after an access check denies them. Close both delivery gaps before merging; the authorization endpoint’s transport configuration also needs owner attention. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new checks improve revocation handling, but they are disabled when the authorization URL is unset. A stream can also deliver a queued item after a periodic check denies access. Whether the URL is configured and reachable in production remains unverified. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
livestream/configs/configs.go-134-134 (1)
134-134: 🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟡 Minor | ⚡ Quick winSecurity Misconfiguration
Reachability: Internal
Exploitability: Difficult
CWE: CWE-319 — Cleartext Transmission of Sensitive InformationReject non-loopback HTTP authorization URLs.
CheckAccessforwards the incoming bearer token tojwt.authorization_urlon the initial request and every 30-second stream check. A plain-HTTP URL sends the token without transport encryption. Require HTTPS, allow HTTP only for loopback URLs used in tests, and document the environment variable.Validate the authorization URL in LoadConfig
import ( "errors" "log" + "net/url" "strings" "github.com/spf13/viper" ) @@ _ = viper.BindEnv("jwt.secret") // LIVESTREAM_JWT_SECRET _ = viper.BindEnv("jwt.secret_fallbacks") // LIVESTREAM_JWT_SECRET_FALLBACKS (comma-separated) - _ = viper.BindEnv("jwt.authorization_url") + _ = viper.BindEnv("jwt.authorization_url") // LIVESTREAM_JWT_AUTHORIZATION_URL @@ if err := viper.Unmarshal(&config); err != nil { return nil, err } + if config.JWT.AuthorizationURL != "" { + u, err := url.Parse(config.JWT.AuthorizationURL) + if err != nil || u.Hostname() == "" || + (!strings.EqualFold(u.Scheme, "https") && + !(strings.EqualFold(u.Scheme, "http") && + (strings.EqualFold(u.Hostname(), "localhost") || u.Hostname() == "127.0.0.1" || u.Hostname() == "::1"))) { + return nil, errors.New("jwt.authorization_url must use HTTPS or loopback HTTP") + } + } + // Set default valuesSource: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: ac60a18d-84bc-4492-9110-edac905f76e8
📒 Files selected for processing (8)
livestream/auth/access.golivestream/auth/access_test.golivestream/configs/configs.golivestream/handlers/handlers.golivestream/handlers/handlers_test.goposthog/api/livestream.pyposthog/api/test/test_livestream.pyposthog/urls.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| permission_classes = [IsAuthenticated] | ||
|
|
||
| def get(self, request: Request) -> Response: | ||
| level = UserPermissions(cast(User, request.user)).team(cast(Team, request.auth)).effective_membership_level |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
Permission checks load access rules for every project
Issue description
For each check, effective_membership_level loads _prefetched_access_controls. That query materializes project access-control rows for every team in all of the user's organizations, even though this endpoint checks one team. With checks running every 15 or 30 seconds per stream, this can add substantial database and memory work as organizations grow.
Why we think it's a valid issue
- Checked: Traced
LivestreamAuthorizationView.getthroughUserPermissions.team(...).effective_membership_leveland checked when access-control data is loaded and reused. - Found: When the organization has access control enabled,
effective_membership_level_for_parent_membershipreads_prefetched_access_controlsbefore filtering rules for the requested team (posthog/user_permissions.py:207-215). That property queries project access-control rows across all of the user’s organizations and builds an in-memory dictionary of every returned row (posthog/user_permissions.py:113-134). The endpoint creates a newUserPermissionsfor each authorization request (posthog/api/livestream.py:51), while livestream checks recur every 15 or 30 seconds (livestream/handlers/handlers.go:172-177,livestream/handlers/handlers.go:323-352). - Impact: Each check can query and materialize unrelated project rules, and the work repeats across active streams. The cost grows with access-control rows in the user’s organizations, so this is a concrete performance concern.
Suggested fix
Use a permission check that filters access-control rows to the requested team, or add a single-team path so periodic checks do not load rules for unrelated projects.
Prompt to fix with AI (copy-paste)
## Context
@posthog/api/livestream.py#L51
<issue_description>
For each check, `effective_membership_level` loads `_prefetched_access_controls`. That query materializes project access-control rows for every team in all of the user's organizations, even though this endpoint checks one team. With checks running every 15 or 30 seconds per stream, this can add substantial database and memory work as organizations grow.
</issue_description>
<issue_validation>
- **Checked:** Traced `LivestreamAuthorizationView.get` through `UserPermissions.team(...).effective_membership_level` and checked when access-control data is loaded and reused.
- **Found:** When the organization has access control enabled, `effective_membership_level_for_parent_membership` reads `_prefetched_access_controls` before filtering rules for the requested team (`posthog/user_permissions.py:207-215`). That property queries project access-control rows across all of the user’s organizations and builds an in-memory dictionary of every returned row (`posthog/user_permissions.py:113-134`). The endpoint creates a new `UserPermissions` for each authorization request (`posthog/api/livestream.py:51`), while livestream checks recur every 15 or 30 seconds (`livestream/handlers/handlers.go:172-177`, `livestream/handlers/handlers.go:323-352`).
- **Impact:** Each check can query and materialize unrelated project rules, and the work repeats across active streams. The cost grows with access-control rows in the user’s organizations, so this is a concrete performance concern.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Use a permission check that filters access-control rows to the requested team, or add a single-team path so periodic checks do not load rules for unrelated projects.
</potential_solution>
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
pauldambra
left a comment
pauldambra
left a comment
There was a problem hiding this comment.
[Robot]
Note
🤖 Automated comment by QA Swarm — not written by a human
QA review found one event delivery risk. See the inline comment.
|
[Robot] Note 🤖 Automated comment by QA Swarm — not written by a human Verdict: APPROVE WITH NITS (round 3 @ 0a06d4c)The final review found no new code issue. Two earlier performance questions remain open for a separate decision. Key findings
ConvergenceBoth reviewers approved the notification fix. The periodic checks do not promise an atomic cutoff at the instant the access service returns a denial. Reviewer summaries
Previous rounds
Automated by QA Swarm — not a human review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f696fcc3b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err != nil { | ||
| return c.JSON(http.StatusUnauthorized, resp{Error: "wrong token claims"}) | ||
| } | ||
| if err := auth.CheckAccess(c.Request().Context(), c.Request().Header); err != nil { |
There was a problem hiding this comment.
When the Live Events page is open, it polls /stats every 1,500 ms (LiveEventsTable.tsx:32,74), so this new call generates about 40 synchronous Django authorization requests per minute per viewer. Each request performs User and Team lookups and, for access-control organizations, UserPermissions loads every project access-control row across all organizations the user belongs to (posthog/user_permissions.py:113-134), despite checking one team. This multiplies organization-wide database scans by active viewers and can make the three-second fail-closed timeout disconnect users under load; cache or throttle the stats authorization decision to the 15–30-second recheck cadence, or use a team-scoped resolver.
Useful? React with 👍 / 👎.
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
Generated-By: PostHog Desktop Task-Id: 160dfc49-dceb-4887-b9f5-a93b2da017ed
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Check access errors before writing queued events. · handlers.go:196-203
livestream/handlers/handlers.go:196-203
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCheck access errors before writing queued events.
periodicAccessCheckssends a denial to a buffered channel and returns. Ifsubscription.EventChanalso contains an event, the stream loop can select the event case and callevent.WriteTobefore processing the denial. The revoked client can therefore receive an event after access is denied.Suggested fix
event := Event{ Data: jsonData, } + select { + case err := <-accessErrors: + log.Warnf("Live stream authorization check failed: %v", err) + return nil + default: + } if err := event.WriteTo(w); err != nil { return err }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: ff82f0e7-d7cf-4e4b-bf54-52282d92134c
📒 Files selected for processing (2)
livestream/handlers/handlers.golivestream/handlers/handlers_test.go
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
Re-add the stamphog label to request another review once you have addressed this.
Two gates refused this pull request, so it needs a human reviewer. The deny-list gate flagged it for matching auth, which covers the new livestream/auth/access.go and the authorization endpoint in posthog/api/livestream.py. The tier gate classified it as T2-never (568 lines across 8 files, spanning two areas: the Go livestream service and the Python API), and that tier is never handled automatically.
The author can ask a human reviewer, such as someone on the owning team for auth or livestream, to review it. Splitting it into a Python API change and a separate Go service change might make that review easier, but the auth match would still need human sign-off.
- coderabbitai[bot] reviewed the current head.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: auth |
| size | ✓ | 167L, 5F substantive, 568L/8F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (568L, 8F, two-areas, fix) |
| stamphog 2.3.0 | .stamphog/policy.yml @ unknown · reviewed head 0a06d4c |
This branch has not been deployed
2 participants
[Robot] Prepared by PostHog Desktop.
Problem
Live streams need to apply current project access when organization or project membership changes.
This is the separate service change from the review of #99248.
Changes
Before:
flowchart LR Client[Client] --> JWT[Token check] --> Stream[Live stream] classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; class Client,Stream phYellow; class JWT phBlue;After configuration:
flowchart LR Client[Client] --> JWT[Token check] --> API[Current access check] --> Stream[Live stream] Stream -->|Periodic check| API classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; class Client,Stream phYellow; class JWT phBlue; class API phRed;How did you test this code?
Release status
Note
The new check is inactive until
LIVESTREAM_JWT_AUTHORIZATION_URLpoints to the regional API/api/livestream/authorize/endpoint.Deploy the API first. Configure one stream service instance, then check authorization failures and stream connections before expanding the rollout.
API failures close streams. Watch reconnect rates and API load during the staged rollout.
Existing tokens must include user and organization claims. Leave the setting empty to preserve current behavior during deployment.
Automatic notifications
Docs update
No existing document under
docs/covers this service setting. The rollout steps are above.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: PostHog Desktop / Codex, GPT-6; gpt-5.6-sol wrote the review fixes. gpt-6-astra validated the authorization and concurrency changes.
Tools: Git, GitHub CLI, hogli, pytest, Go, mypy, Ruff, Semgrep, and CodeRabbit.
Skills: improving-drf-endpoints, writing-tests, writing-code-comments, writing-user-facing-copy, setting-up-devbox, running-ci-preflight, reviewing-with-coderabbit, writing-pr-descriptions, pr-shepherd, qa-swarm, review-triage, paul-pair, simplify, ci-shepherd, and security-audit.
The open PR search found no separate membership fix. Test data is invented from the public code contract.
The first CodeRabbit review found no issues. The combined stack review raised outage handling and the optional rollout setting. Both are intentional: authorization failures stop access, and the setting permits API-first deployment.
Created with PostHog Desktop