PATCH resource handler fires client-selected before_<action> hooks before any per-action permission check

Status

Repository / Component

Plain-English Description

When a user sends a file-management request, the server reads the requested action straight from the URL and runs the matching admin-configured 'before' command first, before checking whether that user is even allowed to do it. A user can trigger commands they were never permitted to run.

Description of the Underlying Issue

resourcePatchHandler reads the action verbatim from client input: action := r.URL.Query().Get("action") (http/resource.go:217). That value is passed unchanged as the evt argument into d.RunHook (http/resource.go:257-259). In runner.RunHook the selected before_<evt> command is chosen from r.Commands["before_"+evt] and executed with a TRIGGER=evt environment variable set (runner/runner.go:26, 74, 95) before the wrapped work function runs. The only action validation lives inside patchAction (http/resource.go:341-377), which defaults unsupported actions to an error and enforces per-action Perm bits — but that code executes as the RunHook callback, i.e. AFTER the before_<evt> hook has already fired. The checker d.Check (http/data.go:29-48) evaluates only path Rules and HideDotfiles, not any Perm bit. As a result the client fully controls which before_<action> hook fires (before_delete, before_upload, before_save, before_copy, before_rename, or any configured name) and the $TRIGGER/$FILE/$DESTINATION values, ahead of any authorization decision, and the hook still fires even for actions the handler will subsequently reject.

Potential Attack

On a deployment with server.EnableExec=true and one or more before_<action> hooks configured, an authenticated user (or any user who can reach PATCH /api/resources) sends PATCH /api/resources/<src>?action=delete&destination=<dst> (or action=upload/save/copy/rename). RunHook selects and runs the before_delete command with client-controlled FILE/DESTINATION/TRIGGER environment values before patchAction reaches — and denies at — its permission/validation check. The attacker thereby drives execution of admin-defined hook commands and controls the environment inputs to them, including for actions the PATCH path does not actually perform.

Outcomes of Potential Attack

The attacker causes the side effects of admin-configured before_<action> hook commands to run with attacker-chosen FILE/DESTINATION/TRIGGER values, without holding the corresponding per-action permission and without the underlying file operation ever executing. Impact depends on what the configured hook does (logging, notifications, outbound calls, file staging, external tooling). If any configured hook command is dispatched through a shell, the attacker-influenced FILE/DESTINATION values become a shell-metacharacter injection surface, amplifying this authorization-ordering flaw toward command injection. This is a trust-boundary/authorization-ordering violation; the defect alone does not grant arbitrary command choice because the command set is admin-defined.

Affected Scope

resourcePatchHandler action selector (http/resource.go:217,257-259) -> runner.RunHook (runner/runner.go:21-53)

Suggested Fix (plain english)

Decide the hook to run from a server-side allowlist derived from the validated action, and confirm the user is allowed to perform that action before running any 'before' hook.

Suggested Fix (detailed)

In resourcePatchHandler, validate the action against a fixed server-side allowlist and map it to a trusted evt before calling d.RunHook, rather than passing the raw query value through (http/resource.go:217,257-259). Hoist the per-action permission checks (Perm.Create/Perm.Rename/Perm.Delete/etc. currently inside patchAction at http/resource.go:341-377) out of the RunHook callback and enforce them before d.RunHook is invoked, mirroring the ordering used by DELETE/POST/PUT (http/resource.go:87,128,182). Reject unsupported actions before any hook fires. Verify by configuring a before_delete hook with EnableExec on and confirming that a PATCH with action=delete from a user lacking Perm.Delete is denied with the hook never executing, and that an unsupported action value never triggers any before_ hook. This record generalizes the copy/rename-specific DOK-100008 and DOK-100067 to the full client-controlled selector; keep them as related, not duplicate. Map the request to a server-side allowlisted evt derived from the validated action, and enforce the per-action permission before d.RunHook.

Validation

Client-controlled hook-selector-before-authorization is confirmed in source and generalizes DOK-100008/67; gated on the EnableExec + configured-hook precondition, so possibly_valid.

first_opinion

Full Evidence

http/resource.go:217 action := r.URL.Query().Get('action') — client-controlled

http/resource.go:257-259 action passed verbatim as the evt arg to d.RunHook

runner/runner.go:26 selects r.Commands['before_'+evt]; runner.go:74/95 sets TRIGGER=evt env

http/data.go:29-48 d.Check evaluates only Rules + HideDotfiles, no Perm bit

http/resource.go:341-377 patchAction only validates action (default->error) INSIDE the RunHook callback, so before_<evt> already fired

Proven fact: The client fully controls which before_<action> hook fires (including before_delete/before_upload/before_save) and the $TRIGGER value, ahead of any per-action permission check; for unsupported actions the before hook still fires (patchAction errors only afterward)

Unvalidated fact: Impact requires server.EnableExec=true AND the specific before_<action> hook configured; the command is admin-defined, and shell metacharacter injection requires the configured command to invoke a shell

http/resource.go:217 — action := r.URL.Query().Get("action"); the hook selector is taken directly from client-controlled query input

http/resource.go:257-259 — action is passed verbatim as the evt argument into d.RunHook, wrapping patchAction in the callback fn

runner/runner.go:26 — RunHook selects r.Commands["before_"+evt]; runner.go:74/95 — sets TRIGGER=evt (and FILE/DESTINATION) env before running fn

runner/runner.go (RunHook body) — the before_<evt> command executes before the wrapped fn() callback

http/resource.go:341-377 — patchAction validates the action (default -> error) and enforces per-action Perm bits, but only inside the RunHook callback, so before_<evt> has already fired

http/data.go:29-48 — d.Check evaluates only path Rules and HideDotfiles; it applies no Perm bit at the checker layer

Proven fact: The before_<action> hook selector is taken verbatim from the client-controlled action query parameter and passed into RunHook before any per-action permission check runs.

Proven fact: The action validation and Perm.* checks live inside patchAction, which executes as the RunHook callback and therefore only after the before_<evt> command has already fired.

Proven fact: For unsupported/unauthorized actions the before hook still fires; patchAction errors only afterward.

Proven fact: d.Check enforces no Perm bit, so the checker layer does not compensate for the missing pre-hook authorization.

Unvalidated fact: Security impact requires server.EnableExec=true AND at least one relevant before_<action> hook configured; the command set is admin-defined, so this defect alone does not yield arbitrary command selection.

Unvalidated fact: Escalation to shell-metacharacter command injection requires that the configured hook command is executed through a shell and consumes the attacker-influenced FILE/DESTINATION values.

Unvalidated fact: The concrete severity depends entirely on the behavior of the specific admin-configured hook commands, which is deployment-dependent.

http/resource.go:217 action := r.URL.Query().Get("action")

http/resource.go:221,252 pre-RunHook gates are d.Check(src)/d.Check(dst) and (override-only) Perm.Modify — no per-action permission

http/resource.go:257-259 d.RunHook(func(){return patchAction(...)}, action, src, dst, d.user)

http/data.go:29-48 (*data).Check evaluates only Rules + HideDotfiles, no Perm.* bit

runner/runner.go:26-33 before_<evt> commands run before fn(); evt used directly as Commands["before_"+evt]

runner/runner.go:73-77,95 evt -> $TRIGGER env/os.Expand (client-controlled)

http/resource.go:341-377 patchAction validates action (switch default->error) and checks Perm.Create/Perm.Rename only inside the RunHook callback

contrast http/resource.go:87,128,182 DELETE/POST/PUT check permission before RunHook and use a server-hardcoded evt