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.

Sign up for GitHub

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

Feat/thumbor bucket in url #521

Closed
wants to merge 8 commits into from
Closed

Feat/thumbor bucket in url #521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f135101
feat(thumbor-urls): add logic to parse bucket from path
bennet-esyoil Nov 30, 2023
2589355
feat: add tests
bennet-esyoil Nov 30, 2023
7aed95c
fix: replace bucket-name in url if present
bennet-esyoil Dec 1, 2023
57137dd
fix: also replace key
bennet-esyoil Dec 1, 2023
7fad979
fix: well
bennet-esyoil Dec 1, 2023
bf21c19
chore(tests): added test to verify that correct bucket gets selected …
bennet-esyoil Dec 11, 2023
52038a1
fix: typo in test title
bennet-esyoil Dec 11, 2023
eabd936
feat: switch to s3:bucket syntax
bennetgallein Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions source/image-handler/image-request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ export class ImageRequest {

imageRequestInfo.requestType = this.parseRequestType(event);
imageRequestInfo.bucket = this.parseImageBucket(event, imageRequestInfo.requestType);
imageRequestInfo.key = this.parseImageKey(event, imageRequestInfo.requestType);
imageRequestInfo.key = this.parseImageKey(event, imageRequestInfo.requestType, imageRequestInfo.bucket);
imageRequestInfo.edits = this.parseImageEdits(event, imageRequestInfo.requestType);

const originalImage = await this.getOriginalImage(imageRequestInfo.bucket, imageRequestInfo.key);
Expand Down Expand Up @@ -223,6 +223,16 @@ export class ImageRequest {
} else if (requestType === RequestTypes.THUMBOR || requestType === RequestTypes.CUSTOM) {
// Use the default image source bucket env var
const sourceBuckets = this.getAllowedSourceBuckets();
// Take the path and split it at "/" to get each "word" in the url as array
let potentialBucket = event.path
.split("/")
.filter(e => e.startsWith('s3:'))
.map(e => e.replace("s3:", ""));
// filter out all parts that are not an bucket-url
potentialBucket = potentialBucket.filter(e => sourceBuckets.includes(e));
// return the first match
if (potentialBucket.length > 0) return potentialBucket[0];

return sourceBuckets[0];
} else {
throw new ImageHandlerError(
Expand Down Expand Up @@ -265,7 +275,7 @@ export class ImageRequest {
* @param requestType Type of the request.
* @returns The name of the appropriate Amazon S3 key.
*/
public parseImageKey(event: ImageHandlerEvent, requestType: RequestTypes): string {
public parseImageKey(event: ImageHandlerEvent, requestType: RequestTypes, bucket: string = null): string {
if (requestType === RequestTypes.DEFAULT) {
// Decode the image request and return the image key
const { key } = this.decodeRequest(event);
Expand Down Expand Up @@ -299,6 +309,7 @@ export class ImageRequest {
.replace(/\/fit-in(?=\/)/g, "")
.replace(/^\/+/g, "")
.replace(/^\/+/, "")
.replace(new RegExp("s3:" + bucket + "\/"), '')
);
}

Expand Down
39 changes: 39 additions & 0 deletions source/image-handler/test/image-request/parse-image-bucket.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,4 +126,43 @@ describe("parseImageBucket", () => {
});
}
});

it("should parse bucket-name from first part in thumbor request but fail since it's not allowed", () => {
// Arrange
const event = { path: "/filters:grayscale()/s3:test-bucket/test-image-001.jpg" };
process.env.SOURCE_BUCKETS = "allowedBucket001, allowedBucket002";

// Act
const imageRequest = new ImageRequest(s3Client, secretProvider);

const bucket = imageRequest.parseImageBucket(event, RequestTypes.THUMBOR);
// Assert
expect(bucket).toEqual("allowedBucket001")
})

it("should parse bucket-name from first part in thumbor request and return it", () => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")
  })

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this should be covered by a totally different test-suite since it's not really part of that function

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thats fair enough

// Arrange
const event = { path: "/filters:grayscale()/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 take bucket-name from env-variable if not present in the URL", () => {
// Arrange
const event = { path: "/filters:grayscale()/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("allowedBucket001")
})
});