From 41467bd4cf2f1f786f33f75195bfc574f577f589 Mon Sep 17 00:00:00 2001 From: Joel Brock Date: Wed, 10 Jun 2026 10:49:28 -0700 Subject: [PATCH] File redirect: per-entity permission check before minting fcs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security-review follow-up to b65bc6d. The file-redirect route signs an fcs JWT that Civi's /civicrm/file handler accepts as proof of access. Any user with the base `access CiviCRM` permission could iterate file IDs and have us laundering tokens past the entity-level ACLs that would normally apply (e.g. a staff user without view permission on a given contact could still pull files attached to that contact). Tighten it: - Resolve the file's linked entity_table + entity_id (was: entity_id only). - Run the entity-type's native permission check before signing the JWT: civicrm_activity -> CRM_Activity_BAO_Activity::checkPermission civicrm_contact -> CRM_Contact_BAO_Contact_Permission::allow Unknown entity types deny by default — adding a new type requires an explicit edit here, so we don't accidentally widen the surface. - Drop JWT lifetime from a week to 10 minutes. The token is minted at click time (the user hits this route fresh on each file click), so the long lifetime served no purpose and made each URL a longer-lived bearer credential. Files missing from civicrm_entity_file or pointing at unsupported entity types now 403 via CRM_Utils_System::permissionDenied() instead of producing a download URL. --- .../webform-mw/CRM/WebformMw/Page/File.php | 85 ++++++++++++------- 1 file changed, 56 insertions(+), 29 deletions(-) diff --git a/civi-extension/webform-mw/CRM/WebformMw/Page/File.php b/civi-extension/webform-mw/CRM/WebformMw/Page/File.php index fdc6421..8f7e0c1 100644 --- a/civi-extension/webform-mw/CRM/WebformMw/Page/File.php +++ b/civi-extension/webform-mw/CRM/WebformMw/Page/File.php @@ -9,51 +9,51 @@ * site's crypto key, which we have here because we're running inside * CiviCRM; the Next.js side doesn't. * - * Permission: `access CiviCRM`. The user is already authenticated in the - * parent Civi tab when they click a link in the iframe; their session - * cookie travels with the new-tab navigation. + * URL shape: /civicrm/webform-mw/file?id= * - * URL shape: - * /civicrm/webform-mw/file?id=[&eid=] - * - * Behavior: - * - Resolve `eid` from civicrm_entity_file if not provided. - * - Mint a short-lived JWT with payload {exp, civi.file: } - * matching Civi's own /civicrm/file token format. - * - 302-redirect to /civicrm/file?reset=1&id=...&eid=...&fcs=. + * Authorization: minting an fcs JWT is effectively "issuing a bearer + * credential for this file" — once issued, /civicrm/file will serve the + * bytes against any session. To avoid becoming a credential-laundering + * IDOR, we look up the file's linked entity and run the entity-type's + * native permission check before signing. The base `access CiviCRM` + * permission only gates reaching this endpoint at all; per-record ACLs + * happen here. Unknown entity types are denied by default. */ class CRM_WebformMw_Page_File extends CRM_Core_Page { public function run() { $fileId = (int) CRM_Utils_Request::retrieve('id', 'Positive', $this, TRUE); - $eid = (int) CRM_Utils_Request::retrieve('eid', 'Positive', $this, FALSE, 0); - // Resolve eid from the entity_file join if the caller didn't supply - // one. Any linked entity works for URL-fingerprint purposes; the JWT - // we mint below is what Civi actually authenticates on. - if (!$eid) { - $dao = CRM_Core_DAO::executeQuery( - "SELECT entity_id FROM civicrm_entity_file WHERE file_id = %1 LIMIT 1", - [1 => [$fileId, 'Positive']] - ); - if ($dao->fetch()) { - $eid = (int) $dao->entity_id; - } + // Look up the file's linked entity. We need both entity_table and + // entity_id — entity_table drives which permission API to call. + $dao = CRM_Core_DAO::executeQuery( + "SELECT entity_table, entity_id FROM civicrm_entity_file WHERE file_id = %1 LIMIT 1", + [1 => [$fileId, 'Positive']] + ); + if (!$dao->fetch()) { + CRM_Utils_System::permissionDenied(); + return; + } + $entityTable = (string) $dao->entity_table; + $entityId = (int) $dao->entity_id; + + if (!$this->canViewEntity($entityTable, $entityId)) { + CRM_Utils_System::permissionDenied(); + return; } - // Mint the fcs JWT. Payload matches the structure Civi's own file - // URL builder emits: {exp, "civi.file": ""}. One-week lifetime — - // these links are typically clicked seconds after the report renders, - // but the staff report can be left open for a while in a Civi tab. + // Mint a short-lived fcs JWT. The token is minted at click time (not + // at report-render time), so 10 minutes covers normal redirect-and-fetch + // latency without making the URL a long-lived bearer credential. $payload = [ - 'exp' => time() + 60 * 60 * 24 * 7, + 'exp' => time() + 60 * 10, 'civi.file' => (string) $fileId, ]; $fcs = \Civi::service('crypto.jwt')->encode($payload); $url = CRM_Utils_System::url( 'civicrm/file', - "reset=1&id={$fileId}&eid={$eid}&fcs=" . urlencode($fcs), + "reset=1&id={$fileId}&eid={$entityId}&fcs=" . urlencode($fcs), FALSE, NULL, FALSE, @@ -62,4 +62,31 @@ class CRM_WebformMw_Page_File extends CRM_Core_Page { CRM_Utils_System::redirect($url); } + /** + * Check whether the current Civi user can view the entity a file is + * linked to. Restricted to the entity types the WebForm-mw staff report + * actually surfaces (activity custom-field files and contact custom-field + * files); everything else denies. New entity types should be added here + * deliberately so we don't accidentally widen the surface. + */ + private function canViewEntity(string $entityTable, int $entityId): bool { + if ($entityId <= 0) { + return FALSE; + } + switch ($entityTable) { + case 'civicrm_activity': + return CRM_Activity_BAO_Activity::checkPermission( + $entityId, + CRM_Core_Permission::VIEW + ); + case 'civicrm_contact': + return CRM_Contact_BAO_Contact_Permission::allow( + $entityId, + CRM_Core_Permission::VIEW + ); + default: + return FALSE; + } + } + }