Repository navigation
fix(integrations): require account admin for GitHub installs, explicit callback origins - #1685
Merged
Merged
Conversation
…t callback origins
GitHub connect: a new installation binding now also checks that the
connecting user administers the installation's account (active org admin
via GET /user/memberships/orgs/{org}, or the personal account itself).
Listing in GET /user/installations only proves access. The App needs the
Organization "Members: Read" permission for the org check; without it the
connect fails closed.
Callback origins: isTrustedCallbackOrigin now accepts only the configured
app and API origins, the deploy's own PR preview hosts, and local hosts in
development, instead of any host under the app's parent domain. The
PlanetScale webhook_config URL is built from MAPLE_API_BASE_URL instead of
forwarded headers.
Maple review🟢 Confidence 9/10 · safe to merge Tightens GitHub connect to require admin of the installation's account and replaces the sibling-parent-domain callback rule with an exact allow list plus a PlanetScale webhook URL built from MAPLE_API_BASE_URL. The logic is sound and well covered; safe to merge once the App permission is granted.
Before merge
Production impactTelemetry this change adds and removes (1)
What was checked
Observability coverage: 1 of 1 changes observable
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 37 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Merged with 2 steps from "Before merge" still open. If they are done, tick them on the review comment; if not, now is the time.
|
This was referenced Oct 22, 2026
Thegreatsura
pushed a commit
to Thegreatsura/maple-analytics
that referenced
this pull request
Oct 23, 2026
…sts (MapleTechLabs#1698) MapleTechLabs#1685's account-admin cases called completeConnect without the initiator MapleTechLabs#1675 made required, so the call failed before the role lookup and every case read no role URLs. Main has failed these 6 tests since both landed. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Thegreatsura
pushed a commit
to Thegreatsura/maple-analytics
that referenced
this pull request
Oct 23, 2026
…#1635) * feat(integrations): connect an AWS account as a source An AWS connection is a pull source backed by an IAM role in the customer's account. Maple mints an external id, the customer deploys a generated CloudFormation stack (CloudShell script, console template or Terraform), and Maple assumes the role from its own identity to confirm the setup. No AWS secret is stored. - aws_connections table (source_id required, cascades with its source), AwsConnectionService and /v2/integrations/aws (status, create, update, setup, check, delete). - The role trusts Maple's account root, conditioned on the exact principal ARN and the connection's external id. Its name is derived from the connection id, so a caller can never point Maple at another role. - Each future module (metrics, inventory, security, cost) is its own policy on the same role; the check reads which are attached and the setup version tag, so a new module is a stack update, not a reconnect. - Hourly alerting lane re-checks connections: paused sources are skipped in SQL, the batch is claimed in one statement, two checks run at a time. A deleted stack reports the source disconnected; AWS outages only record the error. - Connect card under Sources: account form, setup panel that re-checks every 10s while open, connection health, disconnect with a cleanup script kept until dismissed. - Docs page integrations/aws. * chore(aws): regenerate the iOS OpenAPI spec and anticipated error identifiers * fix(aws): the unavailable card drops the steps it cannot offer * fix(aws): a background check only closes its own setup; pending setup is not a failure * fix(aws): the hourly sweep runs batches until nothing is due, never-checked first * fix(aws): a sweep checks its batch in due order, never-checked first * fix(aws): no check starts past the sweep budget * fix(aws): a disconnect closes only its own dialog; document and test PATCH A completed disconnect read the last-clicked connection, so it could close another connection's dialog and setup. The update request now documents that omitted modules stay unchanged, and update is tested in the service and route. * test(web): the in-flight click case starts a poll before it clicks The case clicked a page with no check in flight, so it never reached the flip that makes a background check the user's. * test(aws): the budget sweep allows a check that timed out unassumed Two checks run at once and each fake call jumps the shared clock 9s, so a call can pass its 20s timeout and count as checked without an AssumeRole. * fix(web): an AWS stack that needs updating reads as needing attention The hub card, sources picker and header now agree with the connection row. * test(aws): another org's admin cannot see or touch a connection; members cannot check it * test(github): pass the initiator in the install admin cases Same one-line fix as MapleTechLabs#1698: main's MapleTechLabs#1675 and MapleTechLabs#1685 crossed, so these cases called completeConnect without the initiator MapleTechLabs#1675 made required.
Thegreatsura
pushed a commit
to Thegreatsura/maple-analytics
that referenced
this pull request
Oct 23, 2026
…ct cases (MapleTechLabs#1699) MapleTechLabs#1675 made completeConnect require the signed-in initiator; the admin-role cases from MapleTechLabs#1685 still called it with three arguments, so every case died on initiator.orgId before reaching the membership check. Backend test files are outside typecheck, so only the test run caught it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GET /user/installationsas owned by the user. That endpoint lists installations the user can access, not ones they administer.adminmembership viaGET /user/memberships/orgs/{org}for an org, or the authenticated user's own id for a personal account. Fails closed when the role cannot be read.adminInstallationIdsrenamed toaccessibleInstallationIds. Same-org reconnects are unchanged.GithubConnectService.test.ts: listed but member / pending / role unreadable / other user's account -> refused, no binding, no sync job; org admin and own account -> bound. Asserts the role endpoint called.isTrustedCallbackOriginaccepted any host under the app's parent domain.MAPLE_APP_BASE_URLandMAPLE_API_BASE_URLorigins (exact scheme/host/port), that PR's ownapp-pr-<n>/api-pr-<n>.maple.devonly whenMAPLE_ENVIRONMENTispr-<n>, and local hosts only indevelopmentwith a local app origin. Callers now passenv.integrations-origin.test.ts(sibling lookalike, other PR number, port, scheme, spoofed host refused; configured and preview origins accepted).integrations.http.test.ts: connect from a sibling host returns 503.webhook_configbuilt its URL from the forwarded host.MAPLE_API_BASE_URL.integrations.http.test.ts: forwarded host ignored.Overlap note: #1673 and #1675 also touch
GithubConnectService.completeConnectandroutes/internal/integrations.http.ts. This diff keeps the service change to the admin verification block insidecompleteConnect(token kept as anOption, check added after the installation detail) and only changes the helper call ininternal/integrations.http.ts, so it should rebase cleanly in either order.Breaking-change risk
docs/github-app-setup.mdupdated.MAPLE_API_BASE_URLnow need it set to their API origin. All alchemy stages (prd, EU, previews, dev) already set both URLs from their domains.Verification
bun run --cwd packages/backend test -- .../github/__tests__/(77 passed),bun run --cwd apps/api test -- integrations-origin integrations.http chat-destinations integrations-chat.identity integrations-gcp(all passed).main(7 in the connect suite, 2 in the HTTP suite) and pass here.typecheckforpackages/backendandapps/api,bun run lint, oxfmt on changed files.Needs a human
Need help on this PR? Tag
@codesmith-botwith what you need.