Feat/thumbor bucket in url #521
Conversation
|
@bennet-esyoil thanks for your idea and submission, we will review it and let you know |
|
@dougtoppin Thanks for your comment. Is there any chance to get any ETA since this is quite important for our current goal? |
|
@bennet-esyoil We discussed your PR and it looks like an excellent addition. We will be it including in an upcoming release. |
| @@ -126,4 +126,30 @@ describe("parseImageBucket", () => { | |||
| }); | |||
| } | |||
| }); | |||
|
|
|||
| it("should parse bucket-name from first part in thubor request but fail since it's not allowed", () => { | |||
There was a problem hiding this comment.
thumbor is spelled incorrectly in the above string and in the next test.
Does this test cover when no bucket is provided? For instance, in my use case I do not want to specify a bucket name in the URL.
There was a problem hiding this comment.
I've found some unrelated cases where the tests did not cover, thus my "akward" commits after creating the PR.
the only problem I currently see with my approach is that if you have a path that contains the bucket-name which you do not want to use, this would fail.
e.g. you have a bucket bucket-1 and want to request the file /bucket-1/test.jpg, from that bucket, the logic would remove the bucket-1 since it's a name. I was thinking about prefixing the parameter with something like bucket:bucket-1/bucket-1/test.jpg which would resolve that issue.
Does this test cover when no bucket is provided? For instance, in my use case I do not want to specify a bucket name in the URL.
If you dont, behaviour is the same as before (takes the first bucket provided in the env-variables)
There was a problem hiding this comment.
Ok thanks for the reply.
Yes, I agree with using a keyword like "bucket:"/ "s3:" similar to the "filters:" in the URL. That would work around the issue and hopefully not cause an issue with the path/bucket name
Example
const event = { path: "/filters:grayscale()/bucket:test-bucket/test-image-001.jpg" };
const event = { path: "/filters:grayscale()/s3:test-bucket/test-image-001.jpg" };
There was a problem hiding this comment.
I think the s3:name looks pretty sleek, kinda ressembles the normal s3://bucket/object syntax. I'll get this changed.
There was a problem hiding this comment.
I kinda don't like how it's not part of the ThumborMapper class, but it's kinda not part of thumbor itself and also needs to happen at an earlier stage, so it makes sense to have it where it is now..
There was a problem hiding this comment.
Hey @t4colingough any update?
There was a problem hiding this comment.
Hello again @t4colingough & @dougtoppin ,
any chance to get this released any time soon?
There was a problem hiding this comment.
Hi @bennet-esyoil,
This change has been made internally and is currently set to be included in the next minor/major release. While our internal processes had us merge the code manually, once the release with this change is out, you'll see yourself included in the external contributors section towards the bottom of the readme and this PR will be closed.
Thanks for your contributions,
Simon
| expect(bucket).toEqual("allowedBucket001") | ||
| }) | ||
|
|
||
| it("should parse bucket-name from first part in thumbor request and return it", () => { |
There was a problem hiding this comment.
It might be worthwhile adding a test to cover the newly fixed thumbor chaining issue: #343
It should have both the legacy and new chaining method
it("should parse bucket-name from first part in thumbor request and return it when using legacy multiple filters", () => {
// Arrange
const event = { path: "/filters:grayscale()/filters:rotate(180)/s3:test-bucket/test-image-001.jpg" };
process.env.SOURCE_BUCKETS = "allowedBucket001, test-bucket";
// Act
const imageRequest = new ImageRequest(s3Client, secretProvider);
const bucket = imageRequest.parseImageBucket(event, RequestTypes.THUMBOR);
// Assert
expect(bucket).toEqual("test-bucket")
})
it("should parse bucket-name from first part in thumbor request and return it when chaining multiple filters", () => {
// Arrange
const event = { path: "/filters:grayscale():rotate(180)/s3:test-bucket/test-image-001.jpg" };
process.env.SOURCE_BUCKETS = "allowedBucket001, test-bucket";
// Act
const imageRequest = new ImageRequest(s3Client, secretProvider);
const bucket = imageRequest.parseImageBucket(event, RequestTypes.THUMBOR);
// Assert
expect(bucket).toEqual("test-bucket")
})
There was a problem hiding this comment.
IMO this should be covered by a totally different test-suite since it's not really part of that function
|
I'd like to point to this article and the security implications of having the S3 bucket name in the URL for everyone to see: https://medium.com/@maciej.pocwierz/how-an-empty-s3-bucket-can-make-your-aws-bill-explode-934a383cb8b1 |
Yes, this would apply for the base64 encoded version as well. Security through obfuscation 🙃 . It should be best practice to deploy an instance of the serverless-image-handler for each bucket without exception anyways. Having the bucket specified in the URL or in the B64 request data is just a quick way for us to switch between different buckets depending on some configuration. |
Hello,
currently when using Thumbor-Style URLs (which is the only way to get readable URLs for SEO) you cannot specify the bucket which will get used. Currently always the first one provided via Env-Settings will be used.
My suggestion is to check if the URL contains any allowed bucket, and if it does, use that one. This would mean one can - but doesn't need to - add the bucket to the URL. If left empty it would not break existing setups.
e.g:
SOURCE_BUCKETS: "test1, test2"
Request: "/fit-in/my-image.jpg"
This would currently load "my-image.jpg" from the bucket "test1".
With my PR you can do "/fit-in/test2/my-image.jpg" which would load from test2.
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.