fix(gitlab): scope /gitlab page tabs to the authenticated user (#1068)

* fix(gitlab): scope /gitlab page tabs to the authenticated user

The Assigned/Authored/Review-requested tabs sent the bare tab token
(`assigned_to_me`, `created_by_me`, `review_requested`) as the `filter`
query param. The backend parsed `filter` as a `key=value` string via
`appendFilter`, so the token contributed nothing — combined with the
hardcoded `scope=all`, the page served the global, unscoped MR/issue
listing instead of the user's own queue.

Add `translateUserSearchFilter` to map the well-known tab tokens to the
correct GitLab params (`scope=assigned_to_me`, `scope=created_by_me`,
or `reviewer_username=<me>&scope=all` for review-requested) and call it
from the two user-search controller handlers. `custom_query` and raw
`key=value` filters keep their existing power-user passthrough.

Cover the fix with unit tests on the translator, wire-shape tests on
`SearchMRsPaged`/`ListIssuesPaged`, and a new `controller_test.go` that
drives the controller end-to-end against an `httptest` GitLab stub.

* fix(gitlab): 500 when review_requested resolves to empty GitLab username

Review follow-up: translateMRFilter previously surfaced a 500 only when
GetAuthenticatedUser returned an error. If the call succeeded but
produced an empty username (NoopClient, an unexpected GitLab response),
translateUserSearchFilter returned an empty string and the controller
silently fell through to SearchMRsPaged with the bare "review_requested"
token — appendFilter parsed it as an empty-value key and the page served
the unscoped global listing, re-introducing the original bug.

Now error out with a 500 in that case and cover it with a regression
test that asserts the upstream /merge_requests endpoint is never even
called when the username comes back blank.

* fix(gitlab): reject review_requested on issues endpoint with 400

Silently falling through to an unscoped listing when filter=review_requested
is sent to /gitlab/user/issues would re-introduce the exact bug this PR fixes
for MRs. GitLab has no reviewer-assigned concept for issues, so the controller
now returns 400 Bad Request and short-circuits before calling ListIssuesPaged.

Also extracts filterTokenReviewRequested as a package constant (goconst
enforces this for the three occurrences across controller.go and
client_helpers.go) and adds a regression test.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(gitlab): use filterTokenReviewRequested constant in translateMRFilter

Missed substitution from the previous commit: translateMRFilter still
compared against the raw "review_requested" literal instead of the new
package constant. No behaviour change.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
João Salavisa
2026-05-25 10:17:49 +01:00
committed by GitHub
parent 777dfdb15b
commit 9fabc7f5cd
5 changed files with 511 additions and 12 deletions
@@ -404,6 +404,49 @@ func buildIssueSearchQuery(filter, customQuery string) string {
return values.Encode()
}
// filterTokenReviewRequested is the /gitlab page tab value that maps to
// GitLab's reviewer_username param. Defined as a constant because it appears
// in multiple spots across the package (translator, MR controller, issues
// controller) and goconst enforces consistency for 3+ occurrences.
const filterTokenReviewRequested = "review_requested"
// userSearchScopeTokens is the set of frontend filter tokens that map
// directly to GitLab's `scope` query param value on /merge_requests and
// /issues. filterTokenReviewRequested is intentionally absent — GitLab has
// no scope=review_requested; that case routes through reviewer_username=<me>
// instead and is handled in translateUserSearchFilter.
var userSearchScopeTokens = map[string]bool{
"assigned_to_me": true,
"created_by_me": true,
}
// translateUserSearchFilter converts the /gitlab page's filter-tab tokens
// into a GitLab API filter string that buildMRSearchQuery /
// buildIssueSearchQuery can splice in via appendFilter. Returns "" when the
// caller should pass the raw filter through unchanged (already in key=value
// form, unknown token, or empty input). The "review_requested" branch needs
// a resolved username because GitLab has no equivalent scope value — the
// controller is responsible for looking the username up and surfacing any
// error before calling here.
func translateUserSearchFilter(token, username string) string {
if token == "" {
return ""
}
if strings.ContainsAny(token, "=&") {
return ""
}
if userSearchScopeTokens[token] {
return "scope=" + token
}
if token == filterTokenReviewRequested {
if username == "" {
return ""
}
return "reviewer_username=" + url.QueryEscape(username) + "&scope=all"
}
return ""
}
// appendFilter parses a `key=value&key2=value2` filter and merges it into
// values. User-supplied keys override defaults set by the caller (so passing
// "state=closed" actually swaps the default "opened" rather than appending
@@ -46,3 +46,55 @@ func TestAppendFilter_UnparseableIgnored(t *testing.T) {
t.Errorf("state = %q, want opened (unparseable filter must not nuke defaults)", got)
}
}
// translateUserSearchFilter is the seam that turns the /gitlab page's tab
// values (assigned_to_me / created_by_me / review_requested) into proper
// GitLab API filter strings. Before this helper existed, the controller
// passed the bare tab tokens straight to appendFilter, which parsed them as
// keys with empty values — the filter contributed nothing and the page
// served the global, unscoped listing.
func TestTranslateUserSearchFilter(t *testing.T) {
cases := []struct {
name string
token string
username string
want string
}{
{"assigned_to_me", "assigned_to_me", "", "scope=assigned_to_me"},
{"created_by_me", "created_by_me", "", "scope=created_by_me"},
{"review_requested_with_user", "review_requested", "alice", "reviewer_username=alice&scope=all"},
// Empty username means the controller failed to resolve /user; the
// translator must refuse so the caller can surface the error instead
// of silently falling back to the global listing.
{"review_requested_without_user", "review_requested", "", ""},
// Already in key=value form — power-user passthrough must not be
// rewritten. Verified end-to-end by TestAppendFilter_*.
{"raw_key_value_passthrough", "labels=bug", "", ""},
{"raw_key_value_with_amp", "labels=bug&state=closed", "", ""},
// Unknown token — leave it to appendFilter to ignore.
{"unknown_token", "mystery", "", ""},
{"empty", "", "", ""},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
got := translateUserSearchFilter(tc.token, tc.username)
if got != tc.want {
t.Errorf("translateUserSearchFilter(%q, %q) = %q, want %q",
tc.token, tc.username, got, tc.want)
}
})
}
}
// Usernames with characters that need URL escaping (rare on GitLab but
// possible on self-managed instances configured to allow them) must not
// land on the wire unescaped — the resulting query string would be
// ambiguous and could either break parsing or worse, accidentally inject
// extra params.
func TestTranslateUserSearchFilter_EscapesUsername(t *testing.T) {
got := translateUserSearchFilter("review_requested", "a&b=c")
want := "reviewer_username=a%26b%3Dc&scope=all"
if got != want {
t.Errorf("got = %q, want %q (username must be URL-escaped)", got, want)
}
}
+62 -12
View File
@@ -222,16 +222,27 @@ func (c *Controller) httpSyncTaskMR(ctx *gin.Context) {
ctx.JSON(http.StatusOK, row)
}
// httpSearchUserMRs surfaces the configured user's MR queue. filter is one of
// "assigned", "authored", "review_requested" (matching the GitLab "scope"
// query param); custom_query passes through verbatim for power users.
// httpSearchUserMRs surfaces the configured user's MR queue. filter is one
// of "assigned_to_me", "created_by_me", "review_requested" (the /gitlab
// page's tab values), or a raw GitLab API filter in `key=value` form;
// custom_query passes through verbatim for power users and disables tab
// translation entirely.
func (c *Controller) httpSearchUserMRs(ctx *gin.Context) {
page, perPage := paginationFromQuery(ctx)
filter := ctx.Query("filter")
customQuery := ctx.Query("custom_query")
if customQuery == "" {
translated, err := c.translateMRFilter(ctx, filter)
if err != nil {
// translateMRFilter has already written the HTTP error.
return
}
if translated != "" {
filter = translated
}
}
result, err := c.service.Client().SearchMRsPaged(
ctx.Request.Context(),
ctx.Query("filter"),
ctx.Query("custom_query"),
page, perPage,
ctx.Request.Context(), filter, customQuery, page, perPage,
)
if err != nil {
ctx.JSON(http.StatusInternalServerError, gin.H{responseErrorKey: err.Error()})
@@ -240,14 +251,27 @@ func (c *Controller) httpSearchUserMRs(ctx *gin.Context) {
ctx.JSON(http.StatusOK, result)
}
// httpSearchUserIssues surfaces the configured user's issue queue.
// httpSearchUserIssues surfaces the configured user's issue queue. filter
// supports "assigned_to_me" and "created_by_me" (the /gitlab page's issue
// tabs). "review_requested" is explicitly rejected with 400 — GitLab has no
// reviewer-assigned concept for issues, and silently serving the global
// unscoped listing would re-introduce the bug this translator layer was
// added to prevent.
func (c *Controller) httpSearchUserIssues(ctx *gin.Context) {
page, perPage := paginationFromQuery(ctx)
filter := ctx.Query("filter")
customQuery := ctx.Query("custom_query")
if customQuery == "" {
if filter == filterTokenReviewRequested {
ctx.JSON(http.StatusBadRequest, gin.H{responseErrorKey: "review_requested is not supported for issues"})
return
}
if translated := translateUserSearchFilter(filter, ""); translated != "" {
filter = translated
}
}
result, err := c.service.Client().ListIssuesPaged(
ctx.Request.Context(),
ctx.Query("filter"),
ctx.Query("custom_query"),
page, perPage,
ctx.Request.Context(), filter, customQuery, page, perPage,
)
if err != nil {
ctx.JSON(http.StatusInternalServerError, gin.H{responseErrorKey: err.Error()})
@@ -256,6 +280,32 @@ func (c *Controller) httpSearchUserIssues(ctx *gin.Context) {
ctx.JSON(http.StatusOK, result)
}
// translateMRFilter resolves the authenticated user (only when the
// review_requested tab needs it) and runs the filter through
// translateUserSearchFilter. On a username-lookup failure — including a
// successful call that returns no username (NoopClient, an unexpected
// GitLab response) — it writes a 500 to ctx and returns an error so the
// caller can short-circuit. Silently falling back to an unfiltered listing
// would re-introduce the very bug this translation layer was added to
// prevent.
func (c *Controller) translateMRFilter(ctx *gin.Context, filter string) (string, error) {
var username string
if filter == filterTokenReviewRequested {
u, err := c.service.Client().GetAuthenticatedUser(ctx.Request.Context())
if err != nil {
ctx.JSON(http.StatusInternalServerError, gin.H{responseErrorKey: err.Error()})
return "", err
}
if u == "" {
err := errors.New("cannot resolve authenticated GitLab user")
ctx.JSON(http.StatusInternalServerError, gin.H{responseErrorKey: err.Error()})
return "", err
}
username = u
}
return translateUserSearchFilter(filter, username), nil
}
// paginationFromQuery reads ?page=&per_page= with the same clamps SearchMRsPaged
// applies internally — surfaced here so 400s on bad input are uniform.
func paginationFromQuery(ctx *gin.Context) (int, int) {
@@ -0,0 +1,279 @@
package gitlab
import (
"net/http"
"net/http/httptest"
"net/url"
"strings"
"sync"
"testing"
"github.com/gin-gonic/gin"
)
// newControllerFixture wires a real PATClient + Controller against an
// httptest.NewServer GitLab stub. The returned *requestLog captures every
// path + query the stub observed so tests can assert exactly which params
// the tab-token translator emitted on the wire.
//
// Most tests only care about the /merge_requests and /issues calls, but
// the stub also satisfies /user so the review_requested path works.
func newControllerFixture(t *testing.T, username string) (*gin.Engine, *requestLog, func()) {
t.Helper()
log := newTestLogger(t)
gin.SetMode(gin.TestMode)
rec := &requestLog{}
mux := http.NewServeMux()
mux.HandleFunc("/api/v4/user", func(w http.ResponseWriter, _ *http.Request) {
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"username":"` + username + `"}`))
})
collect := func(w http.ResponseWriter, r *http.Request) {
rec.add(r.URL.Path, r.URL.Query())
w.Header().Set("X-Total", "0")
_, _ = w.Write([]byte(`[]`))
}
mux.HandleFunc("/api/v4/merge_requests", collect)
mux.HandleFunc("/api/v4/issues", collect)
srv := httptest.NewServer(mux)
client := NewPATClient(srv.URL, "tok")
svc := NewService(srv.URL, client, AuthMethodPAT, nil, log)
ctrl := NewController(svc, log)
router := gin.New()
ctrl.RegisterHTTPRoutes(router)
return router, rec, srv.Close
}
type recordedRequest struct {
Path string
Query url.Values
}
type requestLog struct {
mu sync.Mutex
requests []recordedRequest
}
func (r *requestLog) add(path string, q url.Values) {
r.mu.Lock()
defer r.mu.Unlock()
r.requests = append(r.requests, recordedRequest{Path: path, Query: q})
}
func (r *requestLog) findByPath(path string) *recordedRequest {
r.mu.Lock()
defer r.mu.Unlock()
for i := range r.requests {
if r.requests[i].Path == path {
return &r.requests[i]
}
}
return nil
}
// hit issues a GET against the in-process router and returns the response.
func hit(router *gin.Engine, target string) *httptest.ResponseRecorder {
req := httptest.NewRequest(http.MethodGet, target, nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, req)
return w
}
// Regression for the /gitlab page tabs: each tab value must reach GitLab
// as a real scoping param. Before the translator was added the bare token
// became an empty-value key (e.g. `assigned_to_me=`) and the page served
// the global, unscoped listing.
func TestHttpSearchUserMRs_TranslatesTabFilters(t *testing.T) {
cases := []struct {
name string
filter string
wantScope string
wantExtras map[string]string
wantMissing []string
}{
{
name: "assigned_to_me",
filter: "assigned_to_me",
wantScope: "assigned_to_me",
},
{
name: "created_by_me",
filter: "created_by_me",
wantScope: "created_by_me",
},
{
name: "review_requested_resolves_username",
filter: "review_requested",
wantScope: "all",
wantExtras: map[string]string{
"reviewer_username": "alice",
},
},
{
// Power-user passthrough: a raw key=value string must still
// reach appendFilter unchanged. The default `scope=all` stays
// because the user didn't override it.
name: "raw_key_value_passthrough",
filter: "labels=bug",
wantScope: "all",
wantExtras: map[string]string{
"labels": "bug",
},
wantMissing: []string{"reviewer_username"},
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
router, rec, stop := newControllerFixture(t, "alice")
defer stop()
resp := hit(router, "/api/v1/gitlab/user/mrs?filter="+url.QueryEscape(tc.filter))
if resp.Code != http.StatusOK {
t.Fatalf("status = %d, body = %s", resp.Code, resp.Body.String())
}
req := rec.findByPath("/api/v4/merge_requests")
if req == nil {
t.Fatal("/api/v4/merge_requests was never called")
}
if got := req.Query.Get("scope"); got != tc.wantScope {
t.Errorf("scope = %q, want %q", got, tc.wantScope)
}
if got := req.Query.Get("state"); got != "opened" {
t.Errorf("state = %q, want opened (default seeded by buildMRSearchQuery)", got)
}
for k, want := range tc.wantExtras {
if got := req.Query.Get(k); got != want {
t.Errorf("query[%q] = %q, want %q", k, got, want)
}
}
for _, k := range tc.wantMissing {
if got := req.Query.Get(k); got != "" {
t.Errorf("query[%q] = %q, want absent", k, got)
}
}
// Belt-and-braces: the buggy code path used to leave the raw
// tab token as an empty-value key — verify it's gone.
if _, present := req.Query[tc.filter]; present && strings.Contains(tc.filter, "_") {
t.Errorf("query contains stray empty-value key %q (regression: bare token leaked through)", tc.filter)
}
})
}
}
// custom_query must completely bypass tab translation — the translator
// must never run when the power-user escape hatch is used.
func TestHttpSearchUserMRs_CustomQueryBypassesTranslation(t *testing.T) {
router, rec, stop := newControllerFixture(t, "alice")
defer stop()
resp := hit(router,
"/api/v1/gitlab/user/mrs?filter=assigned_to_me&custom_query="+
url.QueryEscape("state=closed&labels=bug"))
if resp.Code != http.StatusOK {
t.Fatalf("status = %d, body = %s", resp.Code, resp.Body.String())
}
req := rec.findByPath("/api/v4/merge_requests")
if req == nil {
t.Fatal("/api/v4/merge_requests was never called")
}
if got := req.Query.Get("state"); got != "closed" {
t.Errorf("state = %q, want closed (custom_query wins)", got)
}
if got := req.Query.Get("labels"); got != "bug" {
t.Errorf("labels = %q, want bug", got)
}
// Translation must not run: no scope param, no leakage of the filter.
if got := req.Query.Get("scope"); got != "" {
t.Errorf("scope = %q, want absent (custom_query bypasses defaults)", got)
}
}
// Defensive backstop: if /user resolves successfully but yields no
// username (NoopClient, or some GitLab response we didn't model), the
// controller must NOT silently fall through to the unscoped /merge_requests
// listing — that would re-expose the original bug. A 500 surfaces the
// problem so the user knows something is wrong instead of seeing a feed
// of unrelated MRs.
func TestHttpSearchUserMRs_ReviewRequestedWithoutUsername_Returns500(t *testing.T) {
rec := &requestLog{}
mux := http.NewServeMux()
// /user succeeds but returns an empty username.
mux.HandleFunc("/api/v4/user", func(w http.ResponseWriter, _ *http.Request) {
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"username":""}`))
})
mux.HandleFunc("/api/v4/merge_requests", func(w http.ResponseWriter, r *http.Request) {
rec.add(r.URL.Path, r.URL.Query())
w.Header().Set("X-Total", "0")
_, _ = w.Write([]byte(`[]`))
})
srv := httptest.NewServer(mux)
defer srv.Close()
gin.SetMode(gin.TestMode)
log := newTestLogger(t)
svc := NewService(srv.URL, NewPATClient(srv.URL, "tok"), AuthMethodPAT, nil, log)
router := gin.New()
NewController(svc, log).RegisterHTTPRoutes(router)
resp := hit(router, "/api/v1/gitlab/user/mrs?filter=review_requested")
if resp.Code != http.StatusInternalServerError {
t.Fatalf("status = %d, want 500 (body: %s)", resp.Code, resp.Body.String())
}
if req := rec.findByPath("/api/v4/merge_requests"); req != nil {
t.Errorf("/api/v4/merge_requests was called with query %v — controller should short-circuit instead", req.Query)
}
}
// review_requested must be rejected with 400 on the issues endpoint — GitLab
// has no reviewer-assigned concept for issues. Accepting it and silently
// falling through to an unscoped listing would re-introduce the same bug
// this PR fixes for MRs.
func TestHttpSearchUserIssues_ReviewRequestedReturns400(t *testing.T) {
router, rec, stop := newControllerFixture(t, "alice")
defer stop()
resp := hit(router, "/api/v1/gitlab/user/issues?filter=review_requested")
if resp.Code != http.StatusBadRequest {
t.Fatalf("status = %d, want 400 (body: %s)", resp.Code, resp.Body.String())
}
if req := rec.findByPath("/api/v4/issues"); req != nil {
t.Errorf("/api/v4/issues was called with query %v — controller should short-circuit", req.Query)
}
}
// Issues counterpart — same regression guarantee plus confirmation that
// review_requested is explicitly rejected (no equivalent GitLab API concept).
func TestHttpSearchUserIssues_TranslatesTabFilters(t *testing.T) {
cases := []struct {
name string
filter string
wantScope string
}{
{"assigned_to_me", "assigned_to_me", "assigned_to_me"},
{"created_by_me", "created_by_me", "created_by_me"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
router, rec, stop := newControllerFixture(t, "alice")
defer stop()
resp := hit(router, "/api/v1/gitlab/user/issues?filter="+url.QueryEscape(tc.filter))
if resp.Code != http.StatusOK {
t.Fatalf("status = %d, body = %s", resp.Code, resp.Body.String())
}
req := rec.findByPath("/api/v4/issues")
if req == nil {
t.Fatal("/api/v4/issues was never called")
}
if got := req.Query.Get("scope"); got != tc.wantScope {
t.Errorf("scope = %q, want %q", got, tc.wantScope)
}
if got := req.Query.Get("state"); got != "opened" {
t.Errorf("state = %q, want opened", got)
}
})
}
}
@@ -6,6 +6,7 @@ import (
"errors"
"net/http"
"net/http/httptest"
"net/url"
"strconv"
"strings"
"testing"
@@ -274,6 +275,80 @@ func TestPATClient_SearchMRsPaged_HonoursTotalHeader(t *testing.T) {
}
}
// The /gitlab page's "Assigned" tab translates through
// translateUserSearchFilter into a `scope=assigned_to_me` filter that
// SearchMRsPaged must place on the wire as-is — previously a bare
// "assigned_to_me" filter ended up as an empty-value key and the page
// served the global, unscoped listing.
func TestPATClient_SearchMRsPaged_ScopeAssignedToMe(t *testing.T) {
var receivedQuery url.Values
host, stop := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
receivedQuery = r.URL.Query()
_, _ = w.Write([]byte(`[]`))
}))
defer stop()
c := NewPATClient(host, "tok")
if _, err := c.SearchMRsPaged(context.Background(), "scope=assigned_to_me", "", 1, 25); err != nil {
t.Fatalf("err = %v", err)
}
if got := receivedQuery.Get("scope"); got != "assigned_to_me" {
t.Errorf("scope = %q, want assigned_to_me", got)
}
if got := receivedQuery.Get("state"); got != "opened" {
t.Errorf("state = %q, want opened (default seeded by buildMRSearchQuery)", got)
}
}
// The "Review requested" tab needs the resolved username because GitLab
// has no scope=review_requested — translateUserSearchFilter emits
// reviewer_username=<me>&scope=all and both keys must land on the wire.
func TestPATClient_SearchMRsPaged_ReviewerUsername(t *testing.T) {
var receivedQuery url.Values
host, stop := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
receivedQuery = r.URL.Query()
_, _ = w.Write([]byte(`[]`))
}))
defer stop()
c := NewPATClient(host, "tok")
if _, err := c.SearchMRsPaged(context.Background(), "reviewer_username=alice&scope=all", "", 1, 25); err != nil {
t.Fatalf("err = %v", err)
}
if got := receivedQuery.Get("reviewer_username"); got != "alice" {
t.Errorf("reviewer_username = %q, want alice", got)
}
if got := receivedQuery.Get("scope"); got != "all" {
t.Errorf("scope = %q, want all (user-supplied value overrides default)", got)
}
}
// Issues counterpart — same regression coverage on the /issues endpoint.
func TestPATClient_ListIssuesPaged_ScopeAssignedToMe(t *testing.T) {
var receivedPath string
var receivedQuery url.Values
host, stop := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
receivedPath = r.URL.Path
receivedQuery = r.URL.Query()
_, _ = w.Write([]byte(`[]`))
}))
defer stop()
c := NewPATClient(host, "tok")
if _, err := c.ListIssuesPaged(context.Background(), "scope=assigned_to_me", "", 1, 25); err != nil {
t.Fatalf("err = %v", err)
}
if receivedPath != "/issues" {
t.Errorf("path = %q, want /issues", receivedPath)
}
if got := receivedQuery.Get("scope"); got != "assigned_to_me" {
t.Errorf("scope = %q, want assigned_to_me", got)
}
if got := receivedQuery.Get("state"); got != "opened" {
t.Errorf("state = %q, want opened", got)
}
}
func TestPATClient_ListIssuesPaged_HonoursTotalHeader(t *testing.T) {
host, stop := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// X-Total drives pagination; the body returns a small slice so the