Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(audits/server): Test that null is allowed for body parameters #28

Merged
merged 3 commits into from
Nov 28, 2022
Merged

feat(audits/server): Test that null is allowed for body parameters #28

merged 3 commits into from
Nov 28, 2022

Conversation

glasser
Copy link
Collaborator

@glasser glasser commented Nov 28, 2022

As per spec:

Note: Specifying null in JSON (or equivalent values in other
formats) as values for optional request parameters is equivalent to
not specifying them at all.

Fixes #26.

@glasser
feat(audits/server): Test that null is allowed for body parameters
665c696 
As per spec:

> Note: Specifying `null` in JSON (or equivalent values in other
> formats) as values for optional request parameters is equivalent to
> not specifying them at all.

Fixes #26.
@glasser glasser mentioned this pull request Nov 28, 2022
Closed
@glasser
Copy link
Collaborator Author

glasser commented Nov 28, 2022

(reopened from #27 as a non-forked PR so the workflows work better)

enisdenjo
enisdenjo approved these changes Nov 28, 2022
View reviewed changes
Copy link
Member

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@enisdenjo enisdenjo merged commit 2dee4ff into master Nov 28, 2022
@enisdenjo enisdenjo deleted the glasser/null-body-parameters branch November 28, 2022 18:23
enisdenjo pushed a commit that referenced this pull request Nov 28, 2022
@semantic-release-bot
chore(release): 🎉 1.9.0 [skip ci]
5d7f0e7 
# [1.9.0](v1.8.0...v1.9.0) (2022-11-28)

### Bug Fixes

* **audits/server:** `url` option can also just be a Promise ([8844aea](8844aea))

### Features

* **audits/server:** Test that `null` is allowed for body parameters ([#28](#28)) ([2dee4ff](2dee4ff))
@enisdenjo
Copy link
Member

🎉 This PR is included in version 1.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@enisdenjo enisdenjo added the released Has been released and published label Nov 28, 2022
@glasser
Copy link
Collaborator Author

glasser commented Nov 28, 2022

@mcollina FYI this removes mercurius from the passing list because it chokes on extensions: null; pretty sure that should be a quick fix for you?

(Technically it should remove apollo-server too because the latest version doesn't pass even if the slightly old version being tested does, but I'll fix that today.)

glasser added a commit to apollographql/apollo-server that referenced this pull request Nov 28, 2022
@glasser
Fix v4.2.0 regression in variables: null (et al)
96d58a1 
In v4.2.0 (#7171) we changed POST handling to be stricter if
`operationName`, `variables`, or `extensions` were provided with a
surprising data type. This was intended to pass more of the optional
recommendations of the GraphQL Over HTTP spec as tested by the
graphql-http audit suite. However, we were overzealous and also banned
providing these parameters as an explicit `null`, which is documented by
the spec as legitimate. (And some clients, such as FIXME, actually send
`variables: null` in practice.)

We added explicit tests for this to the `graphql-http` test suite
(graphql/graphql-http#28) and this commit allows
these `null`s again.

Fixes #7200.
glasser added a commit to apollographql/apollo-server that referenced this pull request Nov 28, 2022
@glasser
Fix v4.2.0 regression in variables: null (et al)
8f36279 
In v4.2.0 (#7171) we changed POST handling to be stricter if
`operationName`, `variables`, or `extensions` were provided with a
surprising data type. This was intended to pass more of the optional
recommendations of the GraphQL Over HTTP spec as tested by the
graphql-http audit suite. However, we were overzealous and also banned
providing these parameters as an explicit `null`, which is documented by
the spec as legitimate. (And some clients, such as FIXME, actually send
`variables: null` in practice.)

We added explicit tests for this to the `graphql-http` test suite
(graphql/graphql-http#28) and this commit allows
these `null`s again.

Fixes #7200.
@glasser glasser mentioned this pull request Nov 28, 2022
Merged
glasser added a commit to apollographql/apollo-server that referenced this pull request Nov 28, 2022
@glasser
Fix v4.2.0 regression in variables: null (et al) (#7203)
2042ee7 
In v4.2.0 (#7171) we changed POST handling to be stricter if
`operationName`, `variables`, or `extensions` were provided with a
surprising data type. This was intended to pass more of the optional
recommendations of the GraphQL Over HTTP spec as tested by the
graphql-http audit suite. However, we were overzealous and also banned
providing these parameters as an explicit `null`, which is documented by
the spec as legitimate. (And some clients, such as FIXME, actually send
`variables: null` in practice.)

We added explicit tests for this to the `graphql-http` test suite
(graphql/graphql-http#28) and this commit allows
these `null`s again.

Fixes #7200.
@benjie
Copy link
Member

benjie commented Nov 28, 2022

Are we certain that this is a requirement we want in the spec? I'm definitely open to removing it if it's causing compatibility issues, I'm not even sure that {query: "{__typename}", extensions: null} should constitute a valid request...

@glasser
Copy link
Collaborator Author

glasser commented Nov 28, 2022

FWIW it does appear that some clients like https://github.com/graphql-rust/graphql-client send variables: null (and operationName: null seems quite common) so it seems like a good requirement. I guess extensions is sketchier but seems reasonable by analogy (and extensions being spec-defined is pretty new anyway...)

@benjie
Copy link
Member

benjie commented Dec 1, 2022

Ah yes, we definitely want existing clients to continue to work, thanks for the reference @glasser. In which case, this rule should increase interoperability and we'll maintain it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enisdenjo enisdenjo enisdenjo approved these changes

Assignees
No one assigned
Labels
released Has been released and published
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Should have audits for accepting nulls
3 participants
@glasser @enisdenjo @benjie