Repository navigation
Base the item upload operation's security on Patch, else Put (#581) - #584
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I want to connect you but faild |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #581
Problem
UploadableResourceMetadataCollectionFactorybuilt the item upload route (POST {iri}/upload) from the resource'sGet, so the upload took the read'ssecurityandsecurityPostDenormalize: usually public. Only the firewall'saccess_controlstopped a visitor from replacing a stored file.Change
Get(provider, contexts, controller, URI variables), but itssecurity,securityMessage,securityPostDenormalize,securityPostDenormalizeMessage,securityPostValidationandsecurityPostValidationMessageare copied from the resource'sPatch, else its firstPut. All six are copied, nulls included, so nothing from theGetis left behind.PatchnorPutgets 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 theGetis the bug, andnullorfalsesecurity 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.POST /…/upload) already copies thePost, so it is unchanged. The download is a read and keeps theGet's security.Overlap with #569
CWA content was already protected.
ContentWriteSecurityResourceMetadataCollectionFactorydecorates further out (priority -1) than this factory (0), so it sees the generated upload operations (methodPOST) and ANDs the publishable permission onto them. AROLE_USERupload to a component was already a 403 onmain.test_a_users_upload_to_a_component_is_refused_by_the_publishable_permissionpasses before and after this change. This fix matters for uploadable resources that are not CWA content, and it gives CWA content itsPatch's own security on top of the permission.Breaking
Get's security for uploads now gets itsPatch's orPut's.PatchorPutlosesPOST {iri}/upload.Tests
Uploads\UploadWriteSecurityTestuses a new non-contentDummyUploadableWithAdminWrite(publicGet, admin-onlyPatchwith a message). The results:ROLE_USERupload is a 403 with thePatch's message, and the file is unchanged (this case failed before the fix);StateOperationMetadataTesthas unit tests for these cases:Patchis preferred overPut, the firstPutis used when there is noPatch, all six fields are copied, and no upload route is built without either.Pureattribute count for the factory drops from 5 to 4.🤖 Generated with Claude Code