graphql-go / graphql-go/graphql

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

Open
#599 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
10.1k
Forks
845
PR merge metrics
No merged PRs in 30d

Description

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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.