graphql-go / graphql

An implementation of GraphQL for Go / Golang
MIT License
9.86k stars 838 forks source link

Exported context field in graphql.Params and others is dangerous as of Go 1.15 #599

Open echlebek opened 3 years ago

echlebek commented 3 years ago

The practice of storing a context as a struct field in graphql.Params can result in users doing things like:

ctx, cancel := context.WithCancel(p.Context)

If p.Context is nil this will cause a panic, as of Go 1.15.

It's not clear what an effective resolution might look like. It's not possible to remove the field without breaking compatibility, and it's quite unsafe to use the field directly.

Perhaps the issue could be mitigated by using methods. For example,

func (p graphql.Params) RequestContext() context.Context {
  if p.Context != nil {
    return p.Context
  }
  return context.Background()
}

If the graphql codebase replaced instances of p.Context with p.RequestContext(), and users did the same, this would at least provide a modicum of safety, assuming these methods were written for any types that feature an exported Context field. Users could use tools like semgrep to make sure they aren't using p.Context values directly.

bhoriuchi commented 3 years ago

by users do you mean developers using this package? I would think it's on the developer to check that the value is not nil.