diff --git a/.github/workflows/build-linux.yml b/.github/workflows/build-linux.yml index 03da92e..150af68 100644 --- a/.github/workflows/build-linux.yml +++ b/.github/workflows/build-linux.yml @@ -14,6 +14,37 @@ on: push: tags: - "v*" + # Pull requests build and verify but never publish (the --publish flag below + # only says "always" for a tag). + # + # paths-ignore, not paths: the default is to build, and only what is provably + # documentation is exempt. An allow-list would silently skip CI for any new + # code directory nobody remembered to add. + # + # This still means some PRs get no run at all, so do NOT mark this workflow as + # a required status check — a required check that never runs leaves the PR + # stuck on "Expected — waiting for status" forever. To have both, drop the + # filter and gate the expensive steps with an `if:` on the changed paths + # instead, so the check always reports a conclusion. + pull_request: + branches: + - main + paths-ignore: + - "**.md" + - "site/**" + - "images/**" + - "LICENSE" + +# One build per ref: a new push to a PR supersedes the one still running. +# +# cancel-in-progress is deliberately limited to pull requests. A tag build ends +# in `electron-builder --publish always`, and killing that mid-upload can leave +# the GitHub Release with a latest-linux.yml pointing at assets that were never +# finished uploading — which is exactly what electron-updater then hands to +# every installed client. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} # Needed for electron-builder to upload artifacts to the GitHub Release on a # tag push. The default GITHUB_TOKEN is read-only, so publishing fails with @@ -95,6 +126,22 @@ jobs: working-directory: ui/electron run: npm ci --ignore-scripts + # Compile the addon against Electron's ABI here, before packaging, purely + # so the audit below can gate the release: on a tag push electron-builder + # publishes as it packages, so a check that runs afterwards is too late. + # electron-builder's own @electron/rebuild step then finds it up to date. + - name: Build native addon for Electron + working-directory: ui/electron + run: npx electron-rebuild --only gitbox-addon --force + + # Guards issue #1. The addon must carry libgit2/libssh2/mbedTLS/OpenSSL + # inside itself; when an archive is missing from binding.gyp nothing fails + # at build time — the app just dies on the user's machine with + # "undefined symbol: EVP_des_ede3_cbc". + - name: Verify the native addon is self-contained + working-directory: core/addon + run: npm test + # electron-builder runs @electron/rebuild first (compiling gitbox_addon # and node-pty against Electron via node-gyp + the vendored libs), then # packages the AppImage/deb/rpm/pacman targets declared in package.json. diff --git a/.github/workflows/build-windows.yml b/.github/workflows/build-windows.yml index 1812c57..c32cf7f 100644 --- a/.github/workflows/build-windows.yml +++ b/.github/workflows/build-windows.yml @@ -15,6 +15,22 @@ on: push: tags: - "v*" + # Same rules as build-linux.yml: PRs build and verify, never publish, and the + # filter exempts documentation rather than allow-listing code. + pull_request: + branches: + - main + paths-ignore: + - "**.md" + - "site/**" + - "images/**" + - "LICENSE" + +# One build per ref, cancelling superseded PR runs only — never a publishing tag +# build. See build-linux.yml for why. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} # Needed for electron-builder to upload artifacts to the GitHub Release on a # tag push. The default GITHUB_TOKEN is read-only, so publishing fails with @@ -96,6 +112,19 @@ jobs: working-directory: ui/electron run: npm ci --ignore-scripts + # Built here (not left to electron-builder) so the audit below runs before + # a tag push publishes the installers. See build-linux.yml for the why. + - name: Build native addon for Electron + working-directory: ui/electron + run: npx electron-rebuild --only gitbox-addon --force + + # Guards issue #1. On Windows this checks the addon imports no DLL beyond + # the system set (dumpbin comes from the MSVC environment above); the + # symbol-level audit needs nm and skips itself here. + - name: Verify the native addon is self-contained + working-directory: core/addon + run: npm test + # electron-builder runs @electron/rebuild first (compiling gitbox_addon # and node-pty against Electron via node-gyp + the vendored libs), then # packages the NSIS and MSI targets declared in package.json. diff --git a/core/addon/binding.gyp b/core/addon/binding.gyp index 8e3f5af..fe0f9e6 100644 --- a/core/addon/binding.gyp +++ b/core/addon/binding.gyp @@ -24,13 +24,25 @@ }, "conditions": [ ["OS=='linux'", { + # Archive order matters: libssh2 pulls symbols out of libcrypto, so + # libcrypto.a must come after it. Linking the vendored static + # libcrypto is NOT optional — libssh2's crypt-method table holds + # direct data relocations to EVP_* (R_X86_64_64), which the loader + # must resolve at dlopen time. Leave it out and the addon silently + # borrows the host process's OpenSSL: plain node exports its own + # (so it "works"), but Electron ships BoringSSL and has no + # EVP_des_ede3_cbc, so the app dies at startup with + # "undefined symbol: EVP_des_ede3_cbc" (issue #1). "libraries": [ "<(module_root_dir)/vendor/libgit2/install/lib/libgit2.a", "<(module_root_dir)/vendor/libssh2/install/lib/libssh2.a", + "<(module_root_dir)/vendor/openssl/install/lib/libcrypto.a", "<(module_root_dir)/vendor/mbedtls/install/lib/libmbedtls.a", "<(module_root_dir)/vendor/mbedtls/install/lib/libmbedx509.a", "<(module_root_dir)/vendor/mbedtls/install/lib/libmbedcrypto.a", - "-lrt" + "-lrt", + "-ldl", + "-lpthread" ], "ldflags": [ "-Wl,-Bsymbolic", @@ -38,9 +50,11 @@ ] }], ["OS=='mac'", { + # Same libssh2 -> libcrypto rule as Linux; see the note above. "libraries": [ "<(module_root_dir)/vendor/libgit2/install/lib/libgit2.a", "<(module_root_dir)/vendor/libssh2/install/lib/libssh2.a", + "<(module_root_dir)/vendor/openssl/install/lib/libcrypto.a", "<(module_root_dir)/vendor/mbedtls/install/lib/libmbedtls.a", "<(module_root_dir)/vendor/mbedtls/install/lib/libmbedx509.a", "<(module_root_dir)/vendor/mbedtls/install/lib/libmbedcrypto.a" diff --git a/core/addon/package.json b/core/addon/package.json index 0eb3c6b..a70ee0d 100644 --- a/core/addon/package.json +++ b/core/addon/package.json @@ -5,7 +5,8 @@ "main": "index.js", "gypfile": true, "scripts": { - "build": "node-gyp rebuild" + "build": "node-gyp rebuild", + "test": "node --test test/self-contained.test.js" }, "dependencies": { "bindings": "^1.5.0", diff --git a/core/addon/test/self-contained.test.js b/core/addon/test/self-contained.test.js new file mode 100644 index 0000000..cbc00be --- /dev/null +++ b/core/addon/test/self-contained.test.js @@ -0,0 +1,271 @@ +'use strict'; + +// Regression tests for the promise that makes GitBox portable: gitbox_addon.node +// must carry libgit2, libssh2, mbedTLS and OpenSSL *inside* itself and import +// nothing from the host beyond the C/C++ runtime and Node's own N-API. +// +// This is what broke in issue #1. binding.gyp linked libssh2.a but not the +// vendored libcrypto.a, so libssh2's crypt-method table kept unresolved data +// relocations (R_X86_64_64) to EVP_des_ede3_cbc & friends. Nothing failed at +// build time, and nothing failed under plain `node` either — node exports its +// own statically linked OpenSSL, so the addon quietly borrowed those symbols. +// Electron ships BoringSSL and exports no EVP_des_ede3_cbc, so the packaged app +// died at startup on any machine that did not happen to have a libcrypto +// already mapped into the process: +// +// Error: .../gitbox_addon.node: undefined symbol: EVP_des_ede3_cbc +// +// Because "does it load?" depends on what the host has lying around, the +// authoritative test here is the static import audit, not a runtime load. +// +// Run with: npm test (from core/addon, after building the addon) +// +// That script names this file explicitly. It is the only form that works +// everywhere: cmd.exe does not expand "test/*.test.js", `--test ` works on +// node 20 but not 24, and `--test ` works on 24 but not 20. New test +// files have to be added to the script by hand. + +const test = require('node:test'); +const assert = require('node:assert'); +const fs = require('node:fs'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +// GITBOX_ADDON_PATH lets this audit run against any build of the addon (a +// packaged one, or a deliberately broken one to prove the audit still bites). +const ADDON = process.env.GITBOX_ADDON_PATH + ? path.resolve(process.env.GITBOX_ADDON_PATH) + : path.join(__dirname, '..', 'build', 'Release', 'gitbox_addon.node'); + +// Symbols that must never be imported: they belong to the libraries we vendor +// and statically link, so seeing one as *undefined* means an archive is missing +// from binding.gyp and the host is being asked to supply it. +const VENDORED_SYMBOLS = [ + // OpenSSL / BoringSSL surface (libssh2's crypto backend) + /^(EVP|SSL|BIO|ERR|RSA|DSA|DH|EC|ECDSA|ECDH|BN|X509|ASN1|PEM|HMAC|CMAC|OPENSSL|OSSL|CRYPTO|RAND|OBJ|NCONF|CONF|UI|ENGINE|PKCS|SHA1|SHA224|SHA256|SHA384|SHA512|MD5|AES|DES|d2i|i2d)_/, + /^(libssh2|git|mbedtls|psa)_/, // libssh2, libgit2, mbedTLS + /^(deflate|inflate|compress|uncompress|crc32|adler32|zlib)/, // bundled zlib +]; + +const ALLOWED_DEPS = { + linux: [ + /^libc\.so\.\d+$/, + /^libm\.so\.\d+$/, + /^libstdc\+\+\.so\.\d+$/, + /^libgcc_s\.so\.\d+$/, + /^libdl\.so\.\d+$/, + /^libpthread\.so\.\d+$/, + /^librt\.so\.\d+$/, + /^ld-linux.*\.so.*$/, + ], + darwin: [/^\/usr\/lib\//, /^\/System\/Library\//], + win32: [ + /^(KERNEL32|USER32|ADVAPI32|CRYPT32|WS2_32|RPCRT4|SECUR32|OLE32|BCRYPT|SHELL32|SHLWAPI|OLEAUT32|VERSION|DBGHELP|WINMM|PSAPI|USERENV|IPHLPAPI|api-ms-win-.*|VCRUNTIME.*|ucrtbase|MSVCP.*|node)\.(dll|DLL|exe|EXE)$/i, + ], +}; + +function tool(name) { + const probe = spawnSync(process.platform === 'win32' ? 'where' : 'which', [name]); + return probe.status === 0; +} + +function run(cmd, args) { + const out = spawnSync(cmd, args, { encoding: 'utf8', maxBuffer: 64 * 1024 * 1024 }); + if (out.status !== 0) throw new Error(`${cmd} ${args.join(' ')} failed: ${out.stderr}`); + return out.stdout; +} + +// Undefined (imported) dynamic symbols, version suffix stripped. +function importedSymbols(file) { + const flag = process.platform === 'darwin' ? '-u' : '-D --undefined-only'; + const out = run('nm', [...flag.split(' '), file]); + return [ + ...new Set( + out + .split('\n') + .map((line) => { + const m = line.match(/^\s*(?:[0-9a-f]*\s+)?U\s+(\S+)$/) || line.match(/^\s*(_?\w[\w.$]*)$/); + return m ? m[1].replace(/@.*$/, '') : null; + }) + .filter(Boolean) + // Mach-O prefixes every C symbol with an underscore. + .map((s) => (process.platform === 'darwin' ? s.replace(/^_/, '') : s)), + ), + ]; +} + +// Symbols the file defines itself (static symbol table, not just exports: the +// linker flags hide the vendored ones from .dynsym on purpose). +function definedSymbols(file) { + const out = run('nm', ['--defined-only', file]); + return new Set( + out + .split('\n') + .map((line) => line.trim().split(/\s+/).pop()) + .filter(Boolean) + .map((s) => (process.platform === 'darwin' ? s.replace(/^_/, '') : s)), + ); +} + +function dynamicDeps(file) { + if (process.platform === 'linux') { + return run('readelf', ['-d', file]) + .split('\n') + .map((l) => l.match(/\(NEEDED\)\s+Shared library: \[(.+)\]/)) + .filter(Boolean) + .map((m) => m[1]); + } + if (process.platform === 'darwin') { + return run('otool', ['-L', file]) + .split('\n') + .slice(1) + .map((l) => l.trim().split(/\s+/)[0]) + .filter(Boolean); + } + return run('dumpbin', ['/dependents', file]) + .split('\n') + .map((l) => l.trim()) + .filter((l) => /\.dll$/i.test(l)); +} + +// Every symbol the C runtime we link against can supply, so the audit does not +// need a hand-maintained list of libc names. +function runtimeExports(file) { + const exports = new Set(); + for (const line of run('ldd', [file]).split('\n')) { + // "libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x…)" plus the dynamic + // loader's own "/lib64/ld-linux-x86-64.so.2 (0x…)" line, which has no + // "=>" — miss it and __tls_get_addr looks like an unresolved import. + const m = line.match(/=>\s*(\/\S+)/) || line.match(/^\s*(\/\S+)\s+\(0x/); + if (!m) continue; + for (const sym of run('nm', ['-D', '--defined-only', m[1]]).split('\n')) { + const name = sym.trim().split(/\s+/).pop(); + if (name) exports.add(name.replace(/@.*$/, '')); + } + } + return exports; +} + +const EXPORTS = ['status', 'log', 'branches', 'fetch', 'pull', 'push', 'clone', 'mergeBranch']; + +const built = fs.existsSync(ADDON); +const canInspect = tool('nm') && (process.platform !== 'linux' || tool('readelf')); + +const skip = built ? false : `${ADDON} not built — run "npm run build" first`; + +// Keep these checks flat. node 20 (what CI runs) does not await subtests +// declared inside a synchronous parent test: it cancels them and reports +// "test did not finish before its parent", so a nested suite passes locally on +// node 22+ while quietly running one of six checks on CI. + +test('imports nothing from the vendored crypto/git libraries (issue #1)', { skip }, (t) => { + if (!canInspect) return t.skip('nm/readelf unavailable on this host'); + + const leaked = importedSymbols(ADDON).filter((s) => VENDORED_SYMBOLS.some((re) => re.test(s))); + + assert.deepStrictEqual( + leaked, + [], + `The addon expects the host to provide ${leaked.length} symbol(s) that should be ` + + `statically linked into it: ${leaked.slice(0, 12).join(', ')}. This is issue #1: an ` + + `archive is missing from binding.gyp's libraries list (libssh2 needs the vendored ` + + `libcrypto.a after it), so the app dies at startup under Electron with ` + + `"undefined symbol: ${leaked[0]}".`, + ); +}); + +test('every imported symbol resolves from the C runtime or N-API', { skip }, (t) => { + if (process.platform !== 'linux' || !canInspect || !tool('ldd')) { + return t.skip('ldd-based resolution check runs on Linux only'); + } + + const available = runtimeExports(ADDON); + const unresolved = importedSymbols(ADDON).filter( + (s) => !available.has(s) && !/^(napi_|node_api_)/.test(s), + ); + + assert.deepStrictEqual( + unresolved, + [], + `Imported symbols that no linked library provides: ${unresolved.join(', ')}. ` + + `They would only resolve if the host process happened to have a matching ` + + `library mapped in — exactly the fragility behind issue #1.`, + ); +}); + +test('links no shared library beyond the C/C++ runtime', { skip }, (t) => { + const allowed = ALLOWED_DEPS[process.platform]; + if (!allowed) return t.skip(`no dependency allowlist for ${process.platform}`); + if (process.platform === 'win32' && !tool('dumpbin')) return t.skip('dumpbin unavailable'); + + const unexpected = dynamicDeps(ADDON).filter((dep) => !allowed.some((re) => re.test(dep))); + + assert.deepStrictEqual( + unexpected, + [], + `The addon must not depend on system libraries: ${unexpected.join(', ')}. ` + + `GitBox ships no git/libgit2/OpenSSL runtime, so anything here breaks the ` + + `AppImage and every distro whose version differs.`, + ); +}); + +test('statically contains libgit2, libssh2, mbedTLS and OpenSSL', { skip }, (t) => { + if (!canInspect) return t.skip('nm unavailable on this host'); + if (process.platform === 'win32') return t.skip('SSH/OpenSSL are not enabled on Windows yet'); + + const defined = definedSymbols(ADDON); + // One representative symbol per vendored library. EVP_des_ede3_cbc is + // the one issue #1 reported missing, so it is the canary here. + for (const symbol of [ + 'git_libgit2_init', + 'libssh2_session_init_ex', + 'mbedtls_ssl_init', + 'EVP_des_ede3_cbc', + ]) { + assert.ok( + defined.has(symbol), + `${symbol} is not compiled into the addon — its library was dropped from ` + + `binding.gyp or built without the feature.`, + ); + } +}); + +// Node-API keeps the addon ABI-stable, so this loads under any node or Electron +// version — the point is that it loads at all. +test('loads and exposes the native git API', { skip }, () => { + const addon = require(ADDON); + for (const fn of EXPORTS) assert.strictEqual(typeof addon[fn], 'function', `missing export: ${fn}`); +}); + +// The runtime that actually broke in issue #1. Weaker than the audits above (a +// host with a compatible libcrypto already mapped into the process can mask a +// missing archive), but it is the exact failing path users hit. +test('loads inside Electron, the runtime that ships BoringSSL', { skip }, (t) => { + const electron = electronBinary(); + if (!electron) return t.skip('no electron binary installed under ui/electron'); + + const probe = spawnSync( + electron, + ['-e', `console.log(Object.keys(require(${JSON.stringify(ADDON)})).join(','))`], + { encoding: 'utf8', env: { ...process.env, ELECTRON_RUN_AS_NODE: '1' } }, + ); + + assert.strictEqual( + probe.status, + 0, + `Electron could not load the addon: ${(probe.stderr || '').trim()}`, + ); + const keys = probe.stdout.trim().split('\n').pop().split(','); + for (const fn of EXPORTS) assert.ok(keys.includes(fn), `missing export: ${fn}`); +}); + +function electronBinary() { + try { + const pkg = path.join(__dirname, '..', '..', '..', 'ui', 'electron', 'node_modules', 'electron'); + const exe = fs.readFileSync(path.join(pkg, 'path.txt'), 'utf8').trim(); + const full = path.join(pkg, 'dist', exe); + return fs.existsSync(full) ? full : null; + } catch { + return null; + } +} diff --git a/core/addon/vendor/build-libgit2.sh b/core/addon/vendor/build-libgit2.sh index 993d473..bdaa5ed 100755 --- a/core/addon/vendor/build-libgit2.sh +++ b/core/addon/vendor/build-libgit2.sh @@ -69,6 +69,10 @@ if [ -d "$MBED_PREFIX/lib64" ] && [ ! -e "$MBED_PREFIX/lib" ]; then ln -s lib64 # OpenSSH 7.8+ would simply be rejected. Verified against a real server: ECDSA # authenticated, RSA-in-OpenSSH-format did not. OpenSSL covers every key type # users actually have, so it is built statically here and linked into libssh2. +# +# NOTE: libcrypto.a must ALSO be listed in binding.gyp after libssh2.a. Linking +# libssh2 alone leaves its EVP_* references to the host process (issue #1), and +# Electron's BoringSSL does not provide them. echo ">> OpenSSL $OPENSSL_TAG -> $SSL_PREFIX" if [ ! -f "$SSL_SRC/Configure" ]; then rm -rf "$SSL_SRC"