integrations gcs and s3#403
Open
lindesvard wants to merge 8 commits into
Open
Conversation
Export a project's analytics events to the user's own S3 or GCS bucket. ClickHouse is the single source of truth for the export — there is no Redis buffer and no per-event ingestion hook, so a stalled/failing bucket can never touch the ingestion path (in either the Kafka or GroupMQ mode). How it works: - A new `inserted_at DateTime64(3)` column on the events table records ingestion time. It DEFAULTs to `created_at` (deterministic for pre-migration parts — a DEFAULT now() would be evaluated lazily at read time and never settle) and is set explicitly at insert time (createEvent + the import production-move), so backdated events (server-side, offline, imports) still get a real insert time. A minmax skip index keeps the windowed scan cheap since inserted_at isn't in the primary key. - The flushExports cron job (every 60s) windows the events table by inserted_at for each (project, integration), batches rows into gzipped JSONL + a manifest, and uploads them. A per-(project, integration) ExportWatermark (Postgres) tracks progress with a composite (inserted_at, id) cursor — the id tie-breaker is required because an import stamps one inserted_at across its whole batch. A safety lag behind now() avoids reading in-flight inserts; the watermark only advances after a successful upload (at-least-once; the manifest is the commit marker). First run starts from now(), so connecting an export doesn't dump all history (historical backfill = reset the watermark). Integrations are organization-scoped, so every active project in the org exports independently and gets its own watermark + object path (layout keyed by project_id). Destinations are a pluggable object-store sink (S3 + GCS adapters) so future targets are small additions. Credentials (S3 secret keys, GCS service-account keys) are encrypted at rest with CREDENTIALS_ENCRYPTION_KEY and decrypted inside the adapters. Export batching and the safety lag are tunable via EXPORT_* env vars. Rebased onto main after the Redpanda/Kafka migration. Claude-Session: https://claude.ai/code/session_01AWucrxmRqWqgWBvr8cC5cj
Phase 1 of the integrations rework. Adds a nullable Integration.projectId: set = project-scoped, null = legacy org-wide. Existing org integrations keep working untouched (additive migration, no backfill). - schema: Integration.projectId (nullable FK) + indexes; Project reverse relation. - export cron: project-scoped integrations export only their project; legacy org-wide rows still fan out across the org's projects. - validation/tRPC: create inputs take projectId (org derived from project); added in-handler getProjectAccess to list + createOrUpdate* (closing a pre-existing authz gap that trusted client-supplied scope); get/delete use project-or-org access by scope. list also surfaces legacy org-wide rows. - notification rules: connected integrations must belong to the same project (or be org-wide in the same org); also added the missing access check on the rule create branch. - Slack OAuth carries projectId through install metadata + callback redirect. - dashboard: integrations moved to project-scoped routes + sidebar; org route and org-level "add integration" CTA removed. typecheck + tests green (657). Claude-Session: https://claude.ai/code/session_01AWucrxmRqWqgWBvr8cC5cj
Phase 2 of the integrations rework. Restructures integrations as three registries keyed by the same `type` literal so adding one is additive — no edits to central switches — while keeping types bulletproof. Type safety (the priority): IIntegrationConfig stays the explicit, hand-written discriminated union (source of truth, feeds Prisma's IPrismaIntegrationConfig). It is NOT derived from the registry. The registries are forced to MATCH it via `satisfies Record<IIntegrationConfig['type'], …>`, and two compile-time guards in validation fail the build if any union member loses its literal `type` discriminant or if the descriptor set drifts from the union. - core (packages/validation/src/integrations.ts): per-type zod schemas + an INTEGRATION_DESCRIPTORS registry (kinds/setup/configSchema/catalog), getDescriptor/isKind, a runtime zIntegrationConfig, a generic zCreateIntegration, and the type-level guards. - server (packages/integrations/src/registry.ts): IServerIntegration<T> + SERVER_INTEGRATIONS with notification.deliver / export.createAdapter / validateConfig / testConnection / encryptCredentials hooks. The bespoke slack/discord/webhook send bodies and s3/gcs adapter+encrypt logic moved here. getServerIntegration<T> holds the one contained cast. - client (apps/start integrations.tsx): CLIENT_INTEGRATIONS (icon + Form) keyed by type; the catalog is derived from descriptors. add-integration.tsx renders via registry lookup instead of a switch. - dispatch made generic: worker notification.ts and cron.flush-exports.ts look up the plugin; the tRPC router collapses to a generic createOrUpdate + testConnection delegating to plugin hooks (export/slack procedures kept as thin aliases for one release). - fixed the discord form's server value-import (browser-bundle leak) by routing its test through trpc.integration.testConnection. Deferred (demand-driven): the generic /oauth/:type route only benefits a second OAuth integration and carries external Slack-redirect-URI risk, so slack OAuth is unchanged. Legacy zCreate* schemas + tRPC aliases kept until the dashboard drops them. typecheck + tests green (657). Claude-Session: https://claude.ai/code/session_01AWucrxmRqWqgWBvr8cC5cj
The flushExports cron lists every integration and filters by
isKind(config, 'export'). A Slack integration that hasn't completed OAuth has
an empty config ({} with no type), and the registry refactor made isKind throw
"Unknown integration type: undefined" on it, failing the whole cron run.
isKind now returns false for unknown/undefined types instead of throwing (it's
a filter predicate, not a guaranteed-known lookup). Also guard the notification
worker against delivering to an unconfigured integration. Adds an isKind
regression test.
Claude-Session: https://claude.ai/code/session_01AWucrxmRqWqgWBvr8cC5cj
There were two AES-256-GCM modules with two env keys for the same purpose: db/src/encryption.ts (ENCRYPTION_KEY, for TOTP/GSC) and common/server/encryption.ts (CREDENTIALS_ENCRYPTION_KEY, for integration creds, needed in @openpanel/integrations which can't import db). Consolidate to one implementation in @openpanel/common/server keyed solely by ENCRYPTION_KEY. It hosts both the plain encrypt/decrypt (unchanged format, so existing TOTP/GSC ciphertext still decrypts) and the prefixed encryptCredential/decryptCredential (idempotent, plaintext-passthrough for the test-connection flow). db/src/encryption.ts now re-exports encrypt/decrypt, so @openpanel/db importers are unchanged. Drops CREDENTIALS_ENCRYPTION_KEY from .env.example. Adds an encryption round-trip test. Note: any integration credentials encrypted on this branch with the old key must be re-saved (no prod data — feature is unmerged). Claude-Session: https://claude.ai/code/session_01AWucrxmRqWqgWBvr8cC5cj
PR-review cleanup: getDescriptor, descriptorsByKind, zCreateIntegration and ICreateIntegration in validation/src/integrations.ts had zero callers (the client derives the catalog from INTEGRATION_DESCRIPTORS directly, isKind uses the type map directly, and the tRPC create procedure inlines its own config-required input). The type-safety guards and isKind are unaffected. Claude-Session: https://claude.ai/code/session_01AWucrxmRqWqgWBvr8cC5cj
Contributor
📝 WalkthroughWalkthroughThis PR adds shared encryption and integration validation, project-scoped integration routes and forms, database support for export cursors and watermarks, and a worker cron job that flushes export batches. ChangesProject-scoped integrations and export pipeline
Sequence Diagram(s)sequenceDiagram
participant bootCron
participant cronJob
participant flushExportsJob
participant db
participant ClickHouse
participant IObjectStoreAdapter
bootCron->>cronJob: schedule flushExports every minute
cronJob->>flushExportsJob: dispatch CronQueuePayloadFlushExports
flushExportsJob->>db: load integrations and exportWatermark rows
flushExportsJob->>ClickHouse: queryWindow(projectId, inserted_at, id)
flushExportsJob->>IObjectStoreAdapter: upload batch files
flushExportsJob->>IObjectStoreAdapter: upload manifest.json
flushExportsJob->>db: saveCursor(exportWatermark)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)packages/common/server/index.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.15][ERROR]: unable to find a config; path packages/common/server/ssrf.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.15][ERROR]: unable to find a config; path packages/trpc/src/routers/integration.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.12][ERROR]: unable to find a config; path
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/start/src/components/integrations/forms/discord-integration.tsx (1)
57-74: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
mutateAsyncfailure is unhandled — wrap the test call in try/catch.
testConnection.mutationOptions()has noonError, andhandleTestawaitsmutateAsyncwithout atry/catch. If the request rejects (network error, server-side validation failure), the promise rejection is unhandled and the user gets no feedback — theFailed to send test notificationtoast only covers theres.success === falsepath.🛡️ Proposed fix
const handleTest = async () => { const url = form.getValues('config.url'); if (!url) { return toast.error('Webhook URL is required'); } - const res = await testMutation.mutateAsync({ - config: { type: 'discord', url }, - }); - if (res.success) { - toast.success('Test notification sent'); - } else { - toast.error('Failed to send test notification'); - } + try { + const res = await testMutation.mutateAsync({ + config: { type: 'discord', url }, + }); + if (res.success) { + toast.success('Test notification sent'); + } else { + toast.error('Failed to send test notification'); + } + } catch (error) { + toast.error('Failed to send test notification'); + } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/start/src/components/integrations/forms/discord-integration.tsx` around lines 57 - 74, The Discord integration test flow in handleTest currently awaits testMutation.mutateAsync without handling rejected promises, so add a try/catch around the call and surface an error toast from the catch path. Keep the existing success/failure handling for a resolved response, but ensure any network or server rejection from trpc.integration.testConnection.mutationOptions() and testMutation is caught and reported so the user always gets feedback.
🧹 Nitpick comments (3)
apps/start/src/components/integrations/integrations.tsx (1)
36-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment references
satisfiesbut the code uses an explicit type annotation.The JSDoc says the registry is "forced to cover every type via
satisfies Record<IIntegrationType, …>", butCLIENT_INTEGRATIONSis declared with an explicit: Record<IIntegrationType, IClientIntegration>annotation. Both enforce exhaustiveness, so behavior is correct; only the comment is inaccurate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/start/src/components/integrations/integrations.tsx` around lines 36 - 43, The JSDoc for CLIENT_INTEGRATIONS is inaccurate because it mentions being forced via satisfies while the declaration actually uses an explicit Record<IIntegrationType, IClientIntegration> annotation. Update the comment to match the current implementation by describing the registry as exhaustively typed with the Record annotation, or change the declaration to use satisfies if that was the intended pattern, keeping the wording aligned with the CLIENT_INTEGRATIONS symbol.packages/integrations/src/object-store/gcs-adapter.ts (1)
137-157: 🩺 Stability & Availability | 🔵 Trivial
testConnectioncan fail for write-only service accounts.bucket.exists()requiresstorage.buckets.get, so credentials that can upload objects but can’t read bucket metadata will be reported as “does not exist or is not accessible.” Consider a connection check aligned with the upload path, such as a minimal write/delete probe or a softer metadata check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/integrations/src/object-store/gcs-adapter.ts` around lines 137 - 157, The testConnection method in gcs-adapter.ts is too strict because bucket.exists() depends on metadata read permissions and can falsely fail for write-only credentials. Update GCSAdapter.testConnection to validate access using a check aligned with the upload path, such as a minimal write-and-delete probe or a less restrictive bucket check, while keeping the existing error handling and return shape intact.packages/integrations/src/object-store/s3-adapter.ts (1)
57-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid casting the cached
Promise<S3Client>as a rawS3Client.
getClientWithAccessKeys()is declared to returnS3Client, but the reuse path returnsthis.clientPromise as unknown as S3Client, which is actually a Promise. It only works because the callergetClient()isasyncand transparently unwraps the returned thenable. This double-casting is brittle: any future synchronous caller of this method would receive a Promise where anS3Clientis expected. Cache the resolved client (or make the method consistently async) instead.♻️ Suggested approach
- private getClientWithAccessKeys(): S3Client { - // Access key clients don't expire, reuse if available - if (this.clientPromise && this.clientExpiresAt === 0) { - return this.clientPromise as unknown as S3Client; - } + private client: S3Client | null = null; + + private getClientWithAccessKeys(): S3Client { + // Access key clients don't expire, reuse if available + if (this.client) { + return this.client; + } @@ - this.clientExpiresAt = 0; - this.clientPromise = Promise.resolve(client); - - return client; + this.client = client; + return client; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/integrations/src/object-store/s3-adapter.ts` around lines 57 - 91, The reuse path in getClientWithAccessKeys is returning this.clientPromise via a double cast, which hides that it is actually a Promise rather than an S3Client. Update the caching approach so the method always returns a real S3Client, either by storing the resolved client instance alongside the promise or by making the access-key client creation path consistently async and awaiting the cached promise in getClient() and related callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/controllers/webhook.controller.ts`:
- Around line 106-111: The redirect in webhook.controller’s installed-flow
currently falls back to an organization-only destination that does not exist, so
older Slack callbacks without a projectId will hit a 404. Update the redirect
logic around the reply.redirect path to always target a valid route, either by
routing to the project-scoped installed page using the available projectId or by
adding a matching org-level route; use the existing dashboardUrl,
organizationId, and projectId handling in this block to locate the fix.
In `@apps/start/src/components/integrations/forms/gcs-export-integration.tsx`:
- Around line 82-88: The GCS test flow in handleTest is missing the same
required name check as the S3 form, so it can call testExportConnection with an
invalid config and trigger a generic schema failure toast. Update handleTest in
gcs-export-integration.tsx to validate name along with bucket and
serviceAccountKey before calling testMutation.mutate, using the same
required-field gating pattern used by the S3 form and aligned with
zCreateGCSExportIntegration.
In `@apps/start/src/components/integrations/forms/s3-export-integration.tsx`:
- Around line 109-123: The S3 test handler currently validates only
bucket/region and auth fields, but the test mutation still uses the full create
payload, so missing Name causes `testExportConnection` to fail before connection
validation. Update `handleTest` in `s3-export-integration.tsx` to also gate on
the `name` field, or change `testMutation`/`testExportConnection` to accept
config-only input. Use the existing `form.getValues()`, `testMutation.mutate`,
and `zCreateS3ExportIntegration` flow to locate the fix.
In `@apps/worker/src/jobs/cron.flush-exports.ts`:
- Around line 33-41: The env-driven numeric constants in cron.flush-exports need
validation because Number.parseInt can produce NaN for malformed values, which
can silently break runWithConcurrency and the export query. Update the parsing
logic for LAG_SECONDS, BATCH_SIZE, MAX_BATCHES_PER_RUN, and CONCURRENCY to use a
validated numeric helper (or equivalent fallback) that rejects NaN and falls
back to safe defaults, then keep the existing symbols so the fix is localized
and easy to find.
- Around line 247-269: The first-run branch in loadCursor is racy because it
does a findUnique followed by create, so overlapping cron executions can hit the
same (projectId, integrationId) pair and throw on the unique constraint. Replace
that TOCTOU flow with a single atomic upsert on exportWatermark for the
existing/new watermark path, keeping the existing Cursor return shape from
loadCursor. Also add a job-level concurrency guard for cron.flush-exports so one
run cannot overlap the next minute’s trigger and reprocess the same watermark
window.
In `@packages/db/prisma/schema.prisma`:
- Around line 611-618: The ExportWatermark model currently allows a projectId
and integrationId pair that may not belong together, so the watermark can point
across projects. Update the ExportWatermark definition in the Prisma schema to
enforce project-scoped consistency by adding a composite relation/constraint
tied to the project and integration ownership, or add an equivalent guard in the
watermark create/update path. Use the ExportWatermark fields projectId and
integrationId and the Integration relation as the key symbols to locate the fix.
In `@packages/integrations/package.json`:
- Around line 19-24: The package manifest currently places `@openpanel/logger`
under devDependencies even though it is used at runtime by s3-adapter.ts via
createLogger. Move `@openpanel/logger` from devDependencies to dependencies in the
package.json for packages/integrations so consumers resolving this package can
load the runtime import correctly.
In `@packages/integrations/src/registry.ts`:
- Around line 133-140: Webhook dispatch in the registry still sends requests
straight to fetch(config.url), so add SSRF protection in the webhook path before
calling it. In the code around the request construction in registry.ts, reject
any config.url that is not HTTP or HTTPS and block private, loopback,
link-local, and metadata IP targets before dispatching. Reuse or add a dedicated
validation helper in the webhook/registry flow so the safety check happens
immediately before fetch and applies to all outgoing webhook requests.
- Around line 119-122: The webhook template execution in registry.ts is still
using executeJavaScriptTemplate with new Function, which allows
computed-property constructor escapes to break out of the validator. Replace
this in-process execution path with a real sandbox/isolate for
config.javascriptTemplate, or add strict blocking for constructor/prototype
access before calling executeJavaScriptTemplate, and make sure payload.event is
only evaluated inside the hardened boundary.
In `@packages/trpc/src/routers/integration.ts`:
- Around line 144-155: The Slack integration update path in integration.ts only
checks organizationId before clearing config, but it also needs to be scoped to
the existing integration’s project. In the input.id branch of the update logic,
load or validate the current integration’s projectId and include it in the
authorization check alongside organizationId before calling
db.integration.update, so only the same-project integration can be updated.
- Around line 64-68: The generic update path in integration.ts currently updates
by id and organizationId only, which can allow cross-project updates within the
same org. In the integration update logic inside the existing `if (input.id)`
branch, first load the current record (for example via the same `db.integration`
model) and verify `existing.projectId === input.projectId` before calling
`db.integration.update`; if it does not match, reject the request. Keep the fix
localized to the update flow around `input.id`, `organizationId`, and
`input.projectId` so the authorization check is enforced for generic updates.
In `@packages/validation/src/integrations.ts`:
- Line 68: The custom S3 endpoint validation in integrations should be tightened
because endpoint is tenant-controlled and can be used for server-side requests.
Update the zod validation around endpoint to only accept HTTPS public endpoints
or an explicit allowlist, and make sure the object-store adapter re-checks the
resolved host/IP before connecting. Use the endpoint schema and the downstream
S3/adapter connection path to locate the fix.
- Around line 111-112: The serviceAccountKey schema in integrations validation
is too permissive because it only checks for a non-empty string, so update the
validator to require parsed service-account JSON with the expected fields. In
the integrations schema for serviceAccountKey, add validation that ensures the
value is valid JSON and contains required keys like project_id, client_email,
and private_key, so malformed credentials are rejected before reaching
gcs-adapter and its JSON.parse path.
---
Outside diff comments:
In `@apps/start/src/components/integrations/forms/discord-integration.tsx`:
- Around line 57-74: The Discord integration test flow in handleTest currently
awaits testMutation.mutateAsync without handling rejected promises, so add a
try/catch around the call and surface an error toast from the catch path. Keep
the existing success/failure handling for a resolved response, but ensure any
network or server rejection from
trpc.integration.testConnection.mutationOptions() and testMutation is caught and
reported so the user always gets feedback.
---
Nitpick comments:
In `@apps/start/src/components/integrations/integrations.tsx`:
- Around line 36-43: The JSDoc for CLIENT_INTEGRATIONS is inaccurate because it
mentions being forced via satisfies while the declaration actually uses an
explicit Record<IIntegrationType, IClientIntegration> annotation. Update the
comment to match the current implementation by describing the registry as
exhaustively typed with the Record annotation, or change the declaration to use
satisfies if that was the intended pattern, keeping the wording aligned with the
CLIENT_INTEGRATIONS symbol.
In `@packages/integrations/src/object-store/gcs-adapter.ts`:
- Around line 137-157: The testConnection method in gcs-adapter.ts is too strict
because bucket.exists() depends on metadata read permissions and can falsely
fail for write-only credentials. Update GCSAdapter.testConnection to validate
access using a check aligned with the upload path, such as a minimal
write-and-delete probe or a less restrictive bucket check, while keeping the
existing error handling and return shape intact.
In `@packages/integrations/src/object-store/s3-adapter.ts`:
- Around line 57-91: The reuse path in getClientWithAccessKeys is returning
this.clientPromise via a double cast, which hides that it is actually a Promise
rather than an S3Client. Update the caching approach so the method always
returns a real S3Client, either by storing the resolved client instance
alongside the promise or by making the access-key client creation path
consistently async and awaiting the cached promise in getClient() and related
callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9ec9155-ebb9-40a8-8d8a-0a4a31aae9ef
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (57)
.env.exampleapps/api/package.jsonapps/api/src/controllers/webhook.controller.tsapps/api/tsdown.config.tsapps/start/src/components/integrations/active-integrations.tsxapps/start/src/components/integrations/forms/discord-integration.tsxapps/start/src/components/integrations/forms/gcs-export-integration.tsxapps/start/src/components/integrations/forms/s3-export-integration.tsxapps/start/src/components/integrations/forms/slack-integration.tsxapps/start/src/components/integrations/forms/webhook-integration.tsxapps/start/src/components/integrations/integrations.tsxapps/start/src/components/sidebar-organization-menu.tsxapps/start/src/components/sidebar-project-menu.tsxapps/start/src/modals/add-integration.tsxapps/start/src/modals/add-notification-rule.tsxapps/start/src/routeTree.gen.tsapps/start/src/routes/_app.$organizationId.$projectId.integrations._tabs.available.tsxapps/start/src/routes/_app.$organizationId.$projectId.integrations._tabs.index.tsxapps/start/src/routes/_app.$organizationId.$projectId.integrations._tabs.installed.tsxapps/start/src/routes/_app.$organizationId.$projectId.integrations._tabs.tsxapps/start/src/routes/_app.$organizationId.integrations._tabs.index.tsxapps/worker/package.jsonapps/worker/src/boot-cron.tsapps/worker/src/boot-debug.tsapps/worker/src/jobs/cron.flush-exports.tsapps/worker/src/jobs/cron.tsapps/worker/src/jobs/notification.tsapps/worker/tsdown.config.tspackages/common/server/encryption.test.tspackages/common/server/encryption.tspackages/common/server/index.tspackages/db/code-migrations/18-add-events-inserted-at.tspackages/db/index.tspackages/db/prisma/migrations/20260622204749_export_watermarks/migration.sqlpackages/db/prisma/migrations/20260623092702_integration_project_scope/migration.sqlpackages/db/prisma/schema.prismapackages/db/src/encryption.tspackages/db/src/exports/batch-creator.tspackages/db/src/exports/export-event.tspackages/db/src/exports/index.tspackages/db/src/exports/manifest.tspackages/db/src/services/event.service.tspackages/db/src/services/import.service.tspackages/db/src/services/notification.service.tspackages/integrations/package.jsonpackages/integrations/src/object-store/gcs-adapter.tspackages/integrations/src/object-store/index.tspackages/integrations/src/object-store/s3-adapter.tspackages/integrations/src/object-store/types.tspackages/integrations/src/registry.tspackages/integrations/src/slack.tspackages/queue/src/queues.tspackages/trpc/src/routers/integration.tspackages/trpc/src/routers/notification.tspackages/validation/src/index.tspackages/validation/src/integrations.test.tspackages/validation/src/integrations.ts
💤 Files with no reviewable changes (1)
- apps/start/src/routes/_app.$organizationId.integrations._tabs.index.tsx
Comment on lines
+82
to
+88
| const handleTest = () => { | ||
| const values = form.getValues(); | ||
| if (!values.config.bucket || !values.config.serviceAccountKey) { | ||
| return toast.error('Bucket and Service Account Key are required'); | ||
| } | ||
| testMutation.mutate(values); | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Same name validation gap as the S3 form.
handleTest validates bucket/serviceAccountKey but not name, while testExportConnection validates the full zCreateGCSExportIntegration schema (which requires name.min(1)). Testing before entering a Name yields a generic validation failure toast instead of a connection result. See the S3 form comment for the suggested fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/start/src/components/integrations/forms/gcs-export-integration.tsx`
around lines 82 - 88, The GCS test flow in handleTest is missing the same
required name check as the S3 form, so it can call testExportConnection with an
invalid config and trigger a generic schema failure toast. Update handleTest in
gcs-export-integration.tsx to validate name along with bucket and
serviceAccountKey before calling testMutation.mutate, using the same
required-field gating pattern used by the S3 form and aligned with
zCreateGCSExportIntegration.
Comment on lines
+109
to
+123
| const handleTest = () => { | ||
| const values = form.getValues(); | ||
| if (!values.config.bucket || !values.config.region) { | ||
| return toast.error('Bucket and Region are required'); | ||
| } | ||
| if (values.config.authMode === 'iam_role' && !values.config.roleArn) { | ||
| return toast.error('IAM Role ARN is required'); | ||
| } | ||
| if (values.config.authMode === 'access_key') { | ||
| if (!values.config.accessKeyId || !values.config.secretAccessKey) { | ||
| return toast.error('Access Key ID and Secret Access Key are required'); | ||
| } | ||
| } | ||
| testMutation.mutate(values); | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm test endpoints validate the full create schema (name/projectId required)
fd -t f 'integration.ts' packages/trpc/src/routers --exec sed -n '180,215p' {}
# Confirm base schema requires non-empty name
rg -nP -C2 'zCreateIntegrationBase|name:\s*z\.string\(\)\.min' packages/validation/src/integrations.tsRepository: Openpanel-dev/openpanel
Length of output: 2549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the S3 export form around handleTest and the form schema/fields.
sed -n '1,220p' apps/start/src/components/integrations/forms/s3-export-integration.tsx
# Inspect the form schema used by this component, if defined nearby or imported.
rg -n "useForm|zod|name|projectId|handleTest|testMutation|createOrUpdateExport|testExportConnection" apps/start/src/components/integrations/forms/s3-export-integration.tsx apps/start/src/components/integrations/forms -g '*.tsx' -g '*.ts'Repository: Openpanel-dev/openpanel
Length of output: 19409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the S3 export form around handleTest and the form schema/fields.
sed -n '1,220p' apps/start/src/components/integrations/forms/s3-export-integration.tsx
# Inspect the form schema used by this component, if defined nearby or imported.
rg -n "useForm|zod|name|projectId|handleTest|testMutation|createOrUpdateExport|testExportConnection" apps/start/src/components/integrations/forms/s3-export-integration.tsx apps/start/src/components/integrations/forms -g '*.tsx' -g '*.ts'Repository: Openpanel-dev/openpanel
Length of output: 19409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the S3 export form around handleTest and the form schema/fields.
sed -n '1,220p' apps/start/src/components/integrations/forms/s3-export-integration.tsx
# Inspect the form schema used by this component, if defined nearby or imported.
rg -n "useForm|zod|name|projectId|handleTest|testMutation|createOrUpdateExport|testExportConnection" apps/start/src/components/integrations/forms/s3-export-integration.tsx apps/start/src/components/integrations/forms -g '*.tsx' -g '*.ts'Repository: Openpanel-dev/openpanel
Length of output: 19409
Gate the S3 test on name too. handleTest sends the full create payload, and testExportConnection still requires name/projectId via zCreateS3ExportIntegration. Leaving Name blank makes the test fail before any connection check runs; either block the test until Name is filled or switch the test mutation to a config-only input.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/start/src/components/integrations/forms/s3-export-integration.tsx`
around lines 109 - 123, The S3 test handler currently validates only
bucket/region and auth fields, but the test mutation still uses the full create
payload, so missing Name causes `testExportConnection` to fail before connection
validation. Update `handleTest` in `s3-export-integration.tsx` to also gate on
the `name` field, or change `testMutation`/`testExportConnection` to accept
config-only input. Use the existing `form.getValues()`, `testMutation.mutate`,
and `zCreateS3ExportIntegration` flow to locate the fix.
Comment on lines
+33
to
+41
| const LAG_SECONDS = Number.parseInt(process.env.EXPORT_LAG_SECONDS || '60', 10); | ||
| // Max rows per object/batch and max batches drained per (project, integration) | ||
| // per run. A backlog drains over subsequent ticks rather than in one giant pass. | ||
| const BATCH_SIZE = Number.parseInt(process.env.EXPORT_BATCH_SIZE || '50000', 10); | ||
| const MAX_BATCHES_PER_RUN = Number.parseInt( | ||
| process.env.EXPORT_MAX_BATCHES_PER_RUN || '20', | ||
| 10 | ||
| ); | ||
| const CONCURRENCY = Number.parseInt(process.env.EXPORT_CONCURRENCY || '4', 10); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard env-driven numeric config against NaN.
Number.parseInt returns NaN for malformed values (e.g. EXPORT_CONCURRENCY=abc); the || '...' fallback only covers empty strings, not garbage. A NaN CONCURRENCY is especially dangerous: in runWithConcurrency, Math.min(NaN, queue.length) → NaN → Array.from({ length: NaN }) yields zero workers, so the job silently exports nothing. A NaN BATCH_SIZE/LAG_SECONDS also breaks the query.
🛡️ Proposed fix: validated numeric env parsing
-const LAG_SECONDS = Number.parseInt(process.env.EXPORT_LAG_SECONDS || '60', 10);
+const numEnv = (val: string | undefined, fallback: number): number => {
+ const n = Number.parseInt(val ?? '', 10);
+ return Number.isFinite(n) && n > 0 ? n : fallback;
+};
+const LAG_SECONDS = numEnv(process.env.EXPORT_LAG_SECONDS, 60);
-const BATCH_SIZE = Number.parseInt(process.env.EXPORT_BATCH_SIZE || '50000', 10);
-const MAX_BATCHES_PER_RUN = Number.parseInt(
- process.env.EXPORT_MAX_BATCHES_PER_RUN || '20',
- 10
-);
-const CONCURRENCY = Number.parseInt(process.env.EXPORT_CONCURRENCY || '4', 10);
+const BATCH_SIZE = numEnv(process.env.EXPORT_BATCH_SIZE, 50000);
+const MAX_BATCHES_PER_RUN = numEnv(process.env.EXPORT_MAX_BATCHES_PER_RUN, 20);
+const CONCURRENCY = numEnv(process.env.EXPORT_CONCURRENCY, 4);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const LAG_SECONDS = Number.parseInt(process.env.EXPORT_LAG_SECONDS || '60', 10); | |
| // Max rows per object/batch and max batches drained per (project, integration) | |
| // per run. A backlog drains over subsequent ticks rather than in one giant pass. | |
| const BATCH_SIZE = Number.parseInt(process.env.EXPORT_BATCH_SIZE || '50000', 10); | |
| const MAX_BATCHES_PER_RUN = Number.parseInt( | |
| process.env.EXPORT_MAX_BATCHES_PER_RUN || '20', | |
| 10 | |
| ); | |
| const CONCURRENCY = Number.parseInt(process.env.EXPORT_CONCURRENCY || '4', 10); | |
| const numEnv = (val: string | undefined, fallback: number): number => { | |
| const n = Number.parseInt(val ?? '', 10); | |
| return Number.isFinite(n) && n > 0 ? n : fallback; | |
| }; | |
| const LAG_SECONDS = numEnv(process.env.EXPORT_LAG_SECONDS, 60); | |
| // Max rows per object/batch and max batches drained per (project, integration) | |
| // per run. A backlog drains over subsequent ticks rather than in one giant pass. | |
| const BATCH_SIZE = numEnv(process.env.EXPORT_BATCH_SIZE, 50000); | |
| const MAX_BATCHES_PER_RUN = numEnv(process.env.EXPORT_MAX_BATCHES_PER_RUN, 20); | |
| const CONCURRENCY = numEnv(process.env.EXPORT_CONCURRENCY, 4); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/worker/src/jobs/cron.flush-exports.ts` around lines 33 - 41, The
env-driven numeric constants in cron.flush-exports need validation because
Number.parseInt can produce NaN for malformed values, which can silently break
runWithConcurrency and the export query. Update the parsing logic for
LAG_SECONDS, BATCH_SIZE, MAX_BATCHES_PER_RUN, and CONCURRENCY to use a validated
numeric helper (or equivalent fallback) that rejects NaN and falls back to safe
defaults, then keep the existing symbols so the fix is localized and easy to
find.
Comment on lines
+247
to
+269
| async function loadCursor( | ||
| projectId: string, | ||
| integrationId: string | ||
| ): Promise<Cursor> { | ||
| const existing = await db.exportWatermark.findUnique({ | ||
| where: { projectId_integrationId: { projectId, integrationId } }, | ||
| }); | ||
| if (existing) { | ||
| return { | ||
| insertedAt: formatCh(existing.lastInsertedAt), | ||
| eventId: existing.lastEventId, | ||
| }; | ||
| } | ||
|
|
||
| // First run for this pair: start from now, so connecting an export doesn't | ||
| // dump the project's entire history. Historical backfill is a separate, | ||
| // explicit operation (reset the watermark). | ||
| const now = new Date(); | ||
| await db.exportWatermark.create({ | ||
| data: { projectId, integrationId, lastInsertedAt: now, lastEventId: NIL_UUID }, | ||
| }); | ||
| return { insertedAt: formatCh(now), eventId: NIL_UUID }; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
loadCursor has a create race; combined with overlapping cron runs it can duplicate work or throw.
findUnique followed by create is a TOCTOU: if the same (projectId, integrationId) first-run is processed concurrently (this cron fires every 60s with no job-level concurrency guard, so a run exceeding the interval overlaps the next), both branches reach create and the second hits a unique-constraint error. More broadly, overlapping runs read the same watermark and re-export the same window. The at-least-once design tolerates duplicate uploads, but the create collision will surface as an unhandled error per pair. Prefer an upsert for the first-run path and consider a concurrency=1 guard / advisory lock on the job.
🛡️ Proposed fix for the first-run race
- const now = new Date();
- await db.exportWatermark.create({
- data: { projectId, integrationId, lastInsertedAt: now, lastEventId: NIL_UUID },
- });
- return { insertedAt: formatCh(now), eventId: NIL_UUID };
+ const now = new Date();
+ const created = await db.exportWatermark.upsert({
+ where: { projectId_integrationId: { projectId, integrationId } },
+ create: { projectId, integrationId, lastInsertedAt: now, lastEventId: NIL_UUID },
+ update: {},
+ });
+ return {
+ insertedAt: formatCh(created.lastInsertedAt),
+ eventId: created.lastEventId,
+ };Consider setting the worker concurrency for this job to 1 (or a Redis lock keyed per pair) so a slow run cannot overlap the next minute's trigger.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function loadCursor( | |
| projectId: string, | |
| integrationId: string | |
| ): Promise<Cursor> { | |
| const existing = await db.exportWatermark.findUnique({ | |
| where: { projectId_integrationId: { projectId, integrationId } }, | |
| }); | |
| if (existing) { | |
| return { | |
| insertedAt: formatCh(existing.lastInsertedAt), | |
| eventId: existing.lastEventId, | |
| }; | |
| } | |
| // First run for this pair: start from now, so connecting an export doesn't | |
| // dump the project's entire history. Historical backfill is a separate, | |
| // explicit operation (reset the watermark). | |
| const now = new Date(); | |
| await db.exportWatermark.create({ | |
| data: { projectId, integrationId, lastInsertedAt: now, lastEventId: NIL_UUID }, | |
| }); | |
| return { insertedAt: formatCh(now), eventId: NIL_UUID }; | |
| } | |
| async function loadCursor( | |
| projectId: string, | |
| integrationId: string | |
| ): Promise<Cursor> { | |
| const existing = await db.exportWatermark.findUnique({ | |
| where: { projectId_integrationId: { projectId, integrationId } }, | |
| }); | |
| if (existing) { | |
| return { | |
| insertedAt: formatCh(existing.lastInsertedAt), | |
| eventId: existing.lastEventId, | |
| }; | |
| } | |
| // First run for this pair: start from now, so connecting an export doesn't | |
| // dump the project's entire history. Historical backfill is a separate, | |
| // explicit operation (reset the watermark). | |
| const now = new Date(); | |
| const created = await db.exportWatermark.upsert({ | |
| where: { projectId_integrationId: { projectId, integrationId } }, | |
| create: { projectId, integrationId, lastInsertedAt: now, lastEventId: NIL_UUID }, | |
| update: {}, | |
| }); | |
| return { | |
| insertedAt: formatCh(created.lastInsertedAt), | |
| eventId: created.lastEventId, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/worker/src/jobs/cron.flush-exports.ts` around lines 247 - 269, The
first-run branch in loadCursor is racy because it does a findUnique followed by
create, so overlapping cron executions can hit the same (projectId,
integrationId) pair and throw on the unique constraint. Replace that TOCTOU flow
with a single atomic upsert on exportWatermark for the existing/new watermark
path, keeping the existing Cursor return shape from loadCursor. Also add a
job-level concurrency guard for cron.flush-exports so one run cannot overlap the
next minute’s trigger and reprocess the same watermark window.
| bucket: z.string().min(1, 'Bucket name is required'), | ||
| prefix: z.string().default('openpanel-exports'), | ||
| region: z.string().min(1, 'Region is required'), | ||
| endpoint: z.string().url().optional(), // For R2, MinIO, etc. |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Guard custom S3 endpoints against SSRF.
endpoint is tenant-controlled and will drive server-side object-store requests. Accepting any URL allows private/link-local/internal hosts; restrict to HTTPS public endpoints or an explicit allowlist, and re-check resolved addresses in the adapter before connecting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/validation/src/integrations.ts` at line 68, The custom S3 endpoint
validation in integrations should be tightened because endpoint is
tenant-controlled and can be used for server-side requests. Update the zod
validation around endpoint to only accept HTTPS public endpoints or an explicit
allowlist, and make sure the object-store adapter re-checks the resolved host/IP
before connecting. Use the endpoint schema and the downstream S3/adapter
connection path to locate the fix.
Comment on lines
+111
to
+112
| // Service account credentials (JSON key as string) | ||
| serviceAccountKey: z.string().min(1, 'Service account key is required'), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== integrations.ts relevant section =="
nl -ba packages/validation/src/integrations.ts | sed -n '90,140p'
echo
echo "== serviceAccountKey references =="
rg -n "serviceAccountKey" -S .
echo
echo "== json parse / service account validation references =="
rg -n "client_email|private_key|project_id|service account|service-account|JSON key" packages . -SRepository: Openpanel-dev/openpanel
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== integrations.ts relevant section =="
nl -ba packages/validation/src/integrations.ts | sed -n '90,140p'
echo
echo "== serviceAccountKey references =="
rg -n "serviceAccountKey" -S .
echo
echo "== json parse / service account validation references =="
rg -n "client_email|private_key|project_id|service account|service-account|JSON key" packages . -SRepository: Openpanel-dev/openpanel
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== integrations.ts relevant section =="
nl -ba packages/validation/src/integrations.ts | sed -n '90,140p'
echo
echo "== serviceAccountKey references =="
rg -n "serviceAccountKey" -S .
echo
echo "== json parse / service account validation references =="
rg -n "client_email|private_key|project_id|service account|service-account|JSON key" packages . -SRepository: Openpanel-dev/openpanel
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== integrations.ts relevant section =="
sed -n '90,140p' packages/validation/src/integrations.ts | cat -n
echo
echo "== serviceAccountKey references =="
rg -n "serviceAccountKey" -S .
echo
echo "== json parse / service account validation references =="
rg -n "client_email|private_key|project_id|service account|service-account|JSON key" packages . -SRepository: Openpanel-dev/openpanel
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== integrations.ts relevant section =="
sed -n '90,140p' packages/validation/src/integrations.ts | cat -n
echo
echo "== serviceAccountKey references =="
rg -n "serviceAccountKey" -S .
echo
echo "== json parse / service account validation references =="
rg -n "client_email|private_key|project_id|service account|service-account|JSON key" packages . -SRepository: Openpanel-dev/openpanel
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("packages/validation/src/integrations.ts")
text = p.read_text()
lines = text.splitlines()
for i in range(95, 141):
if i <= len(lines):
print(f"{i:4d}: {lines[i-1]}")
PY
echo
echo "== serviceAccountKey references =="
python3 - <<'PY'
from pathlib import Path
for path in Path(".").rglob("*"):
if path.is_file() and path.suffix in {".ts",".tsx",".js",".jsx",".json",".md"}:
try:
text = path.read_text(errors="ignore")
except Exception:
continue
if "serviceAccountKey" in text:
for i, line in enumerate(text.splitlines(), 1):
if "serviceAccountKey" in line:
print(f"{path}:{i}:{line}")
PY
echo
echo "== json parse / service account validation references =="
python3 - <<'PY'
from pathlib import Path
terms = ["client_email", "private_key", "project_id", "service account", "service-account", "JSON key"]
for path in Path(".").rglob("*"):
if path.is_file() and path.suffix in {".ts",".tsx",".js",".jsx",".json",".md"}:
try:
text = path.read_text(errors="ignore")
except Exception:
continue
if any(term in text for term in terms):
for i, line in enumerate(text.splitlines(), 1):
if any(term in line for term in terms):
print(f"{path}:{i}:{line}")
PYRepository: Openpanel-dev/openpanel
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("packages/validation/src/integrations.ts")
text = p.read_text()
lines = text.splitlines()
for i in range(95, 141):
if i <= len(lines):
print(f"{i:4d}: {lines[i-1]}")
PY
echo
echo "== serviceAccountKey references =="
python3 - <<'PY'
from pathlib import Path
for path in Path(".").rglob("*"):
if path.is_file() and path.suffix in {".ts",".tsx",".js",".jsx",".json",".md"}:
try:
text = path.read_text(errors="ignore")
except Exception:
continue
if "serviceAccountKey" in text:
for i, line in enumerate(text.splitlines(), 1):
if "serviceAccountKey" in line:
print(f"{path}:{i}:{line}")
PY
echo
echo "== json parse / service account validation references =="
python3 - <<'PY'
from pathlib import Path
terms = ["client_email", "private_key", "project_id", "service account", "service-account", "JSON key"]
for path in Path(".").rglob("*"):
if path.is_file() and path.suffix in {".ts",".tsx",".js",".jsx",".json",".md"}:
try:
text = path.read_text(errors="ignore")
except Exception:
continue
if any(term in text for term in terms):
for i, line in enumerate(text.splitlines(), 1):
if any(term in line for term in terms):
print(f"{path}:{i}:{line}")
PYRepository: Openpanel-dev/openpanel
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== gcs-adapter.ts relevant section =="
python3 - <<'PY'
from pathlib import Path
p = Path("packages/integrations/src/object-store/gcs-adapter.ts")
lines = p.read_text().splitlines()
for i in range(1, min(len(lines), 120) + 1):
print(f"{i:4d}: {lines[i-1]}")
PY
echo
echo "== registry.ts relevant section =="
python3 - <<'PY'
from pathlib import Path
p = Path("packages/integrations/src/registry.ts")
lines = p.read_text().splitlines()
for i in range(145, 185):
if i <= len(lines):
print(f"{i:4d}: {lines[i-1]}")
PY
echo
echo "== GCS credential validation references =="
rg -n "client_email|private_key|project_id|JSON.parse\\(this\\.config\\.serviceAccountKey\\)|Invalid service account key JSON" packages/integrations packages/validation apps/start -SRepository: Openpanel-dev/openpanel
Length of output: 6107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== gcs-adapter.ts relevant section =="
python3 - <<'PY'
from pathlib import Path
p = Path("packages/integrations/src/object-store/gcs-adapter.ts")
lines = p.read_text().splitlines()
for i in range(1, min(len(lines), 120) + 1):
print(f"{i:4d}: {lines[i-1]}")
PY
echo
echo "== registry.ts relevant section =="
python3 - <<'PY'
from pathlib import Path
p = Path("packages/integrations/src/registry.ts")
lines = p.read_text().splitlines()
for i in range(145, 185):
if i <= len(lines):
print(f"{i:4d}: {lines[i-1]}")
PY
echo
echo "== GCS credential validation references =="
rg -n "client_email|private_key|project_id|JSON.parse\\(this\\.config\\.serviceAccountKey\\)|Invalid service account key JSON" packages/integrations packages/validation apps/start -SRepository: Openpanel-dev/openpanel
Length of output: 6107
Validate serviceAccountKey as parsed service-account JSON.
z.string().min(1) still accepts malformed payloads that only fail later in packages/integrations/src/object-store/gcs-adapter.ts when JSON.parse runs; require the expected service-account fields here, including project_id, client_email, and private_key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/validation/src/integrations.ts` around lines 111 - 112, The
serviceAccountKey schema in integrations validation is too permissive because it
only checks for a non-empty string, so update the validator to require parsed
service-account JSON with the expected fields. In the integrations schema for
serviceAccountKey, add validation that ensures the value is valid JSON and
contains required keys like project_id, client_email, and private_key, so
malformed credentials are rejected before reaching gcs-adapter and its
JSON.parse path.
Integrations became project-scoped, so the org-level /$organizationId/integrations/installed route was removed. The slack callback's projectId-absent fallback still pointed there, 404-ing older in-flight installs (metadata without projectId). Fall back to the org landing page instead. Claude-Session: https://claude.ai/code/session_01AWucrxmRqWqgWBvr8cC5cj
Security-review findings (all verified against current code): - Cross-project update (integration.ts): the generic upsert and the Slack create/update authorized against the client-supplied input.projectId, so a user with access to one project could update another project's integration in the same org. Updates now load the existing row and authorize against ITS scope via assertIntegrationAccess (project access, or org access for legacy org-wide rows). - Webhook SSRF (registry.ts): user-configured webhook URLs were fetched directly. Now guarded by assertSafeUrl before dispatch. - S3 custom-endpoint SSRF (s3-adapter.ts): a tenant-controlled endpoint could point at internal services. The resolved host is now SSRF-checked in getClient() before connecting (covers upload + testConnection). assertSafeUrl is a new shared helper in @openpanel/common/server: rejects non-http(s) schemes and hosts resolving to loopback/private/CGNAT/link-local (incl. 169.254.169.254 metadata). Skipped on SELF_HOSTED, where the single operator already controls the network and internal targets are legitimate (blocking them would regress existing behavior). Adds unit tests. Skipped: tightening the S3 endpoint zod to https-only — it would break legitimate self-hosted http MinIO/internal stores, and the runtime guard (scheme + resolved-IP, cloud-gated) is the actual SSRF protection. Claude-Session: https://claude.ai/code/session_01AWucrxmRqWqgWBvr8cC5cj
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/common/server/ssrf.ts`:
- Around line 66-92: The webhook delivery flow still has a redirect-based SSRF
gap because assertSafeUrl only validates the initial URL while fetch follows
redirects automatically. Update the webhook request path to either disable
automatic redirects in fetch(config.url, ...) or explicitly validate each
redirect target before following it, using assertSafeUrl and the surrounding
webhook delivery logic to keep private/metadata targets blocked.
- Around line 70-73: The SSRF bypass in assertSafeUrl is currently using raw
truthiness for SELF_HOSTED, which can incorrectly disable the guard for values
like false or 0. Update the SELF_HOSTED check in assertSafeUrl to only return
early for explicit enabled values, preserving support for both
SELF_HOSTED='true' and SELF_HOSTED='1' while keeping the URL safety validation
active for all other values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24dc1166-860d-452e-a7a8-4c66dbfa6131
📒 Files selected for processing (7)
apps/api/src/controllers/webhook.controller.tspackages/common/server/index.tspackages/common/server/ssrf.test.tspackages/common/server/ssrf.tspackages/integrations/src/object-store/s3-adapter.tspackages/integrations/src/registry.tspackages/trpc/src/routers/integration.ts
✅ Files skipped from review due to trivial changes (1)
- packages/common/server/ssrf.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/common/server/index.ts
- packages/integrations/src/registry.ts
- packages/integrations/src/object-store/s3-adapter.ts
- packages/trpc/src/routers/integration.ts
Comment on lines
+66
to
+92
| * Note: DNS is resolved here and again by the client, so a deliberate DNS-rebind | ||
| * between the two is not covered; this stops the common cases (literal | ||
| * private/metadata URLs and hostnames pointing at internal IPs). | ||
| */ | ||
| export async function assertSafeUrl(rawUrl: string): Promise<void> { | ||
| if (process.env.SELF_HOSTED) { | ||
| return; | ||
| } | ||
|
|
||
| let url: URL; | ||
| try { | ||
| url = new URL(rawUrl); | ||
| } catch { | ||
| throw new Error('Invalid URL'); | ||
| } | ||
| if (url.protocol !== 'http:' && url.protocol !== 'https:') { | ||
| throw new Error('URL must use http or https'); | ||
| } | ||
|
|
||
| const resolved = await lookup(url.hostname, { all: true }); | ||
| if ( | ||
| resolved.length === 0 || | ||
| resolved.some((r) => isBlockedAddress(r.address)) | ||
| ) { | ||
| throw new Error('URL resolves to a disallowed address'); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all guarded outbound clients define an explicit redirect policy or revalidate redirects.
rg -n -C6 '\bassertSafeUrl\s*\(' packages apps
rg -n -C8 '\bfetch\s*\(' packages appsRepository: Openpanel-dev/openpanel
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/integrations/src/registry.ts
printf '\n--- ssrf.ts ---\n'
sed -n '1,220p' packages/common/server/ssrf.ts
printf '\n--- redirect policies ---\n'
rg -n -C2 "redirect:\s*'manual'|redirect:\s*'error'|redirect:\s*'follow'" packages/integrations packages/common packages/sdks appsRepository: Openpanel-dev/openpanel
Length of output: 10296
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/integrations/src/registry.ts
printf '\n--- ssrf.ts ---\n'
sed -n '1,220p' packages/common/server/ssrf.ts
printf '\n--- redirect policies ---\n'
rg -n -C2 "redirect:\s*'manual'|redirect:\s*'error'|redirect:\s*'follow'" packages/integrations packages/common packages/sdks appsRepository: Openpanel-dev/openpanel
Length of output: 10296
Close the redirect-based SSRF gap in webhook delivery.
assertSafeUrl(config.url) only checks the initial URL, but fetch(config.url, ...) follows redirects by default. A public webhook URL can still 30x to a private/metadata host after validation. Set redirect: 'manual' or revalidate each redirect target before following it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/common/server/ssrf.ts` around lines 66 - 92, The webhook delivery
flow still has a redirect-based SSRF gap because assertSafeUrl only validates
the initial URL while fetch follows redirects automatically. Update the webhook
request path to either disable automatic redirects in fetch(config.url, ...) or
explicitly validate each redirect target before following it, using
assertSafeUrl and the surrounding webhook delivery logic to keep
private/metadata targets blocked.
Comment on lines
+70
to
+73
| export async function assertSafeUrl(rawUrl: string): Promise<void> { | ||
| if (process.env.SELF_HOSTED) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 '\bSELF_HOSTED\b' .env.example packages appsRepository: Openpanel-dev/openpanel
Length of output: 11770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## ssrf.ts\n'
sed -n '1,140p' packages/common/server/ssrf.ts
printf '\n## ssrf.test.ts\n'
sed -n '1,140p' packages/common/server/ssrf.test.ts
printf '\n## startup env check\n'
sed -n '1,80p' apps/start/src/server/get-envs.ts
printf '\n## Dockerfile env\n'
sed -n '1,40p' apps/start/Dockerfile
printf '\n## docs snippet\n'
sed -n '540,556p' apps/public/content/docs/self-hosting/environment-variables.mdxRepository: Openpanel-dev/openpanel
Length of output: 7368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## other SELF_HOSTED contracts\n'
rg -n -C1 "SELF_HOSTED === 'true'|SELF_HOSTED !== undefined|SELF_HOSTED=1|SELF_HOSTED=true|SELF_HOSTED=false" \
packages apps | sed -n '1,220p'Repository: Openpanel-dev/openpanel
Length of output: 3758
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## get-envs.ts\n'
sed -n '1,40p' apps/start/src/server/get-envs.ts
printf '\n## code-migrations/helpers.ts\n'
sed -n '1,60p' packages/db/code-migrations/helpers.ts
printf '\n## clickhouse client self-hosted gate\n'
sed -n '70,95p' packages/db/src/clickhouse/client.tsRepository: Openpanel-dev/openpanel
Length of output: 2455
Gate the SSRF bypass on explicit self-hosted values packages/common/server/ssrf.ts:71 should treat only SELF_HOSTED='true' or SELF_HOSTED='1' as enabled; raw truthiness lets false/0 skip the guard, and === 'true' alone would break the existing SELF_HOSTED=1 self-hosted setup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/common/server/ssrf.ts` around lines 70 - 73, The SSRF bypass in
assertSafeUrl is currently using raw truthiness for SELF_HOSTED, which can
incorrectly disable the guard for values like false or 0. Update the SELF_HOSTED
check in assertSafeUrl to only return early for explicit enabled values,
preserving support for both SELF_HOSTED='true' and SELF_HOSTED='1' while keeping
the URL safety validation active for all other values.
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
1 participant
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 by CodeRabbit
New Features
Bug Fixes
Chores