feat(deploy): add deployment.one procedure and unify access checks (#5168) - #5452
fliptrigga13 wants to merge 2 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return; | ||
| } | ||
|
|
||
| await checkPermission(ctx, permission); |
There was a problem hiding this comment.
Preview, backup, and volume-backup deployment rows contain only previewDeploymentId, backupId, or volumeBackupId, but findDeploymentById does not load those parent relations. As a result, this helper cannot validate a service, server, or schedule organization and grants access after only checking the user's role in their active organization. A user with deployment-read permission who obtains another organization's deployment ID can retrieve that deployment through deployment.one. Resolve and validate ownership through each deployment type's parent resource before granting access.
How this was verified: The affected creation paths omit every resource field checked here, and checkPermission validates the active-organization role without checking ownership of the requested deployment.
| columns: { composeId: true, appName: true, name: true, serverId: true }, | ||
| }, | ||
| schedule: true, | ||
| rollback: true, |
There was a problem hiding this comment.
Loading the complete rollback relation makes deployment.one return rollback.fullContext without filtering it. Rollback creation stores registry objects fetched with credentials in this JSON, including registry passwords, so any client with deployment-read access now receives those credentials. Return only the non-sensitive rollback fields or redact the credential-bearing context.
How this was verified: Rollback creation stores credential-bearing registry objects in fullContext, the registry schema includes a password field, and the new procedure returns the eagerly loaded deployment unchanged.
| const checkDeploymentAccess = async ( | ||
| ctx: Context, | ||
| deployment: MockDeployment, | ||
| permission: { deployment: ("read" | "cancel")[] } = { deployment: ["read"] }, | ||
| deps: { | ||
| checkServicePermissionAndAccess: ( | ||
| ctx: Context, | ||
| serviceId: string, | ||
| permission: any, | ||
| ) => Promise<void>; | ||
| checkPermission: (ctx: Context, permission: any) => Promise<void>; | ||
| findServerById: (serverId: string) => Promise<{ serverId: string; organizationId: string }>; | ||
| }, | ||
| ) => { | ||
| if (!ctx.session || !ctx.user) { | ||
| throw new TRPCError({ | ||
| code: "UNAUTHORIZED", | ||
| message: "UNAUTHORIZED", | ||
| }); | ||
| } | ||
|
|
||
| const serviceId = | ||
| deployment.applicationId || | ||
| deployment.composeId || | ||
| deployment.schedule?.applicationId || | ||
| deployment.schedule?.composeId; | ||
| if (serviceId) { | ||
| await deps.checkServicePermissionAndAccess(ctx, serviceId, permission); | ||
| return; | ||
| } | ||
|
|
||
| await deps.checkPermission(ctx, permission); | ||
|
|
||
| const serverId = | ||
| deployment.serverId || | ||
| deployment.schedule?.serverId || | ||
| deployment.application?.serverId || | ||
| deployment.compose?.serverId; | ||
| if (serverId) { | ||
| const targetServer = await deps.findServerById(serverId); | ||
| if (targetServer.organizationId !== ctx.session.activeOrganizationId) { | ||
| throw new TRPCError({ | ||
| code: "UNAUTHORIZED", | ||
| message: "You don't have access to this deployment.", | ||
| }); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (deployment.schedule?.organizationId) { | ||
| if ( | ||
| deployment.schedule.organizationId !== ctx.session.activeOrganizationId | ||
| ) { | ||
| throw new TRPCError({ | ||
| code: "UNAUTHORIZED", | ||
| message: "You don't have access to this deployment.", | ||
| }); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const handleFindOneDeployment = async ( | ||
| input: { deploymentId: string }, | ||
| ctx: Context, | ||
| deps: { | ||
| findDeploymentById: (id: string) => Promise<MockDeployment | null>; | ||
| checkServicePermissionAndAccess: ( | ||
| ctx: Context, | ||
| serviceId: string, | ||
| permission: any, | ||
| ) => Promise<void>; | ||
| checkPermission: (ctx: Context, permission: any) => Promise<void>; | ||
| findServerById: (serverId: string) => Promise<{ serverId: string; organizationId: string }>; | ||
| }, | ||
| ) => { | ||
| const deployment = await deps.findDeploymentById(input.deploymentId); | ||
| if (!deployment) { | ||
| throw new TRPCError({ | ||
| code: "NOT_FOUND", | ||
| message: "Deployment not found", | ||
| }); | ||
| } | ||
| await checkDeploymentAccess(ctx, deployment, { deployment: ["read"] }, deps); | ||
| return deployment; | ||
| }; |
There was a problem hiding this comment.
This test defines its own TRPCError, access helper, and procedure handler instead of invoking deploymentRouter.one. Its assertions therefore do not exercise the production procedure, middleware, permission services, or lookup behavior. The copy has already diverged by implementing session and null-result checks in different layers, so future production authorization regressions can leave this suite green. Exercise the real router through a caller with mocked dependencies, or extract and directly test the production helper.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…back context, fail closed (Dokploy#5168)
Update on Greptile Review & Access Hardening (commit \315b670a8)All feedback from the automated security review on commit \7c812d6bb\ has been fully addressed in commit \315b670a8:
Ready for maintainer review @Siumauricio! |
|
Closing this PR as part of a bulk cleanup of automated/bot-generated submissions from this account. These PRs were opened in a tight, non-interactive burst (most within a ~2 hour window) with no accompanying human review, discussion, or testing evidence, so we're not able to verify the correctness or safety of the changes as submitted. If any of the underlying issue(s) this PR references are still valid, please feel free to open a new PR with a human review process behind it, and we're happy to take another look. |
2 participants
Summary
Fixes #5168
Problem
In Dokploy, schedule executions and jobs are recorded in the
deploymenttable with a nullablescheduleId, and manual runs return{ status, deploymentId, logPath }. However, there was no API procedure to query a single deployment by itsdeploymentId. To inspect the status or details of a specific execution, external pipelines and callers had to calldeployment.allByType?id=<scheduleId>&type=scheduleand filter the entire deployment history client-side. Additionally, authorization checks acrosscancel,remove, andreadLogswere repeated and fragmented.Solution
deployment.oneQuery Procedure: Addedoneprotected procedure toapps/dokploy/server/api/routers/deployment.tstaking{ deploymentId: string }.checkDeploymentAccess, guaranteeing consistent authorization checks acrossone,cancel,remove, andreadLogs:applicationorcomposeservice.ctx.session.activeOrganizationIdwhen linked to a remote server.apiFindOneDeploymentschema inpackages/server/src/db/schema/deployment.ts.findDeploymentByIdinpackages/server/src/services/deployment.tsto includerollback: true.createCallerFactoryinapps/dokploy/server/api/trpc.ts.apps/dokploy/__test__/deploy/deployment-one.test.ts(9/9 passing tests) verifying authorized read,NOT_FOUNDhandling, unauthorized service protection, server organization boundaries, schedule organization boundaries, and rollback attachment.This PR is not safe to merge until cross-organization deployment access is closed and credential-bearing rollback context is removed from the API response.
Summary
Reviews (1) · Last reviewed commit: "feat(deploy): add deployment.one procedu..."