fix(security): validate untrusted values before git, fs and logs

CHANGELOG.md

@@ -2,6 +2,42 @@
 
 All notable changes to the Vim Plugin Manager will be documented in this file.
 
+## [Unreleased]
+
+### Security
+- **Option injection via `.gitmodules` branch** (#9, HIGH, proven RCE):
+  `submodule.<path>.branch` is passed to `git pull origin <branch>` - a
+  leading-dash value is parsed by git as an option
+  (`--upload-pack=<cmd>` executes the command, demonstrated locally with
+  git 2.43 despite shell quoting).  New `core#util#sanitize_branch`
+  (first char may not be `-`, charset `^[A-Za-z0-9._/-]+$`) applied at
+  the `.gitmodules` read (update pull flow) and at
+  `git submodule add -b`; refused values warn and fall back to the
+  default branch resolution.
+- **Pathspec injection via `.gitmodules` path** (#9, HIGH): module paths
+  reach `git rm -f` / `git submodule deinit -f` as pathspecs - `path = *`
+  would remove every tracked file.  New `core#util#validate_module_path`
+  (refuses `* ? [ ]`, `..`, leading `-`, absolute paths; requires a
+  repo-relative path under `plugins_dir`) applied in `remove` (covers
+  `:PluginManager remove` and `gc`); refused values warn and abort the
+  removal.
+- **Directory traversal via the `dir` option** (#9, MED): clone/install
+  targets accepted `..`, absolute paths and nested paths.  New
+  `core#util#validate_dir_name` (plain basename only) applied to both
+  the dict and the deprecated positional forms; refused values warn and
+  fall back to the default plugin name.
+- **Credential leak** (#9, MED): `https://user:token@host` userinfo no
+  longer reaches pushed commit messages ("Add/Remove <url> plugin"),
+  error messages (REPO_NOT_FOUND), command traces (git#execute,
+  run_in_dir) or sidebar output.  New `core#util#sanitize_url` /
+  `sanitize_cmd` strip the userinfo at every display point; the raw
+  value is still used for the actual git call.
+
+### Added
+- `tests/security.vader`: unit tests for the four validators plus an
+  integration test proving a hostile `.gitmodules` branch never
+  propagates to the pull flow.
+
 ## [2.2.8] - 2026-09-20
 
 ### Fixed

autoload/plugin_manager/cmd/add.vim

@@ -26,7 +26,8 @@ function! plugin_manager#cmd#add#execute(...) abort
 
     " For remote plugins, check repository exists
     if !l:is_local && !plugin_manager#git#repository_exists(l:module_url)
-      call plugin_manager#core#throw('add', 'REPO_NOT_FOUND', 'Repository not found: ' . l:module_url)
+      call plugin_manager#core#throw('add', 'REPO_NOT_FOUND',
+            \ 'Repository not found: ' . plugin_manager#core#util#sanitize_url(l:module_url))
     endif
 
     " Install

autoload/plugin_manager/cmd/declare.vim

@@ -227,7 +227,8 @@ function! s:on_clone_done(ctx, name, url, options, target, result) abort
     else
       call plugin_manager#ui#complete_operation(l:op_id, 'fail', 'Failed')
       call plugin_manager#ui#log_detail('declare',
-            \ 'submodule add failed for ' . a:name . ' (' . a:url . ')',
+            \ 'submodule add failed for ' . a:name . ' ('
+            \ . plugin_manager#core#util#sanitize_url(a:url) . ')',
             \ 'warn')
       let a:ctx.errors += 1
     endif

autoload/plugin_manager/cmd/remove.vim

@@ -167,6 +167,16 @@ function! s:remove_module(module_name, module_path) abort
   " absolute path (filesystem search): normalize so the vim_dir prefix is
   " never applied twice.
   let l:rel_path = plugin_manager#core#util#make_relative_path(a:module_path)
+  " .gitmodules is user-writable content: its path value is passed as a
+  " pathspec to `git rm` / `git submodule deinit` - a glob ('*') would match
+  " every tracked file (issue #9).  Refuse + abort the removal on invalid.
+  let l:rel_path = plugin_manager#core#util#validate_module_path(l:rel_path)
+  if empty(l:rel_path)
+    call plugin_manager#ui#complete_operation(l:op_id, 'fail',
+          \ 'Refusing to remove: untrusted module path (see log)')
+    call plugin_manager#git#refresh_modules_cache()
+    return 0
+  endif
 
   let l:deinit_result = plugin_manager#git#execute(
         \ 'git submodule deinit -f ' . shellescape(l:rel_path), l:vim_dir, 0, 0)
@@ -239,7 +249,8 @@ function! s:commit_removal(module_name, module_info) abort
   let l:vim_dir = plugin_manager#core#util#get_config('vim_dir', '')
 
   if !empty(a:module_info) && has_key(a:module_info, 'url')
-    let l:commit_msg .= " (" . a:module_info.url . ")"
+    " Pushed to remotes: strip credentials from the URL first.
+    let l:commit_msg .= " (" . plugin_manager#core#util#sanitize_url(a:module_info.url) . ")"
   endif
 
   " Stage .gitmodules (updated by git rm); run in vim_dir for repo-root scope.

autoload/plugin_manager/core/util.vim

@@ -115,7 +115,8 @@ function! plugin_manager#core#util#run_in_dir(cmd, dir) abort
   let l:full_cmd = empty(a:dir) ? a:cmd : 'cd ' . shellescape(a:dir) . ' && ' . a:cmd
 
   if plugin_manager#core#util#get_config('trace_commands', 0)
-    call plugin_manager#core#log#trace('util', 'run_in_dir: ' . l:full_cmd)
+    call plugin_manager#core#log#trace('util',
+          \ 'run_in_dir: ' . plugin_manager#core#util#sanitize_cmd(l:full_cmd))
   endif
 
   let l:output = system(l:full_cmd)
@@ -154,6 +155,76 @@ function! plugin_manager#core#util#get_plugin_dir(type) abort
   return l:plugins_dir . '/' . l:start_dir
 endfunction
 
+" ------------------------------------------------------------------------------
+" UNTRUSTED INPUT VALIDATION (issue #9)
+"
+" Contract: warn + safe fallback (empty string), never a hard throw.  The
+" degraded state is visible in the log, the operation stays usable.
+" ------------------------------------------------------------------------------
+
+" Strip https/http userinfo (user:token@) from a URL: logs, error messages
+" and commit texts must never carry credentials.
+function! plugin_manager#core#util#sanitize_url(url) abort
+  return substitute(a:url, '\(https\?://\)[^/@]*@', '\1', '')
+endfunction
+
+" Strip userinfo from every https/http URL inside a command string (traces
+" and error messages embed whole commands).
+function! plugin_manager#core#util#sanitize_cmd(cmd) abort
+  return substitute(a:cmd, '\(https\?://\)[^/@]*@', '\1', 'g')
+endfunction
+
+" Validate a branch name coming from .gitmodules or user declarations.
+" Returns the value when it is a plain refname, '' otherwise (callers fall
+" back to default branch resolution).  A leading dash is parsed by git as an
+" option (--upload-pack = remote code execution, proven in issue #9);
+" anything outside ^[A-Za-z0-9._/-]+$ is not a refname git would track.
+function! plugin_manager#core#util#sanitize_branch(branch) abort
+  " First character may not be '-': a leading dash is parsed by git as an
+  " option (--upload-pack = remote code execution, proven in issue #9).
+  if a:branch =~# '^[A-Za-z0-9._/][A-Za-z0-9._/-]*$'
+    return a:branch
+  endif
+  call plugin_manager#core#log#warn('util', 'untrusted branch ignored: ' . a:branch)
+  return ''
+endfunction
+
+" Validate a module path coming from .gitmodules before it is used as a
+" pathspec (git rm / git submodule deinit) or a filesystem target.  Returns
+" the value when it is a repo-relative path under plugins_dir, '' otherwise.
+" Glob characters as a pathspec expand to unrelated files ('*' removes
+" everything); '..' and absolute paths escape the config repo.
+function! plugin_manager#core#util#validate_module_path(path) abort
+  if empty(a:path) || a:path =~# '^-' || a:path =~# '^[/~]'
+        \ || a:path =~# '[*?\[]' || a:path =~# '\.\.'
+    call plugin_manager#core#log#warn('util',
+          \ 'untrusted module path ignored: ' . a:path)
+    return ''
+  endif
+  " plugins_dir may be configured absolute: compare in repo-relative form.
+  let l:plugins_dir = plugin_manager#core#util#make_relative_path(
+        \ plugin_manager#core#util#get_config('plugins_dir', ''))
+  if stridx(a:path, l:plugins_dir . '/') != 0
+    call plugin_manager#core#log#warn('util',
+          \ 'untrusted module path ignored: ' . a:path)
+    return ''
+  endif
+  return a:path
+endfunction
+
+" Validate a `dir` option (custom install name).  Returns the value when it
+" is a plain basename, '' otherwise (callers fall back to the default plugin
+" name).  '..' and absolute paths would clone outside plugins_dir.
+function! plugin_manager#core#util#validate_dir_name(dir) abort
+  if empty(a:dir) || a:dir =~# '\.\.' || a:dir =~# '^[/~]'
+        \ || fnamemodify(a:dir, ':t') !=# a:dir
+    call plugin_manager#core#log#warn('util',
+          \ 'untrusted dir option ignored: ' . a:dir)
+    return ''
+  endif
+  return a:dir
+endfunction
+
 " ------------------------------------------------------------------------------
 " URL AND PLUGIN NAME UTILITIES
 " ------------------------------------------------------------------------------
@@ -268,6 +339,9 @@ function! plugin_manager#core#util#process_plugin_options(args) abort
           echohl WarningMsg
           echomsg "Invalid 'load' value: " . l:val . ". Using default: 'start'"
           echohl None
+        elseif l:key ==# 'dir'
+          " Traversal guard: dir is a clone/install target (issue #9).
+          let l:options.dir = plugin_manager#core#util#validate_dir_name(l:val)
         else
           let l:options[l:key] = l:val
         endif
@@ -278,7 +352,7 @@ function! plugin_manager#core#util#process_plugin_options(args) abort
       endif
     endfor
   elseif len(a:args) >= 1 && type(a:args[0]) == v:t_string
-    let l:options.dir = a:args[0]
+    let l:options.dir = plugin_manager#core#util#validate_dir_name(a:args[0])
     if len(a:args) >= 2 && a:args[1] ==# 'opt'
       let l:options.load = 'opt'
     endif

autoload/plugin_manager/git.vim

@@ -302,13 +302,17 @@ function! plugin_manager#git#execute(cmd, dir, ...) abort
     let l:full_cmd = 'git -C ' . shellescape(a:dir) . strpart(a:cmd, 3)
   endif
 
+  " Display/traced variant: commands embed user URLs which may carry
+  " credentials - never write the userinfo to logs, UI or error messages.
+  let l:display_cmd = plugin_manager#core#util#sanitize_cmd(l:full_cmd)
+
   " Trace the command to the debug log if enabled
   if get(g:, 'plugin_manager_trace_commands', 0)
-    call plugin_manager#core#log#trace('git', 'exec: ' . l:full_cmd)
+    call plugin_manager#core#log#trace('git', 'exec: ' . l:display_cmd)
   endif
 
   if l:output_to_ui && exists('*plugin_manager#ui#update_sidebar')
-    call plugin_manager#ui#update_sidebar(['Executing: ' . a:cmd], 1)
+    call plugin_manager#ui#update_sidebar(['Executing: ' . l:display_cmd], 1)
   endif
 
   let l:output = system(l:full_cmd)
@@ -322,8 +326,9 @@ function! plugin_manager#git#execute(cmd, dir, ...) abort
   endif
 
   if !l:success && l:throw_on_error
-    " Standardized error handling
-    call plugin_manager#core#throw('git', 'COMMAND_FAILED', 'Command failed: ' . a:cmd . ' - ' . l:output)
+    " Standardized error handling (sanitized: no credentials in messages)
+    call plugin_manager#core#throw('git', 'COMMAND_FAILED',
+          \ 'Command failed: ' . l:display_cmd . ' - ' . l:output)
   endif
 
   return {'success': l:success, 'output': l:output}
@@ -390,6 +395,13 @@ function! plugin_manager#git#collect_status_local(module_path) abort
         \ ' submodule.' . shellescape(l:rel_path) . '.branch',
         \ '', 0, 0)
   let l:remote_branch = l:res.success ? substitute(l:res.output, '\n', '', 'g') : ''
+  " .gitmodules is user-writable config content: its branch value is passed
+  " to `git pull origin <branch>` - a leading dash would be parsed as a git
+  " option (proven RCE via --upload-pack, issue #9).  Refuse + fall back to
+  " the default branch resolution.
+  if !empty(l:remote_branch)
+    let l:remote_branch = plugin_manager#core#util#sanitize_branch(l:remote_branch)
+  endif
 
   " If not found in .gitmodules, try to determine from the current branch's upstream
   if empty(l:remote_branch) && l:result.branch !=# 'detached'
@@ -530,9 +542,15 @@ function! plugin_manager#git#add_submodule(url, install_dir, options) abort
     let l:cmd = 'git -c protocol.file.allow=always submodule add'
   endif
 
-  " Add branch option if specified
+  " Add branch option if specified.  The value may come from the vimrc (or
+  " an API caller): refuse option-like values instead of passing them to
+  " `git submodule add -b` (issue #9).
+  let l:branch = ''
   if !empty(a:options.branch)
-    let l:cmd .= ' -b ' . shellescape(a:options.branch)
+    let l:branch = plugin_manager#core#util#sanitize_branch(a:options.branch)
+    if !empty(l:branch)
+      let l:cmd .= ' -b ' . shellescape(l:branch)
+    endif
   endif
 
   " Add URL and path
@@ -565,10 +583,11 @@ function! plugin_manager#git#add_submodule(url, install_dir, options) abort
 
   " Commit changes (must run at repo root).  A failed pointer commit
   " leaves the submodule unrecorded: never swallow it - warn and fail the
-  " add so the caller reports the degraded outcome.
-  let l:commit_msg = 'Add ' . a:url . ' plugin'
-  if !empty(a:options.branch)
-    let l:commit_msg .= ' (branch: ' . a:options.branch . ')'
+  " add so the caller reports the degraded outcome.  The URL may embed
+  " credentials: strip userinfo before it lands in a pushed commit.
+  let l:commit_msg = 'Add ' . plugin_manager#core#util#sanitize_url(a:url) . ' plugin'
+  if !empty(l:branch)
+    let l:commit_msg .= ' (branch: ' . l:branch . ')'
   elseif !empty(a:options.tag)
     let l:commit_msg .= ' (tag: ' . a:options.tag . ')'
   endif
@@ -596,7 +615,8 @@ function! plugin_manager#git#add_remote(url, name) abort
 
   " Check if the repository exists
   if !plugin_manager#git#repository_exists(a:url)
-    call plugin_manager#core#throw('remote', 'REPO_NOT_FOUND', 'Repository not found: ' . a:url)
+    call plugin_manager#core#throw('remote', 'REPO_NOT_FOUND',
+          \ 'Repository not found: ' . plugin_manager#core#util#sanitize_url(a:url))
   endif
 
   " Generate remote name if not provided

tests/security.vader

@@ -0,0 +1,110 @@
+" Security validators (issue #9): untrusted values must never reach git
+" commands, filesystem targets, or logs unvalidated.  Contract: warn + safe
+" fallback (empty string), never a hard throw.  Unit-level tests; the
+" hostile-branch case exercises the full collect_status_local path.
+
+Before:
+  let g:_pm_sec_saved = {
+        \ 'vim_dir':     get(g:, 'plugin_manager_vim_dir', ''),
+        \ 'plugins_dir': get(g:, 'plugin_manager_plugins_dir', ''),
+        \ 'logging':     get(g:, 'plugin_manager_enable_logging', 0),
+        \ }
+  let g:plugin_manager_vim_dir = '/tmp/pm-sec-test/vim'
+  let g:plugin_manager_plugins_dir = '/tmp/pm-sec-test/vim/pack/plugins'
+  call delete('/tmp/pm-sec-test', 'rf')
+  call mkdir('/tmp/pm-sec-test/vim', 'p')
+  let g:plugin_manager_enable_logging = 1
+  unlet! g:plugin_manager_debug_mode
+  function! g:_pm_sec_log() abort
+    let l:p = '/tmp/pm-sec-test/vim/logs/plugin_manager.log'
+    return filereadable(l:p) ? join(readfile(l:p), "\n") : ''
+  endfunction
+
+After:
+  call delete('/tmp/pm-sec-test', 'rf')
+  let g:plugin_manager_vim_dir     = g:_pm_sec_saved.vim_dir
+  let g:plugin_manager_plugins_dir = g:_pm_sec_saved.plugins_dir
+  let g:plugin_manager_enable_logging = g:_pm_sec_saved.logging
+  unlet g:_pm_sec_saved
+
+Execute (sanitize_branch accepts ordinary refnames):
+  AssertEqual 'main', plugin_manager#core#util#sanitize_branch('main')
+  AssertEqual 'feature/x.y-z', plugin_manager#core#util#sanitize_branch('feature/x.y-z')
+
+Execute (sanitize_branch refuses option-like and hostile values):
+  AssertEqual '', plugin_manager#core#util#sanitize_branch('--upload-pack=/bin/true')
+  AssertEqual '', plugin_manager#core#util#sanitize_branch('-b')
+  AssertEqual '', plugin_manager#core#util#sanitize_branch('main;rm -rf /')
+  AssertEqual '', plugin_manager#core#util#sanitize_branch('main$(x)')
+  AssertEqual '', plugin_manager#core#util#sanitize_branch('ma in')
+  Assert g:_pm_sec_log() =~# 'untrusted branch',
+        \ 'a refused branch must be logged, got: ' . g:_pm_sec_log()
+
+Execute (validate_module_path accepts repo-relative plugin paths):
+  AssertEqual 'pack/plugins/start/myplugin',
+        \ plugin_manager#core#util#validate_module_path('pack/plugins/start/myplugin')
+  AssertEqual 'pack/plugins/opt/lazy',
+        \ plugin_manager#core#util#validate_module_path('pack/plugins/opt/lazy')
+
+Execute (validate_module_path refuses glob chars, traversal and escapes):
+  AssertEqual '', plugin_manager#core#util#validate_module_path('*')
+  AssertEqual '', plugin_manager#core#util#validate_module_path('my*')
+  AssertEqual '', plugin_manager#core#util#validate_module_path('pack/plugins/start/[a]b')
+  AssertEqual '', plugin_manager#core#util#validate_module_path('pack/plugins/start/../evil')
+  AssertEqual '', plugin_manager#core#util#validate_module_path('evil')
+  AssertEqual '', plugin_manager#core#util#validate_module_path('-x')
+  AssertEqual '', plugin_manager#core#util#validate_module_path('')
+  Assert g:_pm_sec_log() =~# 'untrusted module path',
+        \ 'a refused path must be logged, got: ' . g:_pm_sec_log()
+
+Execute (validate_dir_name accepts plain basenames only):
+  AssertEqual 'myplugin', plugin_manager#core#util#validate_dir_name('myplugin')
+  AssertEqual '', plugin_manager#core#util#validate_dir_name('../evil')
+  AssertEqual '', plugin_manager#core#util#validate_dir_name('/abs/path')
+  AssertEqual '', plugin_manager#core#util#validate_dir_name('a/b')
+  AssertEqual '', plugin_manager#core#util#validate_dir_name('')
+  Assert g:_pm_sec_log() =~# 'untrusted dir option',
+        \ 'a refused dir must be logged, got: ' . g:_pm_sec_log()
+
+Execute (sanitize_url strips https userinfo, keeps the rest):
+  AssertEqual 'https://github.com/a/b.git',
+        \ plugin_manager#core#util#sanitize_url('https://user:token@github.com/a/b.git')
+  AssertEqual 'https://github.com/a/b.git',
+        \ plugin_manager#core#util#sanitize_url('https://token@github.com/a/b.git')
+  AssertEqual 'http://host/x',
+        \ plugin_manager#core#util#sanitize_url('http://u@host/x')
+  AssertEqual 'https://github.com/a/b.git',
+        \ plugin_manager#core#util#sanitize_url('https://github.com/a/b.git')
+  AssertEqual 'git@github.com:a/b.git',
+        \ plugin_manager#core#util#sanitize_url('git@github.com:a/b.git')
+
+Execute (a hostile .gitmodules branch never reaches git pull):
+  " Minimal module: a real repo with a main branch, a crafted .gitmodules
+  " whose branch value is a git option.
+  let g:_pm_sec_mod = '/tmp/pm-sec-test/vim/pack/plugins/start/hostile'
+  call mkdir(g:_pm_sec_mod, 'p')
+  call system('git init -q ' . shellescape(g:_pm_sec_mod))
+  call system('git -C ' . shellescape(g:_pm_sec_mod) . ' config user.email "t@t.com"')
+  call system('git -C ' . shellescape(g:_pm_sec_mod) . ' config user.name "T"')
+  call writefile(['x'], g:_pm_sec_mod . '/f')
+  call system('git -C ' . shellescape(g:_pm_sec_mod) . ' add .')
+  call system('git -C ' . shellescape(g:_pm_sec_mod) . ' commit -qm init')
+  call writefile([
+        \ '[submodule "pack/plugins/start/hostile"]',
+        \ "\tpath = pack/plugins/start/hostile",
+        \ "\turl = https://example.com/a/b.git",
+        \ "\tbranch = --upload-pack=/bin/true",
+        \ ], '/tmp/pm-sec-test/vim/.gitmodules')
+  call plugin_manager#git#refresh_modules_cache()
+  let g:_pm_sec_exc = ''
+  try
+    let g:_pm_sec_st = plugin_manager#git#collect_status_local(g:_pm_sec_mod)
+  catch
+    let g:_pm_sec_exc = v:exception . ' @ ' . v:throwpoint
+  endtry
+  AssertEqual '', g:_pm_sec_exc, 'collect_status_local must not throw: ' . g:_pm_sec_exc
+  Assert stridx(g:_pm_sec_st.remote_branch, '--') == -1,
+        \ 'the hostile branch value must not propagate, got: ' . g:_pm_sec_st.remote_branch
+  Assert g:_pm_sec_log() =~# 'untrusted branch',
+        \ 'the refused branch must be logged, got: ' . g:_pm_sec_log()
+  unlet g:_pm_sec_exc g:_pm_sec_st