fix: Atomic download checks for binaries + codesign for macos (#252)

Author: dmtrKovalenko

.github/workflows/external-tests.yml

@@ -14,6 +14,8 @@ jobs:
   lua-tests:
     name: e2e (${{ matrix.os }})
     runs-on: ${{ matrix.os }}
+    # e2e tests could be flaky on CI so we do not block release creation if they failed
+    continue-on-error: ${{ github.ref == 'refs/heads/main' && github.event_name == 'push' }}
     strategy:
       fail-fast: false
       matrix:

.github/workflows/release.yaml

@@ -84,6 +84,10 @@ jobs:
           MACOSX_DEPLOYMENT_TARGET="13" cargo build --release --target ${{ matrix.target }} -p fff-nvim
           mv "${{ matrix.artifact_name }}" "${{ matrix.target }}.${{ matrix.ext }}"
 
+      - name: Ad-hoc sign macOS binary
+        if: contains(matrix.os, 'macos')
+        run: codesign --force --sign - "${{ matrix.target }}.${{ matrix.ext }}"
+
       - name: Build for Windows
         if: contains(matrix.os, 'windows')
         shell: bash
@@ -190,6 +194,10 @@ jobs:
           MACOSX_DEPLOYMENT_TARGET="13" cargo build --release --target ${{ matrix.target }} -p fff-c
           mv "${{ matrix.artifact_name }}" "c-lib-${{ matrix.target }}.${{ matrix.ext }}"
 
+      - name: Ad-hoc sign macOS binary
+        if: contains(matrix.os, 'macos')
+        run: codesign --force --sign - "c-lib-${{ matrix.target }}.${{ matrix.ext }}"
+
       - name: Build for Windows
         if: contains(matrix.os, 'windows')
         shell: bash

lua/fff/download.lua

@@ -25,7 +25,26 @@ end
 local function binary_exists(plugin_dir)
   local binary_path = get_binary_path(plugin_dir)
   local stat = vim.uv.fs_stat(binary_path)
-  return stat and stat.type == 'file'
+  if stat and stat.type == 'file' then return true end
+
+  -- On Windows the rename over a loaded DLL fails, so a verified binary may be
+  -- left at binary_path .. '.tmp'. Promote it now that the old session is gone.
+  local tmp_path = binary_path .. '.tmp'
+  local tmp_stat = vim.uv.fs_stat(tmp_path)
+  if tmp_stat and tmp_stat.type == 'file' then
+    -- Verify the .tmp is a valid library before promoting it, in case the
+    -- process was killed between the loadlib check and the rename attempt
+    -- during a previous download, leaving a corrupt or partial .tmp on disk.
+    local loader = package.loadlib(tmp_path, 'luaopen_fff_nvim')
+    if not loader then
+      vim.uv.fs_unlink(tmp_path)
+      return false
+    end
+    local ok = vim.uv.fs_rename(tmp_path, binary_path)
+    return ok ~= nil
+  end
+
+  return false
 end
 
 local function download_file(url, output_path, opts, callback)
@@ -53,6 +72,12 @@ local function download_file(url, output_path, opts, callback)
       table.insert(curl_args, opts.proxy)
     end
 
+    if opts.extra_curl_args then
+      for _, arg in ipairs(opts.extra_curl_args) do
+        table.insert(curl_args, arg)
+      end
+    end
+
     table.insert(curl_args, url)
     vim.system(curl_args, {}, function(result)
       if result.code ~= 0 then
@@ -64,39 +89,143 @@ local function download_file(url, output_path, opts, callback)
   end)
 end
 
+--- Verify the SHA256 of a file against an expected hash string.
+--- @param file_path string
+--- @param expected_hash string  lowercase hex SHA256
+--- @param callback fun(ok: boolean, err: string|nil)
+local function verify_sha256(file_path, expected_hash, callback)
+  local cmd
+  local sysname = vim.uv.os_uname().sysname:lower()
+  if sysname:match('windows') then
+    cmd = { 'certutil', '-hashfile', file_path, 'SHA256' }
+  elseif sysname == 'darwin' then
+    cmd = { 'shasum', '-a', '256', file_path }
+  else
+    cmd = { 'sha256sum', file_path }
+  end
+
+  vim.system(cmd, {}, function(result)
+    if result.code ~= 0 then
+      local detail = (result.stderr and result.stderr ~= '' and result.stderr)
+        or (result.stdout and result.stdout ~= '' and result.stdout)
+        or 'unknown error'
+      callback(false, 'sha256 command failed: ' .. detail)
+      return
+    end
+
+    local actual_hash = (result.stdout or ''):match('^%s*([0-9a-fA-F]+)')
+    if not actual_hash then
+      callback(false, 'Could not parse sha256 output: ' .. tostring(result.stdout))
+      return
+    end
+
+    if actual_hash:lower() ~= expected_hash:lower() then
+      callback(false, string.format('SHA256 mismatch: expected %s, got %s', expected_hash, actual_hash:lower()))
+      return
+    end
+
+    callback(true, nil)
+  end)
+end
+
 local function download_from_github(version, binary_path, opts, callback)
   opts = opts or {}
 
   local triple = system.get_triple()
   local extension = system.get_lib_extension()
   local binary_name = triple .. '.' .. extension
   local url = string.format('https://github.com/%s/releases/download/%s/%s', GITHUB_REPO, version, binary_name)
+  local sha_url = url .. '.sha256'
+
   vim.schedule(function()
     vim.notify(string.format('Downloading fff.nvim binary for ' .. version), vim.log.levels.INFO)
     vim.notify(string.format('Do not open fff until you see a success notification.'), vim.log.levels.WARN)
   end)
 
-  download_file(url, binary_path, {
-    proxy = opts.proxy,
-    extra_curl_args = opts.extra_curl_args,
-  }, function(success, err)
-    if not success then
-      callback(false, err)
+  -- Download to a temp path first so we can verify before replacing the live binary.
+  -- If we wrote directly to binary_path and the current process already has the old
+  -- library loaded, package.loadlib() on the same path returns the *cached* handle —
+  -- meaning a partial or corrupt download would pass verification silently.
+  -- Using a distinct temp path forces dlopen to load the new file for real.
+  local tmp_path = binary_path .. '.tmp'
+  local tmp_sha_path = tmp_path .. '.sha256'
+
+  -- Download the SHA256 checksum file first so we can verify the binary.
+  download_file(sha_url, tmp_sha_path, { proxy = opts.proxy }, function(sha_success, sha_err)
+    if not sha_success then
+      callback(false, 'Failed to download sha256: ' .. (sha_err or 'unknown error'))
       return
     end
 
-    -- Verify the binary can be loaded
-    local ok, err_msg = pcall(function() package.loadlib(binary_path, 'luaopen_fff_nvim') end)
+    -- Read expected hash (first token on first line)
+    local sha_file = io.open(tmp_sha_path, 'r')
+    local expected_hash = sha_file and sha_file:read('*l'):match('^%s*([0-9a-fA-F]+)')
+    if sha_file then sha_file:close() end
+    vim.uv.fs_unlink(tmp_sha_path)
 
-    if not ok then
-      vim.uv.fs_unlink(binary_path)
-      callback(false, 'Downloaded binary is not valid: ' .. (err_msg or 'unknown error'))
+    if not expected_hash or #expected_hash ~= 64 then
+      callback(false, 'Invalid sha256 file contents')
       return
     end
 
-    vim.schedule(function() vim.notify('fff.nvim binary downloaded successfully!', vim.log.levels.INFO) end)
-    callback(true, nil)
-  end)
+    download_file(url, tmp_path, {
+      proxy = opts.proxy,
+      extra_curl_args = opts.extra_curl_args,
+    }, function(success, err)
+      if not success then
+        vim.uv.fs_unlink(tmp_path)
+        callback(false, err)
+        return
+      end
+
+      -- Verify integrity before doing anything else with the binary.
+      verify_sha256(tmp_path, expected_hash, function(hash_ok, hash_err)
+        vim.schedule(function()
+          if not hash_ok then
+            vim.uv.fs_unlink(tmp_path)
+            callback(false, 'Binary integrity check failed: ' .. (hash_err or 'unknown error'))
+            return
+          end
+
+          -- Verify the NEW binary (temp path is not yet loaded by this process,
+          -- so dlopen actually loads and validates the downloaded file).
+          -- Note: package.loadlib returns (nil, error_string) on failure rather than throwing,
+          -- so we check the return value directly instead of using pcall.
+          local loader, load_err = package.loadlib(tmp_path, 'luaopen_fff_nvim')
+
+          if not loader then
+            vim.uv.fs_unlink(tmp_path)
+            callback(false, 'Downloaded binary is not valid: ' .. (load_err or 'unknown error'))
+            return
+          end
+
+          -- Atomically replace the live binary only after successful verification.
+          -- On Windows the old .dll may be locked by the current process, so rename can
+          -- fail if fff is already loaded. In that case, leave the verified .tmp on disk
+          -- so the next Neovim start can pick it up automatically.
+          local rename_ok, rename_err = vim.uv.fs_rename(tmp_path, binary_path)
+          if not rename_ok then
+            if vim.uv.os_uname().sysname:lower():match('windows') then
+              vim.notify(
+                'fff.nvim binary downloaded to '
+                  .. tmp_path
+                  .. '.\nThe live binary is locked by the current session — please restart Neovim to apply the update.',
+                vim.log.levels.WARN
+              )
+              callback(true, nil)
+            else
+              vim.uv.fs_unlink(tmp_path)
+              callback(false, 'Failed to install binary: ' .. (rename_err or 'unknown error'))
+            end
+            return
+          end
+
+          vim.notify('fff.nvim binary downloaded successfully!', vim.log.levels.INFO)
+          callback(true, nil)
+        end)
+      end) -- verify_sha256
+    end) -- binary download_file
+  end) -- sha download_file
 end
 
 function M.ensure_downloaded(opts, callback)
@@ -131,7 +260,9 @@ function M.download_binary(callback)
       if callback then
         callback(false, err)
       else
-        error('Failed to download fff.nvim binary: ' .. (err or 'unknown error'))
+        vim.schedule(function()
+          vim.notify('Failed to download fff.nvim binary: ' .. (err or 'unknown error'), vim.log.levels.ERROR)
+        end)
       end
       return
     end