PATCH /api/resources action parameter unvalidated, firing unauthorized before_/after_ hooks (missing authorization/input allowlist)

Status

Repository / Component

Plain-English Description

The file-move endpoint accepts an 'action' name from the URL without checking it against a list of allowed actions. That name is used to fire configured shell hooks, so a user with no permissions can trigger hooks for operations they are not allowed to perform, such as a delete hook.

Description of the Underlying Issue

resourcePatchHandler (http/resource.go:213-262) enforces no top-level Perm.* gate for the caller — only scope checks d.Check(src)/d.Check(dst) (:221) and checkParent (:231); the sole permission check (Perm.Modify, :252) fires only when override==true. The action is read verbatim at :217 (r.URL.Query().Get("action")) with no allowlist and passed as the event name to d.RunHook(fn, action, src, dst, d.user) (:257-259). RunHook (runner/runner.go:25-36) executes every configured before_<evt> command BEFORE calling fn(), and after_<evt> afterward (:41-50). patchAction validates the action only in its default case (:375 ErrInvalidRequestParams) and the copy/rename Perm checks (:344,:350) are all inside fn() — i.e. after the before-hook already ran. The broken invariant: the event selector that drives privileged hook execution must be validated and authorized before any hook fires.

Potential Attack

An authenticated user whose permissions are all false sends PATCH /api/resources/a.txt?destination=/nonexistent-b.txt&action=delete. The request passes d.Check and checkParent (Rel yields ../nonexistent-b.txt), skips the 409/override branches (dst nonexistent, override/rename false), and reaches RunHook, which fires the configured before_delete command before patchAction returns ErrInvalidRequestParams. The same works for before_upload/before_save/before_copy/before_rename or any custom before_<key>, with FILE/DESTINATION env confined to the caller's own scope.

Outcomes of Potential Attack

The attacker causes execution of before_/after_ hook commands for events they are not authorized to perform and that the endpoint never actually carries out — notably before_delete, which the real DELETE path gates behind Perm.Delete. The concrete blast radius is whatever the admin-configured hook does; the security delta is the bypass of the permission gate that normally guards that event.

Affected Scope

http/resource.go:213-262 resourcePatchHandler (PATCH /api/resources); the unvalidated 'action' query parameter is passed verbatim as the RunHook event name (runner/runner.go:21-53), firing before_<event>/after_<event> shell hooks for events the caller is not authorized to and does not perform. Protected asset: hook events other than copy/rename (e.g. before_delete, which the DELETE handler gates behind Perm.Delete).

Suggested Fix (plain english)

Validate the 'action' value against an allowlist of permitted actions before doing anything with it, and check the caller's permission for that action before running any hook.

Suggested Fix (detailed)

At the top of resourcePatchHandler (http/resource.go:214), validate action against the {copy, rename} allowlist and reject anything else BEFORE calling RunHook (:257). Additionally hoist the Perm.Create/Perm.Rename checks ahead of RunHook (the DOK-100067 fix) so no before-hook can run for an unauthorized or invalid operation. Verify by confirming a zero-permission user cannot trigger before_delete (or any non-copy/rename hook) via PATCH. At the top of resourcePatchHandler validate action against the {copy, rename} allowlist and reject anything else BEFORE calling RunHook; additionally hoist the Perm.Create/Perm.Rename checks ahead of RunHook (the DOK-100067 fix) so no before-hook can run for an unauthorized or invalid operation.

Validation

Code-level authorization/input-validation defect is fully proven (no top-level perm gate, unvalidated action reaches RunHook before patchAction's default-case rejection, DELETE's Perm.Delete gate bypassed). Distinct from DOK-100067 (which is the copy/rename ordering defect) — the root cause here is the missing action allowlist. possibly_valid because impact hinges on runtime config (EnableExec + a configured non-copy/rename hook) not settleable from source.

first_opinion

Full Evidence

http/resource.go:214-235 resourcePatchHandler applies no top-level Perm.* gate — only d.Check(src)&&d.Check(dst) (:221) and checkParent (:231); the only permission check (:252 Perm.Modify) fires solely when override==true

http/resource.go:217 action := r.URL.Query().Get("action") — taken verbatim, no allowlist

http/resource.go:257-259 d.RunHook(func(){patchAction(...)}, action, src, dst, d.user) — action is the evt argument

runner/runner.go:25-36 RunHook runs every r.Commands["before_"+evt] command BEFORE calling fn(); runner.go:41-50 runs after_<evt> after fn()

http/resource.go:341-377 patchAction validates action only in its default case (:375 ErrInvalidRequestParams), which executes inside fn() — after the before-hook already fired; copy/rename Perm checks (:344,:350) are likewise inside fn()

http/resource.go:85-116 resourceDeleteHandler gates the real delete behind Perm.Delete (:87) and then RunHook evt="delete" — the same before_delete a zero-perm user can fire via PATCH action=delete

http/http.go:65 PATCH /api/resources is registered via monkey(=handle) with only auth middleware (http/data.go:50-85), no route-level permission gate

http/data.go:63 Runner{Enabled: server.EnableExec} — hooks execute only when EnableExec is true

Proven fact: resourcePatchHandler enforces no permission check for the caller other than scope (d.Check) and checkParent; a user with all permissions false can reach RunHook.

Proven fact: 'action' is unvalidated at entry and reaches RunHook as the event selector; before_<action> runs before patchAction, and patchAction rejects an unknown action only afterward (inside fn).

Proven fact: A request PATCH /api/resources/a.txt?destination=/nonexistent-b.txt&action=delete passes d.Check and checkParent (Rel yields ../nonexistent-b.txt), skips the 409/override branches (dst nonexistent, override/rename false), and fires before_delete — an event the legitimate DELETE path gates behind Perm.Delete.

Proven fact: The same works for before_upload/before_save/before_copy/before_rename/any custom before_<key>, with FILE/DESTINATION env confined to the caller's own scope.

Unvalidated fact: Impact is contingent on runtime configuration not present in source: (a) server.EnableExec=true, and (b) the admin has configured at least one before_<event>/after_<event> command for an event other than copy/rename. With no such hook configured, the unauthorized event selector fires nothing and there is no security impact.

Unvalidated fact: What the fired hooks actually do (and thus the concrete blast radius) is entirely admin-defined and cannot be bounded from the filebrowser source.

http/resource.go:217 action := r.URL.Query().Get("action") — verbatim, no allowlist

runner/runner.go:25-36 RunHook runs every before_<evt> command BEFORE fn(); :41-50 runs after_<evt> after fn()

http/resource.go:341-377 patchAction validates action only in its default case (:375), inside fn() — after the before-hook already fired; copy/rename Perm checks (:344,:350) are also inside fn()

http/resource.go:85-116 resourceDeleteHandler gates the real delete behind Perm.Delete (:87) then RunHook evt="delete" — the same before_delete a zero-perm user can fire via PATCH action=delete

http/http.go:65 PATCH /api/resources registered with only auth middleware; http/data.go:63 Runner{Enabled: server.EnableExec} — hooks execute only when EnableExec is true

Proven fact: resourcePatchHandler enforces no caller permission check other than scope (d.Check) and checkParent; an all-permissions-false user can reach RunHook.

Proven fact: 'action' is unvalidated at entry and reaches RunHook as the event selector; before_<action> runs before patchAction, which rejects an unknown action only afterward (inside fn).

Proven fact: PATCH /api/resources/a.txt?destination=/nonexistent-b.txt&action=delete passes d.Check and checkParent, skips the 409/override branches, and fires before_delete — an event the legitimate DELETE path gates behind Perm.Delete.

Proven fact: The same works for before_upload/before_save/before_copy/before_rename/any custom before_<key>, with env confined to the caller's own scope.

Unvalidated fact: Impact is contingent on runtime configuration not in source: (a) server.EnableExec=true, and (b) the admin configured at least one before_/after_ command for an event other than copy/rename. With no such hook, the unauthorized event selector fires nothing and there is no impact.

Unvalidated fact: What the fired hooks actually do (and thus the concrete blast radius) is admin-defined and cannot be bounded from filebrowser source.

http/resource.go:214-221 (no top-level Perm gate; only Check+checkParent)

http/resource.go:217 (action from query, unvalidated)

http/resource.go:257-259 (RunHook evt=action verbatim)

runner/runner.go:26,36 (before_<evt> fires before fn())

http/resource.go:375-377 (unknown action rejected only after hook, inside fn())

http/resource.go:87 (DELETE handler gates the real delete path behind Perm.Delete — the guard bypassed here)