From 13ca859655a005b2a87a4935764694302b4f2fda Mon Sep 17 00:00:00 2001
From: Alexander Medvednikov <alexander@medvednikov.com>
Date: Wed, 17 Jun 2026 15:59:30 +0300
Subject: [PATCH] security: fix auth, injection, SSRF, CI-callback, and
 password hardening

Addresses a security review of the forge:

- Repo authorization: replace check_repo_owner(ctx.user.username, repo_name),
  which re-queried a repo by the logged-in user's own name, with
  can_admin_repo(ctx, repo) checking the already-loaded target repo. Owning a
  repo with the same name no longer grants admin over another user's repo
  (settings/features/delete/move + all webhook routes).

- Private-repo access: add can_read_repo(ctx, repo) (public -> anyone,
  private -> owner) and enforce it on blob, raw, contributors, commit/diff,
  commits list, tag-archive download, branches, and releases, which previously
  served private repo contents with no check.

- Command injection: rewrite create_file_in_bare_repo to run git via argument
  arrays (new Git.exec_in_dir_with_env) instead of /bin/sh strings, so commit
  messages, author, file paths and branch names can no longer inject shell
  commands. Validate branch (is_safe_ref) and path (is_valid_repo_file_path).

- Git HTTP auth: require two header fields AND Basic type (was ||); avoid an
  out-of-bounds read on a malformed header.

- user_has_repo returned count >= 0 (always true); now count > 0 so repo move
  blocks name collisions.

- Session cookie hardened with HttpOnly and SameSite=Lax.

- Webhook SSRF: is_safe_webhook_url resolves the destination and rejects
  loopback/private/CGNAT/link-local/metadata addresses (IPv4 and IPv6),
  enforced at webhook creation and at delivery.

- CI status callback authenticated with HMAC-SHA256 over the body using a
  shared ci_secret (constant-time compare); unauthenticated only when no
  secret is configured, with a warning. (Runner-side signing lives in the
  untracked gitly_ci/ module.)

- Passwords hashed with bcrypt (cost 12); legacy salted-SHA-256 hashes still
  verify and are upgraded to bcrypt on next login (web login + git-HTTP auth).

- Add security_test.v covering the input validators, webhook IP classifiers,
  and password hashing.
---
 ci/ci_routes.v         |  24 +++++++
 commit/commit_routes.v |   8 +++
 config/loader.v        |  10 ++-
 git/git.v              |  27 ++++++++
 repo/branch_routes.v   |   3 +
 repo/file_routes.v     | 144 +++++++++++++++++++++++++----------------
 repo/git_routes.v      |   8 ++-
 repo/release_routes.v  |   4 ++
 repo/repo.v            |  25 +++++--
 repo/repo_routes.v     |  25 +++++--
 repo/tag_routes.v      |   4 ++
 security_test.v        |  98 ++++++++++++++++++++++++++++
 user/user.v            |  66 +++++++++++++++++--
 user/user_routes.v     |   3 +
 webhook.v              | 113 ++++++++++++++++++++++++++++++++
 webhook_routes.v       |  15 +++--
 16 files changed, 492 insertions(+), 85 deletions(-)
 create mode 100644 security_test.v

diff --git a/ci/ci_routes.v b/ci/ci_routes.v
index a2b33b5..d2dbf38 100644
--- a/ci/ci_routes.v
+++ b/ci/ci_routes.v
@@ -6,6 +6,9 @@ import x.json2 as json
 import net.http
 import time
 import git
+import crypto.hmac
+import crypto.sha256
+import encoding.hex
 
 struct CiStatusCallback {
 	run_id      string
@@ -19,6 +22,10 @@ struct CiStatusCallback {
 @['/api/v1/ci/status'; post]
 pub fn (mut app App) handle_ci_status_callback() veb.Result {
 	body := ctx.req.data
+	if !app.verify_ci_callback_signature(ctx, body) {
+		ctx.res.set_status(.unauthorized)
+		return ctx.json_error('Invalid or missing CI callback signature')
+	}
 	callback := json.decode[CiStatusCallback](body) or {
 		return ctx.json_error('Invalid request body')
 	}
@@ -37,6 +44,23 @@ pub fn (mut app App) handle_ci_status_callback() veb.Result {
 	})
 }
 
+// verify_ci_callback_signature checks the HMAC-SHA256 signature of the raw
+// callback body against the shared ci_secret, using a constant-time compare.
+// When no secret is configured it allows the request (preserving the previous
+// behaviour) but logs a warning; set `ci_secret` in both gitly and gitly_ci to
+// require signed callbacks and stop anyone from spoofing CI status.
+fn (mut app App) verify_ci_callback_signature(ctx &Context, body string) bool {
+	secret := app.config.ci_secret
+	if secret == '' {
+		app.warn('CI status callback accepted WITHOUT authentication; set ci_secret in gitly and gitly_ci to require signed callbacks')
+		return true
+	}
+	provided := ctx.get_custom_header('X-Gitly-CI-Signature') or { return false }
+	mac := hmac.new(secret.bytes(), body.bytes(), sha256.sum, sha256.block_size)
+	expected := 'sha256=' + hex.encode(mac)
+	return hmac.equal(provided.bytes(), expected.bytes())
+}
+
 // 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 {
diff --git a/commit/commit_routes.v b/commit/commit_routes.v
index a969f38..0480970 100644
--- a/commit/commit_routes.v
+++ b/commit/commit_routes.v
@@ -31,6 +31,10 @@ fn (mut app App) handle_commits_count(mut ctx Context, username string, repo_nam
 pub fn (mut app App) commits(mut ctx Context, username string, repo_name string, branch_name string, page 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) {
+		return ctx.not_found()
+	}
+
 	page_i := page.int()
 	branch := app.find_repo_branch_by_name(repo.id, branch_name)
 	commits_count := app.get_repo_commit_count(repo.id, branch.id)
@@ -80,6 +84,10 @@ pub fn (mut app App) commits(mut ctx Context, username string, repo_name string,
 pub fn (mut app App) commit(mut ctx Context, username string, repo_name string, hash 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) {
+		return ctx.not_found()
+	}
+
 	is_patch_request := hash.ends_with('.patch')
 
 	if is_patch_request {
diff --git a/config/loader.v b/config/loader.v
index cb91a7b..4f176d3 100644
--- a/config/loader.v
+++ b/config/loader.v
@@ -10,9 +10,13 @@ pub:
 	avatars_path      string
 	hostname          string
 	ci_service_url    string
-	port              int
-	pg                PgConfig
-	sqlite            SqliteConfig
+	// ci_secret is the shared secret used to authenticate CI status callbacks
+	// from gitly_ci (HMAC-SHA256 over the request body). Must match gitly_ci's
+	// ci_secret. When empty, callbacks are accepted unauthenticated (insecure).
+	ci_secret string
+	port      int
+	pg        PgConfig
+	sqlite    SqliteConfig
 }
 
 pub struct PgConfig {
diff --git a/git/git.v b/git/git.v
index b54b6b5..06f6f88 100644
--- a/git/git.v
+++ b/git/git.v
@@ -25,6 +25,33 @@ pub fn Git.exec_shell(command string) os.Result {
 	return os.exec(['/bin/sh', '-c', command])
 }
 
+// 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.
+pub fn Git.exec_in_dir_with_env(dir string, args []string, extra_env map[string]string) os.Result {
+	mut full_args := ['-C', dir]
+	full_args << args
+	mut p := os.new_process('git')
+	p.set_args(full_args)
+	mut merged := os.environ()
+	for k, v in extra_env {
+		merged[k] = v
+	}
+	p.set_environment(merged)
+	p.set_redirect_stdio()
+	p.run()
+	output := p.stdout_slurp() + p.stderr_slurp()
+	p.wait()
+	code := p.code
+	p.close()
+	return os.Result{
+		exit_code: code
+		output:    output
+	}
+}
+
 pub fn Git.clone(url string, path string) os.Result {
 	println('new clone url="${url}" path="${path}"')
 	return os.exec(['git', 'clone', '--bare', url, path])
diff --git a/repo/branch_routes.v b/repo/branch_routes.v
index 5a9202f..11dc4ba 100644
--- a/repo/branch_routes.v
+++ b/repo/branch_routes.v
@@ -28,6 +28,9 @@ pub fn (mut app App) branches(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')
 	}
+	if !app.can_read_repo(ctx, repo) {
+		return ctx.not_found()
+	}
 	branches := app.get_all_repo_branches(repo.id)
 	return $veb.html()
 }
diff --git a/repo/file_routes.v b/repo/file_routes.v
index d4cff55..4c225fb 100644
--- a/repo/file_routes.v
+++ b/repo/file_routes.v
@@ -185,8 +185,22 @@ fn (mut app App) create_file_in_bare_repo(mut repo Repo, branch string, file_pat
 	git_dir := repo.git_dir
 	app.info('Creating file ${file_path} in ${git_dir} on branch ${branch}')
 
+	// Validate untrusted inputs before they reach git. Every git invocation
+	// below passes its arguments as an array (never through a shell), but we
+	// still reject values git itself could treat as flags/refs or that contain
+	// control characters. This guards both the create-file and update-file
+	// routes, which both funnel through here.
+	if !is_safe_ref(branch) {
+		app.warn('Refusing to write file: invalid branch name "${branch}"')
+		return false
+	}
+	if !is_valid_repo_file_path(file_path) {
+		app.warn('Refusing to write file: invalid file path "${file_path}"')
+		return false
+	}
+
 	// Write content to a temp file, then hash it into git
-	tmp_file := '/tmp/gitly_newfile_${repo.id}'
+	tmp_file := os.join_path(os.temp_dir(), 'gitly_newfile_${repo.id}_${os.getpid()}')
 	os.write_file(tmp_file, content) or {
 		app.warn('Failed to write temp file: ${err}')
 		return false
@@ -195,81 +209,81 @@ fn (mut app App) create_file_in_bare_repo(mut repo Repo, branch string, file_pat
 		os.rm(tmp_file) or {}
 	}
 
-	// 1. Hash the blob
-	blob_hash := sh('git -C ${git_dir} hash-object -w ${tmp_file}')
+	// 1. Hash the blob into the object store
+	hash_res := git.Git.exec_in_dir(git_dir, ['hash-object', '-w', tmp_file])
+	if hash_res.exit_code != 0 {
+		app.warn('hash-object failed: ${hash_res.output}')
+		return false
+	}
+	blob_hash := hash_res.output.trim_space()
 	if blob_hash == '' {
-		app.warn('hash-object failed')
+		app.warn('hash-object produced no hash')
 		return false
 	}
 
-	// 2. Read the current tree for this branch (if it exists)
-	mut parent_commit := ''
-	existing_tree := sh('git -C ${git_dir} rev-parse "${branch}^{tree}"')
+	// 2. Find the current tree and parent commit for this branch (both may be
+	//    empty when committing to a brand-new branch).
+	tree_res := git.Git.exec_in_dir(git_dir, ['rev-parse', '${branch}^{tree}'])
+	existing_tree := if tree_res.exit_code == 0 { tree_res.output.trim_space() } else { '' }
 	has_existing_tree := existing_tree != ''
 
-	// Get parent commit hash
-	parent_commit = sh('git -C ${git_dir} rev-parse ${branch}')
-
-	// 3. Build a new tree
-	mut new_tree_hash := ''
-	if has_existing_tree {
-		tmp_index := '/tmp/gitly_index_${repo.id}'
-		defer {
-			os.rm(tmp_index) or {}
-		}
+	parent_res := git.Git.exec_in_dir(git_dir, ['rev-parse', branch])
+	parent_commit := if parent_res.exit_code == 0 { parent_res.output.trim_space() } else { '' }
 
-		// Read existing tree into temp index
-		r1 :=
-			git.Git.exec_shell('GIT_INDEX_FILE=${tmp_index} git -C ${git_dir} read-tree ${existing_tree}')
-		if r1.exit_code != 0 {
-			app.warn('read-tree failed: ${r1.output}')
-			return false
-		}
+	// 3. Build the new tree inside an isolated temp index, so neither the
+	//    repo's own index nor a concurrent edit of the same repo is affected.
+	//    Starting from an empty index (no read-tree) handles the new-branch
+	//    case without needing `mktree`.
+	tmp_index := os.join_path(os.temp_dir(), 'gitly_index_${repo.id}_${os.getpid()}')
+	os.rm(tmp_index) or {}
+	defer {
+		os.rm(tmp_index) or {}
+	}
+	index_env := {
+		'GIT_INDEX_FILE': tmp_index
+	}
 
-		// Add the new blob to the index
-		r2 :=
-			git.Git.exec_shell('GIT_INDEX_FILE=${tmp_index} git -C ${git_dir} update-index --add --cacheinfo 100644,${blob_hash},${file_path}')
-		if r2.exit_code != 0 {
-			app.warn('update-index failed: ${r2.output}')
+	if has_existing_tree {
+		read_res := git.Git.exec_in_dir_with_env(git_dir, ['read-tree', existing_tree], index_env)
+		if read_res.exit_code != 0 {
+			app.warn('read-tree failed: ${read_res.output}')
 			return false
 		}
+	}
 
-		// Write the tree
-		r3 := git.Git.exec_shell('GIT_INDEX_FILE=${tmp_index} git -C ${git_dir} write-tree')
-		if r3.exit_code != 0 {
-			app.warn('write-tree failed: ${r3.output}')
-			return false
-		}
-		new_tree_hash = r3.output.trim_space()
-	} else {
-		// No existing tree — create from scratch using mktree
-		tree_entry := '100644 blob ${blob_hash}\t${file_path}'
-		tmp_tree := '/tmp/gitly_tree_${repo.id}'
-		os.write_file(tmp_tree, tree_entry + '\n') or { return false }
-		defer {
-			os.rm(tmp_tree) or {}
-		}
-		r := git.Git.exec_shell('git -C ${git_dir} mktree < ${tmp_tree}')
-		if r.exit_code != 0 {
-			app.warn('mktree failed: ${r.output}')
-			return false
-		}
-		new_tree_hash = r.output.trim_space()
+	add_res := git.Git.exec_in_dir_with_env(git_dir, ['update-index', '--add', '--cacheinfo',
+		'100644,${blob_hash},${file_path}'], index_env)
+	if add_res.exit_code != 0 {
+		app.warn('update-index failed: ${add_res.output}')
+		return false
 	}
 
+	write_res := git.Git.exec_in_dir_with_env(git_dir, ['write-tree'], index_env)
+	if write_res.exit_code != 0 {
+		app.warn('write-tree failed: ${write_res.output}')
+		return false
+	}
+	new_tree_hash := write_res.output.trim_space()
 	if new_tree_hash == '' {
 		app.warn('Failed to create tree')
 		return false
 	}
 
-	// 4. Create a commit
-	mut parent_flag := ''
+	// 4. Create the commit. The message and author are passed as plain
+	//    arguments / environment values, so any shell metacharacters they
+	//    contain are inert.
+	mut commit_args := ['commit-tree', new_tree_hash]
 	if parent_commit != '' {
-		parent_flag = '-p ${parent_commit}'
+		commit_args << ['-p', parent_commit]
 	}
-
-	commit_sh := 'GIT_AUTHOR_NAME="${author}" GIT_AUTHOR_EMAIL="${author}@gitly" GIT_COMMITTER_NAME="${author}" GIT_COMMITTER_EMAIL="${author}@gitly" git -C ${git_dir} commit-tree ${new_tree_hash} ${parent_flag} -m "${message}"'
-	r4 := git.Git.exec_shell(commit_sh)
+	commit_args << ['-m', message]
+	commit_env := {
+		'GIT_AUTHOR_NAME':     author
+		'GIT_AUTHOR_EMAIL':    '${author}@gitly'
+		'GIT_COMMITTER_NAME':  author
+		'GIT_COMMITTER_EMAIL': '${author}@gitly'
+	}
+	r4 := git.Git.exec_in_dir_with_env(git_dir, commit_args, commit_env)
 	if r4.exit_code != 0 {
 		app.warn('commit-tree failed: ${r4.output}')
 		return false
@@ -294,3 +308,21 @@ fn sh(cmd string) string {
 	}
 	return r.output.trim_space()
 }
+
+// is_valid_repo_file_path rejects empty/over-long paths, absolute paths, leading
+// dashes, parent-directory traversal, and any control characters (including NUL
+// and newlines).
+fn is_valid_repo_file_path(path string) bool {
+	if path.len == 0 || path.len > 4096 {
+		return false
+	}
+	if path.starts_with('/') || path.starts_with('-') || path.contains('..') {
+		return false
+	}
+	for c in path {
+		if c < 0x20 || c == 0x7f {
+			return false
+		}
+	}
+	return true
+}
diff --git a/repo/git_routes.v b/repo/git_routes.v
index 4a4f695..bb5c09c 100644
--- a/repo/git_routes.v
+++ b/repo/git_routes.v
@@ -119,9 +119,11 @@ fn (mut app App) check_git_http_access(mut ctx Context, repository_owner string,
 fn (ctx &Context) check_basic_authorization_header() bool {
 	auth_header := ctx.get_header(.authorization) or { return false }
 	auth_header_parts := auth_header.fields()
-	auth_type := auth_header_parts[0]
-	is_basic_auth_type := auth_type == 'Basic'
-	return auth_header_parts.len == 2 || is_basic_auth_type
+	if auth_header_parts.len != 2 {
+		return false
+	}
+	is_basic_auth_type := auth_header_parts[0] == 'Basic'
+	return is_basic_auth_type
 }
 
 fn (ctx &Context) extract_user_credentials() ?(string, string) {
diff --git a/repo/release_routes.v b/repo/release_routes.v
index e21af9f..8e309dc 100644
--- a/repo/release_routes.v
+++ b/repo/release_routes.v
@@ -14,6 +14,10 @@ pub fn (mut app App) releases_default(mut ctx Context, username string, repo_nam
 pub fn (mut app App) releases(mut ctx Context, username string, repo_name string, page 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) {
+		return ctx.not_found()
+	}
+
 	page_i := page.int()
 	repo_id := repo.id
 	mut releases := []Release{}
diff --git a/repo/repo.v b/repo/repo.v
index 8d9be4e..e3aae2d 100644
--- a/repo/repo.v
+++ b/repo/repo.v
@@ -371,7 +371,7 @@ fn (mut app App) user_has_repo(user_id int, repo_name string) bool {
 	count := sql app.db {
 		select count from Repo where user_id == user_id && name == repo_name && is_deleted == false
 	} or { 0 }
-	return count >= 0
+	return count > 0
 }
 
 fn (mut app App) update_repo_from_fs(mut repo Repo, recompute_lang_stats bool) ! {
@@ -1183,6 +1183,18 @@ fn find_license_file(items []File) ?File {
 	return files[0]
 }
 
+// can_read_repo reports whether the current request may read the given,
+// already-loaded repo's contents (tree/blob/raw/contributors/...). Public
+// repos are readable by anyone, including anonymous visitors; private repos
+// are readable only by their owner. Call this on every repo-scoped read route
+// before running a git command or returning repo data.
+fn (app &App) can_read_repo(ctx Context, repo Repo) bool {
+	if repo.is_public {
+		return true
+	}
+	return ctx.logged_in && ctx.user.id != 0 && repo.user_id == ctx.user.id
+}
+
 fn (app &App) has_user_repo_read_access(ctx Context, user_id int, repo_id int) bool {
 	if !ctx.logged_in {
 		return false
@@ -1204,8 +1216,11 @@ fn (app &App) has_user_repo_read_access_by_repo_name(ctx Context, user_id int, r
 	return app.has_user_repo_read_access(ctx, user_id, repo.id)
 }
 
-fn (app &App) check_repo_owner(username string, repo_name string) bool {
-	user := app.get_user_by_username(username) or { return false }
-	repo := app.find_repo_by_name_and_user_id(repo_name, user.id) or { return false }
-	return repo.user_id == user.id
+// can_admin_repo reports whether the currently logged-in user is allowed to
+// administer (settings/delete/move/webhooks/...) the given, already-loaded
+// target repo. It must be called with the repo loaded from the URL owner, not
+// re-queried by the logged-in user's name — otherwise a user could pass the
+// check for someone else's repo simply by owning a repo with the same name.
+fn (app &App) can_admin_repo(ctx Context, repo Repo) bool {
+	return ctx.logged_in && ctx.user.id != 0 && repo.user_id == ctx.user.id
 }
diff --git a/repo/repo_routes.v b/repo/repo_routes.v
index 5911bff..6709c4c 100644
--- a/repo/repo_routes.v
+++ b/repo/repo_routes.v
@@ -53,7 +53,7 @@ pub fn (mut app App) repo_settings(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)
 	}
-	is_owner := app.check_repo_owner(ctx.user.username, repo_name)
+	is_owner := app.can_admin_repo(ctx, repo)
 
 	if !is_owner {
 		return ctx.redirect_to_repository(username, repo_name)
@@ -67,7 +67,7 @@ pub fn (mut app App) handle_update_repo_settings(username string, repo_name stri
 	repo := app.find_repo_by_name_and_username(repo_name, username) or {
 		return ctx.redirect_to_repository(username, repo_name)
 	}
-	is_owner := app.check_repo_owner(ctx.user.username, repo_name)
+	is_owner := app.can_admin_repo(ctx, repo)
 
 	if !is_owner {
 		return ctx.redirect_to_repository(username, repo_name)
@@ -86,7 +86,7 @@ pub fn (mut app App) handle_update_repo_features(username string, repo_name stri
 	repo := app.find_repo_by_name_and_username(repo_name, username) or {
 		return ctx.redirect_to_repository(username, repo_name)
 	}
-	is_owner := app.check_repo_owner(ctx.user.username, repo_name)
+	is_owner := app.can_admin_repo(ctx, repo)
 
 	if !is_owner {
 		return ctx.redirect_to_repository(username, repo_name)
@@ -108,7 +108,7 @@ pub fn (mut app App) handle_repo_delete(username string, repo_name string) veb.R
 	repo := app.find_repo_by_name_and_username(repo_name, username) or {
 		return ctx.redirect_to_repository(username, repo_name)
 	}
-	is_owner := app.check_repo_owner(ctx.user.username, repo_name)
+	is_owner := app.can_admin_repo(ctx, repo)
 
 	if !is_owner {
 		return ctx.redirect_to_repository(username, repo_name)
@@ -129,7 +129,7 @@ pub fn (mut app App) handle_repo_move(username string, repo_name string, dest st
 	repo := app.find_repo_by_name_and_username(repo_name, username) or {
 		return ctx.redirect_to_index()
 	}
-	is_owner := app.check_repo_owner(ctx.user.username, repo_name)
+	is_owner := app.can_admin_repo(ctx, repo)
 
 	if !is_owner {
 		return ctx.redirect_to_repository(username, repo_name)
@@ -244,7 +244,8 @@ pub fn (mut app App) handle_new_repo(mut ctx Context, name string, clone_url str
 		owner_name = org.name
 		owner_org_id = org.id
 	}
-	if owner_org_id == 0 && !ctx.is_admin() && app.get_count_user_repos(ctx.user.id) >= max_user_repos {
+	if owner_org_id == 0 && !ctx.is_admin()
+		&& app.get_count_user_repos(ctx.user.id) >= max_user_repos {
 		ctx.error('You have reached the limit for the number of repositories')
 		return app.new(mut ctx)
 	}
@@ -795,6 +796,10 @@ pub fn (mut app App) handle_api_repo_files(mut ctx Context, repo_id_str string)
 pub fn (mut app App) contributors(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) {
+		return ctx.not_found()
+	}
+
 	contributors := app.find_repo_registered_contributor(repo.id)
 
 	return $veb.html()
@@ -804,6 +809,10 @@ pub fn (mut app App) contributors(mut ctx Context, username string, repo_name st
 pub fn (mut app App) blob(mut ctx Context, username string, repo_name string, branch_name string, path 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) {
+		return ctx.not_found()
+	}
+
 	mut path_parts := path.split('/')
 	path_parts.pop()
 
@@ -834,6 +843,10 @@ pub fn (mut app App) handle_raw(mut ctx Context, username string, repo_name stri
 	user := app.get_user_by_username(username) or { return ctx.not_found() }
 	repo := app.find_repo_by_name_and_user_id(repo_name, user.id) or { return ctx.not_found() }
 
+	if !app.can_read_repo(ctx, repo) {
+		return ctx.not_found()
+	}
+
 	// TODO: throw error when git returns non-zero status
 	file_source := repo.git('--no-pager show ${branch_name}:${path}')
 
diff --git a/repo/tag_routes.v b/repo/tag_routes.v
index abd3304..ed95c2d 100644
--- a/repo/tag_routes.v
+++ b/repo/tag_routes.v
@@ -9,6 +9,10 @@ pub fn (mut app App) handle_download_tag_archive(username string, repo_name stri
 	user := app.get_user_by_username(username) or { return ctx.not_found() }
 	repo := app.find_repo_by_name_and_user_id(repo_name, user.id) or { return ctx.not_found() }
 
+	if !app.can_read_repo(ctx, repo) {
+		return ctx.not_found()
+	}
+
 	archive_abs_path := os.abs_path(app.config.archive_path)
 	snapshot_format := if format == 'zip' { 'zip' } else { 'tar.gz' }
 	snapshot_name := '${username}_${repo_name}_${tag}.${snapshot_format}'
diff --git a/security_test.v b/security_test.v
new file mode 100644
index 0000000..a5d76af
--- /dev/null
+++ b/security_test.v
@@ -0,0 +1,98 @@
+module main
+
+import crypto.sha256
+
+// Regression tests for the input validation that guards git command
+// construction in create_file_in_bare_repo (see repo/file_routes.v). These
+// inputs used to be interpolated into shell strings; they are now passed to
+// git as plain arguments, but we still reject values git could misread as
+// flags/refs or that contain control characters.
+
+fn test_is_valid_repo_file_path_accepts_normal_paths() {
+	assert is_valid_repo_file_path('README.md')
+	assert is_valid_repo_file_path('src/main.v')
+	assert is_valid_repo_file_path('dir/file,with,commas.txt')
+	assert is_valid_repo_file_path('a/b/c/d.e')
+}
+
+fn test_is_valid_repo_file_path_rejects_dangerous_paths() {
+	assert !is_valid_repo_file_path('')
+	assert !is_valid_repo_file_path('/etc/passwd') // absolute
+	assert !is_valid_repo_file_path('-rf') // looks like a flag
+	assert !is_valid_repo_file_path('../../etc/passwd') // traversal
+	assert !is_valid_repo_file_path('a/../../b') // traversal
+	assert !is_valid_repo_file_path('file\nname') // newline
+	assert !is_valid_repo_file_path('file\x00name') // NUL
+	assert !is_valid_repo_file_path('a\tb') // tab / control char
+}
+
+fn test_is_safe_ref_accepts_normal_branches() {
+	assert is_safe_ref('master')
+	assert is_safe_ref('feature/new-thing')
+	assert is_safe_ref('release-1.2.3')
+}
+
+fn test_is_safe_ref_rejects_injection_attempts() {
+	assert !is_safe_ref('')
+	assert !is_safe_ref('--upload-pack=touch /tmp/pwned') // leading dash + space
+	assert !is_safe_ref('master;rm -rf /') // shell metacharacters
+	assert !is_safe_ref('master$(whoami)')
+	assert !is_safe_ref('master`id`')
+	assert !is_safe_ref('a..b') // ref traversal
+	assert !is_safe_ref('branch with spaces')
+}
+
+// Webhook SSRF guard: the IP classifiers must reject internal destinations and
+// allow public ones. (is_safe_webhook_url itself does DNS and isn't unit-tested.)
+
+fn test_is_blocked_ipv4_blocks_internal_ranges() {
+	assert is_blocked_ipv4('127.0.0.1') // loopback
+	assert is_blocked_ipv4('10.1.2.3') // private
+	assert is_blocked_ipv4('172.16.5.5') // private
+	assert is_blocked_ipv4('172.31.255.255') // private (edge)
+	assert is_blocked_ipv4('192.168.0.1') // private
+	assert is_blocked_ipv4('169.254.169.254') // link-local / cloud metadata
+	assert is_blocked_ipv4('0.0.0.0') // unspecified
+	assert is_blocked_ipv4('100.64.0.1') // CGNAT
+	assert is_blocked_ipv4('224.0.0.1') // multicast
+	assert is_blocked_ipv4('garbage') // unparseable -> fail closed
+}
+
+fn test_is_blocked_ipv4_allows_public() {
+	assert !is_blocked_ipv4('8.8.8.8')
+	assert !is_blocked_ipv4('1.1.1.1')
+	assert !is_blocked_ipv4('172.32.0.1') // just outside 172.16/12
+	assert !is_blocked_ipv4('172.15.0.1') // just outside 172.16/12
+	assert !is_blocked_ipv4('93.184.216.34')
+}
+
+fn test_is_blocked_ipv6() {
+	assert is_blocked_ipv6('::1') // loopback
+	assert is_blocked_ipv6('::') // unspecified
+	assert is_blocked_ipv6('fe80::1') // link-local
+	assert is_blocked_ipv6('fc00::1') // unique-local
+	assert is_blocked_ipv6('fd12:3456::1') // unique-local
+	assert is_blocked_ipv6('::ffff:127.0.0.1') // IPv4-mapped loopback
+	assert !is_blocked_ipv6('2606:4700:4700::1111') // public
+	assert !is_blocked_ipv6('::ffff:8.8.8.8') // IPv4-mapped public
+}
+
+// Password hashing: new hashes must be bcrypt, and legacy salted-SHA-256 hashes
+// must still verify (so existing users aren't locked out before re-login).
+
+fn test_new_passwords_are_bcrypt() {
+	h := hash_password_with_salt('s3cret-pw', 'ignored-salt')
+	assert h.starts_with('$2') // bcrypt hash
+	assert !password_hash_is_legacy(h)
+	assert compare_password_with_hash('s3cret-pw', 'ignored-salt', h)
+	assert !compare_password_with_hash('wrong-pw', 'ignored-salt', h)
+}
+
+fn test_legacy_sha256_hashes_still_verify() {
+	salt := 'abc123'
+	// Legacy scheme was sha256('${password}${salt}').
+	legacy := sha256.sum('hunter2${salt}'.bytes()).hex()
+	assert password_hash_is_legacy(legacy)
+	assert compare_password_with_hash('hunter2', salt, legacy)
+	assert !compare_password_with_hash('nope', salt, legacy)
+}
diff --git a/user/user.v b/user/user.v
index d76c731..a9165d5 100644
--- a/user/user.v
+++ b/user/user.v
@@ -1,9 +1,14 @@
 module main
 
 import crypto.sha256
+import crypto.bcrypt
 import time
 import os
 
+// bcrypt_cost is the work factor for password hashing. 12 is a good balance of
+// security and speed on current hardware.
+const bcrypt_cost = 12
+
 struct User {
 	id              int @[primary; sql: serial]
 	full_name       string
@@ -52,14 +57,54 @@ pub fn (mut app App) set_user_admin_status(user_id int, status bool) ! {
 	}!
 }
 
+// hash_password_with_salt returns a bcrypt hash of the password. bcrypt
+// generates and embeds its own random salt with a tunable cost factor, so the
+// `salt` argument is ignored for new hashes (kept only so existing callers and
+// the User.salt column are unaffected).
 fn hash_password_with_salt(password string, salt string) string {
-	salted_password := '${password}${salt}'
-
-	return sha256.sum(salted_password.bytes()).hex().str()
+	return bcrypt.generate_from_password(password.bytes(), bcrypt_cost) or { '' }
 }
 
+// compare_password_with_hash verifies a password against a stored hash. It
+// accepts both new bcrypt hashes ($2...) and legacy salted-SHA-256 hashes, so
+// users created before the bcrypt migration can still log in; their hash is
+// upgraded to bcrypt on next login (see maybe_upgrade_password_hash).
 fn compare_password_with_hash(password string, salt string, hashed string) bool {
-	return hash_password_with_salt(password, salt) == hashed
+	if password_hash_is_legacy(hashed) {
+		legacy := sha256.sum('${password}${salt}'.bytes()).hex()
+		return legacy == hashed
+	}
+	bcrypt.compare_hash_and_password(password.bytes(), hashed.bytes()) or { return false }
+	return true
+}
+
+// password_hash_is_legacy reports whether a stored hash uses the old
+// salted-SHA-256 scheme (anything that is not a bcrypt `$2...` hash) and should
+// be upgraded to bcrypt after a successful login.
+fn password_hash_is_legacy(hashed string) bool {
+	return !hashed.starts_with('$2')
+}
+
+// maybe_upgrade_password_hash rehashes a legacy password with bcrypt after the
+// user has successfully authenticated, so old SHA-256 hashes are phased out
+// transparently. The plaintext password is only available at login time.
+fn (mut app App) maybe_upgrade_password_hash(user User, password string) {
+	if !password_hash_is_legacy(user.password) {
+		return
+	}
+	new_hash := hash_password_with_salt(password, '')
+	if new_hash == '' {
+		return
+	}
+	app.update_user_password_hash(user.id, new_hash) or {
+		app.info('failed to upgrade password hash for user ${user.id}: ${err}')
+	}
+}
+
+fn (mut app App) update_user_password_hash(user_id int, hashed string) ! {
+	sql app.db {
+		update User set password = hashed where id == user_id
+	}!
 }
 
 pub fn (mut app App) register_user(username string, password string, salt string, emails []string, github bool, is_admin bool) !bool {
@@ -432,7 +477,18 @@ pub fn (mut app App) auth_user(mut ctx Context, user User, ip string) ! {
 	token := app.add_token(user.id, ip)!
 	app.update_user_login_attempts(user.id, 0)!
 	expire_date := time.now().add_days(200)
-	ctx.set_cookie(name: 'token', value: token, expires: expire_date)
+	// HttpOnly keeps the session token out of reach of JavaScript (so an XSS
+	// payload can't steal it); SameSite=Lax stops the cookie from riding along
+	// on cross-site requests, mitigating CSRF. Set `secure: true` as well when
+	// deploying behind HTTPS.
+	ctx.set_cookie(
+		name:      'token'
+		value:     token
+		expires:   expire_date
+		path:      '/'
+		http_only: true
+		same_site: .same_site_lax_mode
+	)
 }
 
 pub fn (mut app App) is_logged_in(mut ctx Context) bool {
diff --git a/user/user_routes.v b/user/user_routes.v
index 87a5508..95ad042 100644
--- a/user/user_routes.v
+++ b/user/user_routes.v
@@ -42,6 +42,9 @@ pub fn (mut app App) handle_login(mut ctx Context, username string, password str
 	if !user.is_registered {
 		return ctx.redirect_to_login()
 	}
+	// Transparently migrate legacy salted-SHA-256 hashes to bcrypt now that we
+	// have the plaintext password and know it is correct.
+	app.maybe_upgrade_password_hash(user, password)
 	if app.user_has_two_factor(user.id) {
 		expires := time.now().unix() + two_factor_pending_ttl
 		token := app.sign_pending_2fa(user, expires)
diff --git a/webhook.v b/webhook.v
index 24e1b28..339edc1 100644
--- a/webhook.v
+++ b/webhook.v
@@ -3,7 +3,9 @@
 module main
 
 import time
+import net
 import net.http
+import net.urllib
 import crypto.hmac
 import crypto.sha256
 import encoding.hex
@@ -188,7 +190,118 @@ fn (mut app App) fan_out_webhook(repo_id int, event string, body string) {
 	}
 }
 
+// is_blocked_ipv4 reports whether the dotted-quad IPv4 string falls in a range
+// a webhook must never reach: unspecified (0/8), loopback (127/8), private
+// (10/8, 172.16/12, 192.168/16), CGNAT (100.64/10), link-local (169.254/16,
+// which includes the cloud metadata address 169.254.169.254), and
+// multicast/reserved (>=224). Unparseable input is treated as blocked.
+fn is_blocked_ipv4(ip string) bool {
+	parts := ip.split('.')
+	if parts.len != 4 {
+		return true
+	}
+	mut o := [4]int{}
+	for i in 0 .. 4 {
+		if parts[i] == '' {
+			return true
+		}
+		n := parts[i].int()
+		if n < 0 || n > 255 {
+			return true
+		}
+		o[i] = n
+	}
+	return o[0] == 0 || o[0] == 10 || o[0] == 127 || (o[0] == 169 && o[1] == 254)
+		|| (o[0] == 172 && o[1] >= 16 && o[1] <= 31) || (o[0] == 192 && o[1] == 168)
+		|| (o[0] == 100 && o[1] >= 64 && o[1] <= 127) || o[0] >= 224
+}
+
+// is_blocked_ipv6 reports whether the IPv6 text (no brackets) is loopback (::1),
+// unspecified (::), unique-local (fc00::/7), link-local (fe80::/10), or an
+// IPv4-mapped address (e.g. ::ffff:127.0.0.1) pointing at a blocked IPv4.
+fn is_blocked_ipv6(ip_in string) bool {
+	ip := ip_in.to_lower()
+	if ip == '::1' || ip == '::' {
+		return true
+	}
+	// IPv4-mapped/-compatible forms end in a dotted quad.
+	if ip.contains('.') {
+		tail := ip.all_after_last(':')
+		if tail.contains('.') {
+			return is_blocked_ipv4(tail)
+		}
+	}
+	if ip.starts_with('fc') || ip.starts_with('fd') {
+		return true
+	}
+	if ip.starts_with('fe8') || ip.starts_with('fe9') || ip.starts_with('fea')
+		|| ip.starts_with('feb') {
+		return true
+	}
+	return false
+}
+
+// is_safe_webhook_url validates a webhook destination before any server-side
+// request is made: the scheme must be http(s), the host must resolve, and none
+// of the resolved addresses may be loopback/private/link-local. Resolving here
+// blocks hostnames that point at internal IPs; it cannot fully defeat DNS
+// rebinding between this check and delivery, but it closes the common SSRF
+// vectors (literal internal IPs, localhost, cloud metadata endpoints).
+fn is_safe_webhook_url(raw string) bool {
+	u := urllib.parse(raw) or { return false }
+	scheme := u.scheme.to_lower()
+	if scheme != 'http' && scheme != 'https' {
+		return false
+	}
+	host := u.hostname()
+	if host == '' {
+		return false
+	}
+	lhost := host.to_lower()
+	if lhost == 'localhost' || lhost.ends_with('.localhost') {
+		return false
+	}
+	port := if u.port() != '' {
+		u.port()
+	} else if scheme == 'https' {
+		'443'
+	} else {
+		'80'
+	}
+	addrs := net.resolve_addrs('${host}:${port}', .unspec, .tcp) or { return false }
+	if addrs.len == 0 {
+		return false
+	}
+	for a in addrs {
+		s := a.str()
+		match a.family() {
+			.ip {
+				if is_blocked_ipv4(s.all_before_last(':')) {
+					return false
+				}
+			}
+			.ip6 {
+				if is_blocked_ipv6(s.find_between('[', ']')) {
+					return false
+				}
+			}
+			else {
+				return false
+			}
+		}
+	}
+	return true
+}
+
 fn (mut app App) deliver_webhook(wh Webhook, event string, body string) {
+	// Re-validate at delivery time: this is the authoritative SSRF gate. It
+	// also protects webhooks created before this check existed and catches
+	// hosts whose DNS now points at an internal address.
+	if !is_safe_webhook_url(wh.url) {
+		app.record_webhook_delivery(wh.id, event, 0,
+			'blocked: destination resolves to a disallowed (internal/loopback) address')
+		return
+	}
 	mut signature := ''
 	if wh.secret != '' {
 		sig_bytes := hmac.new(wh.secret.bytes(), body.bytes(), sha256.sum, sha256.block_size)
diff --git a/webhook_routes.v b/webhook_routes.v
index ad3f2de..42f320d 100644
--- a/webhook_routes.v
+++ b/webhook_routes.v
@@ -8,7 +8,7 @@ import validation
 @['/:username/:repo_name/settings/webhooks']
 pub fn (mut app App) repo_webhooks(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.check_repo_owner(ctx.user.username, repo_name) {
+	if !app.can_admin_repo(ctx, repo) {
 		return ctx.redirect_to_repository(username, repo_name)
 	}
 	webhooks := app.list_repo_webhooks(repo.id)
@@ -18,7 +18,7 @@ pub fn (mut app App) repo_webhooks(mut ctx Context, username string, repo_name s
 @['/:username/:repo_name/settings/webhooks/new']
 pub fn (mut app App) new_webhook(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.check_repo_owner(ctx.user.username, repo_name) {
+	if !app.can_admin_repo(ctx, repo) {
 		return ctx.redirect_to_repository(username, repo_name)
 	}
 	return $veb.html('templates/new/webhook.html')
@@ -27,13 +27,14 @@ pub fn (mut app App) new_webhook(mut ctx Context, username string, repo_name str
 @['/:username/:repo_name/settings/webhooks'; post]
 pub fn (mut app App) handle_create_webhook(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.check_repo_owner(ctx.user.username, repo_name) {
+	if !app.can_admin_repo(ctx, repo) {
 		return ctx.redirect_to_repository(username, repo_name)
 	}
 	url := ctx.form['url'].trim_space()
 	secret := ctx.form['secret']
-	if validation.is_string_empty(url) || !(url.starts_with('http://')
-		|| url.starts_with('https://')) {
+	// Reject empty URLs, non-http(s) schemes, and destinations that resolve to
+	// internal/loopback/link-local addresses (SSRF protection).
+	if validation.is_string_empty(url) || !is_safe_webhook_url(url) {
 		return ctx.redirect('/${username}/${repo_name}/settings/webhooks/new')
 	}
 	mut events := []string{}
@@ -53,7 +54,7 @@ pub fn (mut app App) handle_create_webhook(mut ctx Context, username string, rep
 @['/:username/:repo_name/settings/webhooks/:id/delete'; post]
 pub fn (mut app App) handle_delete_webhook(mut ctx Context, username string, repo_name string, id string) veb.Result {
 	repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() }
-	if !app.check_repo_owner(ctx.user.username, repo_name) {
+	if !app.can_admin_repo(ctx, repo) {
 		return ctx.redirect_to_repository(username, repo_name)
 	}
 	wh := app.find_webhook_by_id(id.int()) or { return ctx.not_found() }
@@ -67,7 +68,7 @@ pub fn (mut app App) handle_delete_webhook(mut ctx Context, username string, rep
 @['/:username/:repo_name/settings/webhooks/:id']
 pub fn (mut app App) view_webhook(mut ctx Context, username string, repo_name string, id string) veb.Result {
 	repo := app.find_repo_by_name_and_username(repo_name, username) or { return ctx.not_found() }
-	if !app.check_repo_owner(ctx.user.username, repo_name) {
+	if !app.can_admin_repo(ctx, repo) {
 		return ctx.redirect_to_repository(username, repo_name)
 	}
 	webhook := app.find_webhook_by_id(id.int()) or { return ctx.not_found() }
-- 
2.39.5