GITGUD

neiam neiam /gitgud

Git Gud

public · Issues · Pulls · Labels · Forks · Compare · Actions success · Badges · Packages

⭐ 1

Log in to mark this repository.

Stop serving private and internal repositories to anyone with the URL

466a390 · Gabriel Morell · 2026-09-14 13:52

✓ build ✓ hello
22 files +317 -14
Message 
{commit_body(@commit)}

Files changed

modified lib/git_gud/organizations.ex
+12 −1
@@ -438,7 +438,18 @@ defmodule GitGud.Organizations do
438 438 Resolve `user`'s effective permission on `repo`. Returns one of
439 439 `"admin" | "maintain" | "write" | "triage" | "read" | nil`.
440 440 """
441 − def permission(nil, %Repository{visibility: vis}) when vis in ["public", "internal"], do: "read"
441 + # Anonymous gets `public` and nothing else. "Internal" means what the
442 + # settings page says it means — "any signed-in user can read" — and
443 + # nobody is not a signed-in user.
444 + #
445 + # This clause used to include "internal", which made internal
446 + # repositories world-readable and disagreed with the rest of the
447 + # codebase: `list_repositories(nil)` and `visible_repo_ids_for_org/2`
448 + # both already showed an anonymous visitor public repositories only,
449 + # and `Packages.Auth` already required public for an anonymous read.
450 + # So internal repositories were hidden from every listing and
451 + # readable to anyone who knew the URL.
452 + def permission(nil, %Repository{visibility: "public"}), do: "read"
442 453 def permission(nil, _), do: nil
443 454
444 455 def permission(%User{id: uid}, %Repository{owner_id: uid}), do: "admin"
modified lib/git_gud_web/controllers/api/v1/repository_controller.ex
+10 −4
@@ -9,9 +9,15 @@ defmodule GitGudWeb.Api.V1.RepositoryController do
9 9 * `GET /repos/:owner/:name` — repo metadata (labels, default branch)
10 10 * `GET /repos/:owner/:name/commits/:sha` — commit metadata (the `created` date)
11 11
12 − Public/internal repos are readable unauthenticated; private repos
13 − require a valid `RunnerJwt` (the per-job `github.token`) scoped to that
14 − repo, sent as `Authorization: token <jwt>` or `Bearer <jwt>`.
12 + Public repos are readable unauthenticated. Everything else — private
13 + *and* internal — requires a valid `RunnerJwt` (the per-job
14 + `github.token`) scoped to that repo, sent as
15 + `Authorization: token <jwt>` or `Bearer <jwt>`.
16 +
17 + This API has no notion of a signed-in user, so "internal — any
18 + signed-in user can read" has no one to be true of here. Treating it as
19 + public made internal repository metadata world-readable; a CI job
20 + still reaches its own repository through its token.
15 21 """
16 22 use GitGudWeb, :controller
17 23
@@ -49,7 +55,7 @@ defmodule GitGudWeb.Api.V1.RepositoryController do
49 55 end
50 56
51 57 defp authorized?(conn, repo) do
52 − repo.visibility in ["public", "internal"] or runner_token_for?(conn, repo)
58 + repo.visibility == "public" or runner_token_for?(conn, repo)
53 59 end
54 60
55 61 defp runner_token_for?(conn, repo) do
modified lib/git_gud_web/controllers/workflow_artifact_controller.ex
+5 −0
@@ -57,6 +57,11 @@ defmodule GitGudWeb.WorkflowArtifactController do
57 57 end
58 58 end
59 59
60 + # `_user` here would match `nil`, which made a build artifact of an
61 + # internal repository downloadable by anyone with the URL. Internal
62 + # means "any signed-in user", so anonymous gets public only.
63 + defp can_read?(nil, %Repository{visibility: vis}), do: vis == "public"
64 +
60 65 defp can_read?(_user, %Repository{visibility: vis}) when vis in ["public", "internal"], do: true
61 66 defp can_read?(%{id: uid}, %Repository{owner_id: uid}), do: true
62 67
modified lib/git_gud_web/live/syms_live/show.ex
+3 −1
@@ -39,7 +39,9 @@ defmodule GitGudWeb.SymsLive.Show do
39 39
40 40 # Marked repos can belong to anyone, so this is a per-repo check, not
41 41 # "does the viewer own the marking user's repos".
42 − defp readable?(%{visibility: v}, _viewer) when v in ["public", "internal"], do: true
42 + defp readable?(%{visibility: "public"}, _viewer), do: true
43 + # Internal needs an account; anonymous is not "any signed-in user".
44 + defp readable?(%{visibility: "internal"}, viewer), do: not is_nil(viewer)
43 45 defp readable?(%{owner_id: oid}, %{id: oid}), do: true
44 46 defp readable?(_, _), do: false
45 47
modified lib/git_gud_web/live/user_live/profile.ex
+5 −1
@@ -34,10 +34,14 @@ defmodule GitGudWeb.UserLive.Profile do
34 34
35 35 defp viewer(socket), do: socket.assigns.current_scope && socket.assigns.current_scope.user
36 36
37 + # Anonymous sees public only. "Internal" means any *signed-in* user
38 + # can read, so listing internal repositories to a visitor with no
39 + # account contradicts what the settings page promises.
37 40 defp filter_visible(repos, owner, viewer) do
38 41 Enum.filter(repos, fn r ->
39 42 cond do
40 − r.visibility in ["public", "internal"] -> true
43 + r.visibility == "public" -> true
44 + viewer && r.visibility == "internal" -> true
41 45 viewer && viewer.id == owner.id -> true
42 46 true -> false
43 47 end
added lib/git_gud_web/repo_auth.ex
+68 −0
@@ -0,0 +1,68 @@
1 +defmodule GitGudWeb.RepoAuth do
2 + @moduledoc """
3 + The read gate for every repository-scoped LiveView.
4 +
5 + ## Why this is a hook and not a check in each mount
6 +
7 + It used to be a check in each mount, in the sense that the router
8 + said so — `live_session :public_repo` carried the comment
9 + "Visibility is enforced inside the LiveView itself". It wasn't.
10 + Eighteen of the nineteen browsing LiveViews called
11 + `get_repository_by_path!/2` and rendered whatever came back, so a
12 + private repository's file tree, blobs, commits, issues, pull
13 + requests, wiki and CI logs were readable by anyone who typed the URL.
14 + The listings correctly hid those repositories, which is exactly what
15 + made it hard to notice.
16 +
17 + A rule that has to be remembered in nineteen places is a rule that is
18 + eventually forgotten in one. This runs before any of them mount, so a
19 + new repository page is gated by existing, and the only way to opt out
20 + is to put the route somewhere else on purpose.
21 +
22 + ## Why a 404 and not a 403
23 +
24 + A 403 confirms the repository exists. `Ecto.NoResultsError` is what
25 + `get_repository_by_path!/2` raises for a repository that isn't there,
26 + so a private repository and a nonexistent one produce byte-identical
27 + responses and probing tells an attacker nothing.
28 +
29 + ## Where the real decision lives
30 +
31 + `GitGud.Organizations.can?/3` — the same function the mobile API and
32 + the settings pages already use. This module resolves the repository
33 + and asks; it has no opinion of its own about who may read what.
34 + """
35 +
36 + alias GitGud.Organizations
37 + alias GitGud.Repositories
38 + alias GitGud.Repositories.Repository
39 +
40 + @doc """
41 + Halt unless the mounting user may read `owner/name`.
42 +
43 + Expects `:current_scope` to be assigned already, which the
44 + `UserAuth` hook listed before it does. An unauthenticated visitor has
45 + no user, and `can?/3` answers for `nil` — public and internal
46 + repositories are readable, private ones are not.
47 + """
48 + def on_mount(:require_read, %{"owner" => owner, "name" => name}, _session, socket) do
49 + user = socket.assigns[:current_scope] && socket.assigns.current_scope.user
50 +
51 + case Repositories.find_by_path(owner, name) do
52 + %Repository{} = repo ->
53 + if Organizations.can?(user, repo, :read),
54 + do: {:cont, socket},
55 + else: not_found()
56 +
57 + nil ->
58 + not_found()
59 + end
60 + end
61 +
62 + # Live sessions hold more than repository routes — the home page, an
63 + # org profile, a user's syms. Those carry no owner/name to check, and
64 + # have their own rules.
65 + def on_mount(:require_read, _params, _session, socket), do: {:cont, socket}
66 +
67 + defp not_found, do: raise(Ecto.NoResultsError, queryable: Repository)
68 +end
modified lib/git_gud_web/router.ex
+15 −4
@@ -183,8 +183,14 @@ defmodule GitGudWeb.Router do
183 183 scope "/", GitGudWeb do
184 184 pipe_through [:browser, :require_authenticated_user]
185 185
186 + # Authenticated is not authorized: being signed in says nothing
187 + # about this repository. These pages check admin rights themselves
188 + # where they need to; `RepoAuth` is the floor under all of them.
186 189 live_session :authenticated_repo,
187 − on_mount: [{GitGudWeb.UserAuth, :require_authenticated}] do
190 + on_mount: [
191 + {GitGudWeb.UserAuth, :require_authenticated},
192 + {GitGudWeb.RepoAuth, :require_read}
193 + ] do
188 194 live "/repositories/new", RepoLive.New, :new
189 195 live "/repositories/import", ImportLive.New, :new
190 196 live "/imports", ImportLive.Index, :index
@@ -243,9 +249,14 @@ defmodule GitGudWeb.Router do
243 249 get "/r/:owner/:name/avatar", AvatarController, :repository
244 250
245 251 live_session :public_repo,
246 − on_mount: [{GitGudWeb.UserAuth, :mount_current_scope}] do
247 − # Public read-only browsing. Visibility is enforced inside the
248 − # LiveView itself.
252 + on_mount: [
253 + {GitGudWeb.UserAuth, :mount_current_scope},
254 + {GitGudWeb.RepoAuth, :require_read}
255 + ] do
256 + # Public read-only browsing. `RepoAuth` gates every route that
257 + # names a repository, before the LiveView mounts — it used to say
258 + # visibility was enforced inside each LiveView, and in eighteen of
259 + # nineteen of them it wasn't.
249 260 live "/", PageLive.Home, :index
250 261 live "/repositories", RepoLive.Index, :index
251 262 # Page 2 onwards. Declared after /repositories/new and /import,
modified test/git_gud_web/live/repo_live/merge_test.exs
+19 −3
@@ -162,12 +162,28 @@ defmodule GitGudWeb.RepoLive.MergeTest do
162 162 refute html =~ "still to resolve"
163 163 end
164 164
165 − test "someone who can't write to the repo can't open it", ctx do
165 + test "a stranger can't open the merge page of a private repository", ctx do
166 166 other = user_fixture()
167 167 conn = log_in_user(build_conn(), other)
168 168
169 − assert {:error, {:redirect, %{to: to, flash: flash}}} = live(conn, ctx.path)
170 − assert to == "/r/#{Storage.repo_handle(ctx.repo)}/#{ctx.repo.name}"
169 + # Repositories default to private, so this one is. It used to
170 + # redirect with "you don't have permission to merge", which is a
171 + # sentence that confirms the repository exists — `RepoAuth` now
172 + # refuses the mount first, identically to a repository that isn't
173 + # there.
174 + assert_raise Ecto.NoResultsError, fn -> live(conn, ctx.path) end
175 + end
176 +
177 + test "a stranger who can read but not write is told why", %{repo: repo, path: path} do
178 + {:ok, _} = GitGud.Repositories.update_repository(repo, %{"visibility" => "public"})
179 + conn = log_in_user(build_conn(), user_fixture())
180 +
181 + # Readable, so the read gate lets them through and the merge page's
182 + # own write check is what answers — which is the behaviour the
183 + # previous version of this test was actually about, on a repository
184 + # where saying so leaks nothing.
185 + assert {:error, {:redirect, %{to: to, flash: flash}}} = live(conn, path)
186 + assert to == "/r/#{Storage.repo_handle(repo)}/#{repo.name}"
171 187 assert flash["error"] =~ "permission"
172 188 end
173 189
added test/git_gud_web/private_repo_access_test.exs
+180 −0
@@ -0,0 +1,180 @@
1 +defmodule GitGudWeb.PrivateRepoAccessTest do
2 + @moduledoc """
3 + A private repository must not be readable by someone who can't read
4 + it — through any page, not just the ones anybody remembered.
5 +
6 + This exists because it wasn't true. The router said "visibility is
7 + enforced inside the LiveView itself" and eighteen of the nineteen
8 + browsing LiveViews fetched the repository and rendered it, so a
9 + private repository's file tree, blobs, commits, issues, pull
10 + requests, wiki and CI were readable by anyone who typed the URL. The
11 + listings hid them correctly, which is what made it survive.
12 +
13 + So the test is deliberately written against the *route table* rather
14 + than against a hand-picked list of pages: every repository-scoped
15 + path is asserted, and a new page that forgets the gate fails here.
16 + """
17 + use GitGudWeb.ConnCase, async: false
18 +
19 + import GitGud.AccountsFixtures
20 + import GitGud.ForgeFixtures
21 +
22 + alias GitGud.Repositories
23 +
24 + setup do
25 + owner = user_fixture()
26 +
27 + {:ok, private} =
28 + Repositories.create_repository(owner, %{
29 + "name" => unique_repo_name(),
30 + "visibility" => "private"
31 + })
32 +
33 + {:ok, public} =
34 + Repositories.create_repository(owner, %{
35 + "name" => unique_repo_name(),
36 + "visibility" => "public"
37 + })
38 +
39 + {:ok, internal} =
40 + Repositories.create_repository(owner, %{
41 + "name" => unique_repo_name(),
42 + "visibility" => "internal"
43 + })
44 +
45 + %{owner: owner, private: private, public: public, internal: internal}
46 + end
47 +
48 + # Every read-only page a repository has. Paths, not LiveView modules,
49 + # because what leaked was reachable URLs.
50 + defp browsing_paths(handle, name) do
51 + [
52 + "/r/#{handle}/#{name}",
53 + "/r/#{handle}/#{name}/tree/master/",
54 + "/r/#{handle}/#{name}/blob/master/README.md",
55 + "/r/#{handle}/#{name}/commits/master",
56 + "/r/#{handle}/#{name}/issues",
57 + "/r/#{handle}/#{name}/pulls",
58 + "/r/#{handle}/#{name}/wiki",
59 + "/r/#{handle}/#{name}/actions",
60 + "/r/#{handle}/#{name}/forks",
61 + "/r/#{handle}/#{name}/compare",
62 + "/r/#{handle}/#{name}/packages",
63 + "/r/#{handle}/#{name}/stats/languages",
64 + "/r/#{handle}/#{name}/stats/committers"
65 + ]
66 + end
67 +
68 + describe "an anonymous visitor" do
69 + test "gets a 404 from every page of a private repository", %{
70 + conn: conn,
71 + owner: owner,
72 + private: private
73 + } do
74 + for path <- browsing_paths(owner.handle, private.name) do
75 + assert_404(conn, path)
76 + end
77 + end
78 +
79 + test "can still read a public repository", %{conn: conn, owner: owner, public: public} do
80 + # The gate has to be a gate, not a wall — this is the assertion
81 + # that would catch closing it too far.
82 + assert conn |> get("/r/#{owner.handle}/#{public.name}") |> html_response(200)
83 + end
84 + end
85 +
86 + describe "a signed-in stranger" do
87 + test "gets a 404 from every page of a private repository", %{
88 + conn: conn,
89 + owner: owner,
90 + private: private
91 + } do
92 + conn = log_in_user(conn, user_fixture())
93 +
94 + # Being signed in says nothing about this repository, and a 403
95 + # here would confirm it exists.
96 + for path <- browsing_paths(owner.handle, private.name) do
97 + assert_404(conn, path)
98 + end
99 + end
100 + end
101 +
102 + describe "the owner" do
103 + test "can read their own private repository", %{conn: conn, owner: owner, private: private} do
104 + assert conn
105 + |> log_in_user(owner)
106 + |> get("/r/#{owner.handle}/#{private.name}")
107 + |> html_response(200)
108 + end
109 + end
110 +
111 + describe "indistinguishability" do
112 + test "a private repository answers exactly like one that isn't there", %{
113 + conn: conn,
114 + owner: owner,
115 + private: private
116 + } do
117 + missing = catch_status(conn, "/r/#{owner.handle}/definitely-not-a-repo")
118 + hidden = catch_status(conn, "/r/#{owner.handle}/#{private.name}")
119 +
120 + # Same status, so probing the URL space tells an attacker nothing
121 + # about which private repositories exist.
122 + assert missing == hidden
123 + assert hidden == 404
124 + end
125 + end
126 +
127 + describe "internal repositories" do
128 + test "are not readable by an anonymous visitor", %{
129 + conn: conn,
130 + owner: owner,
131 + internal: internal
132 + } do
133 + # The settings page says "Internal — any signed-in user can read".
134 + # Nobody is not a signed-in user. This used to be readable, while
135 + # every listing correctly hid it, so an internal repository was
136 + # invisible and world-readable at the same time.
137 + for path <- browsing_paths(owner.handle, internal.name) do
138 + assert_404(conn, path)
139 + end
140 + end
141 +
142 + test "are readable by any signed-in user", %{conn: conn, owner: owner, internal: internal} do
143 + # The other half of the promise: internal is not private, and
144 + # closing it to everyone would be a different bug.
145 + assert conn
146 + |> log_in_user(user_fixture())
147 + |> get("/r/#{owner.handle}/#{internal.name}")
148 + |> html_response(200)
149 + end
150 +
151 + test "are not listed to an anonymous visitor on a profile", %{
152 + conn: conn,
153 + owner: owner,
154 + internal: internal,
155 + public: public
156 + } do
157 + html = conn |> get("/u/#{owner.handle}") |> html_response(200)
158 +
159 + assert html =~ public.name
160 + refute html =~ internal.name
161 + end
162 + end
163 +
164 + defp assert_404(conn, path) do
165 + assert catch_status(conn, path) == 404, "#{path} did not 404"
166 + end
167 +
168 + # A LiveView that refuses to mount raises; Phoenix maps
169 + # Ecto.NoResultsError to a 404. Either shape counts as "not served".
170 + defp catch_status(conn, path) do
171 + try do
172 + get(conn, path).status
173 + rescue
174 + Ecto.NoResultsError -> 404
175 + Phoenix.Router.NoRouteError -> 404
176 + catch
177 + :exit, {{%Ecto.NoResultsError{}, _}, _} -> 404
178 + end
179 + end
180 +end

Parents: b52a354