feat(backend): Add query guardrails to prevent potential issues (#2149)
## Summary Implements query guardrails in the backend to prevent execution of expensive or malformed queries that could impact customer environments. Part of https://github.com/grafana/oss-big-tent-squad/issues/127 ## Changes ### New guardrails added: 1. **Item ID validation** (`queryItemIdData`) - Validates that item IDs are non-empty - Validates that item IDs contain only numeric values 2. **Time range validation** (`QueryData`) - Validates that `From` timestamp is before `To` timestamp 3. **API method allowlist** (`ZabbixAPIHandler`) - Only allows Zabbix API methods defined in the frontend type `zabbixMethodName` - Blocks any write/delete/update operations not in the allowlist ### New files: - `pkg/datasource/guardrails.go` - Validation functions and error definitions - `pkg/datasource/guardrails_test.go` - Unit tests for all validation functions ### Modified files: - `pkg/datasource/datasource.go` - Added time range validation - `pkg/datasource/zabbix.go` - Added item ID validation - `pkg/datasource/resource_handler.go` - Added API method validation ## Reasoning - Allowed functions might be unnecessary as we've already prevent using those in [types.ts](https://github.com/grafana/grafana-zabbix/blob/main/src/datasource/zabbix/types.ts#L1-L23) but it's nice to be cautious. - itemid and time validation is just for sanity. - Time range validation will be necessary in the future to warn user agains running expensive queries.
This commit is contained in:
@@ -119,6 +119,9 @@ func (ds *ZabbixDatasource) QueryData(ctx context.Context, req *backend.QueryDat
|
|||||||
ds.logger.Debug("DS query", "query", q)
|
ds.logger.Debug("DS query", "query", q)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
res = backend.ErrorResponseWithErrorSource(err)
|
res = backend.ErrorResponseWithErrorSource(err)
|
||||||
|
} else if err := ValidateTimeRange(query.TimeRange); err != nil {
|
||||||
|
// Validate time range before processing any query
|
||||||
|
res = backend.ErrorResponseWithErrorSource(err)
|
||||||
} else if query.QueryType == MODE_METRICS {
|
} else if query.QueryType == MODE_METRICS {
|
||||||
frames, err := zabbixDS.queryNumericItems(ctx, &query)
|
frames, err := zabbixDS.queryNumericItems(ctx, &query)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
134
pkg/datasource/guardrails.go
Normal file
134
pkg/datasource/guardrails.go
Normal file
@@ -0,0 +1,134 @@
|
|||||||
|
package datasource
|
||||||
|
|
||||||
|
import (
|
||||||
|
"errors"
|
||||||
|
"regexp"
|
||||||
|
"strings"
|
||||||
|
"time"
|
||||||
|
|
||||||
|
"github.com/grafana/grafana-plugin-sdk-go/backend"
|
||||||
|
)
|
||||||
|
|
||||||
|
// Guardrail errors
|
||||||
|
var (
|
||||||
|
ErrEmptyItemIDs = errors.New("itemids cannot be empty for item ID query mode")
|
||||||
|
ErrInvalidItemID = errors.New("itemid must be a valid numeric value")
|
||||||
|
ErrInvalidTimeRange = errors.New("invalid time range: 'from' must be before 'to'")
|
||||||
|
ErrTimeRangeTooLarge = errors.New("time range exceeds maximum allowed duration")
|
||||||
|
ErrTooManyItems = errors.New("query would return too many items")
|
||||||
|
ErrAPIMethodNotAllowed = errors.New("API method is not allowed")
|
||||||
|
)
|
||||||
|
|
||||||
|
// Default guardrail limits
|
||||||
|
// These limits help prevent performance issues and excessive resource consumption.
|
||||||
|
// Based on Zabbix best practices:
|
||||||
|
// - Zabbix "Max period for time selector" defaults to 2 years (range: 1-10 years)
|
||||||
|
// See: https://www.zabbix.com/documentation/current/en/manual/web_interface/frontend_sections/administration/general
|
||||||
|
// - Zabbix search_limit parameter defaults to 1000 items
|
||||||
|
// See: https://www.zabbix.com/documentation/current/en/manual/api/reference/settings/object
|
||||||
|
//
|
||||||
|
// Our limits are conservative to ensure good performance:
|
||||||
|
const (
|
||||||
|
DefaultMaxTimeRangeDays = 365 // 1 year
|
||||||
|
DefaultMaxItems = 500
|
||||||
|
)
|
||||||
|
|
||||||
|
// ValidateItemIDs validates that itemids string is not empty and contains valid numeric IDs
|
||||||
|
func ValidateItemIDs(itemids string) error {
|
||||||
|
trimmed := strings.TrimSpace(itemids)
|
||||||
|
if trimmed == "" {
|
||||||
|
return backend.DownstreamError(ErrEmptyItemIDs)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Split and validate each ID
|
||||||
|
ids := strings.Split(trimmed, ",")
|
||||||
|
numericPattern := regexp.MustCompile(`^\d+$`)
|
||||||
|
hasValidID := false
|
||||||
|
|
||||||
|
for _, id := range ids {
|
||||||
|
id = strings.TrimSpace(id)
|
||||||
|
if id == "" {
|
||||||
|
continue // Skip empty entries from trailing commas
|
||||||
|
}
|
||||||
|
if !numericPattern.MatchString(id) {
|
||||||
|
return backend.DownstreamError(ErrInvalidItemID)
|
||||||
|
}
|
||||||
|
hasValidID = true
|
||||||
|
}
|
||||||
|
|
||||||
|
if !hasValidID {
|
||||||
|
return backend.DownstreamError(ErrEmptyItemIDs)
|
||||||
|
}
|
||||||
|
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// ValidateTimeRange validates that the time range is valid (from < to)
|
||||||
|
func ValidateTimeRange(timeRange backend.TimeRange) error {
|
||||||
|
if !timeRange.From.Before(timeRange.To) {
|
||||||
|
return backend.DownstreamError(ErrInvalidTimeRange)
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// ValidateTimeRangeDuration validates that the time range doesn't exceed the maximum allowed duration
|
||||||
|
func ValidateTimeRangeDuration(timeRange backend.TimeRange, maxDays int) error {
|
||||||
|
if maxDays <= 0 {
|
||||||
|
maxDays = DefaultMaxTimeRangeDays
|
||||||
|
}
|
||||||
|
|
||||||
|
duration := timeRange.To.Sub(timeRange.From)
|
||||||
|
maxDuration := time.Duration(maxDays) * 24 * time.Hour
|
||||||
|
|
||||||
|
if duration > maxDuration {
|
||||||
|
return backend.DownstreamError(ErrTimeRangeTooLarge)
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// ValidateItemCount validates that the number of items doesn't exceed the maximum allowed
|
||||||
|
func ValidateItemCount(count int, maxItems int) error {
|
||||||
|
if maxItems <= 0 {
|
||||||
|
maxItems = DefaultMaxItems
|
||||||
|
}
|
||||||
|
|
||||||
|
if count > maxItems {
|
||||||
|
return backend.DownstreamError(ErrTooManyItems)
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// AllowedAPIMethods defines the Zabbix API methods that are allowed to be called.
|
||||||
|
// This list should be kept in sync with src/datasource/zabbix/types.ts (zabbixMethodName type)
|
||||||
|
var AllowedAPIMethods = map[string]bool{
|
||||||
|
"alert.get": true,
|
||||||
|
"apiinfo.version": true,
|
||||||
|
"application.get": true,
|
||||||
|
"event.acknowledge": true,
|
||||||
|
"event.get": true,
|
||||||
|
"history.get": true,
|
||||||
|
"host.get": true,
|
||||||
|
"hostgroup.get": true,
|
||||||
|
"item.get": true,
|
||||||
|
"problem.get": true,
|
||||||
|
"proxy.get": true,
|
||||||
|
"script.execute": true,
|
||||||
|
"script.get": true,
|
||||||
|
"service.get": true,
|
||||||
|
"service.getsla": true,
|
||||||
|
"sla.get": true,
|
||||||
|
"sla.getsli": true,
|
||||||
|
"trend.get": true,
|
||||||
|
"trigger.get": true,
|
||||||
|
"user.get": true,
|
||||||
|
"usermacro.get": true,
|
||||||
|
"valuemap.get": true,
|
||||||
|
}
|
||||||
|
|
||||||
|
// ValidateAPIMethod checks if the API method is in the allowed list
|
||||||
|
func ValidateAPIMethod(method string) error {
|
||||||
|
if _, allowed := AllowedAPIMethods[method]; !allowed {
|
||||||
|
return backend.DownstreamError(ErrAPIMethodNotAllowed)
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
}
|
||||||
307
pkg/datasource/guardrails_test.go
Normal file
307
pkg/datasource/guardrails_test.go
Normal file
@@ -0,0 +1,307 @@
|
|||||||
|
package datasource
|
||||||
|
|
||||||
|
import (
|
||||||
|
"testing"
|
||||||
|
"time"
|
||||||
|
|
||||||
|
"github.com/grafana/grafana-plugin-sdk-go/backend"
|
||||||
|
"github.com/stretchr/testify/assert"
|
||||||
|
)
|
||||||
|
|
||||||
|
func TestValidateItemIDs(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
itemids string
|
||||||
|
wantErr bool
|
||||||
|
errType error
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "valid single itemid",
|
||||||
|
itemids: "12345",
|
||||||
|
wantErr: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "valid multiple itemids",
|
||||||
|
itemids: "12345,67890,11111",
|
||||||
|
wantErr: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "valid itemids with spaces",
|
||||||
|
itemids: "12345, 67890, 11111",
|
||||||
|
wantErr: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "valid itemids with trailing comma",
|
||||||
|
itemids: "12345,67890,",
|
||||||
|
wantErr: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "empty string",
|
||||||
|
itemids: "",
|
||||||
|
wantErr: true,
|
||||||
|
errType: ErrEmptyItemIDs,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "only whitespace",
|
||||||
|
itemids: " ",
|
||||||
|
wantErr: true,
|
||||||
|
errType: ErrEmptyItemIDs,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "non-numeric itemid",
|
||||||
|
itemids: "abc123",
|
||||||
|
wantErr: true,
|
||||||
|
errType: ErrInvalidItemID,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "mixed valid and invalid itemids",
|
||||||
|
itemids: "12345,abc,67890",
|
||||||
|
wantErr: true,
|
||||||
|
errType: ErrInvalidItemID,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "itemid with special characters",
|
||||||
|
itemids: "123-45",
|
||||||
|
wantErr: true,
|
||||||
|
errType: ErrInvalidItemID,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "only commas",
|
||||||
|
itemids: ",,,",
|
||||||
|
wantErr: true,
|
||||||
|
errType: ErrEmptyItemIDs,
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
err := ValidateItemIDs(tt.itemids)
|
||||||
|
if tt.wantErr {
|
||||||
|
assert.Error(t, err)
|
||||||
|
// Check that the underlying error matches
|
||||||
|
if tt.errType != nil {
|
||||||
|
assert.ErrorIs(t, err, tt.errType)
|
||||||
|
}
|
||||||
|
} else {
|
||||||
|
assert.NoError(t, err)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestValidateTimeRange(t *testing.T) {
|
||||||
|
now := time.Now()
|
||||||
|
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
timeRange backend.TimeRange
|
||||||
|
wantErr bool
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "valid time range",
|
||||||
|
timeRange: backend.TimeRange{
|
||||||
|
From: now.Add(-1 * time.Hour),
|
||||||
|
To: now,
|
||||||
|
},
|
||||||
|
wantErr: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "from equals to",
|
||||||
|
timeRange: backend.TimeRange{
|
||||||
|
From: now,
|
||||||
|
To: now,
|
||||||
|
},
|
||||||
|
wantErr: true,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "from after to",
|
||||||
|
timeRange: backend.TimeRange{
|
||||||
|
From: now,
|
||||||
|
To: now.Add(-1 * time.Hour),
|
||||||
|
},
|
||||||
|
wantErr: true,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "large valid range",
|
||||||
|
timeRange: backend.TimeRange{
|
||||||
|
From: now.Add(-365 * 24 * time.Hour),
|
||||||
|
To: now,
|
||||||
|
},
|
||||||
|
wantErr: false,
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
err := ValidateTimeRange(tt.timeRange)
|
||||||
|
if tt.wantErr {
|
||||||
|
assert.Error(t, err)
|
||||||
|
assert.ErrorIs(t, err, ErrInvalidTimeRange)
|
||||||
|
} else {
|
||||||
|
assert.NoError(t, err)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestValidateTimeRangeDuration(t *testing.T) {
|
||||||
|
now := time.Now()
|
||||||
|
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
timeRange backend.TimeRange
|
||||||
|
maxDays int
|
||||||
|
wantErr bool
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "within limit",
|
||||||
|
timeRange: backend.TimeRange{
|
||||||
|
From: now.Add(-7 * 24 * time.Hour),
|
||||||
|
To: now,
|
||||||
|
},
|
||||||
|
maxDays: 30,
|
||||||
|
wantErr: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "exactly at limit",
|
||||||
|
timeRange: backend.TimeRange{
|
||||||
|
From: now.Add(-30 * 24 * time.Hour),
|
||||||
|
To: now,
|
||||||
|
},
|
||||||
|
maxDays: 30,
|
||||||
|
wantErr: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "exceeds limit",
|
||||||
|
timeRange: backend.TimeRange{
|
||||||
|
From: now.Add(-31 * 24 * time.Hour),
|
||||||
|
To: now,
|
||||||
|
},
|
||||||
|
maxDays: 30,
|
||||||
|
wantErr: true,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "uses default limit when maxDays is 0",
|
||||||
|
timeRange: backend.TimeRange{
|
||||||
|
From: now.Add(-30 * 24 * time.Hour),
|
||||||
|
To: now,
|
||||||
|
},
|
||||||
|
maxDays: 0, // Should use DefaultMaxTimeRangeDays (365)
|
||||||
|
wantErr: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "exceeds default limit",
|
||||||
|
timeRange: backend.TimeRange{
|
||||||
|
From: now.Add(-400 * 24 * time.Hour),
|
||||||
|
To: now,
|
||||||
|
},
|
||||||
|
maxDays: 0, // Should use DefaultMaxTimeRangeDays (365)
|
||||||
|
wantErr: true,
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
err := ValidateTimeRangeDuration(tt.timeRange, tt.maxDays)
|
||||||
|
if tt.wantErr {
|
||||||
|
assert.Error(t, err)
|
||||||
|
assert.ErrorIs(t, err, ErrTimeRangeTooLarge)
|
||||||
|
} else {
|
||||||
|
assert.NoError(t, err)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestValidateItemCount(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
count int
|
||||||
|
maxItems int
|
||||||
|
wantErr bool
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "within limit",
|
||||||
|
count: 100,
|
||||||
|
maxItems: 500,
|
||||||
|
wantErr: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "exactly at limit",
|
||||||
|
count: 500,
|
||||||
|
maxItems: 500,
|
||||||
|
wantErr: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "exceeds limit",
|
||||||
|
count: 501,
|
||||||
|
maxItems: 500,
|
||||||
|
wantErr: true,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "uses default limit when maxItems is 0",
|
||||||
|
count: 100,
|
||||||
|
maxItems: 0, // Should use DefaultMaxItems (500)
|
||||||
|
wantErr: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "exceeds default limit",
|
||||||
|
count: 501,
|
||||||
|
maxItems: 0, // Should use DefaultMaxItems (500)
|
||||||
|
wantErr: true,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "zero items",
|
||||||
|
count: 0,
|
||||||
|
maxItems: 500,
|
||||||
|
wantErr: false,
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
err := ValidateItemCount(tt.count, tt.maxItems)
|
||||||
|
if tt.wantErr {
|
||||||
|
assert.Error(t, err)
|
||||||
|
assert.ErrorIs(t, err, ErrTooManyItems)
|
||||||
|
} else {
|
||||||
|
assert.NoError(t, err)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestValidateAPIMethod(t *testing.T) {
|
||||||
|
// Test a few allowed methods (matching src/datasource/zabbix/types.ts)
|
||||||
|
allowedMethods := []string{
|
||||||
|
"host.get",
|
||||||
|
"history.get",
|
||||||
|
"item.get",
|
||||||
|
"event.acknowledge",
|
||||||
|
"script.execute",
|
||||||
|
}
|
||||||
|
for _, method := range allowedMethods {
|
||||||
|
t.Run("allowed_"+method, func(t *testing.T) {
|
||||||
|
err := ValidateAPIMethod(method)
|
||||||
|
assert.NoError(t, err)
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
// Test blocked methods (not in the allowlist)
|
||||||
|
blockedMethods := []string{
|
||||||
|
"host.create",
|
||||||
|
"host.delete",
|
||||||
|
"host.update",
|
||||||
|
"user.create",
|
||||||
|
"item.delete",
|
||||||
|
"unknown.method",
|
||||||
|
"",
|
||||||
|
}
|
||||||
|
for _, method := range blockedMethods {
|
||||||
|
t.Run("blocked_"+method, func(t *testing.T) {
|
||||||
|
err := ValidateAPIMethod(method)
|
||||||
|
assert.Error(t, err)
|
||||||
|
assert.ErrorIs(t, err, ErrAPIMethodNotAllowed)
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -51,6 +51,13 @@ func (ds *ZabbixDatasource) ZabbixAPIHandler(rw http.ResponseWriter, req *http.R
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Validate API method is allowed (guardrail)
|
||||||
|
if err := ValidateAPIMethod(reqData.Method); err != nil {
|
||||||
|
ds.logger.Warn("Blocked API method", "method", reqData.Method)
|
||||||
|
writeError(rw, http.StatusForbidden, err)
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
ctx := req.Context()
|
ctx := req.Context()
|
||||||
pluginCxt := backend.PluginConfigFromContext(ctx)
|
pluginCxt := backend.PluginConfigFromContext(ctx)
|
||||||
dsInstance, err := ds.getDSInstance(ctx, pluginCxt)
|
dsInstance, err := ds.getDSInstance(ctx, pluginCxt)
|
||||||
|
|||||||
@@ -78,6 +78,11 @@ func (ds *ZabbixDatasourceInstance) queryNumericItems(ctx context.Context, query
|
|||||||
}
|
}
|
||||||
|
|
||||||
func (ds *ZabbixDatasourceInstance) queryItemIdData(ctx context.Context, query *QueryModel) ([]*data.Frame, error) {
|
func (ds *ZabbixDatasourceInstance) queryItemIdData(ctx context.Context, query *QueryModel) ([]*data.Frame, error) {
|
||||||
|
// Validate itemids before processing
|
||||||
|
if err := ValidateItemIDs(query.ItemIDs); err != nil {
|
||||||
|
return nil, err
|
||||||
|
}
|
||||||
|
|
||||||
itemids := strings.Split(query.ItemIDs, ",")
|
itemids := strings.Split(query.ItemIDs, ",")
|
||||||
for i, id := range itemids {
|
for i, id := range itemids {
|
||||||
itemids[i] = strings.Trim(id, " ")
|
itemids[i] = strings.Trim(id, " ")
|
||||||
|
|||||||
Reference in New Issue
Block a user