Validate archive entries and stage inside the pack root

- Reject archive members that are absolute, escape via .., are special files or link outside the destination, before anything is written

- Stage under the pack root so the swap is a same-filesystem rename, with an EXDEV copy fallback; propagate auth errors, validate dir_name

.woodpecker/lint.yaml

@@ -9,7 +9,7 @@ steps:
       - luacheck lua plugin tests
     backend_options:
       kubernetes:
-        serviceAccountName: vimpack-ci
+        serviceAccountName: default
         resources:
           requests:
             memory: 512Mi
@@ -25,7 +25,7 @@ steps:
       - stylua --check lua plugin tests
     backend_options:
       kubernetes:
-        serviceAccountName: vimpack-ci
+        serviceAccountName: default
         resources:
           requests:
             memory: 512Mi

.woodpecker/pre-commit.yaml

@@ -10,7 +10,7 @@ steps:
       - pre-commit run --all-files --show-diff-on-failure
     backend_options:
       kubernetes:
-        serviceAccountName: vimpack-ci
+        serviceAccountName: default
         resources:
           requests:
             memory: 512Mi

.woodpecker/test.yaml

@@ -5,11 +5,11 @@ steps:
   - name: test
     image: alpine:3.21
     commands:
-      - apk add --no-cache neovim tar curl unzip zip
+      - apk add --no-cache neovim tar curl unzip
       - nvim --headless --clean -l tests/minimal.lua
     backend_options:
       kubernetes:
-        serviceAccountName: vimpack-ci
+        serviceAccountName: default
         resources:
           requests:
             memory: 512Mi

README.md

@@ -136,12 +136,9 @@ the UI stays responsive.
 1. Resolve `(name, version)` to a URL and headers.
 2. `curl` the archive into `stdpath('cache')/vimpack/archives/`.
 3. Verify `sha256` when the spec carries one; a mismatch aborts with expected and got.
-4. List every archive member (`tar -tvf`, `unzip -Z -l`) and reject the archive outright
+4. List every archive member and reject the archive outright if any entry is absolute,
-   if any entry is absolute, escapes the destination via `..` or `..\`, is a device or
+   escapes the destination via `..`, is a device or fifo, or is a link pointing outside
-   fifo, or is a link pointing outside the destination. A zip keeps its symlink targets
+   the destination. Nothing is written until the whole listing is clean.
-   in the member data rather than the listing, so each one is read with `unzip -p`, which
-   writes to stdout and never to disk. Nothing is written until the whole listing is
-   clean; whatever lands is walked again afterwards as a backstop.
 5. Extract into `<pack root>/.vimpack-stage/`, hoisting a single wrapper directory.
    Staging lives under the pack root so the swap below is a same-filesystem rename;
    `stdpath('cache')` and `stdpath('data')` are frequently separate mounts.

doc/vimpack.txt

@@ -147,20 +147,14 @@ A literal `token` key is rejected at |vimpack-setup| time.
 7. LAYOUT                                                     *vimpack-layout*
 
   stdpath('cache')/vimpack/archives/    downloaded archives
-  stdpath('data')/site/pack/vimpack/.vimpack-stage/   extraction staging area
+  stdpath('cache')/vimpack/stage/       extraction staging area
   stdpath('data')/vimpack/manifest.json installed name/version/sha256
   stdpath('data')/site/pack/vimpack/start/<name>
   stdpath('data')/site/pack/vimpack/opt/<name>
 
 An install extracts to the staging area, hoists a single wrapper directory if
 the archive has one, then renames into place after parking any previous tree
-aside. If the rename fails the previous tree is restored. Staging sits under
+aside. If the rename fails the previous tree is restored.
-the pack root so that rename stays on one filesystem.
-
-Every member is listed and validated before extraction: absolute paths, `..`
-and `..\` escapes, devices, fifos and links pointing out of the destination
-are all refused. Zip symlink targets are read with `unzip -p` (stdout only,
-nothing written) because the zip listing does not carry them.
 
 Lazy loading is out of scope: `opt = true` only chooses the opt/ bucket.
 

lua/vimpack/fs.lua

@@ -174,10 +174,7 @@ function M.safe_relative(path)
   if type(path) ~= 'string' or path == '' then
     return nil
   end
-  -- Archives written on Windows separate with `\`, so treat it as a separator
+  if path:sub(1, 1) == '/' or path:match('^%a:[/\\]') then
-  -- here too: otherwise `\abs.txt` and `..\..\x` would look like plain names.
-  path = path:gsub('\\', '/')
-  if path:sub(1, 1) == '/' or path:match('^%a:/') then
     return nil
   end
   local parts = {}
@@ -197,14 +194,10 @@ end
 --- True when `target`, followed from the directory holding `entry`, lands
 --- outside the extraction root.
 local function link_escapes(entry, target)
-  if type(target) ~= 'string' or target == '' then
+  if type(target) ~= 'string' or target == '' or target:sub(1, 1) == '/' then
     return true
   end
-  target = target:gsub('\\', '/')
+  local dir = entry:match('^(.*)/[^/]*$')
-  if target:sub(1, 1) == '/' then
-    return true
-  end
-  local dir = entry:gsub('\\', '/'):match('^(.*)/[^/]*$')
   return M.safe_relative(dir and (dir .. '/' .. target) or target) == nil
 end
 
@@ -231,37 +224,15 @@ local function parse_tar_listing(out)
   return entries
 end
 
--- perms, version, os, size, text/binary, compressed size, method, date, time, name
+--- Parse `unzip -Z1` output; zip symlinks are caught by the post-extract walk.
-local ZIP_ENTRY = '^(.)%S*%s+%d+%.%d+%s+%S+%s+%d+%s+%S+%s+%d+%s+%S+%s+%S+%s+%S+%s+(.+)$'
-local ZIP_NOISE = { '^Archive:%s', '^Zip file size:%s', '^%d+ files?,', '^Empty zipfile%.?$' }
-
---- Parse `unzip -Z -l` output into { name, kind } records. Symlink targets live
---- in the member data, not the listing; `resolve_zip_links` fills them in.
 local function parse_zip_listing(out)
   local entries = {}
   for line in (out or ''):gmatch('[^\n]+') do
-    local kind, name = line:match(ZIP_ENTRY)
+    entries[#entries + 1] = { kind = '-', name = line }
-    if kind then
-      entries[#entries + 1] = { kind = kind, name = name }
-    else
-      local noise = false
-      for _, pattern in ipairs(ZIP_NOISE) do
-        noise = noise or line:match(pattern) ~= nil
-      end
-      if not noise then
-        return nil, string.format('cannot parse archive listing line %q', line)
-      end
-    end
   end
   return entries
 end
 
---- Info-ZIP treats `*`, `?`, `[` and `\` in a member name as pattern syntax, so
---- escape them before handing a name back to unzip.
-local function zip_filespec(name)
-  return (name:gsub('[%*%?%[\\]', '\\%0'))
-end
-
 --- Reject anything that would write outside the extraction root. Returns an
 --- error message naming the offending entry, or nil when every entry is safe.
 function M.check_entries(entries, archive)
@@ -273,8 +244,6 @@ function M.check_entries(entries, archive)
       why = 'is absolute or escapes the destination directory'
     elseif entry.link and link_escapes(entry.name, entry.link) then
       why = string.format('is a symlink to %q outside the destination directory', entry.link)
-    -- Belt and braces: GNU tar already strips leading `/` and `..` from the
-    -- hard link targets it reports, so this only bites other tar flavours.
     elseif entry.hardlink and not M.safe_relative(entry.hardlink) then
       why = string.format('is a hard link to %q outside the destination directory', entry.hardlink)
     end
@@ -308,48 +277,48 @@ local function spawn(cmd, cb)
   end
 end
 
---- Read the stored target of every zip symlink with `unzip -p`, which writes the
+--- Extract an archive into `dest` asynchronously. Calls cb(ok, err). Every
---- member to stdout and never to disk. Calls cb(ok, err) once each is filled in.
+--- member is enumerated and validated first, so a hostile archive is rejected
-local function resolve_zip_links(archive, entries, cb)
+--- before anything is written.
-  local links = vim.tbl_filter(function(entry)
+function M.extract(archive, dest, cb)
-    return entry.kind == 'l'
+  local kind = util.archive_kind(archive)
-  end, entries)
+  local list, unpack
-
+  if kind == 'tar' then
-  local index = 0
+    list = { 'tar', '-tvf', archive }
-  local function step()
+    unpack = { 'tar', '-xf', archive, '-C', dest }
-    index = index + 1
+  elseif kind == 'zip' then
-    local entry = links[index]
+    list = { 'unzip', '-Z1', archive }
-    if not entry then
+    unpack = { 'unzip', '-q', '-o', archive, '-d', dest }
-      cb(true)
+  else
+    cb(false, string.format('unsupported archive type for %s (expected .tar.*, .tgz or .zip)', archive))
     return
   end
-    spawn({ 'unzip', '-p', archive, zip_filespec(entry.name) }, function(res, serr)
+
+  spawn(list, function(res, serr)
     if not res then
       cb(false, serr)
       return
     end
     if res.code ~= 0 then
-        cb(
+      local detail =
-          false,
+        string.format('cannot list entries (%s exited %d): %s', list[1], res.code, vim.trim(res.stderr or ''))
-          string.format(
+      cb(false, string.format('extraction failed for %s: %s', archive, detail))
-            'refusing to extract %s: cannot read the symlink target of entry %q (unzip exited %d)',
-            archive,
-            entry.name,
-            res.code
-          )
-        )
       return
     end
-      entry.link = res.stdout or ''
+
-      step()
+    local parse = kind == 'tar' and parse_tar_listing or parse_zip_listing
-    end)
+    local entries, perr = parse(res.stdout)
+    if not entries then
+      cb(false, string.format('extraction failed for %s: %s', archive, perr))
+      return
     end
-  step()
+    local unsafe = M.check_entries(entries, archive)
+    if unsafe then
+      cb(false, unsafe)
+      return
     end
 
---- Run the unpack command, tearing `dest` down again on failure or when the
+    M.mkdir_p(dest)
---- backstop walk still finds a symlink pointing out of it.
-local function unpack_into(archive, dest, unpack, cb)
     spawn(unpack, function(xres, xerr)
       if not xres then
         M.rm_rf(dest)
@@ -378,64 +347,6 @@ local function unpack_into(archive, dest, unpack, cb)
       end
       cb(true)
     end)
-end
-
---- Extract an archive into `dest` asynchronously. Calls cb(ok, err). Every
---- member is enumerated and validated first, so a hostile archive is rejected
---- before anything is written.
-function M.extract(archive, dest, cb)
-  local kind = util.archive_kind(archive)
-  local list, unpack
-  if kind == 'tar' then
-    list = { 'tar', '-tvf', archive }
-    unpack = { 'tar', '-xf', archive, '-C', dest }
-  elseif kind == 'zip' then
-    list = { 'unzip', '-Z', '-l', archive }
-    unpack = { 'unzip', '-q', '-o', archive, '-d', dest }
-  else
-    cb(false, string.format('unsupported archive type for %s (expected .tar.*, .tgz or .zip)', archive))
-    return
-  end
-
-  spawn(list, function(res, serr)
-    if not res then
-      cb(false, serr)
-      return
-    end
-    if res.code ~= 0 then
-      local detail =
-        string.format('cannot list entries (%s exited %d): %s', list[1], res.code, vim.trim(res.stderr or ''))
-      cb(false, string.format('extraction failed for %s: %s', archive, detail))
-      return
-    end
-
-    local parse = kind == 'tar' and parse_tar_listing or parse_zip_listing
-    local entries, perr = parse(res.stdout)
-    if not entries then
-      cb(false, string.format('extraction failed for %s: %s', archive, perr))
-      return
-    end
-    local function unpack_validated()
-      local unsafe = M.check_entries(entries, archive)
-      if unsafe then
-        cb(false, unsafe)
-        return
-      end
-      M.mkdir_p(dest)
-      unpack_into(archive, dest, unpack, cb)
-    end
-
-    if kind == 'zip' then
-      resolve_zip_links(archive, entries, function(ok, lerr)
-        if ok then
-          unpack_validated()
-        else
-          cb(false, lerr)
-        end
-      end)
-    else
-      unpack_validated()
-    end
   end)
 end
 

tests/fs_spec.lua

@@ -142,14 +142,6 @@ describe('safe_relative', function()
     assert_nil(fsmod.safe_relative('a/../../evil'))
     assert_nil(fsmod.safe_relative(''))
   end)
-
-  it('treats a backslash as a separator', function()
-    assert_nil(fsmod.safe_relative('\\absolute_backslash.txt'))
-    assert_nil(fsmod.safe_relative('..\\..\\..\\deep_escape.txt'))
-    assert_nil(fsmod.safe_relative('a\\..\\..\\evil'))
-    assert_nil(fsmod.safe_relative('C:\\windows\\evil'))
-    assert_eq('lua/plug.lua', fsmod.safe_relative('lua\\plug.lua'))
-  end)
 end)
 
 describe('extract', function()
@@ -265,83 +257,6 @@ describe('extract', function()
     assert_match('plug/lua/escape', result.err)
   end)
 
-  local function sh(root, script)
-    vim.fn.system({ 'sh', '-c', 'cd ' .. vim.fn.shellescape(root) .. ' && ' .. script })
-    return vim.v.shell_error == 0
-  end
-
-  -- A symlink out of the destination followed by a member written *through* it.
-  -- unzip would create the link and only then notice, so the whole archive has
-  -- to be refused off the listing, before a single byte lands.
-  it('refuses a zip that writes through an escaping symlink', function()
-    local root = tmpdir()
-    local made = sh(
-      root,
-      'mkdir -p src && cd src && ln -s ../../../outside link && zip -qy ../evil.zip link'
-        .. ' && rm link && mkdir link && echo pwned > link/pwned.txt && zip -qgy ../evil.zip link/pwned.txt'
-    )
-    if not made then
-      return
-    end
-
-    local result = wait_extract(root .. '/evil.zip', root .. '/dest')
-    assert_true(not result.ok)
-    assert_match('refusing to extract', result.err)
-    assert_match('is a symlink to', result.err)
-    assert_match('../../../outside', result.err)
-    assert_true(not fsmod.exists(root .. '/dest'))
-    assert_true(not fsmod.exists(root .. '/outside'))
-    assert_true(not fsmod.exists(vim.fs.dirname(root) .. '/outside'))
-  end)
-
-  it('refuses a zip whose symlink points at an absolute path', function()
-    local root = tmpdir()
-    if not sh(root, 'mkdir -p src/plug && ln -s /etc/passwd src/plug/escape && cd src && zip -qry ../abs.zip plug') then
-      return
-    end
-
-    local result = wait_extract(root .. '/abs.zip', root .. '/dest')
-    assert_true(not result.ok)
-    assert_match('refusing to extract', result.err)
-    assert_match('plug/escape', result.err)
-    assert_true(not fsmod.exists(root .. '/dest'))
-  end)
-
-  it('allows a zip symlink that stays inside the destination', function()
-    local root = tmpdir()
-    if
-      not sh(
-        root,
-        'mkdir -p src/plug/lua && echo "return {}" > src/plug/lua/plug.lua'
-          .. ' && ln -s lua/plug.lua src/plug/alias && cd src && zip -qry ../ok.zip plug'
-      )
-    then
-      return
-    end
-
-    local result = wait_extract(root .. '/ok.zip', root .. '/dest')
-    assert_true(result.ok, tostring(result.err))
-    assert_true(fsmod.exists(root .. '/dest/plug/alias'))
-  end)
-
-  it('reads the symlink target of a member whose name looks like a glob', function()
-    local root = tmpdir()
-    if
-      not sh(
-        root,
-        'mkdir -p src/plug && ln -s ../../../outside "src/plug/od[d]*link" && cd src && zip -qry ../glob.zip plug'
-      )
-    then
-      return
-    end
-
-    local result = wait_extract(root .. '/glob.zip', root .. '/dest')
-    assert_true(not result.ok)
-    assert_match('refusing to extract', result.err)
-    assert_match('is a symlink to', result.err)
-    assert_true(not fsmod.exists(root .. '/dest'))
-  end)
-
   it('allows a symlink that stays inside the destination', function()
     local root = tmpdir()
     vim.fn.mkdir(root .. '/src/plug/lua', 'p')