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.

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(target-gh): Check asset MD5 hash by downloading the asset #328

Merged
merged 6 commits into from
Nov 18, 2021
Merged
Changes from 2 commits
Commits
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
79 changes: 51 additions & 28 deletions src/targets/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -378,12 +378,30 @@ export class GithubTarget extends BaseTarget {
);
}

// XXX: This is a bit hacky as we rely on two things:
// 1. GitHub issuing a redirect to S3, where they store the artifacts,
// or at least pass those request headers unmodified to us
// 2. AWS S3 using the MD5 hash of the file for its ETag cache header
// when we issue a HEAD request.
const response = await this.github.request(`HEAD ${url}`, {
const remoteChecksum = await this.getRemoteChecksum(url);
const localChecksum = createHash('md5').update(file).digest('hex');
iker-barriocanal marked this conversation as resolved.
Show resolved Hide resolved
if (localChecksum !== remoteChecksum) {
throw new Error(
`Uploaded asset MD5 checksum does not match local asset checksum for "${name} (${localChecksum} != ${remoteChecksum})`
);
}
return url;
} catch (e) {
uploadSpinner.fail(`Cannot upload asset "${name}".`);

throw e;
}
}

private async getRemoteChecksum(url: string): Promise<string> {
// XXX: This is a bit hacky as we rely on two things:
// 1. GitHub issuing a redirect to S3, where they store the artifacts,
// or at least pass those request headers unmodified to us
// 2. AWS S3 using the MD5 hash of the file for its ETag cache header
// when we issue a HEAD request.
let response;
try {
response = await this.github.request(`HEAD ${url}`, {
headers: {
// WARNING: You **MUST** pass this accept header otherwise you'll
// get a useless JSON API response back, instead of getting
Expand All @@ -395,32 +413,37 @@ export class GithubTarget extends BaseTarget {
Accept: DEFAULT_CONTENT_TYPE,
},
});
} catch (e) {
throw new Error(
`Cannot get asset on GitHub. Status: ${(e as any).status}\n` + e
);
}

const etag = response.headers['etag'];
if (etag && etag.length > 0) {
// ETag header comes in quotes for some reason so strip those
const remoteChecksum = response.headers['etag']?.slice(1, -1);
if (remoteChecksum) {
// XXX: This local checksum calculation only holds for simple cases:
// https://github.com/getsentry/craft/issues/322#issuecomment-964303174
const localChecksum = createHash('md5').update(file).digest('hex');
if (localChecksum !== remoteChecksum) {
logger.trace(`Checksum mismatch for "${name}"`, response.headers);
throw new Error(
`Uploaded asset MD5 checksum does not match local asset checksum for "${name} (${localChecksum} != ${remoteChecksum})`
);
}
uploadSpinner.succeed(`Uploaded asset "${name}".`);
} else {
logger.warn(`Response headers:`, response.headers);
uploadSpinner.succeed(
`Uploaded asset "${name}, but failed to find a remote checksum. Cannot verify uploaded file is as expected.`
);
}
return url;
} catch (e) {
uploadSpinner.fail(`Cannot upload asset "${name}".`);
return etag.slice(1, -1);
}

throw e;
return await this.md5FromUrl(url);
}

private async md5FromUrl(url: string): Promise<string> {
this.logger.debug('Downloading asset from GitHub to check MD5 hash: ', url);
let response;
try {
response = await this.github.request(`GET ${url}`, {
headers: {
Accept: DEFAULT_CONTENT_TYPE,
},
});
} catch (e) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if catching the original exception and throwing a new one is actually good here as you obscure the original issue. In debug mode, we already log the response status etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @BYK !!

In this case here, we were seeing something like "Failed: Not Found" mixed up in the logs. So the RequestError (the e) from Octokit didn't provide much context, and not knowing clearly if the problem was in HEAD (the original ETag method) or GET (the new download method added in this PR) was not a great prospect.

The original error is still printed along with a custom message. Do you think we could do better?

throw new Error(
`Cannot download asset from GitHub. Status: ${(e as any).status}\n` + e
chadwhitacre marked this conversation as resolved.
Show resolved Hide resolved
);
}

return createHash('md5').update(Buffer.from(response.data)).digest('hex');
}

private async handleGitHubUpload(
Expand Down