Skip to content

Base the item upload operation's security on Patch, else Put (#581) - #584

Merged
silverbackdan merged 3 commits into
mainfrom
fix/581-upload-write-security
Nov 11, 2026
Merged

silverbackdan merged 3 commits into
mainfrom
fix/581-upload-write-security

Conversation

@silverbackdan

Copy link
Copy Markdown
Collaborator

Fixes #581

Problem

UploadableResourceMetadataCollectionFactory built the item upload route (POST {iri}/upload) from the resource's Get, so the upload took the read's security and securityPostDenormalize: usually public. Only the firewall's access_control stopped a visitor from replacing a stored file.

Change

  • The item upload is still built from the Get (provider, contexts, controller, URI variables), but its security, securityMessage, securityPostDenormalize, securityPostDenormalizeMessage, securityPostValidation and securityPostValidationMessage are copied from the resource's Patch, else its first Put. All six are copied, nulls included, so nothing from the Get is left behind.
  • A resource with neither Patch nor Put gets no item upload route. There is no write security to base the route on. Every option that keeps a route would make one up: falling back to the Get is the bug, and null or false security is a guess about the app's intent. Leaving it out follows the least-exposure principle. Replacing a stored file is a write, and the resource does not offer item writes.
  • The collection upload (POST /…/upload) already copies the Post, so it is unchanged. The download is a read and keeps the Get's security.

Overlap with #569

CWA content was already protected. ContentWriteSecurityResourceMetadataCollectionFactory decorates further out (priority -1) than this factory (0), so it sees the generated upload operations (method POST) and ANDs the publishable permission onto them. A ROLE_USER upload to a component was already a 403 on main. test_a_users_upload_to_a_component_is_refused_by_the_publishable_permission passes before and after this change. This fix matters for uploadable resources that are not CWA content, and it gives CWA content its Patch's own security on top of the permission.

Breaking

  • An app that relied on the Get's security for uploads now gets its Patch's or Put's.
  • A resource without Patch or Put loses POST {iri}/upload.

Tests

  • Uploads\UploadWriteSecurityTest uses a new non-content DummyUploadableWithAdminWrite (public Get, admin-only Patch with a message). The results:
    • an anonymous upload is a 401;
    • a ROLE_USER upload is a 403 with the Patch's message, and the file is unchanged (this case failed before the fix);
    • an admin upload is stored;
    • the download stays public.
  • StateOperationMetadataTest has unit tests for these cases: Patch is preferred over Put, the first Put is used when there is no Patch, all six fields are copied, and no upload route is built without either.
  • Full paratest suite and listener mode (Uploads and ContentWriteSecurity) pass. Infection on the changed lines: 15 of 15 mutants killed.
  • PHPStan baseline: the Pure attribute count for the factory drops from 5 to 4.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.09%. Comparing base (7fb6f6e) to head (3271cd1).

Additional details and impacted files
@@            Coverage Diff            @@
##               main     #584   +/-   ##
=========================================
  Coverage     97.09%   97.09%           
- Complexity     5342     5350    +8     
=========================================
  Files           438      438           
  Lines         13821    13837   +16     
=========================================
+ Hits          13419    13435   +16     
  Misses          402      402           
Flag Coverage Δ
phpunit 97.09% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@a1itltdoffice

Copy link
Copy Markdown

I want to connect you but faild

@silverbackdan
silverbackdan merged commit d0436eb into main Nov 11, 2026
14 of 15 checks passed
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.

Upload routes inherit the Get operation's security, not Patch's

2 participants