tenseleyflow/shithub / 4b9dfc2

Browse files

notifications: existence + visibility check on thread subscribe (SR2 H7+L4+L5+L7)

H7: threadAction now loads the issue by id, asserts kind matches
the URL kind (issue vs pr), then runs policy.IsVisibleTo on the
parent repo before upserting notification_threads. Pre-fix any
logged-in user could pollute the table with rows for private or
non-existent issues.

L4: dropped the raw `r.Header.Get("Referer")` redirect; route
through notificationReturnPath which already validates the
return_to form value via safeNotificationReturnPath.

L5: kind check moved before VerifyUnsubscribe so unknown kinds
fast-fail without a constant-time HMAC compare.

L7: 5 silent `_ = h.d.Render.RenderPage(...)` callsites in
orgs/orgs.go and orgs/teams.go now log on render error so a
broken template surfaces instead of returning a blank 200.
Authored by espadonne
SHA
4b9dfc2af4405aba144b9d56bc8ffa571e2271fe
Parents
38697c4
Tree
b859cd5

3 changed files

StatusFile+-
M internal/web/handlers/notifications/notifications.go 50 10
M internal/web/handlers/orgs/orgs.go 12 6
M internal/web/handlers/orgs/teams.go 8 4
internal/web/handlers/notifications/notifications.gomodified
@@ -23,8 +23,11 @@ import (
2323
 	"github.com/go-chi/chi/v5"
2424
 	"github.com/jackc/pgx/v5/pgxpool"
2525
26
+	"github.com/tenseleyFlow/shithub/internal/auth/policy"
27
+	issuesdb "github.com/tenseleyFlow/shithub/internal/issues/sqlc"
2628
 	"github.com/tenseleyFlow/shithub/internal/notif"
2729
 	notifdb "github.com/tenseleyFlow/shithub/internal/notif/sqlc"
30
+	reposdb "github.com/tenseleyFlow/shithub/internal/repos/sqlc"
2831
 	"github.com/tenseleyFlow/shithub/internal/web/middleware"
2932
 	"github.com/tenseleyFlow/shithub/internal/web/render"
3033
 )
@@ -190,6 +193,17 @@ func (h *Handlers) unsubscribe(w http.ResponseWriter, r *http.Request) {
190193
 // inserts (or updates) a row with subscribed=true; `false` flips it
191194
 // off. The fan-out worker honors the explicit row over the auto-sub
192195
 // derivation.
196
+//
197
+// SR2 H7: pre-fix this handler accepted any (kind, id) pair without
198
+// checking that the thread existed or that the viewer could read
199
+// the parent repo, letting any logged-in user pollute the thread
200
+// table with rows pointing at private/non-existent issues. Now we
201
+// load the issue, confirm the kind matches, and run the policy
202
+// visibility gate before upserting.
203
+//
204
+// SR2 L4: Referer is origin-checked before redirecting (was open-
205
+// redirect-shaped on Referer manipulation if the cookie surface
206
+// ever relaxed SameSite).
193207
 func (h *Handlers) threadAction(w http.ResponseWriter, r *http.Request, subscribed bool) {
194208
 	viewer := middleware.CurrentUserFromContext(r.Context())
195209
 	if viewer.IsAnonymous() {
@@ -202,10 +216,40 @@ func (h *Handlers) threadAction(w http.ResponseWriter, r *http.Request, subscrib
202216
 		return
203217
 	}
204218
 	id, err := strconv.ParseInt(chi.URLParam(r, "id"), 10, 64)
205
-	if err != nil {
219
+	if err != nil || id <= 0 {
206220
 		h.d.Render.HTTPError(w, r, http.StatusBadRequest, "")
207221
 		return
208222
 	}
223
+
224
+	// Existence + kind match. Issues and PRs share the issues table;
225
+	// the kind column distinguishes. Mismatched kind (e.g. /pr/<issue-id>)
226
+	// is treated as not-found, same as a non-existent id.
227
+	issue, err := issuesdb.New().GetIssueByID(r.Context(), h.d.Pool, id)
228
+	if err != nil {
229
+		h.d.Render.HTTPError(w, r, http.StatusNotFound, "")
230
+		return
231
+	}
232
+	wantKind := issuesdb.IssueKindIssue
233
+	if kindStr == "pr" {
234
+		wantKind = issuesdb.IssueKindPr
235
+	}
236
+	if issue.Kind != wantKind {
237
+		h.d.Render.HTTPError(w, r, http.StatusNotFound, "")
238
+		return
239
+	}
240
+
241
+	// Visibility — viewer must be able to read the parent repo.
242
+	repo, err := reposdb.New().GetRepoByID(r.Context(), h.d.Pool, issue.RepoID)
243
+	if err != nil {
244
+		h.d.Render.HTTPError(w, r, http.StatusNotFound, "")
245
+		return
246
+	}
247
+	pdeps := policy.Deps{Pool: h.d.Pool}
248
+	if !policy.IsVisibleTo(r.Context(), pdeps, viewer.PolicyActor(), policy.NewRepoRefFromRepo(repo)) {
249
+		h.d.Render.HTTPError(w, r, http.StatusNotFound, "")
250
+		return
251
+	}
252
+
209253
 	reason := "manual"
210254
 	if !subscribed {
211255
 		reason = "manual_unsubscribe"
@@ -220,13 +264,7 @@ func (h *Handlers) threadAction(w http.ResponseWriter, r *http.Request, subscrib
220264
 		h.d.Logger.WarnContext(r.Context(), "notifications: thread action",
221265
 			"kind", kindStr, "id", id, "subscribed", subscribed, "error", err)
222266
 	}
223
-	// Bounce back to the thread the viewer was on. Best-effort: when
224
-	// the Referer is missing, fall back to /notifications.
225
-	dest := r.Header.Get("Referer")
226
-	if dest == "" {
227
-		dest = "/notifications"
228
-	}
229
-	http.Redirect(w, r, dest, http.StatusSeeOther)
267
+	http.Redirect(w, r, notificationReturnPath(r), http.StatusSeeOther)
230268
 }
231269
232270
 // unsubscribeViaToken handles the email's one-click List-Unsubscribe
@@ -246,11 +284,13 @@ func (h *Handlers) unsubscribeViaToken(w http.ResponseWriter, r *http.Request) {
246284
 		h.d.Render.HTTPError(w, r, http.StatusBadRequest, "")
247285
 		return
248286
 	}
249
-	if !notif.VerifyUnsubscribe(h.d.UnsubscribeKey, uid, tk, tid, sig) {
287
+	// SR2 L5: kind check before HMAC compare so an unknown kind
288
+	// fast-fails without burning a constant-time compare.
289
+	if tk != "issue" && tk != "pr" {
250290
 		h.d.Render.HTTPError(w, r, http.StatusBadRequest, "")
251291
 		return
252292
 	}
253
-	if tk != "issue" && tk != "pr" {
293
+	if !notif.VerifyUnsubscribe(h.d.UnsubscribeKey, uid, tk, tid, sig) {
254294
 		h.d.Render.HTTPError(w, r, http.StatusBadRequest, "")
255295
 		return
256296
 	}
internal/web/handlers/orgs/orgs.gomodified
@@ -162,12 +162,14 @@ func (h *Handlers) createSubmit(w http.ResponseWriter, r *http.Request) {
162162
 }
163163
164164
 func (h *Handlers) renderNewForm(w http.ResponseWriter, r *http.Request, slug, errMsg string) {
165
-	_ = h.d.Render.RenderPage(w, r, "orgs/new", map[string]any{
165
+	if err := h.d.Render.RenderPage(w, r, "orgs/new", map[string]any{
166166
 		"Title":     "New organization",
167167
 		"CSRFToken": middleware.CSRFTokenForRequest(r),
168168
 		"Slug":      slug,
169169
 		"Error":     errMsg,
170
-	})
170
+	}); err != nil {
171
+		h.d.Logger.ErrorContext(r.Context(), "orgs: render", "tpl", "orgs/new", "error", err)
172
+	}
171173
 }
172174
173175
 // ─── people ────────────────────────────────────────────────────────
@@ -193,14 +195,16 @@ func (h *Handlers) peoplePage(w http.ResponseWriter, r *http.Request) {
193195
 			pending, _ = q.ListPendingInvitationsForOrg(r.Context(), h.d.Pool, org.ID)
194196
 		}
195197
 	}
196
-	_ = h.d.Render.RenderPage(w, r, "orgs/people", map[string]any{
198
+	if err := h.d.Render.RenderPage(w, r, "orgs/people", map[string]any{
197199
 		"Title":     org.Slug + " · people",
198200
 		"CSRFToken": middleware.CSRFTokenForRequest(r),
199201
 		"Org":       org,
200202
 		"Members":   members,
201203
 		"Pending":   pending,
202204
 		"IsOwner":   isOwner,
203
-	})
205
+	}); err != nil {
206
+		h.d.Logger.ErrorContext(r.Context(), "orgs: render", "tpl", "orgs/people", "error", err)
207
+	}
204208
 }
205209
206210
 func (h *Handlers) invite(w http.ResponseWriter, r *http.Request) {
@@ -318,13 +322,15 @@ func (h *Handlers) invitationView(w http.ResponseWriter, r *http.Request) {
318322
 		h.d.Render.HTTPError(w, r, http.StatusNotFound, "")
319323
 		return
320324
 	}
321
-	_ = h.d.Render.RenderPage(w, r, "orgs/invitation", map[string]any{
325
+	if err := h.d.Render.RenderPage(w, r, "orgs/invitation", map[string]any{
322326
 		"Title":      "Organization invitation",
323327
 		"CSRFToken":  middleware.CSRFTokenForRequest(r),
324328
 		"Org":        org,
325329
 		"Invitation": inv,
326330
 		"Token":      tok,
327
-	})
331
+	}); err != nil {
332
+		h.d.Logger.ErrorContext(r.Context(), "orgs: render", "tpl", "orgs/invitation", "error", err)
333
+	}
328334
 }
329335
330336
 func (h *Handlers) invitationAccept(w http.ResponseWriter, r *http.Request) {
internal/web/handlers/orgs/teams.gomodified
@@ -46,13 +46,15 @@ func (h *Handlers) teamsList(w http.ResponseWriter, r *http.Request) {
4646
 	if !viewer.IsAnonymous() {
4747
 		isOwner, _ = orgs.IsOwner(r.Context(), h.deps(), org.ID, viewer.ID)
4848
 	}
49
-	_ = h.d.Render.RenderPage(w, r, "orgs/teams_list", map[string]any{
49
+	if err := h.d.Render.RenderPage(w, r, "orgs/teams_list", map[string]any{
5050
 		"Title":     org.Slug + " · teams",
5151
 		"CSRFToken": middleware.CSRFTokenForRequest(r),
5252
 		"Org":       org,
5353
 		"Teams":     visible,
5454
 		"IsOwner":   isOwner,
55
-	})
55
+	}); err != nil {
56
+		h.d.Logger.ErrorContext(r.Context(), "orgs: render", "tpl", "orgs/teams_list", "error", err)
57
+	}
5658
 }
5759
5860
 // teamCreate handles POST /{org}/teams. Owner-only.
@@ -109,7 +111,7 @@ func (h *Handlers) teamView(w http.ResponseWriter, r *http.Request) {
109111
 	if !viewer.IsAnonymous() {
110112
 		isOwner, _ = orgs.IsOwner(r.Context(), h.deps(), org.ID, viewer.ID)
111113
 	}
112
-	_ = h.d.Render.RenderPage(w, r, "orgs/team_view", map[string]any{
114
+	if err := h.d.Render.RenderPage(w, r, "orgs/team_view", map[string]any{
113115
 		"Title":     string(org.Slug) + "/" + string(team.Slug),
114116
 		"CSRFToken": middleware.CSRFTokenForRequest(r),
115117
 		"Org":       org,
@@ -117,7 +119,9 @@ func (h *Handlers) teamView(w http.ResponseWriter, r *http.Request) {
117119
 		"Members":   members,
118120
 		"Repos":     repos,
119121
 		"IsOwner":   isOwner,
120
-	})
122
+	}); err != nil {
123
+		h.d.Logger.ErrorContext(r.Context(), "orgs: render", "tpl", "orgs/team_view", "error", err)
124
+	}
121125
 }
122126
123127
 // teamMemberAddRemove handles POST .../members. Form action=add|remove.