Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FR: implement URL tampering encoding (v3/Thumbor) #111

Closed
timkelty opened this issue Jun 21, 2019 · 9 comments
Closed

FR: implement URL tampering encoding (v3/Thumbor) #111

timkelty opened this issue Jun 21, 2019 · 9 comments

Comments

@timkelty
Copy link

timkelty commented Jun 21, 2019

In 3.x, I generated safe URLs by setting a SECURITY_KEY env var, which is used by Thumbor to create the encoded url: https://github.com/thumbor/thumbor/wiki/security

I could then use this same SECURITY_KEY to generate a valid url with another backend-system (eg PHP.

How can I do something similar with 4.x/Sharp?

Related: #106

@timkelty timkelty changed the title What encoding method is used with SharpJS version? What encoding method is used for URLs with SharpJS version? Jun 21, 2019
@timkelty
Copy link
Author

…I see, it looks like its just the base64 encoded JSON: https://github.com/awslabs/serverless-image-handler/blob/master/source/image-handler/image-request.js#L197

So it doesn't really have the same level of URL tampering that Thumbor has?

@kewang
Copy link

kewang commented Jun 26, 2019

I have the same issue Orz

@timkelty timkelty changed the title What encoding method is used for URLs with SharpJS version? FR: implement URL tampering encoding Jun 26, 2019
@timkelty
Copy link
Author

As Sharp doesn't really concern itself with the serving of anything, this would need to be implemented in this project, I believe.

@timkelty timkelty changed the title FR: implement URL tampering encoding FR: implement URL tampering encoding (like Thumbor had) Jun 26, 2019
@timkelty timkelty changed the title FR: implement URL tampering encoding (like Thumbor had) FR: implement URL tampering encoding (v3/Thumbor) Jun 26, 2019
@hayesry
Copy link
Member

hayesry commented Jun 26, 2019

@timkelty Thanks for adding this request, I'm copying it into our feature backlog to be looked into and addressed in a future release. Your feedback has been super helpful in the improvement of this solution!

@abalsekar
Copy link

Agree safe urls HMAC signed are essential as provided by the last version with Thumbor. Malicious users could decode the image key then automate requests for other assets.
Lucky the Thumbor implementation is still working.

@soupy1976
Copy link

soupy1976 commented Jul 18, 2019

Due to the fact that AWS have updated the lambda execution environment, which caused our old thumbor implementation to break, we had to implement this ourselves. If anyone is interested, this is the code we have so far (this is not tested in production yet, so no guarantees...but maybe helpful to someone)

     /**
     * verifies that the security hash sent in the path is valid
     * @param {Object} event - The request body.
     * @param {String} security_key - The security key from environment variable.*     
     */
    verifySecurityHash(event, security_key) {
        var crypto = require('crypto');

        var path = event.path;
        const pathParts = path.split('/');
        var securityHash = pathParts[securityHashPathIndex];
        path = path.replace('/' + securityHash + '/', '');
        var hash = crypto.createHmac('sha1', security_key).update(path).digest('base64').replace(/\+/g, "-").replace(/\//g, "_");

        if (securityHash !== hash) {
            throw new Error('ThumborMapping::VerifySecurityHash::InvalidSecurityHash');
        }
        return true;
    }

securityHashPathIndex (for us at least) is 1. We get security_key from the environment variables (ie. process.env.SECURITY_KEY) and set that in the AWS lambda environment vars.

Here are a couple of unit tests we wrote for it, with the keys and paths removed:

// ----------------------------------------------------------------------------
// verifySecurityHash()
// ----------------------------------------------------------------------------
describe('verifySecurityHash()', function () {
    describe('001/validHash', function () {
        it(`Should pass if the security hash is being validated correctly`, function () {

                // Arrange
                var security_key = '{[[insert your security key here]]';
                var securityHash = '[[insert your 'correct' hash of the security key here]]';

                const event = {
                    path: '/' + securityHash +'[[insert your path which results in the above hash here]]'
                }
                // Act
                const thumborMapping = new ThumborMapping();
                var result = thumborMapping.verifySecurityHash(event, security_key);
                // Assert
                const expectedResult = true;
                assert.deepEqual(result, expectedResult);
            });
    });
    describe('002/InvalidHash', function () {
        it(`Should pass if it is throwing error for invalid security hash`, function () {

            // Arrange
			var security_key = '{[[insert your security key here]]';
			var securityHash = '[[insert your 'incorrect' hash of the security key here]]';

			const event = {
				path: '/' + securityHash +'[[insert your path which does not result in the above hash here]]'
			}
			
            // Act
            const thumborMapping = new ThumborMapping();
            // Assert
            assert.throws(function () {
                thumborMapping.verifySecurityHash(event, security_key);
            }, Error, 'ThumborMapping::VerifySecurityHash::InvalidSecurityHash');
        });
    });
});

@powertoaster
Copy link

This is also critical for us.
I will implement this locally and do some production testing.

@rakk7
Copy link

rakk7 commented Dec 12, 2019

@timkelty Thanks for adding this request, I'm copying it into our feature backlog to be looked into and addressed in a future release. Your feedback has been super helpful in the improvement of this solution!

Is this future gonna be released soon? or you have decided to not add it.. and leave it to the user zone?

@beomseoklee
Copy link
Member

Thanks for waiting, all. We've added a security feature to v5.1.0 to meet the requirement. Please take a look at the latest version.

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

No branches or pull requests

9 participants