Merge pull request 'Fetch the ref and url the spec actually names' (#8) from benvin/url-significant-refs into main

Reviewed-on: #8

README.md

@@ -93,8 +93,10 @@ and never neither. Plugins install to
 `tag` and `branch` are mutually exclusive: a spec setting both, or neither, is
 rejected with an error naming the plugin and nothing is installed for it. Both
 are one path segment of the archive URL, so both are trimmed of surrounding
-whitespace and rejected if they contain inner whitespace, `/`, `\`, or are `.`
-or `..`. A branch whose name contains `/` therefore needs an explicit `url`.
+whitespace and rejected if they are `.` or `..`, or contain inner whitespace,
+`/`, `\`, or any of `#`, `?`, `%`, `[`, `]`, `{`, `}` — each of those would
+make curl fetch something other than the ref you named. A branch whose name
+contains `/` therefore needs an explicit `url`.
 
 ### Options
 

doc/arti-pack.txt

@@ -74,9 +74,10 @@ Each entry is a table whose first element is "owner/repo".
 Exactly one of "tag" and "branch" is required. A spec that sets both, or
 neither, is rejected with an error naming the plugin, and nothing is installed
 for it. Both are one path segment of the archive URL, so both are trimmed of
-surrounding whitespace and rejected if they contain inner whitespace, "/" or
-"\", or are "." or "..". A branch name containing "/" therefore needs an
-explicit "url".
+surrounding whitespace and rejected if they are "." or "..", or contain inner
+whitespace, "/", "\", or any of "#", "?", "%", "[", "]", "{", "}" — each of
+those would make curl fetch something other than the ref you named. A branch
+name containing "/" therefore needs an explicit "url".
 
 The archive URL is built as: >
     <registry>/<owner>/<repo>/archive/refs/tags/<tag>.<ext>

lua/arti-pack/installer.lua

@@ -116,6 +116,10 @@ local function download(ctx, spec, archive, done)
     tostring(ctx.timeout),
     '-o',
     tmp,
+    -- curl reads a leading-dash positional as an option, so a spec url of
+    -- "--stderr=/path" or "--config=/path" would run as a flag instead of
+    -- being fetched. "--" ends option parsing.
+    '--',
     url,
   }
 

lua/arti-pack/spec.lua

@@ -16,8 +16,13 @@ local function has_nul(s)
   return s:find('\0', 1, true) ~= nil
 end
 
+-- A ref is spliced into the archive URL verbatim, so a character that is
+-- significant to a URL makes the fetched path differ from the validated one:
+-- "#" truncates at the fragment, "?" starts a query, "%" is decoded by the
+-- server ("%2F" reopens the traversal "/" already blocks), and curl expands
+-- "[]" and "{}" as globs into other URLs entirely.
 local function bad_name(name)
-  return name == '' or name == '.' or name == '..' or name:find('[/\\%s]') ~= nil
+  return name == '' or name == '.' or name == '..' or name:find('[/\\%s#?%%%[%]{}]') ~= nil
 end
 
 function M.parse(entry)

tests/installer_spec.lua

@@ -135,6 +135,43 @@ t.describe('installer.run with a branch spec', function()
   end)
 end)
 
+-- curl reads a leading-dash positional as an option, so a url that looks like a
+-- flag must reach it as a url or not at all. These fixtures are built in Lua,
+-- so they run with no zip or tar around.
+t.describe('installer.run with a url that looks like a curl option', function()
+  t.it('does not let a url write a file of its choosing', function()
+    local ctx = new_ctx()
+    local victim = tmpdir() .. '/written-by-curl'
+    local s = assert(spec_mod.parse({ 'owner/sneaky', tag = 'v1', url = '--stderr=' .. victim }))
+    local results = run(ctx, { s })
+
+    t.eq('failed', results[1].status)
+    t.falsy(fs.exists(victim), 'the url ran as a curl option and created a file')
+  end)
+
+  t.it('does not let a url pull curl options in from a config file', function()
+    local ctx = new_ctx()
+    local archive = zipfile.write(tmpdir() .. '/sneaky-v1.zip', 'sneaky-v1', 'first')
+    local conf = write(tmpdir() .. '/curl.conf', string.format('url = "file://%s"\n', archive))
+    local s = assert(spec_mod.parse({ 'owner/sneaky', tag = 'v1', url = '--config=' .. conf }))
+    local results = run(ctx, { s })
+
+    t.eq('failed', results[1].status, 'curl fetched a url the spec never named')
+    t.falsy(fs.exists(ctx.pack_dir .. '/start/sneaky'), 'nothing was installed')
+    t.falsy(ctx.lock:get('sneaky'), 'nothing was recorded')
+  end)
+
+  t.it('still installs from a plain file:// url', function()
+    local ctx = new_ctx()
+    local archive = zipfile.write(tmpdir() .. '/honest-v1.zip', 'honest-v1', 'first')
+    local s = assert(spec_mod.parse({ 'owner/honest', tag = 'v1', url = 'file://' .. archive }))
+    local results = run(ctx, { s })
+
+    t.eq('installed', results[1].status, results[1].err)
+    t.truthy(fs.exists(ctx.pack_dir .. '/start/honest/lua/marker.lua'))
+  end)
+end)
+
 t.describe('installer.run', function()
   if not have('zip') or not have('tar') then
     t.it('SKIPPED: zip/tar unavailable to build fixtures', function() end)

tests/spec_spec.lua

@@ -8,6 +8,20 @@ local NUL = string.char(0)
 -- rejected before it can diverge from what was validated and locked.
 local NULS = { 'mid' .. NUL .. 'dle', 'trailing' .. NUL, NUL }
 
+-- Each of these makes curl fetch something other than the ref it names: "#"
+-- drops the rest of the path, "?" turns it into a query, "%" is decoded by the
+-- server, and "[]"/"{}" are curl globs that fan out into other URLs.
+local URL_SIGNIFICANT = {
+  'v1#frag',
+  'v1?q=1',
+  'v1%2e%2e',
+  'v1%2Fetc',
+  'v1[1-3]',
+  'v1{a,b}',
+  'v1]x',
+  'v1}x',
+}
+
 t.describe('spec.parse', function()
   t.it('parses a full table spec', function()
     local s = assert(spec.parse({ 'nvim-lua/plenary.nvim', tag = 'v0.1.4', opt = true }))
@@ -169,6 +183,32 @@ t.describe('spec.parse', function()
     end
   end)
 
+  t.it('rejects a tag that would change the fetched url', function()
+    for _, bad in ipairs(URL_SIGNIFICANT) do
+      local s, err = spec.parse({ 'owner/repo', tag = bad })
+      t.falsy(s, 'rejects ' .. vim.inspect(bad))
+      t.matches('invalid tag', err)
+    end
+  end)
+
+  t.it('rejects a branch that would change the fetched url', function()
+    for _, bad in ipairs(URL_SIGNIFICANT) do
+      local s, err = spec.parse({ 'owner/repo', branch = bad })
+      t.falsy(s, 'rejects ' .. vim.inspect(bad))
+      t.matches('invalid branch', err)
+    end
+  end)
+
+  -- curl sends these through the path byte for byte, so they still name the ref
+  -- they were validated as. Rejecting them would lock out real upstream tags.
+  t.it('keeps accepting ref characters that reach the server unchanged', function()
+    for _, good in ipairs({ 'v1+2', 'v1@host', 'v1:2', 'release_1.2', 'v1~rc1', 'v1,2', 'v1;x', 'v1&x' }) do
+      local s = spec.parse({ 'owner/repo', tag = good })
+      t.truthy(s, 'accepts ' .. vim.inspect(good))
+      t.eq(REGISTRY .. '/owner/repo/archive/refs/tags/' .. good .. '.zip', spec.url(s, REGISTRY))
+    end
+  end)
+
   t.it('rejects a source carrying a NUL byte', function()
     for _, bad in ipairs(NULS) do
       local s, err = spec.parse({ 'owner/' .. bad, tag = 'v1' })