fix: Raw filter removed from GET /api/v1/groups

README.md

@@ -154,21 +154,26 @@ curl -k -X PUT https://localhost:8080/api/v1/users \
 
 #### Search Groups
 
-A `query` (minimum 2 characters) or an explicit `filter` is **required**. Listing every
-group is not supported: it walks the entire directory and is prohibitively expensive on
-real domains. `query` matches `cn` and `sAMAccountName` as a substring, so partial input
-returns partial matches. Results are capped by `AD_MAX_SEARCH_RESULTS`.
+A `query` of at least 2 characters is **required**. Listing every group is not supported:
+it walks the entire directory and is prohibitively expensive on real domains. `query`
+matches `cn` and `sAMAccountName` as a substring, so partial input returns partial
+matches. Results are capped by `AD_MAX_SEARCH_RESULTS`.
+
+Raw LDAP filters are **not** accepted on this endpoint. `query` is escaped and wrapped in
+`AD_GROUP_FILTER` server-side, so a caller can only narrow the search, never widen it.
+Use `/api/v1/search` if you need raw-filter access.
 
 ```bash
 curl -k "https://localhost:8080/api/v1/groups?query=admins" \
   -H "X-Session-ID: your-session-id"
 
-# With optional baseDN
+# With optional baseDN — must be at or below the configured search base
 curl -k "https://localhost:8080/api/v1/groups?query=admins&baseDN=ou=Groups,dc=example,dc=com" \
   -H "X-Session-ID: your-session-id"
 ```
 
-Omitting both `query` and `filter` returns `400`.
+Returns `400` when `query` is missing or shorter than 2 characters, when a `filter`
+parameter is supplied, or when `baseDN` falls outside the configured search base.
 
 #### Resolve Groups by DN
 
@@ -200,6 +205,10 @@ curl -k -X POST https://localhost:8080/api/v1/groups/remove-member \
 ```
 
 #### Search (with custom Base DN)
+
+This endpoint accepts a raw LDAP `filter` for advanced use. A supplied `baseDN` must be
+at or below the configured search base; anything outside it returns `400`.
+
 ```bash
 # GET request with query parameters
 curl -k "https://localhost:8080/api/v1/search?baseDN=ou=Users,dc=example,dc=com&filter=(objectClass=user)&attributes=cn,mail,title&sizeLimit=100" \

handlers/handler.go

@@ -24,7 +24,9 @@
 	errMsgSessionNotFound    = "Session not found"
 	errMsgInvalidRequestBody = "Invalid request body"
 	errMsgSearchNotPermitted = "Search is not permitted for this user"
-	errMsgQueryRequired      = "A query or filter is required; listing all groups is not supported"
+	errMsgQueryRequired      = "A query is required; listing all groups is not supported"
+	errMsgFilterNotAccepted  = "Raw LDAP filters are not accepted; use the query parameter"
+	errMsgBaseDNOutOfScope   = "baseDN must be within the configured search base"
 	logKeyUserDN             = "userDN"
 
 	// minGroupQueryLen mirrors the UI's debounce threshold. Shorter queries match
@@ -273,40 +275,53 @@
 		return
 	}
 
-	// Get optional baseDN from query params
+	// An optional baseDN may narrow the search, but only within the configured base.
 	baseDN := r.URL.Query().Get("baseDN")
 	if baseDN == "" {
 		baseDN = h.config.AD.GetSearchBaseDN()
+	} else if !h.isWithinSearchBase(baseDN) {
+		writeJSON(w, http.StatusBadRequest, models.GroupsResponse{
+			Success: false,
+			Error:   errMsgBaseDNOutOfScope,
+		})
+		return
+	}
+
+	// A query is mandatory: an unfiltered listing walks every group under the base DN,
+	// which is prohibitively expensive on real directories. Raw LDAP filters are not
+	// accepted here — they would bypass the length floor and the configured group
+	// filter, letting a caller re-create the unbounded listing this endpoint refuses.
+	if raw := r.URL.Query().Get("filter"); raw != "" {
+		writeJSON(w, http.StatusBadRequest, models.GroupsResponse{
+			Success: false,
+			Error:   errMsgFilterNotAccepted,
+		})
+		return
 	}
 
-	// A query (or an explicit filter) is mandatory: an unfiltered listing walks every
-	// group under the base DN, which is prohibitively expensive on real directories.
 	query := strings.TrimSpace(r.URL.Query().Get("query"))
-	explicitFilter := r.URL.Query().Get("filter")
-	if query == "" && explicitFilter == "" {
+	if query == "" {
 		writeJSON(w, http.StatusBadRequest, models.GroupsResponse{
 			Success: false,
 			Error:   errMsgQueryRequired,
 		})
 		return
 	}
-	if query != "" && len([]rune(query)) < minGroupQueryLen {
+	if len([]rune(query)) < minGroupQueryLen {
 		writeJSON(w, http.StatusBadRequest, models.GroupsResponse{
 			Success: false,
 			Error:   fmt.Sprintf("Query must be at least %d characters", minGroupQueryLen),
 		})
 		return
 	}
 
-	// query builds a safe escaped filter; an explicit filter is used verbatim.
-	filter := explicitFilter
-	if query != "" {
-		escaped := ldap.EscapeFilter(query)
-		filter = fmt.Sprintf(
-			"(&%s(|(cn=*%s*)(sAMAccountName=*%s*)))",
-			h.config.AD.GroupFilter, escaped, escaped,
-		)
-	}
+	// Escaped and wrapped in the configured group filter, so a caller can only ever
+	// narrow the search, never widen it.
+	escaped := ldap.EscapeFilter(query)
+	filter := fmt.Sprintf(
+		"(&%s(|(cn=*%s*)(sAMAccountName=*%s*)))",
+		h.config.AD.GroupFilter, escaped, escaped,
+	)
 
 	// Search for groups
 	searchReq := ldap.NewSearchRequest(
@@ -739,9 +754,16 @@
 		sizeLimit = req.SizeLimit
 	}
 
-	// Use defaults if not provided
+	// Use defaults if not provided. A supplied baseDN may only narrow the search:
+	// the raw filter below is deliberately open, so scope stays anchored here.
 	if baseDN == "" {
 		baseDN = h.config.AD.GetSearchBaseDN()
+	} else if !h.isWithinSearchBase(baseDN) {
+		writeJSON(w, http.StatusBadRequest, models.SearchResponse{
+			Success: false,
+			Error:   errMsgBaseDNOutOfScope,
+		})
+		return
 	}
 	// query takes precedence over filter: build a safe, escaped filter server-side.
 	if query != "" {
@@ -844,6 +866,25 @@
 	return false
 }
 
+// isWithinSearchBase reports whether dn is the configured search base or sits beneath
+// it. A caller-supplied base DN may only narrow the search: without this check it could
+// point at an unrelated part of the tree and escape the scoping SearchBaseDN enforces.
+// Comparison is done on lowercased DNs: ldap.DN compares attribute values
+// case-sensitively, whereas AD treats them case-insensitively, so "DC=Example" and
+// "dc=example" must both be accepted. Parsing still happens on the real string, so
+// escaping and multi-valued RDNs are handled structurally rather than by string match.
+func (h *Handler) isWithinSearchBase(dn string) bool {
+	base, err := ldap.ParseDN(strings.ToLower(h.config.AD.GetSearchBaseDN()))
+	if err != nil {
+		return false
+	}
+	candidate, err := ldap.ParseDN(strings.ToLower(dn))
+	if err != nil {
+		return false
+	}
+	return base.Equal(candidate) || base.AncestorOf(candidate)
+}
+
 // isExcludedGroup checks whether a group CN or DN matches the excluded groups list.
 func (h *Handler) isExcludedGroup(cn, dn string) bool {
 	for _, excluded := range h.config.AD.ExcludedGroups {

handlers/handler_test.go

@@ -16,8 +16,12 @@ import (
 	"adel/session"
 )
 
-// testUsername is the session username shared by handler tests.
-const testUsername = "testuser"
+// Fixture values shared by handler tests.
+const (
+	testUsername    = "testuser"
+	testGroupFilter = "(objectClass=group)"
+	testBaseDN      = "dc=example,dc=com"
+)
 
 func TestIsUserEnabled(t *testing.T) {
 	tests := []struct {
@@ -372,7 +376,7 @@ func TestGetAllGroupsForbiddenOutsideAllowedGroups(t *testing.T) {
 func TestGetAllGroupsRejectsUnboundedListing(t *testing.T) {
 	// An allowed user must still not be able to list the whole directory: the query
 	// requirement is a cost control, independent of the search allow-list.
-	cfg := &config.Config{AD: config.ADConfig{GroupFilter: "(objectClass=group)"}}
+	cfg := &config.Config{AD: config.ADConfig{GroupFilter: testGroupFilter}}
 	h := NewHandler(cfg, nil)
 
 	tests := []struct {
@@ -408,9 +412,100 @@ func TestGetAllGroupsRejectsUnboundedListing(t *testing.T) {
 	}
 }
 
+func TestGetAllGroupsRejectsRawFilter(t *testing.T) {
+	// A raw filter would bypass both the length floor and AD_GROUP_FILTER, letting a
+	// caller re-create the unbounded listing the query requirement exists to prevent.
+	cfg := &config.Config{AD: config.ADConfig{
+		GroupFilter: testGroupFilter,
+		BaseDN:      testBaseDN,
+	}}
+	h := NewHandler(cfg, nil)
+
+	tests := []struct {
+		name string
+		url  string
+	}{
+		{"filter alone", "/api/v1/groups?filter=%28objectClass%3D%2A%29"},
+		{"filter alongside a valid query", "/api/v1/groups?query=admins&filter=%28objectClass%3D%2A%29"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			req := httptest.NewRequest(http.MethodGet, tt.url, nil)
+			sess := &session.Session{Username: testUsername}
+			ctx := context.WithValue(req.Context(), middleware.SessionContextKey, sess)
+			rr := httptest.NewRecorder()
+
+			h.GetAllGroups(rr, req.WithContext(ctx))
+
+			if rr.Code != http.StatusBadRequest {
+				t.Errorf("status = %d, want %d", rr.Code, http.StatusBadRequest)
+			}
+			var response models.GroupsResponse
+			if err := json.NewDecoder(rr.Body).Decode(&response); err != nil {
+				t.Fatalf("failed to decode response: %v", err)
+				return
+			}
+			if response.Success || response.Error == "" {
+				t.Errorf("response = %+v, want unsuccessful response with an error", response)
+			}
+		})
+	}
+}
+
+func TestIsWithinSearchBase(t *testing.T) {
+	cfg := &config.Config{AD: config.ADConfig{BaseDN: testBaseDN}}
+	h := NewHandler(cfg, nil)
+
+	tests := []struct {
+		name string
+		dn   string
+		want bool
+	}{
+		{"the base itself", "dc=example,dc=com", true},
+		{"a child OU", "ou=Groups,dc=example,dc=com", true},
+		{"a deeper descendant", "cn=Admins,ou=Groups,dc=example,dc=com", true},
+		{"differing case and spacing", "OU=Groups, DC=Example, DC=Com", true},
+		{"an unrelated tree", "dc=evil,dc=com", false},
+		{"a parent of the base", "dc=com", false},
+		{"a suffix-matching impostor", "dc=notexample,dc=com", false},
+		{"malformed input", "not-a-dn", false},
+		{"empty input", "", false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := h.isWithinSearchBase(tt.dn); got != tt.want {
+				t.Errorf("isWithinSearchBase(%q) = %v, want %v", tt.dn, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestGetAllGroupsRejectsBaseDNOutsideSearchBase(t *testing.T) {
+	// A baseDN outside the configured base would escape the scoping SearchBaseDN exists
+	// to enforce, so it is refused before the search is issued.
+	cfg := &config.Config{AD: config.ADConfig{
+		GroupFilter: testGroupFilter,
+		BaseDN:      testBaseDN,
+	}}
+	h := NewHandler(cfg, nil)
+
+	req := httptest.NewRequest(http.MethodGet, "/api/v1/groups?query=admins&baseDN=dc%3Devil%2Cdc%3Dcom", nil)
+	sess := &session.Session{Username: testUsername}
+	ctx := context.WithValue(req.Context(), middleware.SessionContextKey, sess)
+	rr := httptest.NewRecorder()
+
+	h.GetAllGroups(rr, req.WithContext(ctx))
+
+	if rr.Code != http.StatusBadRequest {
+		t.Errorf("status = %d, want %d", rr.Code, http.StatusBadRequest)
+	}
+}
+
 func TestResolveGroupsEmptyInputSkipsSearch(t *testing.T) {
 	// No valid DNs means no LDAP search, so a nil session connection must not panic.
-	cfg := &config.Config{AD: config.ADConfig{GroupFilter: "(objectClass=group)"}}
+	cfg := &config.Config{AD: config.ADConfig{GroupFilter: testGroupFilter}}
 	h := NewHandler(cfg, nil)
 
 	body := strings.NewReader(`{"dns":["","   ","not-a-valid-dn"]}`)