graphql icon indicating copy to clipboard operation
graphql copied to clipboard

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

Open echlebek opened this issue 4 years ago • 1 comments

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.

echlebek avatar Feb 22 '21 22:02 echlebek

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.

bhoriuchi avatar Mar 14 '21 01:03 bhoriuchi