perf: Update to @apollo/server 5.0.0 #9888
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! ❌ Please fill out all fields with a placeholder |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughReplaces Apollo Express adapter with @as-integrations/express5, bumps @apollo/server to 5.0.0, adds AST-based __type introspection detection and guard logic, and adds tests covering aliased and key-authorized introspection scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Express
participant ParseGraphQLServer
participant ApolloServer
participant IntrospectionPlugin
Client->>Express: HTTP GraphQL request
Express->>ParseGraphQLServer: route handling
ParseGraphQLServer->>ApolloServer: forward request
ApolloServer->>IntrospectionPlugin: onRequest / requestDidStart
IntrospectionPlugin->>IntrospectionPlugin: fast string check for "__schema"
alt contains "__schema"
IntrospectionPlugin-->>ApolloServer: throw introspection error
else contains "__type"
IntrospectionPlugin->>ParseGraphQLServer: parse query AST (hasTypeIntrospection)
alt AST indicates type introspection
IntrospectionPlugin-->>ApolloServer: throw introspection error
else
IntrospectionPlugin-->>ApolloServer: allow request
end
else
IntrospectionPlugin-->>ApolloServer: allow request
end
ApolloServer-->>Client: response / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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. 🔧 ast-grep (0.39.6)spec/ParseGraphQLServer.spec.jsThanks 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)
16-52: Consider using Apollo Server v5's native introspection controls instead of string-based detection.Apollo Server v5 provides built-in introspection controls via the
introspectionboolean option, thenodeEnvoption to override NODE_ENV-derived defaults, andvalidationRulesincluding a disable-introspection validation rule that runs before execution. The verification found no existing false positive scenarios in the codebase (no user-defined fields or classes containing__schema), so the current string-based detection works correctly. However, leveraging Apollo's nativeintrospectionoption or validation rules would be more robust and maintainable than a custom plugin withincludes('__schema')detection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(1 hunks)src/GraphQL/ParseGraphQLServer.js(2 hunks)
🔇 Additional comments (2)
src/GraphQL/ParseGraphQLServer.js (1)
4-4: LGTM: Correct import for Apollo Server v5.The migration from
@apollo/server/express4to@as-integrations/express5aligns with Apollo Server v5's architectural changes, where Express integration was moved to a separate package.package.json (1)
23-24: No breaking changes or security issues detected with Apollo Server 5.0.0 upgrade.Apollo Server 5.0.0 has no known security vulnerabilities, and all critical breaking changes are properly addressed:
- Node.js v20.0.0+ is required; package.json correctly constrains versions to compatible ranges (>=20.18.0, >=22.12.0)
- graphql >= v16.11.0 is required as a peer dependency; the codebase specifies 16.11.0
- Express middleware import has changed; this is addressed in the codebase migration
| introspection: true, | ||
| plugins: [ApolloServerPluginCacheControlDisabled(), IntrospectionControlPlugin(this.config.graphQLPublicIntrospection)], |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the introspection behavior change with Apollo Server v5 migration.
The introspection configuration has changed from dynamic (config.graphQLPublicIntrospection) to always true with plugin-based access control. This architectural shift delegates enforcement to IntrospectionControlPlugin rather than Apollo Server's native configuration.
According to the PR description, the intended behavior is:
- Master key: introspection always enabled ✓
- Public: introspection remains opt-in via
config.graphQLPublicIntrospection✓
Please verify:
- Introspection queries with master key work correctly
- Introspection queries without master key are blocked when
graphQLPublicIntrospectionis false - Introspection queries without master key succeed when
graphQLPublicIntrospectionis true
You may also want to check Apollo Server v5 documentation for the recommended introspection control pattern:
🌐 Web query:
Apollo Server 5 introspection control best practices
💡 Result:
Short best-practices checklist for controlling introspection in Apollo Server 5
-
Make introspection environment-aware — disable in production, enable in dev/staging. Use an env var to set ApolloServer.introspection. [1][2]
Example: set introspection: process.env.NODE_ENV !== 'production'. [2]
-
If you must allow introspection in production, restrict it to authenticated/trusted users only — gate via middleware or a custom validation rule (e.g., NoSchemaIntrospectionCustomRule when unauthenticated). [3][5]
-
Use a schema registry / Apollo Studio instead of public introspection for team discovery and change history — publish schemas so developers don’t need endpoint introspection. [1][2]
-
Layer other protections (don’t rely on “security through obscurity” alone):
- Query depth/complexity and cost analysis.
- Rate limiting, per-client quotas, and field-level throttling.
- Operation whitelisting (allow only vetted queries).
- Strong auth & authorization for every resolver.
- Mask detailed error/stack traces in production. [1][3][5]
-
CI / deployment controls:
- Run automated breaking-change checks (graphql-inspector or schema checks) before deploy.
- Treat introspection toggles as part of config reviewed in releases. [5]
-
Monitoring & incident response:
- Log and monitor unusual query patterns and failed introspection attempts.
- Alert on spikes that may indicate automated schema-recon attacks. [1][4]
Recommended reading / references
- Apollo GraphOS / Security guidance (turn off production introspection, use Studio). [1]
- Apollo blog: “Why You Should Disable GraphQL Introspection in Production” (how-to example). [2]
- GraphQL.org: introspection and NoSchemaIntrospectionCustomRule guidance. [3]
- “Going to Production” / graphql-js production checklist (depth limits, validation rules). [5]
References
[1] Apollo GraphQL — Graph Security / Security pages.
[2] Apollo GraphQL blog — Why You Should Disable GraphQL Introspection In Production.
[3] graphql.org — Security / Introspection docs and NoSchemaIntrospectionCustomRule.
[4] Apollo GraphQL — 9 Ways To Secure your GraphQL API (security checklist).
[5] graphql-js — Going to Production guidance.
Would you like a short Apollo Server 5 code example showing environment-based introspection + a conditional validation rule?
Apollo Server 5 introspection configuration contradicts security best practices — introspection should be disabled at server level.
Apollo Server 5 best practices recommend making introspection environment-aware (disable in production, enable in dev/staging) and if introspection must be allowed in production, restricting it to authenticated/trusted users only via middleware or custom validation rules.
The current implementation sets introspection: true unconditionally, which contradicts this guidance. The recommended pattern is to use a schema registry/Apollo Studio instead of public introspection, and layer other protections such as query depth/complexity analysis and rate limiting.
Fix: Change line 107 from introspection: true to introspection: false (or environment-aware: introspection: process.env.NODE_ENV !== 'production'), and let IntrospectionControlPlugin handle conditional enablement for authenticated master keys only. This follows the principle of fail-closed (deny by default) with opt-in access for trusted clients.
🤖 Prompt for AI Agents
In src/GraphQL/ParseGraphQLServer.js around lines 107 to 108, the server is
configured with unconditional introspection enabled which violates best
practices; change the server-level introspection setting to fail-closed (e.g.
set introspection to false or make it environment-aware like
process.env.NODE_ENV !== 'production') so the default is disabled, and rely on
IntrospectionControlPlugin for conditional/exempted access
(authenticated/trusted clients) while also ensuring any related tests/configs
reflect the new default.
There was a problem hiding this comment.
Help me understand this change. We just added this option to restrict introspection. Does this make introspection available only with masterKey? Is there any other place in code where the config option is used?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all occurrences of graphQLPublicIntrospection in the codebase
echo "=== Searching for graphQLPublicIntrospection usage ==="
rg -n "graphQLPublicIntrospection" -C 3
echo -e "\n=== Finding related test files ==="
fd -e spec.js -e test.js | xargs rg -l "introspection" 2>/dev/null | head -20
echo -e "\n=== Checking ParseGraphQLController for config definition ==="
fd "ParseGraphQLController" --type f -e js -e ts | head -5
Length of output: 5735
🏁 Script executed:
#!/bin/bash
# Check the introspection test to understand expected behavior
echo "=== Introspection test details ==="
rg -n -A 30 "should have public introspection enabled if enabled" spec/ParseGraphQLServer.spec.js
echo -e "\n=== Check for related introspection tests ==="
rg -n -B 5 -A 20 "public introspection" spec/ParseGraphQLServer.spec.js | head -100
Length of output: 2884
@mtrezza Great questions! Let me clarify how this change works:
How the introspection control works now:
The graphQLPublicIntrospection config option is still used and functional — it's passed to the IntrospectionControlPlugin on line 108. The architectural change is that Apollo Server's native introspection setting is now always true, but the actual access control is handled by the custom plugin.
Behavior breakdown:
-
With
graphQLPublicIntrospection: false(default):- ✅ Master key or maintenance key → introspection allowed
- ❌ No master key → introspection blocked (403 error)
-
With
graphQLPublicIntrospection: true:- ✅ Anyone can introspect (with or without master key)
Why this change was needed:
Apollo Server's native introspection setting is binary (on/off), but the requirement is ternary:
- Master/maintenance keys should always have introspection access
- Public access should be opt-in via config
The IntrospectionControlPlugin (lines 17-50) implements this logic:
if (publicIntrospection) {
return; // Allow all
}
if (isMasterOrMaintenance) {
return; // Allow master/maintenance
}
// Otherwise block introspection queriesOther places the config is used:
- Security checks:
src/Security/CheckGroups/CheckGroupServerConfig.js(warns if public introspection is enabled) - Config definitions:
src/Options/Definitions.js,src/Options/index.js - Tests:
spec/ParseGraphQLServer.spec.jsvalidates all three scenarios - Documentation:
src/Options/docs.js
The tests confirm this behavior works as expected across all scenarios.
🧬 Code Graph Analysis Results
[ParseGraphQLServer.js, lines 4-22]
const IntrospectionControlPlugin = (publicIntrospection) => ({
requestDidStart: (requestContext) => ({
didResolveOperation: async () => {
// If public introspection is enabled, we allow all introspection queries
if (publicIntrospection) {
return;
}
const isMasterOrMaintenance = requestContext.contextValue.auth?.isMaster || requestContext.contextValue.auth?.isMaintenance
if (isMasterOrMaintenance) {
return;
}
// Now we check if the query is an introspection query
// this check strategy should work in 99.99% cases
// we can have an issue if a user name a field or class __schemaSomething
// we want to avoid a full AST check
const isIntrospectionQuery =
requestContext.request.query?.includes('__schema')
if (isIntrospectionQuery) {
throw new GraphQLError('Introspection is not allowed', {
extensions: {
http: {
status: 403,
},
}
});
}
},
})
});
[ParseGraphQLServer.js, lines 40-50]
const apollo = new ApolloServer({
csrfPrevention: {
// See https://www.apollographql.com/docs/router/configuration/csrf/
// needed since we use graphql upload
requestHeaders: ['X-Parse-Application-Id'],
},
introspection: true,
plugins: [ApolloServerPluginCacheControlDisabled(), IntrospectionControlPlugin(this.config.graphQLPublicIntrospection)],
schema,
});
await apollo.start();
this._server = expressMiddleware(apollo, {
context,
});
return this._server;
}
There was a problem hiding this comment.
So there is no security issue with this change?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the introspection tests for security scenarios
echo "=== Checking introspection test coverage ==="
rg -n -A 40 "describe.*introspection" spec/ParseGraphQLServer.spec.js
echo -e "\n=== Look for __type queries (another introspection mechanism) ==="
rg -n "__type" spec/ParseGraphQLServer.spec.js
echo -e "\n=== Check if the plugin handles all introspection query types ==="
rg -n "__schema|__type|__typename" src/GraphQL/ParseGraphQLServer.js
Length of output: 21398
🏁 Script executed:
#!/bin/bash
# Search for tests that verify introspection is blocked
echo "=== Looking for tests that verify introspection blocking ==="
rg -n -B 5 -A 15 "should.*not.*introspection|introspection.*blocked|introspection.*disabled" spec/ParseGraphQLServer.spec.js
echo -e "\n=== Looking for tests with graphQLPublicIntrospection config ==="
rg -n -B 3 -A 10 "graphQLPublicIntrospection" spec/ParseGraphQLServer.spec.js
echo -e "\n=== Check for any introspection security tests ==="
rg -n -B 5 -A 10 "403.*introspection|Introspection is not allowed" spec/ParseGraphQLServer.spec.js
Length of output: 1967
🏁 Script executed:
#!/bin/bash
# Check a sample __type query test to see if it uses master key
echo "=== Examining __type query test context (around line 741) ==="
sed -n '720,780p' spec/ParseGraphQLServer.spec.js
echo -e "\n=== Check if tests without master key use __type ==="
# Look for tests that explicitly don't use master key and check if they use __type
rg -n -B 10 "__type.*name.*Query" spec/ParseGraphQLServer.spec.js | head -60
echo -e "\n=== Check the test setup for apollo client configuration ==="
rg -n -A 5 "createApolloClient|apolloClient.*=|new ApolloClient" spec/ParseGraphQLServer.spec.js | head -40
Length of output: 3483
🏁 Script executed:
#!/bin/bash
# Check the test context around the introspection and __type tests
echo "=== Looking at test setup before introspection tests ==="
sed -n '600,650p' spec/ParseGraphQLServer.spec.js
echo -e "\n=== Looking at apolloClient setup in the test file ==="
sed -n '460,490p' spec/ParseGraphQLServer.spec.js
echo -e "\n=== Check if httpLink includes master key ==="
rg -n -B 5 -A 5 "httpLink.*=|createHttpLink" spec/ParseGraphQLServer.spec.js | head -50
Length of output: 4268
🏁 Script executed:
#!/bin/bash
# Check what the headers variable contains
echo "=== Looking for headers variable definition ==="
rg -n -B 2 -A 5 "^[ ]*headers[ ]*=" spec/ParseGraphQLServer.spec.js | head -60
echo -e "\n=== Check what headers are used by default in tests ==="
sed -n '1,100p' spec/ParseGraphQLServer.spec.js | grep -A 10 "headers"
echo -e "\n=== Look for default header setup in the test ==="
rg -n "X-Parse-Master-Key.*test|masterKey.*headers" spec/ParseGraphQLServer.spec.js | head -20
Length of output: 1225
🏁 Script executed:
#!/bin/bash
# Check the describe block setup to see default headers
echo "=== Looking at the main describe block setup ==="
sed -n '40,100p' spec/ParseGraphQLServer.spec.js
echo -e "\n=== Looking for where headers constant is defined ==="
rg -n "const headers|let headers" spec/ParseGraphQLServer.spec.js | head -10
echo -e "\n=== Check around line 460 for full context of apolloClient setup ==="
sed -n '440,480p' spec/ParseGraphQLServer.spec.js
Length of output: 3680
🏁 Script executed:
#!/bin/bash
# Check what headers is set to at line 279
echo "=== Looking at headers definition at line 279 ==="
sed -n '275,285p' spec/ParseGraphQLServer.spec.js
echo -e "\n=== Looking at the broader context around line 279 ==="
sed -n '270,320p' spec/ParseGraphQLServer.spec.js
echo -e "\n=== Check if headers includes master key by default ==="
sed -n '275,295p' spec/ParseGraphQLServer.spec.js
Length of output: 1930
@mtrezza Yes, there is a security issue with this change. I've identified a critical vulnerability in the introspection control implementation.
The Security Problem
The IntrospectionControlPlugin only blocks queries containing __schema, but GraphQL has multiple introspection mechanisms:
__schema- blocked by the plugin ✅__type- NOT blocked by the plugin ❌__typename- typically allowed (used in normal queries)
Proof of the Vulnerability
Looking at the test suite:
Default headers (line 279):
const headers = {
'X-Parse-Application-Id': 'test',
'X-Parse-Javascript-Key': 'test',
// NO MASTER KEY
};These __type introspection queries run without master key throughout the tests:
- Line 741:
__type(name: "Object") - Line 756:
__type(name: "Date") - Line 931:
__type(name: "Query") - Hundreds more...
All these tests pass, meaning __type queries work without master key even when graphQLPublicIntrospection: false, effectively bypassing the introspection restriction.
Why This Change Makes It Worse
Before (Apollo Server v4):
introspection: this.config.graphQLPublicIntrospection- Apollo Server natively blocked all introspection queries when set to
false - This included both
__schemaand__type
After (This PR):
introspection: true // Always enabled at Apollo level- Apollo Server allows all introspection
- Plugin only blocks
__schemaqueries __typequeries completely bypass security
Recommended Fix
The plugin needs to check for all introspection query types:
const isIntrospectionQuery =
requestContext.request.query?.includes('__schema') ||
requestContext.request.query?.includes('__type')Or better yet, consider reverting to Apollo Server's native introspection configuration and using validation rules for conditional master-key access, as that's more robust than string matching.
There was a problem hiding this comment.
@coderabbitai is there a way to limit public __type introspection to what is needed for the Parse.File functionality?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Parse.File GraphQL schema definition and __type usage
echo "=== Searching for Parse.File type definitions in GraphQL schema ==="
rg -n "__type" --type js -C 5 src/GraphQL/
echo -e "\n=== Looking for File type schema definitions ==="
rg -n "type.*File|FileInfo|_File" src/GraphQL/ --type js -C 3 | head -80
echo -e "\n=== Searching for __type field usage in tests ==="
rg -n "__type" spec/ParseGraphQLServer.spec.js -C 3 | head -100
echo -e "\n=== Check Parse.File related GraphQL operations ==="
rg -n "Parse\.File|FileInfo" spec/ParseGraphQLServer.spec.js -C 5 | head -60
Length of output: 32638
@mtrezza Yes, there's a way to limit public __type introspection while preserving Parse.File functionality!
The Key Distinction
The search results reveal that __type is used in two completely different ways:
-
Parse's internal JSON representation (data):
{ __type: 'File', name: 'foo.jpg', url: 'https://...' } { __type: 'Date', iso: '2016-05-22T09:00:00.000Z' } { __type: 'Pointer', className: 'User', objectId: '...' }
-
GraphQL introspection query (schema exploration):
query { __type(name: "FileInfo") { # ← Note the parentheses! kind fields { name } } }
The Solution
Introspection queries always use function call syntax with parentheses: __type(
Data fields never have parentheses: just __type as a property
The plugin can check for __type( to block introspection while allowing Parse's internal __type property:
const isIntrospectionQuery =
requestContext.request.query?.includes('__schema') ||
requestContext.request.query?.includes('__type(')This works because:
- ✅ Blocks introspection:
query { __type(name: "User") { ... } }contains__type( - ✅ Allows Parse.File: GraphQL never sends the JSON representation as part of the query string — the
{ __type: 'File' }object exists only in response data and variables, not in the query text itself
Why This Is Safe
When you save a Parse.File, the client sends:
mutation {
createObject(fields: { photo: { file: "photo.jpg" } })
}The { __type: 'File', name: 'photo.jpg' } representation is created server-side by the transformers (see src/GraphQL/transformers/mutation.js lines 98-100). The query string itself never contains __type as a field access.
Recommendation
Update the plugin on line 36-37 from:
const isIntrospectionQuery =
requestContext.request.query?.includes('__schema')To:
const isIntrospectionQuery =
requestContext.request.query?.includes('__schema') ||
requestContext.request.query?.includes('__type(')This provides complete introspection blocking (both __schema and __type mechanisms) while preserving all Parse.File and other special type functionality.
🧬 Code Graph Analysis Results
[src/GraphQL/ParseGraphQLServer.js] Introspection control mechanism
Snippet (complete):
const IntrospectionControlPlugin = (publicIntrospection) => ({
requestDidStart: (requestContext) => ({
didResolveOperation: async () => {
// If public introspection is enabled, we allow all introspection queries
if (publicIntrospection) {
return;
}
const isMasterOrMaintenance = requestContext.contextValue.auth?.isMaster || requestContext.contextValue.auth?.isMaintenance
if (isMasterOrMaintenance) {
return;
}
// Now we check if the query is an introspection query
// this check strategy should work in 99.99% cases
// we can have an issue if a user name a field or class __schemaSomething
// we want to avoid a full AST check
const isIntrospectionQuery =
requestContext.request.query?.includes('__schema')
if (isIntrospectionQuery) {
throw new GraphQLError('Introspection is not allowed', {
extensions: {
http: {
status: 403,
},
}
});
}
},
})
});
There was a problem hiding this comment.
@Moumouls what do you think? Are we onto something here?
There was a problem hiding this comment.
@mtrezza coderabbit suggestion is wrong, because GQL spec allow line return after "__type"
Example query
The suggested change will not work, and can be bypassed easily
A smart move could be:
- string match on __type and __schema
- if __type match, let's parse AST to ensure that it's not the root field introspection query
So we only parse the request in case the request looks suspicious :)
There was a problem hiding this comment.
i'll add it it's not that hard
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9888 +/- ##
==========================================
- Coverage 93.01% 93.00% -0.02%
==========================================
Files 187 187
Lines 15163 15177 +14
Branches 177 177
==========================================
+ Hits 14104 14115 +11
- Misses 1047 1050 +3
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mtrezza
changed the title
…/parse-server into moumouls/upgrade-appollo-server
|
@mtrezza i improved the system |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
spec/ParseGraphQLServer.spec.js (3)
733-771: Harden negative introspection tests with fragments and variables.Great coverage for direct and aliased __type; consider adding fragment- and variable-based cases to prevent bypass via AST shape changes.
Example additions:
+ it('should block __type introspection in fragment without master key', async () => { + try { + await apolloClient.query({ + query: gql` + query { + ...Frag + } + fragment Frag on Query { + __type(name: "User") { name } + } + `, + }); + fail('should have thrown an error'); + } catch (e) { + expect(e.message).toEqual('Response not successful: Received status code 403'); + expect(e.networkError.result.errors[0].message).toEqual('Introspection is not allowed'); + } + }); + + it('should block variable-based __type introspection without master key', async () => { + try { + await apolloClient.query({ + query: gql` + query TypeIntrospection($name: String!) { + __type(name: $name) { name } + } + `, + variables: { name: 'User' }, + }); + fail('should have thrown an error'); + } catch (e) { + expect(e.message).toEqual('Response not successful: Received status code 403'); + expect(e.networkError.result.errors[0].message).toEqual('Introspection is not allowed'); + } + });
773-792: Strengthen positive assertions for allowed __type results.When introspection is permitted, assert specific fields (e.g., name === 'User', kind === 'OBJECT') to catch regressions beyond mere success.
Example:
- expect(introspection.data).toBeDefined(); - expect(introspection.data.__type).toBeDefined(); + expect(introspection.data?.__type?.name).toBe('User'); + expect(introspection.data?.__type?.kind).toBe('OBJECT'); + expect(introspection.errors).toBeUndefined();Also applies to: 794-813, 815-834, 836-852
1678-1681: Avoid redundant cache reset before recreating the server.You reset caches then replace parseGraphQLServer with a new instance, which already starts clean. Drop the reset to shave test time.
- await parseGraphQLServer.setGraphQLConfig({}); - await resetGraphQLCache(); - await createGQLFromParseServer(parseServer, { graphQLPublicIntrospection: true }); + await parseGraphQLServer.setGraphQLConfig({}); + await createGQLFromParseServer(parseServer, { graphQLPublicIntrospection: true });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/ParseGraphQLServer.spec.js(5 hunks)src/GraphQL/ParseGraphQLServer.js(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/ParseGraphQLServer.spec.js
🧬 Code graph analysis (1)
spec/ParseGraphQLServer.spec.js (1)
spec/helper.js (2)
parseServer(167-167)reconfigureServer(180-214)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Node 20
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Code Analysis (javascript)
- GitHub Check: Docker Build
🔇 Additional comments (8)
src/GraphQL/ParseGraphQLServer.js (4)
4-4: LGTM! Import changes align with Apollo Server v5 migration.The migration from
@apollo/server/express4to@as-integrations/express5and the addition ofparsefor AST-based introspection detection are necessary for the upgrade.Also applies to: 7-7
38-46: LGTM! Clean error handling extraction.The helper function provides consistent error messaging and proper HTTP 403 status for introspection denial.
64-78: LGTM! Well-optimized two-stage introspection check.The implementation balances security and performance effectively:
- Fast path for
__schema(simple string match)- Smart path for
__type(AST parsing only when string is present)- Comments clearly explain the rationale
This addresses the performance concerns and bypass risks discussed in previous comments. The AST-based approach correctly handles whitespace variations that would bypass simple string matching like
__type(.Note: Once the fragment bypass issue in
hasTypeIntrospectionis fixed, this logic will be fully secure.
138-138: LGTM! Introspection control architecture is sound.Based on the extensive discussion in previous review comments, setting
introspection: trueat the Apollo Server level is the correct approach for Apollo Server v5. TheIntrospectionControlPluginhandles the ternary access control logic (master key always allowed, public opt-in viagraphQLPublicIntrospection), which cannot be expressed through Apollo's native binaryintrospectionsetting.spec/ParseGraphQLServer.spec.js (4)
857-859: Enabling public introspection per-suite is appropriate.This localizes schema type checks that rely on __type without requiring master key. LGTM.
1019-1021: Relay suite setup looks good.Recreating the server with graphQLPublicIntrospection avoids auth headers in type lookups. LGTM.
1567-1568: Parse Class Types suite setup is consistent.Per-suite enablement keeps tests independent. LGTM.
7968-7969: Function mutations suite setup is sound.Recreating with public introspection avoids auth headers for enum/type checks. LGTM. Based on learnings.
| const hasTypeIntrospection = (query) => { | ||
| try { | ||
| const ast = parse(query); | ||
| // Check only root-level fields in the query | ||
| // Note: selection.name.value is the actual field name, so this correctly handles | ||
| // aliases like "myAlias: __type(...)" where name.value === "__type" | ||
| for (const definition of ast.definitions) { | ||
| if (definition.kind === 'OperationDefinition' && definition.selectionSet) { | ||
| for (const selection of definition.selectionSet.selections) { | ||
| if (selection.kind === 'Field' && selection.name.value === '__type') { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } catch (e) { | ||
| // If parsing fails, we assume it's not a valid query and let Apollo handle it | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Critical: Fragment definitions can bypass introspection control.
The function only checks OperationDefinition selections, but GraphQL allows defining introspection queries in named fragments that are then spread into operations:
fragment IntrospectionFragment on Query {
__type(name: "User") { fields { name } }
}
query {
...IntrospectionFragment
}This bypasses the check entirely because FragmentDefinition nodes are not inspected.
Apply this diff to check fragment definitions:
const hasTypeIntrospection = (query) => {
try {
const ast = parse(query);
// Check only root-level fields in the query
// Note: selection.name.value is the actual field name, so this correctly handles
// aliases like "myAlias: __type(...)" where name.value === "__type"
for (const definition of ast.definitions) {
- if (definition.kind === 'OperationDefinition' && definition.selectionSet) {
+ if ((definition.kind === 'OperationDefinition' || definition.kind === 'FragmentDefinition') && definition.selectionSet) {
for (const selection of definition.selectionSet.selections) {
if (selection.kind === 'Field' && selection.name.value === '__type') {
return true;
}
}
}
}
return false;
} catch (e) {
// If parsing fails, we assume it's not a valid query and let Apollo handle it
return false;
}
};🤖 Prompt for AI Agents
In src/GraphQL/ParseGraphQLServer.js around lines 16 to 36, the current
hasTypeIntrospection only inspects OperationDefinition root fields and misses
FragmentDefinition usage; update it to build a map of FragmentDefinition nodes
from the AST and traverse selections recursively (handle Field, InlineFragment,
and FragmentSpread), resolving FragmentSpread by looking up the fragment in the
map and descending into its selectionSet, and maintain a visited-fragment set to
avoid cycles; return true if any visited Field has name.value === '__type' (or
nested contains it), otherwise false, and keep the existing try/catch behavior.
There was a problem hiding this comment.
@Moumouls, sounds good! Let me know if you need any clarification on the fragment bypass issue or if you'd like me to help verify the fix once you've implemented it. 🐰
mtrezza
added
the
state:breaking
Marked as breaking change; can be merged with Parse Server 9 which will drop Node 18 support. |
3 participants
Pull Request
Issue
Closes: FILL_THIS_OUT
Approach
Update to V5, something changed on how introspection is managed internally, or maybe we got a bug before, introspection is always activated for master key, and public introspection is stil an option to opt in
Tasks
Summary by CodeRabbit
Chores
Features
Tests