[metif12]

Summary

Audit of the reported command-injection surface, done against the current tree rather than the 2023 reports, which cite a src/ layout that no longer exists.

  git/git.v
- pub fn Git.exec_shell(command string) os.Result {
- 	return os.exec(['/bin/sh', '-c', command])
- }
+ // NOTE: there is deliberately no `exec_shell` wrapper here. Handing a
+ // concatenated command string to `/bin/sh -c` is how command injection gets
+ // introduced, and it is never needed: every caller below builds an argument
+ // vector and passes it to `os.exec`, so arguments are never re-parsed by a
+ // shell. Add an argument-array helper rather than a string one.

Plus SECURITY.md, which the reporter of #246 asked for in 2023 and which the repository still does not have.

Evidence

The 2023 report's cited files do not exist. src/repo_routes.v, src/commit_routes.v, and src/repo_service.v are all gone; the tree was restructured and the helpers are now in git/git.v and repo/repo.v. So the report cannot be acted on as written and has to be re-derived.

Every external command runs through argv. All four os.exec call sites in git/git.v pass an array:

pub fn Git.exec(args []string) os.Result {
	mut git_args := ['git']
	git_args << args
	return os.exec(git_args)
}

// Git.exec_in_dir_with_env runs `git -C <dir> <args...>` without going through a
// shell, with `extra_env` added on top of the inherited environment. Use this
// instead of building a shell string when any argument or environment value is
// user-controlled (file paths, branch names, commit messages, author names),
// since arguments are passed directly to git and never re-parsed by /bin/sh.

split_command (git/git.v:218) is purely lexical — word splitting and unquoting, with no command substitution, variable expansion, or globbing — and its result goes to argv.

exec_shell had zero callers. A repository-wide search for exec_shell returns exactly one match, its own definition:

D:\MyProjects\gitly\git\git.v:
  Line 33: pub fn Git.exec_shell(command string) os.Result {

Private-repository visibility (#247) is already fixed. repo/repo.v:1796:

fn (app &App) can_read_repo(ctx Context, repo Repo) bool {
	if repo.is_public {
		return true
	}
	return ctx.logged_in && app.user_has_repo_read_access(ctx.user.id, repo)
}

That check is called at 43 sites across every repo-scoped read route, including tree() in repo/repo_routes.v:753, which is the route the report described (/:username/:repo_name). Regression tests already exist in api/endpoints_test.v:919 (test_api_v1_private_repo_visibility_from_other_user, which asserts 404 for a private repo from another authenticated user, and 200 for the owner) and pr_authorization_test.v.

Build still passes after the removal:

$ v -d sqlite -o gitly.exe .
EXIT=0
True

Merge Danger

Door: Two-way. exec_shell had no callers, so removing it cannot change behaviour; the rest is comments and a new file. Reverting is clean.

Blast Radius: Small, but one thing deserves scrutiny.

I deliberately did not fix the residual issue I found, and named it in the code instead. Repo.git() guards against & and ;, which reads like a shell guard but is not one, because there is no shell. The actual residual risk is argument injection: a ref name beginning with - can still be interpreted by git as an option, and split_command splits on whitespace, so a ref containing a space becomes several arguments. The callers that interpolate a caller-supplied ref are pr_routes.v:859 / pr_routes.v:886 (log ${base}..${head}, diff ${base}...${head}) and repo/branch.v:67.

Fixing that properly means moving those callers onto Git.exec_in_dir_with_env with an argument array and a -- separator, which is a behavioural change I cannot test here beyond a compile. It is a separate change and should be reviewed on its own merits, not smuggled into a cleanup.

[medvednikov]

The unused shell-wrapper removal looks narrowly scoped, but SECURITY.md has a broken fallback disclosure channel: it tells reporters who cannot use GitHub private reporting to email maintainers listed in .github/FUNDING.yml. That file contains only Patreon/GitHub Sponsors entries (patreon: vlang, github: [medvednikov])—no email addresses. Please provide a confirmed private security contact or remove that fallback; otherwise reporters have no actionable second channel. I'd also have maintainers explicitly confirm the promised 14-day acknowledgement SLA, and clarify in Hardening Notes that argv avoids shell injection but does not by itself prevent the Git option/argument injection already called out in repo/repo.v.

[metif12]

Done - addressed in the latest push.

[medvednikov]

Follow-up on the revised security policy: the incorrect .github/FUNDING.yml email fallback is gone, but the current SECURITY.md still has a malformed path/reference (see \nepo/repo.v git()), offers a draft-advisory workaround ordinary reporters might not have permission to use, and promises a 14-day acknowledgement without confirmed maintainer agreement. Also, -- is not a universal safe place for revision arguments (git log/git diff may interpret post--- arguments as paths). Please distinguish validated/resolved refs from path operands and tighten the disclosure guidance.

[metif12]

Done - fixed in fc9d56a: repaired the malformed repo/repo.v reference, removed the draft-advisory workaround, softened the 14-day promise to best-effort, and clarified that -- only separates path operands while revision arguments need validation via is_safe_ref/rev-parse. v check-md is clean.