Skip to content

Commit

Permalink
PATCH: AR-3095 Handle failed Image Attachment pipes
Browse files Browse the repository at this point in the history
The problem is two-fold:

  1. S3 authentication sometimes fails because we no longer
	   pass a callback to `getSignedUrl()`.  Were we to pass a
		 callback, the expired, temporary credentials would be refreshed.
		 This was broken by commit 90238b9
		 in apps/meteor/app/file-upload/ufs/AmazonS3/server.ts

  2. When proxying the file from S3, if the authentication fails,
	   or there is any other error leading to us not receiving the
		 file, we blindly pass along the (probably empty) body as if
		 that is the actual file.

Solutions:

  1. If the authentication fails, return 500.
	   Fastly will retry and presumably succeed the second time
		 around.  Or the user can click "Retry" and likewise succeed.
	2. If any other error occurs, return 500.
	   Same consequences as above.

Solution 2 is more catch-all.
Solution 1 saves us from having to hit S3 needlessly.
  • Loading branch information
nmagedman committed Sep 4, 2023
1 parent 17beca7 commit 6dc7a25
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 2 deletions.
9 changes: 8 additions & 1 deletion apps/meteor/app/file-upload/server/config/AmazonS3.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,14 @@ const get: FileUploadClass['get'] = async function (this: FileUploadClass, file,
const forceDownload = typeof query.download !== 'undefined';

const fileUrl = await this.store.getRedirectURL(file, forceDownload);
if (!fileUrl || !file.store) {
if (!fileUrl || !file.store || fileUrl === 'https://s3.us-west-2.amazonaws.com/') {
// If the credentials fail, getRedirectURL can return the string above,
// which just 307s (incidentally to https://aws.amazon.com/s3/).
// Proxying that 307 (in proxyFile(), below) just sends an empty body and closes the connection,
// causing us to falsey return an empty file.
// HACK: just fail 500 and hopefully it will succeed the next time around.
res.setHeader('x-rc-debug-s3url', fileUrl || '');
res.writeHead(500);
res.end();
return;
}
Expand Down
16 changes: 15 additions & 1 deletion apps/meteor/app/file-upload/server/lib/FileUpload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,21 @@ export const FileUpload = {
) {
res.setHeader('Content-Disposition', `${forceDownload ? 'attachment' : 'inline'}; filename="${encodeURI(fileName)}"`);

request.get(fileUrl, (fileRes) => fileRes.pipe(res));
request.get(fileUrl, (fileRes) => {
if (fileRes.statusCode !== 200) {
res.setHeader('x-rc-proxyfile-status', fileRes.statusCode);
res.setHeader('content-length', 0);
res.writeHead(500);
res.end();
return;
}

['content-length', 'content-type'].forEach((header) => {
fileRes.headers[header] && res.setHeader(header, fileRes.headers[header]);
});

fileRes.pipe(res);
});
},

generateJWTToFileUrls({ rid, userId, fileId }: { rid: string; userId: string; fileId: string }) {
Expand Down

0 comments on commit 6dc7a25

Please sign in to comment.