BUG Don't auto grant session access when resampling images #477
Conversation
This file contains 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
maxime-rainville
commented
Feb 8, 2022
|
Not 100% sure if this is worth covering with Unit test ... I can add some if we think it is. |
emteknetnz
requested changes
Feb 8, 2022
emteknetnz
requested changes
Feb 8, 2022
|
Just added some unit test ... I also specifically tested that they would have failed with the old logic |
emteknetnz
requested changes
Feb 14, 2022
tests/php/ImageTest.php
Outdated
| $assetStore = Injector::inst()->get(AssetStore::class); | ||
| $this->assertFalse( | ||
| $assetStore->isGranted('folder/a870de278b/test-image-high-quality__Resampled.jpg'), | ||
| 'Current user is not automatically granted access to resampled image' |
There was a problem hiding this comment.
Suggested change
| 'Current user is not automatically granted access to resampled image' | |
| 'Logged out user was granted access to draft resampled image' |
There was a problem hiding this comment.
What about this?
Suggested change
| 'Current user is not automatically granted access to resampled image' | |
| 'Current user should not automatically be granted access to resampled image' |
tests/php/ImageManipulationTest.php
Outdated
| $assetStore = Injector::inst()->get(AssetStore::class); | ||
| $this->assertFalse( | ||
| $assetStore->isGranted($fileUrl), | ||
| 'Current user is not automatically granted access' |
There was a problem hiding this comment.
Suggested change
| 'Current user is not automatically granted access' | |
| 'Logged out user was granted access to draft resampled image' |
There was a problem hiding this comment.
I prefer assertion message to describe what is being asserted, rather than what has failed.
But I can see the message here could be confusing if you think Current user is not automatically granted access describes what has failed.
What about this instead?
Suggested change
| 'Current user is not automatically granted access' | |
| 'Current user should not automatically be granted access to view thumbnail' |
maxime-rainville
force-pushed
the
pulls/1/no-auto-grant
branch
from
33f81f2
to
d4880e1
Compare
emteknetnz
approved these changes
Feb 16, 2022
|
Merge on green |
maxime-rainville
force-pushed
the
pulls/1/no-auto-grant
branch
from
5da55df
to
78987e9
Compare
|
Just fixed the linting issue. |
emteknetnz
approved these changes
Feb 17, 2022
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.
We considered treating this as a security issue but decided that the amount of sensitive information in an image does not warrant it.
In some context, the CMS will grant your session permission to view a file irrespective of if you have access to view it. The specific thing we are trying to address here is being able to view a restricted image if it's added to a campaign.
Previous places where we addressed this included an option to allow automatic session grant via a config. I don't think we need to do this anymore since tho AssetStore now automatically grant you access to view files.