Add a Signer Utility to sign filters, run php-cs-fixer on bundle #405
Conversation
|
@trsteel88 would it be possible to split the PR into two? cs fixes one and signer. That would be much easier to review. |
|
Is there a way I can split it without doing it manually? |
|
Also, how do you submit a separate PR on github. They all just go into my fork by the looks of it |
|
as far as I can see you can cherry pick d1fa795 commit to a new branch (based on master) and drop it from this branch while rebase -i |
|
Ok, that should be all good now. |
… percentage sigsn which may not play well with some filesystems
|
@makasim I decided to commit the fix for #399 here as it requires the SignerInterface. It is all contained in 1 commit: a8b5c79 I am not quite sure why those 2 tests are failing. It doesn't seem to fail when I run them locally. The only thing I can think of is to commit with the hashes in the exception so I can see the output for testing. Are you able to take a look at the tests? |
|
Regarding #399. We can introduce new route for filters with runtime config. As you mentioned it can have prefix What do you think? As far as I can see the urls (filter and filter + runtime config) would never conflict. |
|
Regarding additionnal params in the url and broken sing. We definitly fix it before releaseing 1.0, It is bloker issue. Though I dont come up with a good solution. Introducing our own singer is not good idea IMO. I do not a security pro and therefor cannot maintain security issues. I would better rely on a good 3rd party library that does such job. Also the interface you introduced contains three parameters, it is looks like over kill. Could it be simplified? Maybe we can sing only |
|
I don't think we need the media/cache/rc because the filters parameter will be present and we can distinguish if it is a runtime config. The code I used for the signer is the same code from the UriSigner for creating the hash. We need to sign both the parameters and the path otherwise you could use the same hash on any image you would like which could be used as an attack. |
yes we can, but the controller looks over loaded with the logic, just want keep it thin. Also two routings seems like easier and shorter fix for the #399 issue |
|
@makasim - I have just had a look at separating the controller into 2 actions. This is a much bigger task than you think. It means that every single resolver is going to need to be passed the runtime config array so it can resolve it to the /media/cache/rc directory. This will mean updating every single method in the resolves. eg resolve, isStore, store, remove. Can I get this merged in and then we will tackle #399 separately? |
|
@makasim fyi, I have a project that needs to use this functionality which I need live by tomorrow. I would prefer not to override my composer using my fork so it would be much appreciated if I could get this through and tagged as alpha5 |
|
nothing has to be changed in any resolvers, only CacheManager::getBrowserPath |
|
@makasim, that doesn't work. Check the controller. It uses the cacheManager to resolve the path which would redirect it to the non rc version I am thinking we can get rid of the $this->cacheManager->resolve($path, $filter) and just redirect to $request->getRequestUri(). This will also fix the issue with the query params being removed: Query params should always stay on the image - Could be used for tracking/caching |
|
Ok I think that is it. However, I think it was more obvious what was happening previously with 1 controller action. |
|
Also, it still doesn't solve #399 because technically the user could still have 'rc/' in their url. It is unlikely though. If we wanted to solve it we could need to have /media/cache/normal/ and /media/cache/rc/ etc. Honestly, I think we are better off merging before 2d60ee0 and having a single action and tackling #399 separately. |
| */ | ||
| public function trimHash($hash) | ||
| { | ||
| return substr(preg_replace('/[^a-zA-Z0-9-_]/', '', $hash), 0, 8); |
There was a problem hiding this comment.
move this logic to the controller. remove the method.
|
@trsteel88 looks good to me, would you take care of the tests? |
| $binary = $this->dataManager->find($filter, $path); | ||
| } catch (NotLoadableException $e) { | ||
|
|
||
| throw new NotFoundHttpException('Source image could not be found', $e); |
There was a problem hiding this comment.
Controller should throw NotFoundHttpException, not NotLoadableException.
There was a problem hiding this comment.
It is the same as master: https://github.com/liip/LiipImagineBundle/blob/master/Controller/ImagineController.php#L88
There was a problem hiding this comment.
But it looks like you deleted this line, or I missed something?
There was a problem hiding this comment.
Sorry, you are right. I was looking at the wrong action. I will restore now.
|
@trsteel88, do you think is this necessary to have full hash in url? I'd suggest to shorten it like github does to 7 symbols. |
|
@makasim asked me to use the entire string because he didn't like the trimHash method Personally I was happy with the trimmed hash but I am at the point where I just need it merged. |
|
@Me1ifaro @trsteel88 I dont have anything against short hashes. We can use them. But in this case the sign method has to return a short one. We dont need a trim method. |
|
Ok, I'll make the entire hash 8 chars Monday morning. |
|
@makasim I can't work out why the tests are failing. It's like the runtime route isn't loaded into the tests. I can't find where this would be loaded though. Are you able to help? |
|
@trsteel88 have you added a route here: https://github.com/liip/LiipImagineBundle/blob/master/Tests/Functional/app/config/routing.yml |
|
That just has an include to _liip_imagine: Which includes https://github.com/trsteel88/LiipImagineBundle/blob/master/Resources/config/routing.xml |
|
@trsteel88 I've fixed tests and did some small impr. Please review my PR https://github.com/trsteel88/LiipImagineBundle/pull/1 |
Runtime config fixes
|
Merged and we're passing @makasim :D |
Add a Signer Utility to sign filters, run php-cs-fixer on bundle
|
and.... finally merged. Thanks for the critical fix @trsteel88 and your unbelievable patients. You did that! Congrats. Tagging as 1.0.0-alpha7 (thinking of beta now). |
|
No worries. Thanks for helping with those tests! |
Closes #398