From e5a260ad25a44c2ac01dd7d12953fb20e24e3dcf Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 01:04:53 +1000 Subject: [PATCH] Fetch the ref and url the spec actually names A ref goes into the archive URL verbatim and an explicit url goes to curl as a positional, so both could name one thing and fetch another. - reject "#", "?", "%", "[", "]", "{" and "}" in a tag or branch, which curl or the server would turn into a different path - pass "--" before the url so curl cannot read it as an option - document the ref rules in README and the help file --- README.md | 6 ++++-- doc/arti-pack.txt | 7 ++++--- lua/arti-pack/installer.lua | 4 ++++ lua/arti-pack/spec.lua | 7 ++++++- tests/installer_spec.lua | 37 ++++++++++++++++++++++++++++++++++ tests/spec_spec.lua | 40 +++++++++++++++++++++++++++++++++++++ 6 files changed, 95 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index b6a3424..8bb085e 100644 --- a/README.md +++ b/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 diff --git a/doc/arti-pack.txt b/doc/arti-pack.txt index 79eb818..323d4ad 100644 --- a/doc/arti-pack.txt +++ b/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: > ///archive/refs/tags/. diff --git a/lua/arti-pack/installer.lua b/lua/arti-pack/installer.lua index f893227..93f50a9 100644 --- a/lua/arti-pack/installer.lua +++ b/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, } diff --git a/lua/arti-pack/spec.lua b/lua/arti-pack/spec.lua index ae6d5c3..a49bbe8 100644 --- a/lua/arti-pack/spec.lua +++ b/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) diff --git a/tests/installer_spec.lua b/tests/installer_spec.lua index e60db8a..4182a36 100644 --- a/tests/installer_spec.lua +++ b/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) diff --git a/tests/spec_spec.lua b/tests/spec_spec.lua index 5f7554c..07363fa 100644 --- a/tests/spec_spec.lua +++ b/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' })