fix: GraphQL "Did you mean" suggestions disclose schema to unauthenticated callers (GHSA-8cph-rgr4-g5vj)#10491
Open
ulsreall wants to merge 1 commit into
Open
Conversation
… unauthenticated callers (GHSA-8cph-rgr4-g5vj) GraphQL validation rules (FieldsOnCorrectTypeRule, KnownArgumentNamesRule, KnownTypeNamesRule, ...) embed "Did you mean ...?" hints sourced from the live schema in their error messages. Those messages are returned to the caller before didResolveOperation runs, so they sidestep IntrospectionControlPlugin and disclose schema identifiers the introspection guard is meant to hide. Fix: Add SchemaSuggestionsControlPlugin that hooks into the validation phase and strips "Did you mean" suffixes from error messages for callers that are not master-key or maintenance-key authenticated, and public introspection is not enabled.
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthroughThis PR adds a new ChangesGraphQL Schema Suggestions Control
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.22.0)OpenGrep fatal error (exit code 2): [00.11][ERROR]: Error: exception Unix_error: No such file or directory stat spec/ParseGraphQLServer.spec.js Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/GraphQL/ParseGraphQLServer.js (1)
100-111: ⚡ Quick winRemove unnecessary
asynckeywords.The
asynckeywords on lines 100, 101, and 111 are not needed since these functions don't useawaitor return promises. While harmless, removing them improves clarity.♻️ Suggested refactor
- requestDidStart: async (requestContext) => ({ - validationDidStart: async () => { + requestDidStart: (requestContext) => ({ + validationDidStart: () => { if (publicIntrospection) { return; } const isMasterOrMaintenance = requestContext.contextValue.auth?.isMaster || requestContext.contextValue.auth?.isMaintenance; if (isMasterOrMaintenance) { return; } - return async (validationErrors) => { + return (validationErrors) => { validationErrors?.forEach(error => { error.message = error.message.replace(/ ?Did you mean(.+?)\?$/, ''); }); }; }, }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/GraphQL/ParseGraphQLServer.js` around lines 100 - 111, Remove the unnecessary async keywords from the Apollo plugin handlers: remove async from the requestDidStart handler definition (requestDidStart: (requestContext) => ...), from the validationDidStart handler (validationDidStart: () => ...), and from the returned validation function inside validationDidStart (the function that accepts validationErrors); these functions do not use await or return Promises so dropping async clarifies intent while preserving behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/ParseGraphQLServer.spec.js`:
- Around line 1020-1127: Add a regression test that verifies unknown type
suggestions (KnownTypeNamesRule) are stripped without auth: create a new it
block similar to the existing typo tests that uses apolloClient.query with a
GraphQL query containing a misspelled input type (e.g., query($x:
UsreWhereInput) { health }) and assert the caught error message
(e.networkError.result.errors[0].message) contains the base error ("Unknown
type" or relevant phrase) but does NOT match /Did you mean/ and does NOT contain
the real type name; reuse helpers reconfigureServer/createGQLFromParseServer
when toggling public introspection and keep test structure consistent with the
other cases that check master/maintenance keys and public introspection.
---
Nitpick comments:
In `@src/GraphQL/ParseGraphQLServer.js`:
- Around line 100-111: Remove the unnecessary async keywords from the Apollo
plugin handlers: remove async from the requestDidStart handler definition
(requestDidStart: (requestContext) => ...), from the validationDidStart handler
(validationDidStart: () => ...), and from the returned validation function
inside validationDidStart (the function that accepts validationErrors); these
functions do not use await or return Promises so dropping async clarifies intent
while preserving behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d072484-a04d-431d-b54a-57764f862594
📒 Files selected for processing (2)
spec/ParseGraphQLServer.spec.jssrc/GraphQL/ParseGraphQLServer.js
Comment on lines
+1020
to
+1127
| it('should strip "Did you mean" field suggestions from validation errors without master or maintenance key', async () => { | ||
| try { | ||
| await apolloClient.query({ | ||
| query: gql` | ||
| query Typo { | ||
| healt | ||
| } | ||
| `, | ||
| }); | ||
| fail('should have thrown a validation error'); | ||
| } catch (e) { | ||
| const message = e.networkError.result.errors[0].message; | ||
| expect(message).toContain('Cannot query field "healt"'); | ||
| expect(message).not.toMatch(/Did you mean/); | ||
| expect(message).not.toContain('health'); | ||
| } | ||
| }); | ||
|
|
||
| it('should strip "Did you mean" argument suggestions from validation errors without master or maintenance key', async () => { | ||
| try { | ||
| await apolloClient.query({ | ||
| query: gql` | ||
| query UnknownArg { | ||
| users(wher: {}) { | ||
| edges { | ||
| node { | ||
| id | ||
| } | ||
| } | ||
| } | ||
| } | ||
| `, | ||
| }); | ||
| fail('should have thrown a validation error'); | ||
| } catch (e) { | ||
| const message = e.networkError.result.errors[0].message; | ||
| expect(message).toContain('Unknown argument "wher"'); | ||
| expect(message).not.toMatch(/Did you mean/); | ||
| expect(message).not.toContain('"where"'); | ||
| } | ||
| }); | ||
|
|
||
| it('should keep "Did you mean" suggestions with master key', async () => { | ||
| try { | ||
| await apolloClient.query({ | ||
| query: gql` | ||
| query Typo { | ||
| healt | ||
| } | ||
| `, | ||
| context: { | ||
| headers: { | ||
| 'X-Parse-Master-Key': 'test', | ||
| }, | ||
| }, | ||
| }); | ||
| fail('should have thrown a validation error'); | ||
| } catch (e) { | ||
| const message = e.networkError.result.errors[0].message; | ||
| expect(message).toContain('Cannot query field "healt"'); | ||
| expect(message).toMatch(/Did you mean/); | ||
| expect(message).toContain('health'); | ||
| } | ||
| }); | ||
|
|
||
| it('should keep "Did you mean" suggestions with maintenance key', async () => { | ||
| try { | ||
| await apolloClient.query({ | ||
| query: gql` | ||
| query Typo { | ||
| healt | ||
| } | ||
| `, | ||
| context: { | ||
| headers: { | ||
| 'X-Parse-Maintenance-Key': 'test2', | ||
| }, | ||
| }, | ||
| }); | ||
| fail('should have thrown a validation error'); | ||
| } catch (e) { | ||
| const message = e.networkError.result.errors[0].message; | ||
| expect(message).toContain('Cannot query field "healt"'); | ||
| expect(message).toMatch(/Did you mean/); | ||
| expect(message).toContain('health'); | ||
| } | ||
| }); | ||
|
|
||
| it('should keep "Did you mean" suggestions when public introspection is enabled', async () => { | ||
| const parseServer = await reconfigureServer(); | ||
| await createGQLFromParseServer(parseServer, { graphQLPublicIntrospection: true }); | ||
|
|
||
| try { | ||
| await apolloClient.query({ | ||
| query: gql` | ||
| query Typo { | ||
| healt | ||
| } | ||
| `, | ||
| }); | ||
| fail('should have thrown a validation error'); | ||
| } catch (e) { | ||
| const message = e.networkError.result.errors[0].message; | ||
| expect(message).toContain('Cannot query field "healt"'); | ||
| expect(message).toMatch(/Did you mean/); | ||
| expect(message).toContain('health'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Add a regression test for unknown type suggestions too.
The security fix is described as covering KnownTypeNamesRule as well, but this block only pins field and argument validation errors. Add a no-auth typo like query($x: UsreWhereInput) { health } and assert that the response omits both Did you mean and the real type name, otherwise one of the documented disclosure paths can regress unnoticed.
💡 Suggested test shape
+ it('should strip "Did you mean" type suggestions from validation errors without master or maintenance key', async () => {
+ try {
+ await apolloClient.query({
+ query: gql`
+ query UnknownType($where: UsreWhereInput) {
+ health
+ }
+ `,
+ variables: {
+ where: {},
+ },
+ });
+ fail('should have thrown a validation error');
+ } catch (e) {
+ const message = e.networkError.result.errors[0].message;
+ expect(message).toContain('Unknown type "UsreWhereInput"');
+ expect(message).not.toMatch(/Did you mean/);
+ expect(message).not.toContain('UserWhereInput');
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should strip "Did you mean" field suggestions from validation errors without master or maintenance key', async () => { | |
| try { | |
| await apolloClient.query({ | |
| query: gql` | |
| query Typo { | |
| healt | |
| } | |
| `, | |
| }); | |
| fail('should have thrown a validation error'); | |
| } catch (e) { | |
| const message = e.networkError.result.errors[0].message; | |
| expect(message).toContain('Cannot query field "healt"'); | |
| expect(message).not.toMatch(/Did you mean/); | |
| expect(message).not.toContain('health'); | |
| } | |
| }); | |
| it('should strip "Did you mean" argument suggestions from validation errors without master or maintenance key', async () => { | |
| try { | |
| await apolloClient.query({ | |
| query: gql` | |
| query UnknownArg { | |
| users(wher: {}) { | |
| edges { | |
| node { | |
| id | |
| } | |
| } | |
| } | |
| } | |
| `, | |
| }); | |
| fail('should have thrown a validation error'); | |
| } catch (e) { | |
| const message = e.networkError.result.errors[0].message; | |
| expect(message).toContain('Unknown argument "wher"'); | |
| expect(message).not.toMatch(/Did you mean/); | |
| expect(message).not.toContain('"where"'); | |
| } | |
| }); | |
| it('should keep "Did you mean" suggestions with master key', async () => { | |
| try { | |
| await apolloClient.query({ | |
| query: gql` | |
| query Typo { | |
| healt | |
| } | |
| `, | |
| context: { | |
| headers: { | |
| 'X-Parse-Master-Key': 'test', | |
| }, | |
| }, | |
| }); | |
| fail('should have thrown a validation error'); | |
| } catch (e) { | |
| const message = e.networkError.result.errors[0].message; | |
| expect(message).toContain('Cannot query field "healt"'); | |
| expect(message).toMatch(/Did you mean/); | |
| expect(message).toContain('health'); | |
| } | |
| }); | |
| it('should keep "Did you mean" suggestions with maintenance key', async () => { | |
| try { | |
| await apolloClient.query({ | |
| query: gql` | |
| query Typo { | |
| healt | |
| } | |
| `, | |
| context: { | |
| headers: { | |
| 'X-Parse-Maintenance-Key': 'test2', | |
| }, | |
| }, | |
| }); | |
| fail('should have thrown a validation error'); | |
| } catch (e) { | |
| const message = e.networkError.result.errors[0].message; | |
| expect(message).toContain('Cannot query field "healt"'); | |
| expect(message).toMatch(/Did you mean/); | |
| expect(message).toContain('health'); | |
| } | |
| }); | |
| it('should keep "Did you mean" suggestions when public introspection is enabled', async () => { | |
| const parseServer = await reconfigureServer(); | |
| await createGQLFromParseServer(parseServer, { graphQLPublicIntrospection: true }); | |
| try { | |
| await apolloClient.query({ | |
| query: gql` | |
| query Typo { | |
| healt | |
| } | |
| `, | |
| }); | |
| fail('should have thrown a validation error'); | |
| } catch (e) { | |
| const message = e.networkError.result.errors[0].message; | |
| expect(message).toContain('Cannot query field "healt"'); | |
| expect(message).toMatch(/Did you mean/); | |
| expect(message).toContain('health'); | |
| } | |
| }); | |
| it('should strip "Did you mean" field suggestions from validation errors without master or maintenance key', async () => { | |
| try { | |
| await apolloClient.query({ | |
| query: gql` | |
| query Typo { | |
| healt | |
| } | |
| `, | |
| }); | |
| fail('should have thrown a validation error'); | |
| } catch (e) { | |
| const message = e.networkError.result.errors[0].message; | |
| expect(message).toContain('Cannot query field "healt"'); | |
| expect(message).not.toMatch(/Did you mean/); | |
| expect(message).not.toContain('health'); | |
| } | |
| }); | |
| it('should strip "Did you mean" argument suggestions from validation errors without master or maintenance key', async () => { | |
| try { | |
| await apolloClient.query({ | |
| query: gql` | |
| query UnknownArg { | |
| users(wher: {}) { | |
| edges { | |
| node { | |
| id | |
| } | |
| } | |
| } | |
| } | |
| `, | |
| }); | |
| fail('should have thrown a validation error'); | |
| } catch (e) { | |
| const message = e.networkError.result.errors[0].message; | |
| expect(message).toContain('Unknown argument "wher"'); | |
| expect(message).not.toMatch(/Did you mean/); | |
| expect(message).not.toContain('"where"'); | |
| } | |
| }); | |
| it('should keep "Did you mean" suggestions with master key', async () => { | |
| try { | |
| await apolloClient.query({ | |
| query: gql` | |
| query Typo { | |
| healt | |
| } | |
| `, | |
| context: { | |
| headers: { | |
| 'X-Parse-Master-Key': 'test', | |
| }, | |
| }, | |
| }); | |
| fail('should have thrown a validation error'); | |
| } catch (e) { | |
| const message = e.networkError.result.errors[0].message; | |
| expect(message).toContain('Cannot query field "healt"'); | |
| expect(message).toMatch(/Did you mean/); | |
| expect(message).toContain('health'); | |
| } | |
| }); | |
| it('should keep "Did you mean" suggestions with maintenance key', async () => { | |
| try { | |
| await apolloClient.query({ | |
| query: gql` | |
| query Typo { | |
| healt | |
| } | |
| `, | |
| context: { | |
| headers: { | |
| 'X-Parse-Maintenance-Key': 'test2', | |
| }, | |
| }, | |
| }); | |
| fail('should have thrown a validation error'); | |
| } catch (e) { | |
| const message = e.networkError.result.errors[0].message; | |
| expect(message).toContain('Cannot query field "healt"'); | |
| expect(message).toMatch(/Did you mean/); | |
| expect(message).toContain('health'); | |
| } | |
| }); | |
| it('should keep "Did you mean" suggestions when public introspection is enabled', async () => { | |
| const parseServer = await reconfigureServer(); | |
| await createGQLFromParseServer(parseServer, { graphQLPublicIntrospection: true }); | |
| try { | |
| await apolloClient.query({ | |
| query: gql` | |
| query Typo { | |
| healt | |
| } | |
| `, | |
| }); | |
| fail('should have thrown a validation error'); | |
| } catch (e) { | |
| const message = e.networkError.result.errors[0].message; | |
| expect(message).toContain('Cannot query field "healt"'); | |
| expect(message).toMatch(/Did you mean/); | |
| expect(message).toContain('health'); | |
| } | |
| }); | |
| it('should strip "Did you mean" type suggestions from validation errors without master or maintenance key', async () => { | |
| try { | |
| await apolloClient.query({ | |
| query: gql` | |
| query UnknownType($where: UsreWhereInput) { | |
| health | |
| } | |
| `, | |
| variables: { | |
| where: {}, | |
| }, | |
| }); | |
| fail('should have thrown a validation error'); | |
| } catch (e) { | |
| const message = e.networkError.result.errors[0].message; | |
| expect(message).toContain('Unknown type "UsreWhereInput"'); | |
| expect(message).not.toMatch(/Did you mean/); | |
| expect(message).not.toContain('UserWhereInput'); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/ParseGraphQLServer.spec.js` around lines 1020 - 1127, Add a regression
test that verifies unknown type suggestions (KnownTypeNamesRule) are stripped
without auth: create a new it block similar to the existing typo tests that uses
apolloClient.query with a GraphQL query containing a misspelled input type
(e.g., query($x: UsreWhereInput) { health }) and assert the caught error message
(e.networkError.result.errors[0].message) contains the base error ("Unknown
type" or relevant phrase) but does NOT match /Did you mean/ and does NOT contain
the real type name; reuse helpers reconfigureServer/createGQLFromParseServer
when toggling public introspection and keep test structure consistent with the
other cases that check master/maintenance keys and public introspection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix for GHSA-8cph-rgr4-g5vj
Vulnerability
GraphQL validation rules (FieldsOnCorrectTypeRule, KnownArgumentNamesRule, KnownTypeNamesRule, etc.) embed "Did you mean ...?" hints sourced from the live schema in their error messages. These messages are returned to the caller before runs, so they sidestep and disclose schema identifiers the introspection guard is meant to hide.
Example: An unauthenticated user sends a query with a typo:
The response contains:
This reveals valid field names without needing introspection access.
Fix
Added a new
SchemaSuggestionsControlPluginthat hooks into the validation phase (validationDidStart) and stripsDid you mean ...?suffixes from error messages for callers that are NOT:graphQLPublicIntrospectionenabledThis ensures the fix works regardless of when the suggestions are generated (before
IntrospectionControlPlugin'sdidResolveOperationhook).Files Changed
src/GraphQL/ParseGraphQLServer.js- AddedSchemaSuggestionsControlPluginand registered it in the Apollo Server pluginsspec/ParseGraphQLServer.spec.js- Added 5 test cases covering:Summary by CodeRabbit