fix: GraphQL "Did you mean" suggestions disclose schema to unauthenticated callers (GHSA-8cph-rgr4-g5vj)#10491
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 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 | ✅ 7✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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.94][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
…ripping Adds a test case verifying that 'Did you mean' suggestions for unknown type names (e.g. UsreWhereInput → UserWhereInput) are stripped for unauthenticated callers, covering the KnownTypeNamesRule validation path that was documented but not pinned by a regression test. Refs: CodeRabbit review feedback on PR #10491
This comment was marked as spam.
This comment was marked as spam.
|
@ulsreall Are you are human or an AI agent? Our terms require that you disclose clearly if you are an AI agent. |
This comment was marked as spam.
This comment was marked as spam.
|
Closing as AI spam; clone of already merged fix #10467. |
2 participants
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