Skip to content

Fix: kb_file attachment download had no permission check - #4

Open
bonelifer wants to merge 1 commit into
sutrus:masterfrom
bonelifer:fix-kb-file-attachment-permission-check
Open

Fix: kb_file attachment download had no permission check#4
bonelifer wants to merge 1 commit into
sutrus:masterfrom
bonelifer:fix-kb-file-attachment-permission-check

Conversation

@bonelifer

Copy link
Copy Markdown

Had Claude review the extension's permission model, and it found kb_file.php served any attachment by attach_id with no check that the requester could actually see the article it belongs to, unlike article.php, which already gates article text behind u_kb_view and the approval status (kb_m_approve). This let anyone, including guests, download files from unapproved or otherwise-inaccessible articles by guessing or enumerating the small sequential attach_id.

This applies the same checks article.php uses before streaming a file:

  • Requires u_kb_view (or a_manage_kb), matching the article page's own gate.
  • For an attachment linked to a saved article, requires that article be approved, or the requester have kb_m_approve/a_manage_kb, the same rule article.php already applies to the article text itself.
  • For an orphaned attachment (uploaded during composition, not yet attached to a saved article), only its own uploader can view it, so the existing preview-while-composing flow in posting.php keeps working.

services.yml gets three new arguments (auth, user, the shared functions_kb helper) and one new table parameter (articles_table) to support the lookups.

… has

Attachments were served by attach_id alone, with no check that the
requester can actually see the article they belong to. u_kb_view and
the approval gate (kb_m_approve) already protect the article text in
article.php; this applies the same checks before streaming a file.

Orphaned attachments (uploaded during composition, not yet attached to
a saved article) are left visible only to their own uploader, so the
existing preview-while-composing flow keeps working.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant