From ec5c955c0ab6204db8bc6a3c4e0a002bf96dae08 Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Wed, 2 Mar 2022 23:43:51 -0500 Subject: [PATCH] summary: additional check for max size limit --- .../core/__tests__/markdown-summary.test.ts | 33 ++++++++-- packages/core/src/markdown-summary.ts | 60 ++++++++++++++++--- 2 files changed, 79 insertions(+), 14 deletions(-) diff --git a/packages/core/__tests__/markdown-summary.test.ts b/packages/core/__tests__/markdown-summary.test.ts index 9b6ac037..6b56a98c 100644 --- a/packages/core/__tests__/markdown-summary.test.ts +++ b/packages/core/__tests__/markdown-summary.test.ts @@ -1,7 +1,11 @@ import * as fs from 'fs' import * as os from 'os' import path from 'path' -import {markdownSummary, SUMMARY_ENV_VAR} from '../src/markdown-summary' +import { + markdownSummary, + SUMMARY_ENV_VAR, + SUMMARY_LIMIT_BYTES +} from '../src/markdown-summary' const testFilePath = path.join(__dirname, 'test', 'test-summary.md') @@ -68,18 +72,37 @@ const fixtures = { } describe('@actions/core/src/markdown-summary', () => { - beforeAll(() => { - process.env[SUMMARY_ENV_VAR] = testFilePath - }) - beforeEach(async () => { + process.env[SUMMARY_ENV_VAR] = testFilePath await fs.promises.writeFile(testFilePath, '', {encoding: 'utf8'}) + markdownSummary.emptyBuffer() }) afterAll(async () => { await fs.promises.unlink(testFilePath) }) + it('throws if summary env var is undefined', async () => { + process.env[SUMMARY_ENV_VAR] = undefined + const write = markdownSummary.add(fixtures.text).write() + + await expect(write).rejects.toThrow() + }) + + it('throws if summary file does not exist', async () => { + await fs.promises.unlink(testFilePath) + const write = markdownSummary.add(fixtures.text).write() + + await expect(write).rejects.toThrow() + }) + + it('throws if write will exceed file limit', async () => { + const aaa = 'a'.repeat(SUMMARY_LIMIT_BYTES + 1) + const write = markdownSummary.add(aaa).write() + + await expect(write).rejects.toThrow() + }) + it('appends text to summary file', async () => { await fs.promises.writeFile(testFilePath, '# ', {encoding: 'utf8'}) await markdownSummary.add(fixtures.text).write() diff --git a/packages/core/src/markdown-summary.ts b/packages/core/src/markdown-summary.ts index 352767ca..c63b9773 100644 --- a/packages/core/src/markdown-summary.ts +++ b/packages/core/src/markdown-summary.ts @@ -1,7 +1,8 @@ import {EOL} from 'os' import {constants, promises} from 'fs' -const {access, appendFile, writeFile} = promises +const {access, appendFile, stat, writeFile} = promises +export const SUMMARY_LIMIT_BYTES = 128_000 export const SUMMARY_ENV_VAR = 'GITHUB_STEP_SUMMARY' export type SummaryTableRow = (SummaryTableCell | string)[] @@ -43,31 +44,62 @@ export interface SummaryImageOptions { class MarkdownSummary { private _buffer: string + private _filePath?: string constructor() { this._buffer = '' } /** - * Finds the summary file path from the environment, rejects if not found + * Finds the summary file path from the environment, rejects if env var is not found or file does not exist + * Also checks r/w permissions. * * @returns step summary file path */ private async filePath(): Promise { - const filePath = process.env[SUMMARY_ENV_VAR] - if (!filePath) { + if (this._filePath) { + return this._filePath + } + + const pathFromEnv = process.env[SUMMARY_ENV_VAR] + if (!pathFromEnv) { throw new Error( - `Unable to find environment variable for $${SUMMARY_ENV_VAR}` + `Unable to find environment variable for $${SUMMARY_ENV_VAR}. Check if your runtime environment supports markdown summaries.` ) } try { - await access(filePath, constants.R_OK | constants.W_OK) + await access(pathFromEnv, constants.R_OK | constants.W_OK) } catch { - throw new Error(`Unable to access summary file: ${filePath}`) + throw new Error( + `Unable to access summary file: '${pathFromEnv}'. Check if the file has correct read/write permissions.` + ) } - return filePath + this._filePath = pathFromEnv + return this._filePath + } + + /** + * Checks if the write of the current buffer will exceed the job summary upload limit + * + * @param {boolean} overwrite if the operation is overwrite (otherwise it's append) + * + * @returns {Promise} whether or not the file will exceed the limit + */ + private async willExceedLimit(overwrite: boolean): Promise { + let expectedSize = 0 + if (!overwrite) { + // if appending, we need to check the current size of the summary file + const filePath = await this.filePath() + const {size} = await stat(filePath) + expectedSize += size + } + + const bufferLen = Buffer.byteLength(this._buffer, 'utf8') + expectedSize += bufferLen + + return expectedSize > SUMMARY_LIMIT_BYTES } /** @@ -96,7 +128,8 @@ class MarkdownSummary { } /** - * Writes text in the buffer to the summary buffer file, will append by default + * Writes text in the buffer to the summary buffer file and empties buffer. Will append by default. + * Checks if resulting file size > SUMMARY_LIMIT_BYTES, will throw and empty buffer * * @param {boolean} [overwrite=false] (optional) replace existing content in summary file with buffer contents, default: false * @@ -104,6 +137,15 @@ class MarkdownSummary { */ async write(overwrite = false): Promise { const filePath = await this.filePath() + + if (await this.willExceedLimit(overwrite)) { + this.emptyBuffer() + const limitK = SUMMARY_LIMIT_BYTES / 1000 + throw new Error( + `Aborting write to summary file. File size would exceed limit of ${limitK}K.` + ) + } + const writeFunc = overwrite ? writeFile : appendFile await writeFunc(filePath, this._buffer, {encoding: 'utf8'}) return this.emptyBuffer()