Skip to content
Merged
219 changes: 219 additions & 0 deletions router-tests/security_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package integration

import (
"github.com/wundergraph/cosmo/router/core"
"net/http"
"testing"

Expand Down Expand Up @@ -109,3 +110,221 @@ func TestParserHardLimits(t *testing.T) {
})
})
}

func TestQueryNamingLimits(t *testing.T) {
t.Parallel()

t.Run("verify operation query naming limits", func(t *testing.T) {
t.Parallel()

t.Run("with large query name and no operation name", func(t *testing.T) {
t.Parallel()
trimSize := 2
queryName := "longstring"

testenv.Run(t, &testenv.Config{
ModifySecurityConfiguration: func(securityConfiguration *config.SecurityConfiguration) {
securityConfiguration.OperationNameLimit = trimSize
},
}, func(t *testing.T, xEnv *testenv.Environment) {
resPost, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{
Query: "query " + queryName + " { employees { id } }",
})
require.NoError(t, err)
require.JSONEq(t, `{"errors":[{"message":"operation name too large"}]}`, resPost.Body)
require.Equal(t, http.StatusBadRequest, resPost.Response.StatusCode)

resGet, err := xEnv.MakeGraphQLRequestOverGET(testenv.GraphQLRequest{
Query: "query " + queryName + " { employees { id } }",
})
require.NoError(t, err)
require.JSONEq(t, `{"errors":[{"message":"operation name too large"}]}`, resGet.Body)
require.Equal(t, http.StatusBadRequest, resGet.Response.StatusCode)
})
})

t.Run("with large query name and small operation name", func(t *testing.T) {
t.Parallel()
trimSize := 6
queryName := "longstring"
operationNameGet := `short`
operationNamePost := `"short"`

testenv.Run(t, &testenv.Config{
ModifySecurityConfiguration: func(securityConfiguration *config.SecurityConfiguration) {
securityConfiguration.OperationNameLimit = trimSize
},
}, func(t *testing.T, xEnv *testenv.Environment) {
resPost, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{
Query: "query " + queryName + " { employees { id } }",
OperationName: []byte(operationNamePost),
})
require.NoError(t, err)
require.JSONEq(t, `{"errors":[{"message":"operation name too large"}]}`, resPost.Body)
require.Equal(t, http.StatusBadRequest, resPost.Response.StatusCode)

resGet, err := xEnv.MakeGraphQLRequestOverGET(testenv.GraphQLRequest{
Query: "query " + queryName + " { employees { id } }",
OperationName: []byte(operationNameGet),
})
require.NoError(t, err)
require.JSONEq(t, `{"errors":[{"message":"operation name too large"}]}`, resGet.Body)
require.Equal(t, http.StatusBadRequest, resGet.Response.StatusCode)
})
})

t.Run("with small query name and large operation name", func(t *testing.T) {
t.Parallel()

trimSize := 6
queryName := "short"
operationNameGet := `longname`
operationNamePost := `"longname"`

testenv.Run(t, &testenv.Config{
ModifySecurityConfiguration: func(securityConfiguration *config.SecurityConfiguration) {
securityConfiguration.OperationNameLimit = trimSize
},
}, func(t *testing.T, xEnv *testenv.Environment) {
resPost, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{
Query: "query " + queryName + " { employees { id } }",
OperationName: []byte(operationNamePost),
})
require.NoError(t, err)
require.JSONEq(t, `{"errors":[{"message":"operation name too large"}]}`, resPost.Body)
require.Equal(t, http.StatusBadRequest, resPost.Response.StatusCode)

resGet, err := xEnv.MakeGraphQLRequestOverGET(testenv.GraphQLRequest{
Query: "query " + queryName + " { employees { id } }",
OperationName: []byte(operationNameGet),
})
require.NoError(t, err)
require.JSONEq(t, `{"errors":[{"message":"operation name too large"}]}`, resGet.Body)
require.Equal(t, http.StatusBadRequest, resGet.Response.StatusCode)
})
})

t.Run("with small query name and small operation name", func(t *testing.T) {
t.Parallel()

trimSize := 7
Comment thread
SkArchon marked this conversation as resolved.
Outdated
queryName := "short"
operationNameGet := `short`
operationNamePost := `"short"`

testenv.Run(t, &testenv.Config{
ModifySecurityConfiguration: func(securityConfiguration *config.SecurityConfiguration) {
securityConfiguration.OperationNameLimit = trimSize
},
}, func(t *testing.T, xEnv *testenv.Environment) {
resPost, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{
Query: "query " + queryName + " { employees { id } }",
OperationName: []byte(operationNamePost),
})
require.NoError(t, err)
require.NotEqual(t, `{"errors":[{"message":"operation name too large"}]}`, resPost.Body)
require.JSONEq(t, `{"data":{"employees":[{"id":1},{"id":2},{"id":3},{"id":4},{"id":5},{"id":7},{"id":8},{"id":10},{"id":11},{"id":12}]}}`, resPost.Body)
require.Equal(t, http.StatusOK, resPost.Response.StatusCode)

resGet, err := xEnv.MakeGraphQLRequestOverGET(testenv.GraphQLRequest{
Query: "query " + queryName + " { employees { id } }",
OperationName: []byte(operationNameGet),
})
require.NoError(t, err)
require.NotEqual(t, `{"errors":[{"message":"operation name too large"}]}`, resGet.Body)
require.JSONEq(t, `{"data":{"employees":[{"id":1},{"id":2},{"id":3},{"id":4},{"id":5},{"id":7},{"id":8},{"id":10},{"id":11},{"id":12}]}}`, resGet.Body)
require.Equal(t, http.StatusOK, resGet.Response.StatusCode)
})
})

t.Run("with multiple queries of which one is large", func(t *testing.T) {
t.Parallel()

trimSize := 6
query1Name := "short"
query2Name := "longstring"

testenv.Run(t, &testenv.Config{
ModifySecurityConfiguration: func(securityConfiguration *config.SecurityConfiguration) {
securityConfiguration.OperationNameLimit = trimSize
},
}, func(t *testing.T, xEnv *testenv.Environment) {
resPost, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{
Query: "query " + query1Name + " { employees { id } } query " + query2Name + " { employees { id } }",
})
require.NoError(t, err)
require.JSONEq(t, `{"errors":[{"message":"operation name too large"}]}`, resPost.Body)
require.Equal(t, http.StatusBadRequest, resPost.Response.StatusCode)

resGet, err := xEnv.MakeGraphQLRequestOverGET(testenv.GraphQLRequest{
Query: "query " + query1Name + " { employees { id } } query " + query2Name + " { employees { id } }",
})
require.NoError(t, err)
require.JSONEq(t, `{"errors":[{"message":"operation name too large"}]}`, resGet.Body)
require.Equal(t, http.StatusBadRequest, resGet.Response.StatusCode)
})
})

t.Run("with multiple queries of which both are small", func(t *testing.T) {
t.Parallel()

trimSize := 6
query1Name := "short1"
query2Name := "short2"

testenv.Run(t, &testenv.Config{
ModifySecurityConfiguration: func(securityConfiguration *config.SecurityConfiguration) {
securityConfiguration.OperationNameLimit = trimSize
},
}, func(t *testing.T, xEnv *testenv.Environment) {
resPost, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{
Query: "query " + query1Name + " { employees { id } } query " + query2Name + " { employees { id } }",
})
require.NoError(t, err)
require.NotEqual(t, `{"errors":[{"message":"operation name too large"}]}`, resPost.Body)
require.JSONEq(t, `{"data":{"employees":[{"id":1},{"id":2},{"id":3},{"id":4},{"id":5},{"id":7},{"id":8},{"id":10},{"id":11},{"id":12}]}}`, resPost.Body)
require.Equal(t, http.StatusOK, resPost.Response.StatusCode)

resGet, err := xEnv.MakeGraphQLRequestOverGET(testenv.GraphQLRequest{
Query: "query " + query1Name + " { employees { id } } query " + query2Name + " { employees { id } }",
})
require.NoError(t, err)
require.NotEqual(t, `{"errors":[{"message":"operation name too large"}]}`, resGet.Body)
require.JSONEq(t, `{"data":{"employees":[{"id":1},{"id":2},{"id":3},{"id":4},{"id":5},{"id":7},{"id":8},{"id":10},{"id":11},{"id":12}]}}`, resGet.Body)
require.Equal(t, http.StatusOK, resGet.Response.StatusCode)
})
})

// In case of introspection checks, we could potentially early return
t.Run("with multiple queries with introspection disabled", func(t *testing.T) {
t.Parallel()

trimSize := 6
query1Name := "longquery"
query2Name := "short2"

testenv.Run(t, &testenv.Config{
ModifySecurityConfiguration: func(securityConfiguration *config.SecurityConfiguration) {
securityConfiguration.OperationNameLimit = trimSize
},
RouterOptions: []core.Option{
core.WithIntrospection(false),
},
}, func(t *testing.T, xEnv *testenv.Environment) {
resPost, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{
Query: "query " + query1Name + " { __schema { __typename } } query " + query2Name + " { employees { id } }",
})
require.NoError(t, err)
require.JSONEq(t, `{"errors":[{"message":"operation name too large"}]}`, resPost.Body)
require.Equal(t, http.StatusBadRequest, resPost.Response.StatusCode)

resGet, err := xEnv.MakeGraphQLRequestOverGET(testenv.GraphQLRequest{
Query: "query " + query1Name + " { employees { id } } query " + query2Name + " { employees { id } }",
})
require.NoError(t, err)
require.JSONEq(t, `{"errors":[{"message":"operation name too large"}]}`, resGet.Body)
require.Equal(t, http.StatusBadRequest, resGet.Response.StatusCode)
})
})
Comment thread
SkArchon marked this conversation as resolved.
})
}
1 change: 1 addition & 0 deletions router/core/graph_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -1207,6 +1207,7 @@ func (s *graphServer) buildGraphMux(
MaxDepth: s.Config.securityConfiguration.ParserLimits.ApproximateDepthLimit,
MaxFields: s.Config.securityConfiguration.ParserLimits.TotalFieldsLimit,
},
OperationNameLimit: s.securityConfiguration.OperationNameLimit,
ApolloCompatibilityFlags: s.apolloCompatibilityFlags,
ApolloRouterCompatibilityFlags: s.apolloRouterCompatibilityFlags,
DisableExposingVariablesContentOnValidationError: s.engineExecutionConfiguration.DisableExposingVariablesContentOnValidationError,
Expand Down
7 changes: 7 additions & 0 deletions router/core/graphql_prehandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,13 @@ func (h *PreHandler) handleOperation(req *http.Request, variablesParser *astjson
}
}

if operationKit.isOperationNameLimitExceeded(operationKit.parsedOperation.Request.OperationName) {
return &httpGraphqlError{
message: "operation name too large",
statusCode: http.StatusBadRequest,
}
}

// Compute the operation sha256 hash as soon as possible for observability reasons
if h.shouldComputeOperationSha256(operationKit) {
if err := operationKit.ComputeOperationSha256(); err != nil {
Expand Down
31 changes: 31 additions & 0 deletions router/core/operation_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ type OperationProcessorOptions struct {
DisableExposingVariablesContentOnValidationError bool
ComplexityLimits *config.ComplexityLimits
ParserTokenizerLimits astparser.TokenizerLimits
OperationNameLimit int
}

// OperationProcessor provides shared resources to the parseKit and OperationKit.
Expand All @@ -131,6 +132,7 @@ type OperationProcessor struct {
parseKitOptions *parseKitOptions
complexityLimits *config.ComplexityLimits
parserTokenizerLimits astparser.TokenizerLimits
operationNameLimit int
}

// parseKit is a helper struct to parse, normalize and validate operations
Expand Down Expand Up @@ -487,6 +489,13 @@ func (o *OperationKit) isIntrospectionQuery() (result bool, err error) {
ref := possibleOperationDefinitionRefs[i]
name := o.kit.doc.OperationDefinitionNameString(ref)

if o.isOperationNameLimitExceeded(name) {
return false, &httpGraphqlError{
message: "operation name too large",
Comment thread
SkArchon marked this conversation as resolved.
Outdated
statusCode: http.StatusBadRequest,
}
}

if o.parsedOperation.Request.OperationName == name {
operationDefinitionRef = ref
break
Expand Down Expand Up @@ -527,6 +536,13 @@ func (o *OperationKit) isIntrospectionQuery() (result bool, err error) {
return false, nil
}

func (o *OperationKit) isOperationNameLimitExceeded(operationName string) bool {
if o.operationProcessor.operationNameLimit == 0 {
return false
}
return len(operationName) > o.operationProcessor.operationNameLimit
}

Comment thread
SkArchon marked this conversation as resolved.
Outdated
// Parse parses the operation, populates the document and set the operation type.
// UnmarshalOperationFromBody must be called before calling this method.
func (o *OperationKit) Parse() error {
Expand Down Expand Up @@ -560,6 +576,11 @@ func (o *OperationKit) Parse() error {
isIntrospection, err := o.isIntrospectionQuery()

if err != nil {
var httpGqlError *httpGraphqlError
if errors.As(err, &httpGqlError) {
return httpGqlError
}

return &httpGraphqlError{
message: "could not determine if operation was an introspection query",
statusCode: http.StatusOK,
Expand All @@ -582,13 +603,22 @@ func (o *OperationKit) Parse() error {
o.kit.numOperations++
ref := o.kit.doc.RootNodes[i].Ref
name := string(o.kit.doc.OperationDefinitionNameBytes(ref))

if len(name) == 0 {
anonymousOperationCount++
if anonymousOperationDefinitionRef == -1 {
anonymousOperationDefinitionRef = ref
}
continue
}

if o.isOperationNameLimitExceeded(name) {
return &httpGraphqlError{
message: "operation name too large",
statusCode: http.StatusBadRequest,
}
}

if o.parsedOperation.Request.OperationName == "" {
o.operationDefinitionRef = ref
o.originalOperationNameRef = o.kit.doc.OperationDefinitions[ref].Name
Expand Down Expand Up @@ -1256,6 +1286,7 @@ func NewOperationProcessor(opts OperationProcessorOptions) *OperationProcessor {
parseKitSemaphore: make(chan int, opts.ParseKitPoolSize),
introspectionEnabled: opts.IntrospectionEnabled,
parserTokenizerLimits: opts.ParserTokenizerLimits,
operationNameLimit: opts.OperationNameLimit,
complexityLimits: opts.ComplexityLimits,
parseKitOptions: &parseKitOptions{
apolloCompatibilityFlags: opts.ApolloCompatibilityFlags,
Expand Down
1 change: 1 addition & 0 deletions router/pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,7 @@ type SecurityConfiguration struct {
ComplexityLimits *ComplexityLimits `yaml:"complexity_limits"`
DepthLimit *QueryDepthConfiguration `yaml:"depth_limit"`
ParserLimits ParserLimitsConfiguration `yaml:"parser_limits"`
OperationNameLimit int `yaml:"operation_name_limit" envDefault:"512"` // 0 is disabled
Comment thread
SkArchon marked this conversation as resolved.
Outdated
}

type ParserLimitsConfiguration struct {
Expand Down
6 changes: 6 additions & 0 deletions router/pkg/config/config.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -2457,6 +2457,12 @@
}
}
},
"operation_name_limit": {
"type": "integer",
"description": "The max allowed limit of the operation name length, 0 would disable the limit validation.",
Comment thread
SkArchon marked this conversation as resolved.
Outdated
"default": "512",
"minimum": 0
},
"depth_limit": {
"type": "object",
"description": "DEPRECATED (move to complexity_limits.depth): The configuration for adding a max depth limit for query (how many nested levels you can have in a query).",
Expand Down
1 change: 1 addition & 0 deletions router/pkg/config/fixtures/full.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,7 @@ security:
enabled: true
limit: 4
ignore_persisted_operations: true
operation_name_limit: 2000
persisted_operations:
safelist:
enabled: true
Expand Down
3 changes: 2 additions & 1 deletion router/pkg/config/testdata/config_defaults.json
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,8 @@
"ParserLimits": {
"ApproximateDepthLimit": 100,
"TotalFieldsLimit": 500
}
},
"OperationNameLimit": 512
},
"EngineExecutionConfiguration": {
"Debug": {
Expand Down
3 changes: 2 additions & 1 deletion router/pkg/config/testdata/config_full.json
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,8 @@
"ParserLimits": {
"ApproximateDepthLimit": 100,
"TotalFieldsLimit": 500
}
},
"OperationNameLimit": 2000
},
"EngineExecutionConfiguration": {
"Debug": {
Expand Down
Loading