diff --git a/src/targets/github.ts b/src/targets/github.ts index c5af1a2b..a99a25d4 100644 --- a/src/targets/github.ts +++ b/src/targets/github.ts @@ -2,7 +2,7 @@ import { Octokit, RestEndpointMethodTypes } from '@octokit/rest'; import { RequestError } from '@octokit/request-error'; import { readFileSync, promises, statSync } from 'fs'; import { basename } from 'path'; -import { createHash } from 'crypto'; +import { BinaryLike, createHash } from 'crypto'; import { getConfiguration } from '../config'; import { @@ -378,12 +378,33 @@ 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.checksumFromUrl(url); + const localChecksum = this.checksumFromData(file); + if (localChecksum !== remoteChecksum) { + throw new Error( + `Uploaded asset checksum does not match local asset checksum for "${name} (${localChecksum} != ${remoteChecksum})` + ); + } + uploadSpinner.succeed(`Uploaded asset "${name}".`); + return url; + } catch (e) { + uploadSpinner.fail(`Cannot upload asset "${name}".`); + + throw e; + } + } + + private async checksumFromUrl(url: string): Promise { + // XXX: This is a bit hacky as we rely on various things: + // 1. GitHub issuing a redirect to AWS S3. + // 2. S3 using the MD5 hash of the file for its ETag cache header. + // 3. The file being small enough to fit in memory. + // + // Note that if assets are large (5GB) assumption 2 is not correct. See + // https://github.com/getsentry/craft/issues/322#issuecomment-964303174 + 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 @@ -395,32 +416,40 @@ 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 { + 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) { + throw new Error( + `Cannot download asset from GitHub. Status: ${(e as any).status}\n` + e + ); } + return this.checksumFromData(Buffer.from(response.data)); + } + + private checksumFromData(data: BinaryLike): string { + return createHash('md5').update(data).digest('hex'); } private async handleGitHubUpload(