diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c6f166df4..e62b89abe2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,8 @@ ### Fixes -- [Fix crashes when path to prototype contains spaces](https://github.com/alphagov/govuk-prototype-kit/pull/1841) +- [#1847: Stop user's home path from being printed in migration script log](https://github.com/alphagov/govuk-prototype-kit/pull/1847) +- [#1841: Fix crashes when path to prototype contains spaces](https://github.com/alphagov/govuk-prototype-kit/pull/1841) ## 13.0.1 diff --git a/__tests__/spec/migrate.js b/__tests__/spec/migrate.js index a957273f84..0a15f1f8d6 100644 --- a/__tests__/spec/migrate.js +++ b/__tests__/spec/migrate.js @@ -1,12 +1,16 @@ /* eslint-env jest */ -const path = require('path') const fs = require('fs') +const os = require('os') +const path = require('path') + const fse = require('fs-extra') + const { spawn } = require('../../lib/exec') const { mkdtempSync } = require('../util') -const projectDirectory = path.join(mkdtempSync(), 'migrate-checks') +const testDirectory = mkdtempSync() +const projectDirectory = path.join(testDirectory, 'migrate-checks') const appDirectory = path.join(projectDirectory, 'app') const assetsDirectory = path.join(appDirectory, 'assets') const fixtureProjectDirectory = path.join(__dirname, '..', 'fixtures', 'test-v11-prototype') @@ -137,4 +141,18 @@ describe('migrate test prototype', () => { '{% extends "govuk-prototype-kit/layouts/govuk-branded.html" %}' + '\n' ) }) + + it('migrate.log does not contain user home directory', () => { + const migrateLog = fs.readFileSync(path.join(projectDirectory, 'migrate.log'), 'utf8') + + expect(migrateLog).not.toContain(os.homedir()) + + // Because we don't run the test in the home directory, we also have to do + // a slightly different check, we make sure all paths except for the + // metadata in the header lines are relative to the prototype directory. + // If this is true then this should mean we won't get the home dir path. + expect(migrateLog.split('\n').filter( + line => line.includes(testDirectory) && !line.startsWith('argv:') && !line.startsWith('cwd:') + )).toEqual([]) + }) }) diff --git a/lib/migrator/fileHelpers.js b/lib/migrator/fileHelpers.js index 2a58608e1e..2c245fb48f 100644 --- a/lib/migrator/fileHelpers.js +++ b/lib/migrator/fileHelpers.js @@ -1,8 +1,10 @@ const path = require('path') -const { packageDir, projectDir } = require('../path-utils') const fs = require('fs').promises + const fse = require('fs-extra') -const { log } = require('./logger') + +const { packageDir, projectDir } = require('../path-utils') +const { log, sanitisePaths } = require('./logger') const verboseLog = async function () { await log(...arguments) @@ -49,29 +51,31 @@ const handleNotFound = resultIfFileNotFound => e => { } const replaceStartOfFile = async ({ filePath, lineToFind, replacement }) => { + const prettyFilePath = path.relative(projectDir, filePath) const fileLines = await getFileAsLines(filePath) const replacementLineNumber = fileLines.indexOf(lineToFind) if (replacementLineNumber > -1) { - await verboseLog(`Found [${lineToFind}] in [${filePath}, replacing`) + await verboseLog(`Found [${lineToFind}] in [${prettyFilePath}], replacing`) const linesFromOldFile = fileLines.splice(replacementLineNumber + 1) await verboseLog('Keeping these lines from old file', linesFromOldFile) const updatedFileLines = [replacement, ...linesFromOldFile] return writeFileLinesToFile(filePath, updatedFileLines) } - await verboseLog(`Could not found [${lineToFind}] in [${filePath}, aborting`) + await verboseLog(`Could not found [${lineToFind}] in [${prettyFilePath}], aborting`) return false } const removeLineFromFile = async ({ filePath, lineToRemove }) => { + const prettyFilePath = path.relative(projectDir, filePath) const linesToRemove = Array.isArray(lineToRemove) ? lineToRemove : [lineToRemove] const fileLines = await getFileAsLines(filePath) await verboseLog(fileLines) const updatedFileLines = fileLines.filter(line => !linesToRemove.includes(line)) if (updatedFileLines.length !== fileLines.length) { - await verboseLog(`Found [${lineToRemove}] in [${filePath}, removing`) + await verboseLog(`Found [${lineToRemove}] in [${prettyFilePath}], removing`) return writeFileLinesToFile(filePath, updatedFileLines) } - await verboseLog(`Could not found [${lineToRemove}] in [${filePath}, aborting`) + await verboseLog(`Could not found [${lineToRemove}] in [${prettyFilePath}], aborting`) return false } @@ -101,7 +105,9 @@ const deleteDirectoryIfEmpty = async (partialPath) => { const copyFileFromStarter = async (starterPath, newPath) => { const src = path.join(packageDir, 'prototype-starter', starterPath) const dest = path.join(projectDir, newPath || starterPath) - await verboseLog(`copying from [${src}] to [${dest}]`) + const prettySrc = src.startsWith(projectDir) ? path.relative(projectDir, src) : sanitisePaths(src) + const prettyDest = path.relative(projectDir, dest) + await verboseLog(`Copying from [${prettySrc}] to [${prettyDest}]`) return fse.copy(src, dest) .then(successOutput) .catch(async e => { diff --git a/lib/migrator/index.js b/lib/migrator/index.js index c14b71c93c..b11d18f02c 100644 --- a/lib/migrator/index.js +++ b/lib/migrator/index.js @@ -58,9 +58,9 @@ const directoriesToDelete = [ ] const directoriesToDeleteIfEmpty = [ - '/app/assets/sass/patterns', - '/app/assets/images', - '/app/views/includes' + 'app/assets/sass/patterns', + 'app/assets/images', + 'app/views/includes' ] const filesToUpdateIfUnchanged = [ diff --git a/lib/migrator/logger.js b/lib/migrator/logger.js index b92a8e5621..3ad1e5f97d 100644 --- a/lib/migrator/logger.js +++ b/lib/migrator/logger.js @@ -1,4 +1,6 @@ +const os = require('os') const path = require('path') + const { projectDir } = require('../path-utils') const fs = require('fs').promises @@ -7,15 +9,19 @@ const packageVersion = require('../../package.json').version const migrateLogFilePath = path.join(projectDir, 'migrate.log') let migrateLogFileHandle +function sanitisePaths (str) { + return str.replaceAll(os.homedir(), '~') +} + async function setup () { if (!migrateLogFileHandle) { migrateLogFileHandle = await fs.open(migrateLogFilePath, 'a') // log some information useful for debugging await module.exports.log(new Date().toISOString()) - await module.exports.log('cwd: ' + process.cwd()) + await module.exports.log('cwd: ' + sanitisePaths(process.cwd())) await module.exports.log(`package: govuk-prototype-kit@${packageVersion}`) - await module.exports.log('argv: ' + process.argv.join(' ')) + await module.exports.log('argv: ' + sanitisePaths(process.argv.join(' '))) } } @@ -31,6 +37,7 @@ async function log (message) { module.exports = { log, + sanitisePaths, setup, teardown } diff --git a/lib/migrator/logger.test.js b/lib/migrator/logger.test.js new file mode 100644 index 0000000000..e444ce02ca --- /dev/null +++ b/lib/migrator/logger.test.js @@ -0,0 +1,20 @@ +/* eslint-env jest */ + +describe('sanitisePaths', () => { + const os = require('os') + const path = require('path') + + const { sanitisePaths } = require('./logger') + + it('replaces occurences of home directory with ~', () => { + expect( + sanitisePaths(path.join(os.homedir(), 'path', 'to', 'folder')) + ).toEqual(path.join('~', 'path', 'to', 'folder')) + }) + + it('replaces all occurences in a string', () => { + expect( + sanitisePaths(process.argv.join(' ')) + ).not.toContain(os.homedir()) + }) +}) diff --git a/lib/migrator/migrationSteps.js b/lib/migrator/migrationSteps.js index 830946aea7..55ec6b562c 100644 --- a/lib/migrator/migrationSteps.js +++ b/lib/migrator/migrationSteps.js @@ -131,7 +131,7 @@ module.exports.deleteUnusedFiles = async (filesToDelete) => { // Do not report files that don't exist return true } - const reporter = await addReporter(`Delete ${filePath}`) + const reporter = await addReporter(`Delete ${file}`) const result = await deleteFile(path.join(projectDir, file)) await reporter(result) return result