From 7d2ae1887d69ce813b56df00c43b23e8458ae021 Mon Sep 17 00:00:00 2001 From: office Date: Sun, 11 Oct 2026 15:45:00 +0330 Subject: [PATCH 1/2] fix: make gitly compile with current V Master did not build at all. Three independent breakages, all verified by reproducing each one before fixing it. 1. 32 vweb route handlers were declared without `mut ctx Context`. Current vweb dispatches `app.$method(mut user_context, ...args)`, so any handler taking at least one parameter must accept the context first. Handlers such as `user_repos` and `handle_admin_edit_user` already used `ctx` in their bodies without declaring it, and call sites such as `app.admin_users(mut ctx, '0')` already passed it, so this restores the signature the code was written against. `method.args` excludes the receiver, which is why one-parameter handlers failed while multi-param ones silently compiled. 2. `struct FileInfo` in repo/file.v collided with `os.FileInfo`. Substituting it into the `api.ApiSuccessResponse[T]` generic made V resolve the bare name in the wrong scope, emitting `os__FileInfo` as the declaration and `main__FileInfo` as the initialiser: src.c:67159:64: error: invalid initializer os__FileInfo __return_array_value_16 = *(main__FileInfo*)(array_get(...)); Renamed to `RepoFileInfo`, which no stdlib type shadows. 3. `change_lang` was the only one-parameter handler, so it surfaced first and masked the rest; it is included in the set above. Verified: - `v -d sqlite -o gitly.exe .` exits 0 and produces the binary. - `v -d sqlite test utils_test.v` -> 1 passed, 1 total. - `git diff --stat` touches only .v files; no build artifacts staged. Note that `v test` compiles the whole project per test file, costing about 887s of comptime for a single file on Windows with VJOBS=2, so the tracked suite cannot be run in full locally. --- admin/admin_routes.v | 4 ++-- ci/ci_routes.v | 8 ++++---- gitly.v | 2 +- project_governance_routes.v | 18 +++++++++--------- repo/branch_routes.v | 2 +- repo/comment.v | 2 +- repo/file.v | 5 ++++- repo/file_routes.v | 10 +++++----- repo/repo_routes.v | 20 ++++++++++---------- repo/tag_routes.v | 4 ++-- 10 files changed, 39 insertions(+), 36 deletions(-) diff --git a/admin/admin_routes.v b/admin/admin_routes.v index 21df0338..26f4a78d 100644 --- a/admin/admin_routes.v +++ b/admin/admin_routes.v @@ -16,7 +16,7 @@ pub fn (mut app App) admin_settings(mut ctx Context) veb.Result { } @['/admin/settings'; post] -pub fn (mut app App) handle_admin_update_settings(oauth_client_id string, oauth_client_secret string) veb.Result { +pub fn (mut app App) handle_admin_update_settings(mut ctx Context, oauth_client_id string, oauth_client_secret string) veb.Result { if !ctx.is_admin() { return ctx.redirect_to_index() } @@ -34,7 +34,7 @@ pub fn (mut app App) handle_admin_update_settings(oauth_client_id string, oauth_ } @['/admin/users/:user'; post] -pub fn (mut app App) handle_admin_edit_user(user_id string) veb.Result { +pub fn (mut app App) handle_admin_edit_user(mut ctx Context, user_id string) veb.Result { if !ctx.is_admin() { return ctx.redirect_to_index() } diff --git a/ci/ci_routes.v b/ci/ci_routes.v index 96ec0dfa..7ce16363 100644 --- a/ci/ci_routes.v +++ b/ci/ci_routes.v @@ -96,7 +96,7 @@ fn (mut app App) verify_ci_callback_signature(ctx &Context, body string) bool { // GET /:username/:repo_name/ci - CI runs list page @['/:username/:repo_name/ci'] -pub fn (mut app App) ci_runs(username string, repo_name string) veb.Result { +pub fn (mut app App) ci_runs(mut ctx Context, username string, repo_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if !app.can_read_repo(ctx, repo) { @@ -141,7 +141,7 @@ pub fn (mut app App) ci_runs(username string, repo_name string) veb.Result { // GET /:username/:repo_name/ci/:run_id_str - CI run detail page @['/:username/:repo_name/ci/:run_id_str'] -pub fn (mut app App) ci_run_detail(username string, repo_name string, run_id_str string) veb.Result { +pub fn (mut app App) ci_run_detail(mut ctx Context, username string, repo_name string, run_id_str string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if !app.can_read_repo(ctx, repo) { @@ -178,7 +178,7 @@ pub fn (mut app App) ci_run_detail(username string, repo_name string, run_id_str // POST /:username/:repo_name/ci/:run_id_str/restart - Restart a CI run @['/:username/:repo_name/ci/:run_id_str/restart'; post] -pub fn (mut app App) ci_restart_run(username string, repo_name string, run_id_str string) veb.Result { +pub fn (mut app App) ci_restart_run(mut ctx Context, username string, repo_name string, run_id_str string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if !app.can_admin_repo(ctx, repo) { @@ -221,7 +221,7 @@ pub fn (mut app App) ci_restart_run(username string, repo_name string, run_id_st // POST /:username/:repo_name/ci/:run_id_str/cancel - Cancel a CI run. @['/:username/:repo_name/ci/:run_id_str/cancel'; post] -pub fn (mut app App) ci_cancel_run(username string, repo_name string, run_id_str string) veb.Result { +pub fn (mut app App) ci_cancel_run(mut ctx Context, username string, repo_name string, run_id_str string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if !app.can_admin_repo(ctx, repo) { return ctx.not_found() diff --git a/gitly.v b/gitly.v index 8ae21a72..14cb6bf3 100644 --- a/gitly.v +++ b/gitly.v @@ -304,7 +304,7 @@ pub fn (mut app App) index(mut ctx Context) veb.Result { } @['/change_lang/:lang'; post] -pub fn (mut app App) change_lang(lang string) veb.Result { +pub fn (mut app App) change_lang(mut ctx Context, lang string) veb.Result { if lang !in ['en', 'ru', 'es', 'jp', 'cn', 'pt'] { ctx.res.set_status(.bad_request) return ctx.json_error('Unsupported language') diff --git a/project_governance_routes.v b/project_governance_routes.v index b13b82dc..2b609212 100644 --- a/project_governance_routes.v +++ b/project_governance_routes.v @@ -5,7 +5,7 @@ module main import veb @['/:username/:repo_name/settings/members'] -pub fn (mut app App) repo_members(username string, repo_name string) veb.Result { +pub fn (mut app App) repo_members(mut ctx Context, username string, repo_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if !app.can_admin_repo(ctx, repo) { return ctx.not_found() @@ -15,7 +15,7 @@ pub fn (mut app App) repo_members(username string, repo_name string) veb.Result } @['/:username/:repo_name/settings/members'; post] -pub fn (mut app App) handle_add_repo_member(username string, repo_name string) veb.Result { +pub fn (mut app App) handle_add_repo_member(mut ctx Context, username string, repo_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if !app.can_admin_repo(ctx, repo) { return ctx.not_found() @@ -42,7 +42,7 @@ pub fn (mut app App) handle_add_repo_member(username string, repo_name string) v } @['/:username/:repo_name/settings/members/:member_id'; post] -pub fn (mut app App) handle_update_repo_member(username string, repo_name string, member_id string) veb.Result { +pub fn (mut app App) handle_update_repo_member(mut ctx Context, username string, repo_name string, member_id string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if !app.can_admin_repo(ctx, repo) { return ctx.not_found() @@ -56,7 +56,7 @@ pub fn (mut app App) handle_update_repo_member(username string, repo_name string } @['/:username/:repo_name/settings/members/:member_id/delete'; post] -pub fn (mut app App) handle_delete_repo_member(username string, repo_name string, member_id string) veb.Result { +pub fn (mut app App) handle_delete_repo_member(mut ctx Context, username string, repo_name string, member_id string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if !app.can_admin_repo(ctx, repo) { return ctx.not_found() @@ -69,7 +69,7 @@ pub fn (mut app App) handle_delete_repo_member(username string, repo_name string } @['/:username/:repo_name/settings/branches'] -pub fn (mut app App) protected_branches(username string, repo_name string) veb.Result { +pub fn (mut app App) protected_branches(mut ctx Context, username string, repo_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if !app.can_admin_repo(ctx, repo) { return ctx.not_found() @@ -80,7 +80,7 @@ pub fn (mut app App) protected_branches(username string, repo_name string) veb.R } @['/:username/:repo_name/settings/branches'; post] -pub fn (mut app App) handle_protect_branch(username string, repo_name string) veb.Result { +pub fn (mut app App) handle_protect_branch(mut ctx Context, username string, repo_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if !app.can_admin_repo(ctx, repo) { return ctx.not_found() @@ -96,7 +96,7 @@ pub fn (mut app App) handle_protect_branch(username string, repo_name string) ve } @['/:username/:repo_name/settings/branches/:rule_id'; post] -pub fn (mut app App) handle_update_protected_branch(username string, repo_name string, rule_id string) veb.Result { +pub fn (mut app App) handle_update_protected_branch(mut ctx Context, username string, repo_name string, rule_id string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if !app.can_admin_repo(ctx, repo) { return ctx.not_found() @@ -110,7 +110,7 @@ pub fn (mut app App) handle_update_protected_branch(username string, repo_name s } @['/:username/:repo_name/settings/branches/:rule_id/delete'; post] -pub fn (mut app App) handle_unprotect_branch(username string, repo_name string, rule_id string) veb.Result { +pub fn (mut app App) handle_unprotect_branch(mut ctx Context, username string, repo_name string, rule_id string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if !app.can_admin_repo(ctx, repo) { return ctx.not_found() @@ -123,7 +123,7 @@ pub fn (mut app App) handle_unprotect_branch(username string, repo_name string, } @['/:username/:repo_name/settings/approvals'; post] -pub fn (mut app App) handle_update_repo_approvals(username string, repo_name string) veb.Result { +pub fn (mut app App) handle_update_repo_approvals(mut ctx Context, username string, repo_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if !app.can_admin_repo(ctx, repo) { return ctx.not_found() diff --git a/repo/branch_routes.v b/repo/branch_routes.v index 3f0bca4e..17614b66 100644 --- a/repo/branch_routes.v +++ b/repo/branch_routes.v @@ -22,7 +22,7 @@ fn (mut app App) handle_branch_count(username string, repo_name string) veb.Resu } @['/:user/:repo/branches'] -pub fn (mut app App) branches(username string, repo_name string) veb.Result { +pub fn (mut app App) branches(mut ctx Context, username string, repo_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.json_error('Not found') } diff --git a/repo/comment.v b/repo/comment.v index 3b6e2a92..f6e30995 100644 --- a/repo/comment.v +++ b/repo/comment.v @@ -16,7 +16,7 @@ mut: } @['/:username/:repo_name/comments'; post] -pub fn (mut app App) handle_add_comment(username string, repo_name string) veb.Result { +pub fn (mut app App) handle_add_comment(mut ctx Context, username string, repo_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } text := ctx.form['text'] issue_id := ctx.form['issue_id'] diff --git a/repo/file.v b/repo/file.v index b9023f84..d72c7743 100644 --- a/repo/file.v +++ b/repo/file.v @@ -84,7 +84,10 @@ fn pretty_size_bytes(size_in_bytes int) string { return '${size_in} ${sizes[index]}' } -struct FileInfo { +// Named `RepoFileInfo` rather than `FileInfo` to avoid colliding with +// `os.FileInfo`, which V resolves in the wrong scope when this type is +// substituted into the `api.ApiSuccessResponse[T]` generic. +struct RepoFileInfo { name string last_msg string last_hash string diff --git a/repo/file_routes.v b/repo/file_routes.v index 3722da50..395d25a9 100644 --- a/repo/file_routes.v +++ b/repo/file_routes.v @@ -8,7 +8,7 @@ import rand // GET /:username/:repo_name/new/:branch_name... - Show create file form. // The catch-all is required because branch names may contain `/`. @['/:username/:repo_name/new/:branch_name...'] -pub fn (mut app App) new_file(username string, repo_name string, branch_name string) veb.Result { +pub fn (mut app App) new_file(mut ctx Context, username string, repo_name string, branch_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if repo.require_signed_commits || !ctx.logged_in || !app.user_can_push_branch(ctx.user.id, repo, branch_name) @@ -23,7 +23,7 @@ pub fn (mut app App) new_file(username string, repo_name string, branch_name str // GET /:username/:repo_name/new-ci-file - Show create .gitly-ci.yml form (pre-filled) @['/:username/:repo_name/new-ci-file'] -pub fn (mut app App) new_ci_file(username string, repo_name string) veb.Result { +pub fn (mut app App) new_ci_file(mut ctx Context, username string, repo_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if repo.require_signed_commits || !ctx.logged_in @@ -46,7 +46,7 @@ pub fn (mut app App) new_ci_file(username string, repo_name string) veb.Result { // GET /:username/:repo_name/edit/:location... - Show edit file form @['/:username/:repo_name/edit/:location...'] -pub fn (mut app App) edit_file(username string, repo_name string, location string) veb.Result { +pub fn (mut app App) edit_file(mut ctx Context, username string, repo_name string, location string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } if repo.require_signed_commits || !ctx.logged_in || !app.user_can_write_repo(ctx.user.id, repo) { return ctx.redirect_to_repository(username, repo_name) @@ -69,7 +69,7 @@ pub fn (mut app App) edit_file(username string, repo_name string, location strin // POST /:username/:repo_name/update-file - Save edited file @['/:username/:repo_name/update-file'; post] -pub fn (mut app App) handle_update_file(username string, repo_name string) veb.Result { +pub fn (mut app App) handle_update_file(mut ctx Context, username string, repo_name string) veb.Result { mut repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } @@ -132,7 +132,7 @@ pub fn (mut app App) handle_update_file(username string, repo_name string) veb.R // POST /:username/:repo_name/create-file - Create a file in the repo @['/:username/:repo_name/create-file'; post] -pub fn (mut app App) handle_create_file(username string, repo_name string) veb.Result { +pub fn (mut app App) handle_create_file(mut ctx Context, username string, repo_name string) veb.Result { mut repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() } diff --git a/repo/repo_routes.v b/repo/repo_routes.v index 5aa9e849..44e934e8 100644 --- a/repo/repo_routes.v +++ b/repo/repo_routes.v @@ -98,7 +98,7 @@ fn repo_sort_indicator(current_sort string, current_dir string, target_sort stri } @['/:username/repos'] -pub fn (mut app App) user_repos(username string) veb.Result { +pub fn (mut app App) user_repos(mut ctx Context, username string) veb.Result { exists, user := app.check_username(username) if !exists { @@ -128,7 +128,7 @@ pub fn (mut app App) user_repos(username string) veb.Result { } @['/:username/stars'] -pub fn (mut app App) user_stars(username string) veb.Result { +pub fn (mut app App) user_stars(mut ctx Context, username string) veb.Result { exists, user := app.check_username(username) if !exists { @@ -145,7 +145,7 @@ pub fn (mut app App) user_stars(username string) veb.Result { } @['/:username/:repo_name/settings'] -pub fn (mut app App) repo_settings(username string, repo_name string) veb.Result { +pub fn (mut app App) repo_settings(mut ctx Context, username string, repo_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.redirect_to_repository(username, repo_name) } @@ -161,7 +161,7 @@ pub fn (mut app App) repo_settings(username string, repo_name string) veb.Result } @['/:username/:repo_name/settings'; post] -pub fn (mut app App) handle_update_repo_settings(username string, repo_name string) veb.Result { +pub fn (mut app App) handle_update_repo_settings(mut ctx Context, username string, repo_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.redirect_to_repository(username, repo_name) } @@ -226,7 +226,7 @@ pub fn (mut app App) handle_update_repo_settings(username string, repo_name stri } @['/:username/:repo_name/settings/features'; post] -pub fn (mut app App) handle_update_repo_features(username string, repo_name string) veb.Result { +pub fn (mut app App) handle_update_repo_features(mut ctx Context, username string, repo_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.redirect_to_repository(username, repo_name) } @@ -246,7 +246,7 @@ pub fn (mut app App) handle_update_repo_features(username string, repo_name stri } @['/:user/:repo_name/delete'; post] -pub fn (mut app App) handle_repo_delete(username string, repo_name string) veb.Result { +pub fn (mut app App) handle_repo_delete(mut ctx Context, username string, repo_name string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.redirect_to_repository(username, repo_name) } @@ -273,7 +273,7 @@ pub fn (mut app App) handle_repo_delete(username string, repo_name string) veb.R } @['/:username/:repo_name/move'; post] -pub fn (mut app App) handle_repo_move(username string, repo_name string, dest string, verify string) veb.Result { +pub fn (mut app App) handle_repo_move(mut ctx Context, username string, repo_name string, dest string, verify string) veb.Result { repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.redirect_to_index() } @@ -1028,9 +1028,9 @@ pub fn (mut app App) handle_api_repo_files(mut ctx Context, repo_id_str string) } items := app.find_repository_items(repo_id, branch, path) - mut result := []FileInfo{} + mut result := []RepoFileInfo{} for item in items { - result << FileInfo{ + result << RepoFileInfo{ name: item.name last_msg: item.last_msg last_hash: item.last_hash @@ -1039,7 +1039,7 @@ pub fn (mut app App) handle_api_repo_files(mut ctx Context, repo_id_str string) } } - return ctx.json(api.ApiSuccessResponse[[]FileInfo]{ + return ctx.json(api.ApiSuccessResponse[[]RepoFileInfo]{ success: true result: result }) diff --git a/repo/tag_routes.v b/repo/tag_routes.v index e8cac1fd..3ec41873 100644 --- a/repo/tag_routes.v +++ b/repo/tag_routes.v @@ -5,7 +5,7 @@ import os import rand @['/:username/:repo_name/tag/:tag/:format'] -pub fn (mut app App) handle_download_tag_archive(username string, repo_name string, tag string, format string) veb.Result { +pub fn (mut app App) handle_download_tag_archive(mut ctx Context, username string, repo_name string, tag string, format string) veb.Result { return stream_tag_archive(mut app, mut ctx, username, repo_name, tag, format) } @@ -13,7 +13,7 @@ pub fn (mut app App) handle_download_tag_archive(username string, repo_name stri // treats each slash as another route segment. Keep it for existing links and // use this catch-all route for canonical release downloads. @['/:username/:repo_name/archive/:tag...'] -pub fn (mut app App) handle_download_tag_archive_path(username string, repo_name string, tag string) veb.Result { +pub fn (mut app App) handle_download_tag_archive_path(mut ctx Context, username string, repo_name string, tag string) veb.Result { format := ctx.query['format'] or { '' } return stream_tag_archive(mut app, mut ctx, username, repo_name, tag, format) } From f61e85a3bacd236992f4748bdaf76ee45304273f Mon Sep 17 00:00:00 2001 From: office Date: Sun, 11 Oct 2026 16:31:53 +0330 Subject: [PATCH 2/2] security: add SECURITY.md, drop the /bin/sh -c footgun, document the real threat model Audit of the reported command injection attack surface, done against the current tree rather than the 2023 reports, which cite a `src/` layout that no longer exists. Findings: - Every external command now runs through an argument vector. `os.exec` is called with an array in all four sites in `git/git.v`, and `Git.exec_in_dir_with_env` sets `p.set_args(...)` directly. Nothing concatenates user input into a string that is later handed to a shell. - `split_command` is purely lexical: it word-splits and unquotes, and does no command substitution, variable expansion, or globbing. Its result is passed as argv, so it cannot introduce shell metacharacters. - `Git.exec_shell` was the one remaining `/bin/sh -c` wrapper and had zero callers anywhere in the repository. Removed, with a note in `git/git.v` telling future authors to add an argument-array helper instead of a string one, so the footgun does not get reintroduced. - `Repo.git()` guards against `&` and `;`, which suggests a shell is involved. It is not. Replaced with a comment stating the guard is not a security control, and naming the actual residual risk: argument injection. A ref beginning with `-` can still be read by git as an option, and `split_command` breaks a ref containing a space into several arguments. The callers that interpolate a caller-supplied ref are in `pr_routes.v` and `repo/branch.v`. Behaviour is deliberately unchanged here; fixing it means moving those to `Git.exec_in_dir_with_env` with a `--` separator, which is a larger change. Private-repository visibility (#247) is already enforced. `App.can_read_repo` in `repo/repo.v` returns 404 for a private repo unless the requester is logged in and has at least reporter access, and every repo-scoped read route calls it before running a git command or returning repo data. `repo/repo_routes.v` `tree()` is one of those callers. Regression tests already exist in `api/endpoints_test.v` and `pr_authorization_test.v`. SECURITY.md is new. The reporter of #246 asked for a security policy in 2023 and the repository still has none, so this is the part of that report that is still open. It covers private disclosure, the supported-versions position, the scope of untrusted input, and the known boundaries that are deliberate design decisions rather than oversights, so a future report can be triaged against them instead of rediscovering them. --- SECURITY.md | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++ git/git.v | 8 ++-- repo/repo.v | 10 +++++ 3 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 SECURITY.md diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 00000000..1be9bf83 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,103 @@ +# Security Policy + +## Supported Versions + +Gitly is beta software. Only the latest commit on `master` receives fixes. There +are no release branches and no backports; upgrade to the current `master` rather +than waiting for a patched version. + +| Version | Supported | +| ------- | --------- | +| `master` (latest) | ✅ | +| anything older | ❌ | + +## Reporting a Vulnerability + +Do not open a public issue for a security problem. + +Report privately through GitHub's private vulnerability reporting, which keeps the +report confidential until a fix is published: + +**https://github.com/vlang/gitly/security/advisories/new** + +If you cannot use that form, email the maintainers listed in `.github/FUNDING.yml` +and the repository owner, and include the words "gitly security" in the subject. + +Please include: + +- the affected route, command, or module +- a concrete reproduction, ideally the smallest program or HTTP request that shows it +- the impact you believe it has, and any preconditions (anonymous vs authenticated, + which configuration flags are required) +- the V version and commit you tested against + +You should receive an acknowledgement within 14 days. If the report is accepted, +the fix lands on `master` and the advisory is published when it is safe to do so. +If it is declined, you will be told why so you can decide whether to disclose it +yourself. + +## Scope + +Gitly is a self-hosted application. It is only security-relevant in the parts that +handle untrusted input, which in practice means: + +- the HTTP routes under `repo/`, `issue_routes.v`, `pr_routes.v`, + `api_v1_routes.v`, `platform_*_routes.v`, and the `*/ *_routes.v` files +- Git transport over HTTP and SSH (`repo/git_routes.v`, `ssh/`) +- the mirror, fork, and import paths, which fetch attacker-supplied URLs + (`mirror.v`, `fork.v`, `repository_management.v`) +- credential and secret storage (`GITLY_STORAGE_SECRET`, mirror passwords, + webhook secrets) +- the Markdown renderer, which parses untrusted documents (`markdown/`) +- the CI status callback, which is unauthenticated apart from its shared secret + (`ci/ci_routes.v`) + +Git transport over SSH delegates to the host OpenSSH server. Bugs in OpenSSH +itself, in the operating system, or in a reverse proxy in front of Gitly are out +of scope here; report them upstream. + +## Out of Scope + +- denial of service against an instance you control +- missing hardening that is documented as a deployment responsibility: HTTPS + termination, reverse-proxy configuration, database credentials, and isolating + the CI runner. See the production configuration section of the README. +- attacks requiring an already-authenticated maintainer or admin session, unless + they cross a privilege boundary +- vulnerabilities in dependencies, including V's own standard library + +## Known Boundaries + +These are deliberate design decisions, not oversights. They are listed so that a +report can be triaged quickly. + +- **The CI status callback is authenticated by a shared secret only.** Set + `ci_secret` (or `GITLY_CI_SECRET`) before enabling CI. An empty secret causes + callbacks to be rejected rather than accepted. +- **Mirror passwords, access tokens, and SSH private keys fail closed without + `GITLY_STORAGE_SECRET`.** Set it before saving credentialed mirrors. +- **CI jobs are untrusted code.** Run the separate `gitly_ci` service on an + isolated runner host with no access to Gitly's database credentials, storage + secret, repository volume, or host filesystem. This is documented in the + README and is the deployment's responsibility, not something Gitly can enforce + from inside its own process. +- **Local object storage and repository paths are on the local filesystem.** + `GITLY_OBJECT_STORAGE_PATH`, `GITLY_REPO_STORAGE_PATH`, `GITLY_ARCHIVE_PATH`, + and `GITLY_AVATARS_PATH` should point at storage that is not shared with other + services. + +## Hardening Notes + +Commands are executed by passing an argument vector to `git` rather than through +a shell, so route parameters, branch names, and file paths are never re-parsed by +`/bin/sh`. `git/git.v` documents this in `Git.exec_in_dir_with_env`. If you add +code that runs an external command, use an argument array; do not concatenate +user input into a string that is later handed to a shell. + +Repository read access goes through `App.can_read_repo` (`repo/repo.v`), which +every repo-scoped read route calls before running a git command or returning repo +data. Add that check to any new repo-scoped route. + +## Disclosure + +Credited reporters are named in the published advisory unless they ask not to be. diff --git a/git/git.v b/git/git.v index d38c40f6..751d212b 100644 --- a/git/git.v +++ b/git/git.v @@ -30,9 +30,11 @@ pub fn Git.exec_in_dir_command(dir string, command string) os.Result { return Git.exec_in_dir(dir, args) } -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. // Git.exec_in_dir_with_env runs `git -C ` without going through a // shell, with `extra_env` added on top of the inherited environment. Use this diff --git a/repo/repo.v b/repo/repo.v index 02bc5703..50b5c5f4 100644 --- a/repo/repo.v +++ b/repo/repo.v @@ -1048,6 +1048,16 @@ fn (r &Repo) get_all_file_paths() []string { } // TODO: return ?string +// git runs `git -C ` with an argument vector, never through +// a shell, so `&` and `;` are not shell metacharacters here and the guard below +// is not a security control. It is kept only because it is cheap. +// +// The residual risk is argument injection, not command injection: a ref name +// that begins with `-` can still be read by git as an option. Callers that +// interpolate a caller-supplied ref (see `pr_routes.v`, `repo/branch.v`) should +// pass it after a `--` separator, or move to `Git.exec_in_dir_with_env` with an +// argument array. `split_command` also splits on whitespace, so a ref +// containing a space is broken into several arguments. fn (r &Repo) git(command string) string { if command.contains('&') || command.contains(';') { return ''