diff --git a/apps/backend/internal/gitlab/client_helpers.go b/apps/backend/internal/gitlab/client_helpers.go index 1df821501d..d0935223f0 100644 --- a/apps/backend/internal/gitlab/client_helpers.go +++ b/apps/backend/internal/gitlab/client_helpers.go @@ -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= +// 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 diff --git a/apps/backend/internal/gitlab/client_helpers_test.go b/apps/backend/internal/gitlab/client_helpers_test.go index f01b4dbe60..e08e0fa893 100644 --- a/apps/backend/internal/gitlab/client_helpers_test.go +++ b/apps/backend/internal/gitlab/client_helpers_test.go @@ -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) + } +} diff --git a/apps/backend/internal/gitlab/controller.go b/apps/backend/internal/gitlab/controller.go index 8a85d8e684..cb4da8f6a1 100644 --- a/apps/backend/internal/gitlab/controller.go +++ b/apps/backend/internal/gitlab/controller.go @@ -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) { diff --git a/apps/backend/internal/gitlab/controller_test.go b/apps/backend/internal/gitlab/controller_test.go new file mode 100644 index 0000000000..d994203e53 --- /dev/null +++ b/apps/backend/internal/gitlab/controller_test.go @@ -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) + } + }) + } +} diff --git a/apps/backend/internal/gitlab/pat_client_test.go b/apps/backend/internal/gitlab/pat_client_test.go index 270c548769..5a145f82ca 100644 --- a/apps/backend/internal/gitlab/pat_client_test.go +++ b/apps/backend/internal/gitlab/pat_client_test.go @@ -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=&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