Skip to content

Commit

Permalink
s3: fix handling of NULL blob.contentType values (#1353)
Browse files Browse the repository at this point in the history
* s3: fix handling of NULL blob.contentType values

Ref #1351
  • Loading branch information
alxndrsn authored Jan 13, 2025
1 parent 26cd17e commit 7574030
Show file tree
Hide file tree
Showing 6 changed files with 35 additions and 13 deletions.
4 changes: 3 additions & 1 deletion lib/external/s3.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ const init = (config) => {
// * https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html#API_GetObject_RequestSyntax
const getRespHeaders = (filename, { contentType }) => ({
'response-content-disposition': contentDisposition(filename),
'response-content-type': contentType,
// "null" is a questionable content-type, but matches current central behaviour
// See: https://github.com/getodk/central-backend/pull/1352
'response-content-type': contentType || 'null',
});

async function urlForBlob(filename, blob) {
Expand Down
1 change: 1 addition & 0 deletions test/e2e/s3/test-forms/1-attachments/cities.geojson
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
2 changes: 2 additions & 0 deletions test/e2e/s3/test-forms/1-attachments/some.nomime
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
I should not be uploaded with a mime type.
TODO what mime type should I be DOWNLOADED with?
8 changes: 8 additions & 0 deletions test/e2e/s3/test-forms/1.xml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@
<value>Smile</value>
<value form="image">jr://images/smile.png</value>
</text>
<text id="geojson">
<value>geojson</value>
<value form="image">jr://images/cities.geojson</value>
</text>
<text id="nomime">
<value>geojson</value>
<value form="image">jr://images/some.nomime</value>
</text>
</translation>
</itext>
<instance>
Expand Down
9 changes: 6 additions & 3 deletions test/e2e/s3/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,13 @@ describe('s3 support', () => {

// given
await setup(1);
await assertNewStatuses({ pending: 11 });
await assertNewStatuses({ pending: 13 });

// when
await cli('upload-pending');

// then
await assertNewStatuses({ uploaded: 11 });
await assertNewStatuses({ uploaded: 13 });
// and
await assertAllRedirect(actualAttachments);
await assertAllDownloadsMatchOriginal(actualAttachments);
Expand Down Expand Up @@ -379,7 +379,10 @@ describe('s3 support', () => {

const filepath = `${attDir}/${name}`;

const expectedContentType = mimetypeFor(name);
// "null" is a questionable content-type, but matches current central behaviour
// See: https://github.com/getodk/central-backend/pull/1352
const expectedContentType = mimetypeFor(name) ?? 'null';

const actualContentType = res.headers.get('content-type');
should.equal(actualContentType, expectedContentType);

Expand Down
24 changes: 15 additions & 9 deletions test/e2e/util/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ async function apiClient(suiteName, { serverUrl, userEmail, userPassword, logPat
});
} else {
const { body, mimeType } = opts;
return apiPost(path, body, { 'Content-Type':mimeType });

const headers = {};
if(mimeType) headers['Content-Type'] = mimeType;

return apiPost(path, body, headers);
}
}

Expand Down Expand Up @@ -119,14 +123,16 @@ function mimetypeFor(f) {
// For more, see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types/Common_types
const extension = extname(f);
switch(extension) {
case '.bin' : return 'application/octet-stream';
case '.jpg' : return 'image/jpeg';
case '.png' : return 'image/png';
case '.svg' : return 'image/svg+xml';
case '.txt' : return 'text/plain';
case '.xls' : return 'application/vnd.ms-excel';
case '.xlsx': return 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet';
case '.xml' : return 'application/xml';
case '.bin' : return 'application/octet-stream';
case '.geojson': return 'application/geo+json';
case '.jpg' : return 'image/jpeg';
case '.nomime' : return null; // used for testing user agents which do not set a mime type for some file extensions
case '.png' : return 'image/png';
case '.svg' : return 'image/svg+xml';
case '.txt' : return 'text/plain';
case '.xls' : return 'application/vnd.ms-excel';
case '.xlsx' : return 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet';
case '.xml' : return 'application/xml';
default: throw new Error(`Unsure what mime type to use for: ${f}`);
}
}
Expand Down

0 comments on commit 7574030

Please sign in to comment.