Skip to content

Fix: Headers not forwarded to clients, incorrect Content-Type #31

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

Open
KevinPayravi wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix: Headers not forwarded to clients, incorrect Content-Type #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93e3b30
Fix: Headers not forwarded to clients
KevinPayravi May 9, 2026
e6bf29f
Override Content-Type
KevinPayravi May 9, 2026
dc26160
Merge branch 'main' into fix-headers
KevinPayravi May 9, 2026
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
20 changes: 8 additions & 12 deletions src/ResponseHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,23 +40,19 @@ export class ResponseHelper {
}

//providers/s3 could set all sorts of weird headers, but we only want to pass along a few
getHeadersFromTarget(headers: Headers): Map<string, string> {
const result = new Map<string, string>();
getHeadersFromTarget(headers: Headers): Record<string, string> {
const result: Record<string, string> = {};

const addHeader = (
result: Map<string, string>,
headers: Headers,
header: string,
) => {
const addHeader = (header: string) => {
const value = headers.get(header);
if (value) {
result.set(header, value);

if (value !== null) {
result[header] = value;
}
};

// Reduce headers to just those that we want to pass through
addHeader(result, headers, "Content-Type");
addHeader(result, headers, "Last-Modified");
addHeader("Content-Type");
addHeader("Last-Modified");

return result;
}
Expand Down
10 changes: 7 additions & 3 deletions src/ThumbnailApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,13 @@ export class ThumbnailApi {
}

try {
expressResponse.set(
this.responseHelper.getHeadersFromTarget(response.headers),
);
const headers = this.responseHelper.getHeadersFromTarget(response.headers);
// All thumbnails in S3 are stored as .jpg,
// but may not be stored with the correct content type,
// causing S3 to return application/octet-stream as the default value.
// This overrides the Content-Type to ensure clients always receive type image/jpeg.
headers["Content-Type"] = "image/jpeg";
expressResponse.set(headers);
Comment on lines +159 to +165
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 9, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add test coverage for the Content-Type override.

The Content-Type override is a key fix in this PR but lacks explicit test verification. The existing test at line 142 ("serveItemFromS3: success") only verifies that set is called, but doesn't confirm that Content-Type: image/jpeg is actually set regardless of the S3 response headers.

📋 Suggested test

Add a test case in test/unit/ThumbnailApi.test.ts that mocks S3 returning application/octet-stream and verifies the override:

test("serveItemFromS3: overrides Content-Type to image/jpeg", async () => {
  const responseHelper = new ResponseHelper();
  
  responseHelper.getRemoteImagePromise = jest.fn(() =>
    Promise.resolve({
      status: 200,
      body: new ReadableStream(),
      headers: new Headers([["content-type", "application/octet-stream"]]),
    } as Response),
  );
  
  // Mock should return what S3 actually sends
  responseHelper.getHeadersFromTarget = jest.fn(() => ({
    "Content-Type": "application/octet-stream"
  }));
  
  responseHelper.pipe = jest.fn();
  
  const thumbnailApi = new ThumbnailApi(
    dplaApi as unknown as DplaApi,
    thumbnailStorage,
    thumbnailCacheQueue,
    responseHelper,
  );
  
  const mockResponse = new ExpressResponse();
  await thumbnailApi.serveItemFromS3(
    itemId,
    mockResponse as unknown as express.Response,
  );
  
  // Verify that the override happened
  expect(mockResponse.set).toHaveBeenCalledWith(
    expect.objectContaining({ "Content-Type": "image/jpeg" })
  );
});
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/ThumbnailApi.ts` around lines 159 - 165, Add a unit test that verifies
ThumbnailApi.serveItemFromS3 overrides Content-Type to image/jpeg: mock
ResponseHelper.getRemoteImagePromise to return a Response with headers
containing "application/octet-stream", mock ResponseHelper.getHeadersFromTarget
to return {"Content-Type": "application/octet-stream"}, and mock
ResponseHelper.pipe; construct ThumbnailApi with the mocked ResponseHelper and
call serveItemFromS3 with a fake express response, then assert that the express
response.set (or setHeaders) was called with an object containing
"Content-Type": "image/jpeg" (reference ThumbnailApi.serveItemFromS3,
ResponseHelper.getRemoteImagePromise, ResponseHelper.getHeadersFromTarget, and
the express response.set call).

} catch (err) {
this.releaseUpstreamBody(response);
const error = err instanceof Error
Expand Down
6 changes: 3 additions & 3 deletions test/unit/ResponseHelper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ describe("ResponseHelper", () => {
["foo", "bar"],
]);
const result = responseHelper.getHeadersFromTarget(headers);
expect(result.get("Content-Type")).toBe("image/jpeg");
expect(result.get("Last-Modified")).toBe("2");
expect(result.get("foo")).toBeUndefined();
expect(result["Content-Type"]).toBe("image/jpeg");
expect(result["Last-Modified"]).toBe("2");
expect(result["foo"]).toBeUndefined();
});

test.each([
Expand Down
8 changes: 4 additions & 4 deletions test/unit/ThumbnailApi.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ describe("ThumbnailApi async tests", () => {

responseHelper.getHeadersFromTarget = (headers: Headers) => {
expect(headers).toBeDefined();
return new Map([["content-type", "image/jpeg"]]);
return { "content-type": "image/jpeg" };
};

const mockPipe = jest.fn();
Expand Down Expand Up @@ -160,7 +160,7 @@ describe("ThumbnailApi async tests", () => {
status: 200,
} as Response),
);
responseHelper.getHeadersFromTarget = jest.fn(() => new Map());
responseHelper.getHeadersFromTarget = jest.fn(() => ({}));
responseHelper.translateStatusCode = jest.fn(() => 200);
responseHelper.okBody = okBody;

Expand Down Expand Up @@ -206,7 +206,7 @@ describe("ThumbnailApi async tests", () => {
} as Response),
);
responseHelper.getHeadersFromTarget = jest.fn(
() => new Map([["content-type", "image/jpeg"]]),
() => ({ "content-type": "image/jpeg" }),
);
responseHelper.translateStatusCode = jest.fn(() => 200);
responseHelper.okBody = okBody;
Expand Down Expand Up @@ -396,7 +396,7 @@ describe("ThumbnailApi async tests", () => {

responseHelper.getHeadersFromTarget = (headers: Headers) => {
expect(headers).toBeDefined();
return new Map([["content-type", "image/jpeg"]]);
return { "content-type": "image/jpeg" };
};

const okStatus = jest.fn(() => false);
Expand Down
Loading