PATCH copy/rename runs before_copy/before_rename hooks before the Perm.Create/Perm.Rename check, letting unpermitted users trigger hook commands

Status

Repository / Component

Plain-English Description

For copy and rename requests, the server runs the admin-configured 'before' hook command first and only afterward checks whether the user is allowed to copy or rename. A user lacking those permissions can still make the hook run with a filename and destination they control.

Description of the Underlying Issue

resourcePatchHandler (http/resource.go:213-262) invokes d.RunHook with the actual copy/rename work wrapped in a callback: d.RunHook(func(){ return patchAction(...) }, action, src, dst, d.user) (http/resource.go:257-259). In runner.RunHook (runner/runner.go:26-36) the before_<evt> command executes before fn() is invoked. The Perm.Create (copy) and Perm.Rename (rename) checks live inside patchAction (http/resource.go:344,350), i.e. inside that callback, so they run only after the before hook has already fired. This inverts the ordering used by DELETE/POST/PUT, which gate Perm.* before calling d.RunHook (http/resource.go:87,128,182). Consequently a user lacking Perm.Create/Perm.Rename can still cause the configured before_copy/before_rename hook to run with client-controlled FILE/DESTINATION environment values, provided the server has EnableExec on and such a hook configured (runner.Enabled gates execution). The subsequent copy/rename itself remains blocked by the permission check.

Potential Attack

With server.EnableExec=true and a before_copy (or before_rename) hook configured, an authenticated user who lacks Perm.Create sends PATCH /api/resources/<src>?action=copy&destination=<dst>. RunHook fires the before_copy command — carrying the client-chosen FILE (source path) and DESTINATION env values — before patchAction reaches its Perm.Create check and denies the copy. The user thereby triggers admin-defined hook side effects they were not authorized to invoke, with inputs they control.

Outcomes of Potential Attack

An unpermitted user can invoke the side effects of admin-configured before_copy/before_rename hook commands (whatever those commands do — e.g. logging, notifications, external calls, file staging) with client-controlled FILE/DESTINATION values. This is a privilege/authorization-ordering violation; it does not itself grant arbitrary command injection (the command is admin-defined) and the actual copy/rename stays blocked. Combined with a shell-invoking hook and attacker-influenced filenames it can amplify into the command-injection surface tracked by related findings.

Affected Scope

resourcePatchHandler (http/resource.go:213-262); Perm.Create/Perm.Rename checks live inside patchAction (resource.go:344,350) which runs as the RunHook callback

Suggested Fix (plain english)

Check that the user is allowed to copy or rename before running any 'before' hook, matching how delete/upload/save already work.

Suggested Fix (detailed)

Hoist the Perm.Create (copy) and Perm.Rename (rename) checks out of patchAction and into resourcePatchHandler before the d.RunHook call (http/resource.go:257-259), mirroring the ordering used by DELETE/POST/PUT at http/resource.go:87,128,182. Verify by configuring a before_copy hook with EnableExec on and confirming that a user without Perm.Create receives a permission denial with the hook never executing. See related DOK-100039 (client-controlled hook selector before authorization) and DOK-100067 for the broader ordering issue. Hoist the Perm.Create/Perm.Rename checks into resourcePatchHandler before d.RunHook, mirroring DELETE/POST/PUT.

Validation

The permission-ordering defect is definite in source; security impact is gated on the EnableExec + configured-hook deployment precondition, so possibly_valid.

first_opinion

Full Evidence

http/resource.go:257-259 d.RunHook(func(){return patchAction(...)}, action, src, dst, d.user) — the permission check is inside fn

runner/runner.go:26-36 before_<evt> commands execute before fn() is invoked

http/resource.go:344,350 patchAction checks Perm.Create (copy) / Perm.Rename (rename) only inside the callback

contrast http/resource.go:87,128,182 DELETE/POST/PUT gate Perm.* BEFORE d.RunHook

Proven fact: before_copy / before_rename hooks fire before the Create/Rename permission check, so an authenticated user lacking those permissions can still trigger the hook commands with client-controlled FILE/DESTINATION env

Unvalidated fact: Impact requires server.EnableExec=true AND before_copy/before_rename hooks configured (deployment config; runner.Enabled gates execution)

Unvalidated fact: The hook command itself is admin-defined (no arbitrary command injection), and the actual copy/rename remains blocked by the permission check after the hook

http/resource.go:257-259 — d.RunHook(func(){return patchAction(...)}, action, src, dst, d.user); the permission check is inside fn

runner/runner.go:26-36 — before_<evt> commands execute before fn() is invoked

http/resource.go:344,350 — patchAction checks Perm.Create (copy) / Perm.Rename (rename) only inside the callback

http/resource.go:87,128,182 — contrast: DELETE/POST/PUT gate Perm.* BEFORE d.RunHook

Proven fact: before_copy / before_rename hooks fire before the Create/Rename permission check, so an authenticated user lacking those permissions can still trigger the hook commands with client-controlled FILE/DESTINATION env.

Proven fact: The ordering differs from DELETE/POST/PUT, which enforce Perm.* prior to RunHook, confirming this is an inconsistency rather than intended design.

Unvalidated fact: Impact requires server.EnableExec=true AND before_copy/before_rename hooks configured (deployment config; runner.Enabled gates execution).

Unvalidated fact: The hook command itself is admin-defined (no arbitrary command injection from this defect alone), and the actual copy/rename remains blocked by the permission check after the hook.

http/resource.go:213-259 resourcePatchHandler performs no Perm.Create/Perm.Rename check before calling d.RunHook

http/resource.go:341-352 patchAction checks Perm.Create (copy) / Perm.Rename (rename) INSIDE the RunHook callback

runner/runner.go:26-36 RunHook runs all before_<evt> commands before invoking the callback fn

runner/runner.go:68-97 hooks receive user-controlled FILE/DESTINATION env

contrast: http/resource.go:87,128,182 DELETE/POST/PUT check permission before RunHook