Skip to content

Commit

Permalink
s3: fix handling of NULL blob.contentType values
Browse files Browse the repository at this point in the history
  • Loading branch information
alxndrsn committed Jan 10, 2025
1 parent 92f1186 commit 0e15018
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 14 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
50 changes: 50 additions & 0 deletions test/e2e/s3/test-forms/1-attachments/cities.geojson
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
{
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"geometry": {
"type": "Point",
"coordinates": [102.0, 0.5]
},
"properties": {
"prop0": "value0"
}
},
{
"type": "Feature",
"geometry": {
"type": "LineString",
"coordinates": [
[102.0, 0.0],
[103.0, 1.0],
[104.0, 0.0],
[105.0, 1.0]
]
},
"properties": {
"prop0": "value0",
"prop1": 0.0
}
},
{
"type": "Feature",
"geometry": {
"type": "Polygon",
"coordinates": [
[
[100.0, 0.0],
[101.0, 0.0],
[101.0, 1.0],
[100.0, 1.0],
[100.0, 0.0]
]
]
},
"properties": {
"prop0": "value0",
"prop1": { "this": "that" }
}
}
]
}
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
25 changes: 15 additions & 10 deletions test/e2e/util/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ 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 @@ -99,7 +102,7 @@ async function apiClient(suiteName, { serverUrl, userEmail, userPassword, logPat
if(isRedirected(res)) return new Redirect(res);
if(!res.ok) {
const responseStatus = res.status;
const responseText = await res.text();
const responseText = await res.text() || res.statusText;

const err = new Error(`${responseStatus}: ${responseText}`);
err.responseStatus = responseStatus;
Expand All @@ -119,14 +122,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 0e15018

Please sign in to comment.