diff --git a/.changeset/cute-planets-move.md b/.changeset/cute-planets-move.md new file mode 100644 index 000000000..2c19040bd --- /dev/null +++ b/.changeset/cute-planets-move.md @@ -0,0 +1,9 @@ +--- +"@apollo/query-planner": patch +"@apollo/federation-internals": patch +"@apollo/gateway": patch +--- + +Fixed several code paths that access response objects to prevent JavaScript prototype pollution and unintended access to the prototype chain. + +See the associated GitHub Advisories [GHSA-pfjj-6f4p-rvmh](https://github.com/apollographql/federation/security/advisories/GHSA-pfjj-6f4p-rvmh) for more information. diff --git a/.cspell/cspell-dict.txt b/.cspell/cspell-dict.txt index 6b6f55bbc..38cf08cc6 100644 --- a/.cspell/cspell-dict.txt +++ b/.cspell/cspell-dict.txt @@ -291,3 +291,5 @@ webp whith wizz woudl +pfjj +rvmh \ No newline at end of file diff --git a/gateway-js/src/__tests__/executeQueryPlan.test.ts b/gateway-js/src/__tests__/executeQueryPlan.test.ts index c9e585f2f..8a89da986 100644 --- a/gateway-js/src/__tests__/executeQueryPlan.test.ts +++ b/gateway-js/src/__tests__/executeQueryPlan.test.ts @@ -9,13 +9,15 @@ import gql from 'graphql-tag'; import { buildOperationContext } from '../operationContext'; import { executeQueryPlan } from '../executeQueryPlan'; import { LocalGraphQLDataSource } from '../datasources/LocalGraphQLDataSource'; +import { RemoteGraphQLDataSource } from '../datasources/RemoteGraphQLDataSource'; +import { Response } from 'node-fetch'; import { astSerializer, queryPlanSerializer, superGraphWithInaccessible, } from 'apollo-federation-integration-testsuite'; import { QueryPlan, QueryPlanner } from '@apollo/query-planner'; -import { ApolloGateway } from '..'; +import { ApolloGateway, GraphQLDataSource } from '..'; import { ApolloServer } from '@apollo/server'; import { getFederatedTestingSchema } from './execution-utils'; import { @@ -64,7 +66,7 @@ describe('executeQueryPlan', () => { operation: Operation, executeRequestContext?: GatewayGraphQLRequestContext, executeSchema?: Schema, - executeServiceMap?: { [serviceName: string]: LocalGraphQLDataSource }, + executeServiceMap?: { [serviceName: string]: GraphQLDataSource }, ): Promise { const supergraphSchema = executeSchema ?? schema; const apiSchema = supergraphSchema.toAPISchema(); @@ -7713,4 +7715,732 @@ describe('executeQueryPlan', () => { }, }); }); + + describe('prototype manipulation prevention', () => { + it('does not allow `constructor` payloads to pollute prototypes when merging entities', async () => { + const s1 = { + name: 'S1', + typeDefs: gql` + type Query { + user: User + } + + type User @key(fields: "id") { + id: ID! + } + `, + }; + + const s2 = { + name: 'S2', + typeDefs: gql` + type User @key(fields: "id") { + id: ID! + homePage: HomePage + } + + type HomePage { + userControlledField: JSON + } + + scalar JSON + `, + }; + + const { schema, queryPlanner } = getFederatedTestingSchema([s1, s2]); + const operation = parseOp( + `#graphql + query { + user { + constructor: homePage { + prototype: userControlledField + } + } + } + `, + schema, + ); + + const queryPlan = buildPlan(operation, queryPlanner); + expect(queryPlan).toMatchInlineSnapshot(` + QueryPlan { + Sequence { + Fetch(service: "S1") { + { + user { + __typename + id + } + } + }, + Flatten(path: "user") { + Fetch(service: "S2") { + { + ... on User { + __typename + id + } + } => + { + ... on User { + constructor: homePage { + prototype: userControlledField + } + } + } + }, + }, + }, + } + `); + // Note that LocalGraphQLDataSource uses null-prototype objects, which + // don't elicit this bug. So we instead have to mock the responses using + // RemoteGraphQLDataSource. + const remoteServiceMap = { + S1: new RemoteGraphQLDataSource({ + url: 'https://s1.example.com/graphql', + fetcher: async () => + new Response( + JSON.stringify({ + data: { + user: { + __typename: 'User', + id: '1', + }, + }, + }), + { + status: 200, + headers: { 'content-type': 'application/json' }, + }, + ), + }), + S2: new RemoteGraphQLDataSource({ + url: 'https://s2.example.com/graphql', + fetcher: async () => + new Response( + JSON.stringify({ + data: { + _entities: [ + { + constructor: { + prototype: { maliciousPayload: true }, + }, + }, + ], + }, + }), + { + status: 200, + headers: { 'content-type': 'application/json' }, + }, + ), + }), + }; + const response = await executePlan( + queryPlan, + operation, + undefined, + schema, + remoteServiceMap, + ); + expect(({} as Record).maliciousPayload).toBeUndefined(); + expect(response.errors).toBeUndefined(); + expect(response.extensions).toBeUndefined(); + expect(response.data).toMatchObject({ + user: { + constructor: { + prototype: { maliciousPayload: true }, + }, + }, + }); + }); + + it('does not allow `__proto__` payloads to pollute prototypes when merging entities', async () => { + const s1 = { + name: 'S1', + typeDefs: gql` + type Query { + user: User + } + + type User @key(fields: "id") { + id: ID! + } + `, + }; + + const s2 = { + name: 'S2', + typeDefs: gql` + type User @key(fields: "id") { + id: ID! + userControlledField: JSON + } + + scalar JSON + `, + }; + + const { schema, queryPlanner } = getFederatedTestingSchema([s1, s2]); + const operation = parseOp( + `#graphql + query { + user { + __proto__: userControlledField + } + } + `, + schema, + ); + + const queryPlan = buildPlan(operation, queryPlanner); + expect(queryPlan).toMatchInlineSnapshot(` + QueryPlan { + Sequence { + Fetch(service: "S1") { + { + user { + __typename + id + } + } + }, + Flatten(path: "user") { + Fetch(service: "S2") { + { + ... on User { + __typename + id + } + } => + { + ... on User { + __proto__: userControlledField + } + } + }, + }, + }, + } + `); + // Note that LocalGraphQLDataSource uses null-prototype objects, which + // don't elicit this bug. So we instead have to mock the responses using + // RemoteGraphQLDataSource. + const remoteServiceMap = { + S1: new RemoteGraphQLDataSource({ + url: 'https://s1.example.com/graphql', + fetcher: async () => + new Response( + JSON.stringify({ + data: { + user: { + __typename: 'User', + id: '1', + }, + }, + }), + { + status: 200, + headers: { 'content-type': 'application/json' }, + }, + ), + }), + S2: new RemoteGraphQLDataSource({ + url: 'https://s2.example.com/graphql', + fetcher: async () => + new Response( + JSON.stringify({ + data: { + _entities: [ + { + ['__proto__']: { maliciousPayload: true }, + }, + ], + }, + }), + { + status: 200, + headers: { 'content-type': 'application/json' }, + }, + ), + }), + }; + const response = await executePlan( + queryPlan, + operation, + undefined, + schema, + remoteServiceMap, + ); + expect(({} as Record).maliciousPayload).toBeUndefined(); + expect(response.errors).toBeUndefined(); + expect(response.extensions).toBeUndefined(); + expect(response.data).toMatchObject({ + user: { + ['__proto__']: { maliciousPayload: true }, + }, + }); + }); + + it('does not read prototype fields when computing responses', async () => { + const s1 = { + name: 'S1', + typeDefs: gql` + type Query { + user: User + } + + type User @key(fields: "id") { + id: ID! + } + `, + }; + + const s2 = { + name: 'S2', + typeDefs: gql` + type User @key(fields: "id") { + id: ID! + erroringField: JSON! + } + + scalar JSON + `, + }; + + const { schema, queryPlanner } = getFederatedTestingSchema([s1, s2]); + const operation = parseOp( + `#graphql + query { + user { + __proto__: erroringField + } + } + `, + schema, + ); + + const queryPlan = buildPlan(operation, queryPlanner); + expect(queryPlan).toMatchInlineSnapshot(` + QueryPlan { + Sequence { + Fetch(service: "S1") { + { + user { + __typename + id + } + } + }, + Flatten(path: "user") { + Fetch(service: "S2") { + { + ... on User { + __typename + id + } + } => + { + ... on User { + __proto__: erroringField + } + } + }, + }, + }, + } + `); + // Note that LocalGraphQLDataSource uses null-prototype objects, which + // don't elicit this bug. So we instead have to mock the responses using + // RemoteGraphQLDataSource. + const remoteServiceMap = { + S1: new RemoteGraphQLDataSource({ + url: 'https://s1.example.com/graphql', + fetcher: async () => + new Response( + JSON.stringify({ + data: { + user: { + __typename: 'User', + id: '1', + }, + }, + }), + { + status: 200, + headers: { 'content-type': 'application/json' }, + }, + ), + }), + S2: new RemoteGraphQLDataSource({ + url: 'https://s2.example.com/graphql', + fetcher: async () => + new Response( + JSON.stringify({ + data: { + _entities: [null], + }, + errors: [ + { + message: 'Error', + path: ['_entities', 0, '__proto__'], + }, + ], + }), + { + status: 200, + headers: { 'content-type': 'application/json' }, + }, + ), + }), + }; + const response = await executePlan( + queryPlan, + operation, + undefined, + schema, + remoteServiceMap, + ); + expect(response.errors).toMatchObject([ + { + message: 'Error', + path: ['user', '__proto__'], + }, + ]); + expect(response.extensions).toBeUndefined(); + expect(response.data).toMatchObject({ + user: null, + }); + }); + + // There appears to be a bug in graphql-js, where it reads non-own property + // names from variables. Until it's fixed, this test won't work as-is, and + // we have a separate test below that uses RemoteGraphQLDataSource as a + // workaround. + it.skip('does not read prototype fields for provided variables', async () => { + const s1 = { + name: 'S1', + typeDefs: gql` + type Query { + user: User + } + + type User @key(fields: "id") { + id: ID! + } + `, + resolvers: { + Query: { + user() { + return { id: '1' }; + }, + }, + }, + }; + + const s2 = { + name: 'S2', + typeDefs: gql` + type User @key(fields: "id") { + id: ID! + isObject(value: JSON): Boolean! + } + + scalar JSON + `, + resolvers: { + User: { + __resolveReference() { + return {}; + }, + isObject(value: object | null | undefined) { + return !!value; + }, + }, + }, + }; + + const { serviceMap, schema, queryPlanner } = getFederatedTestingSchema([ + s1, + s2, + ]); + const operation = parseOp( + `#graphql + query($__proto__: JSON) { + user { + isObject(value: $__proto__) + } + } + `, + schema, + ); + + const queryPlan = buildPlan(operation, queryPlanner); + expect(queryPlan).toMatchInlineSnapshot(` + QueryPlan { + Sequence { + Fetch(service: "S1") { + { + user { + __typename + id + } + } + }, + Flatten(path: "user") { + Fetch(service: "S2") { + { + ... on User { + __typename + id + } + } => + { + ... on User { + isObject(value: $__proto__) + } + } + }, + }, + }, + } + `); + const response = await executePlan( + queryPlan, + operation, + undefined, + schema, + serviceMap, + ); + expect(response.errors).toBeUndefined(); + expect(response.extensions).toBeUndefined(); + expect(response.data).toMatchObject({ + user: { isObject: false }, + }); + }); + + // This test is a workaround test; see the comment on the test immediately + // above for why this exists. + it('does not read prototype fields for provided variables (workaround)', async () => { + const s1 = { + name: 'S1', + typeDefs: gql` + type Query { + user: User + } + + type User @key(fields: "id") { + id: ID! + } + `, + resolvers: { + Query: { + user() { + return { id: '1' }; + }, + }, + }, + }; + + const s2 = { + name: 'S2', + typeDefs: gql` + type User @key(fields: "id") { + id: ID! + isObject(value: JSON): Boolean! + } + + scalar JSON + `, + }; + + const { schema, queryPlanner, ...rest } = getFederatedTestingSchema([ + s1, + s2, + ]); + const operation = parseOp( + `#graphql + query($__proto__: JSON) { + user { + isObject(value: $__proto__) + } + } + `, + schema, + ); + + const queryPlan = buildPlan(operation, queryPlanner); + expect(queryPlan).toMatchInlineSnapshot(` + QueryPlan { + Sequence { + Fetch(service: "S1") { + { + user { + __typename + id + } + } + }, + Flatten(path: "user") { + Fetch(service: "S2") { + { + ... on User { + __typename + id + } + } => + { + ... on User { + isObject(value: $__proto__) + } + } + }, + }, + }, + } + `); + let serviceMap: { + [k: string]: GraphQLDataSource; + } = rest.serviceMap; + serviceMap.S2 = new RemoteGraphQLDataSource({ + url: 'https://s2.example.com/graphql', + fetcher: async (_, init) => { + const variables = { + __proto__: null, + ...JSON.parse(init?.body as string)['variables'], + }; + return new Response( + JSON.stringify({ + data: { + _entities: [ + { + isObject: !!variables['__proto__'], + }, + ], + }, + }), + { + status: 200, + headers: { 'content-type': 'application/json' }, + }, + ); + }, + }); + const response = await executePlan( + queryPlan, + operation, + undefined, + schema, + serviceMap, + ); + expect(response.errors).toBeUndefined(); + expect(response.extensions).toBeUndefined(); + expect(response.data).toMatchObject({ + user: { isObject: false }, + }); + }); + + it('does not read prototype fields for condition nodes', async () => { + const s1 = { + name: 'S1', + typeDefs: gql` + type Query { + user: User + } + + type User @key(fields: "id") { + id: ID! + } + `, + resolvers: { + Query: { + user() { + return { id: '1' }; + }, + }, + }, + }; + + const s2 = { + name: 'S2', + typeDefs: gql` + type User @key(fields: "id") { + id: ID! + hello: String! + } + `, + resolvers: { + User: { + __resolveReference() { + return {}; + }, + hello() { + return 'world'; + }, + }, + }, + }; + + const { serviceMap, schema, queryPlanner } = getFederatedTestingSchema([ + s1, + s2, + ]); + const operation = parseOp( + `#graphql + query($__proto__: Boolean! = true) { + user @include(if: $__proto__) { + hello + } + } + `, + schema, + ); + + const queryPlan = buildPlan(operation, queryPlanner); + expect(queryPlan).toMatchInlineSnapshot(` + QueryPlan { + Include(if: $__proto__) { + Sequence { + Fetch(service: "S1") { + { + user { + __typename + id + } + } + }, + Flatten(path: "user") { + Fetch(service: "S2") { + { + ... on User { + __typename + id + } + } => + { + ... on User { + hello + } + } + }, + }, + } + }, + } + `); + const response = await executePlan( + queryPlan, + operation, + undefined, + schema, + serviceMap, + ); + expect(response.errors).toBeUndefined(); + expect(response.extensions).toBeUndefined(); + expect(response.data).toMatchObject({ + user: { hello: 'world' }, + }); + }); + }); }); diff --git a/gateway-js/src/__tests__/resultShaping.test.ts b/gateway-js/src/__tests__/resultShaping.test.ts index a33602964..b6126c167 100644 --- a/gateway-js/src/__tests__/resultShaping.test.ts +++ b/gateway-js/src/__tests__/resultShaping.test.ts @@ -659,4 +659,70 @@ describe('gateway post-processing', () => { } `); }); + + describe('prototype manipulation prevention', () => { + it('does not allow default variable value collection to change prototypes', () => { + const schema = buildSchemaFromAST(gql` + type Query { + hello(arg: JSON): String! + } + + scalar JSON + `); + + const operation = parseOperation( + schema, + `#graphql + query( + $normalArg: Boolean, + $__proto__: JSON = { normalArg: true } + ) { + hello(arg: $__proto__) @skip(if: $normalArg) + } + `, + ); + + expect( + operation.collectDefaultedVariableValues().normalArg, + ).toBeUndefined(); + }); + + it('sets own property during default variable value collection', () => { + const schema = buildSchemaFromAST(gql` + type Query { + hello: String! + } + `); + + const input = { + skipped: 'world', + included: 'world', + }; + + const operation = parseOperation( + schema, + `#graphql + query($__proto__: Boolean = true) { + skipped: hello @skip(if: $__proto__) + included: hello @include(if: $__proto__) + } + `, + ); + + expect( + computeResponse({ + operation, + input, + introspectionHandling, + }), + ).toMatchInlineSnapshot(` + Object { + "data": Object { + "included": "world", + }, + "errors": Array [], + } + `); + }); + }); }); diff --git a/gateway-js/src/dataRewrites.ts b/gateway-js/src/dataRewrites.ts index fafc8f9c6..18ca0794f 100644 --- a/gateway-js/src/dataRewrites.ts +++ b/gateway-js/src/dataRewrites.ts @@ -1,6 +1,7 @@ import { FetchDataRewrite } from "@apollo/query-planner"; import { assert } from "@apollo/federation-internals"; import { GraphQLSchema, isAbstractType, isInterfaceType, isObjectType } from "graphql"; +import { defineOwn, getOwn } from './utilities/own'; const FRAGMENT_PREFIX = '... on '; @@ -31,13 +32,17 @@ function rewriteAtPathFunction(rewrite: FetchDataRewrite, fieldAtPath: string): switch (rewrite.kind) { case 'ValueSetter': return (obj) => { + defineOwn(obj, fieldAtPath); obj[fieldAtPath] = rewrite.setValueTo; }; case 'KeyRenamer': return (obj) => { - const objAtPath = obj[fieldAtPath]; + const objAtPath = getOwn(obj, fieldAtPath); if (objAtPath) { - obj[rewrite.renameKeyTo] = obj[fieldAtPath]; + defineOwn(obj, rewrite.renameKeyTo); + obj[rewrite.renameKeyTo] = objAtPath; + // No need to define the property here, since it's guaranteed to exist + // by the if clause. obj[fieldAtPath] = undefined; } }; @@ -79,7 +84,7 @@ function applyAtPath(schema: GraphQLSchema, path: string[], value: any, fct: (ob const { kind, value: eltValue } = parsePathElement(first); switch (kind) { case 'fieldName': - applyAtPath(schema, rest, value[eltValue], fct); + applyAtPath(schema, rest, getOwn(value, eltValue), fct); break; case 'typeName': // When we apply rewrites, we don't always have the __typename of all object we would need to, but the code expects that diff --git a/gateway-js/src/executeQueryPlan.ts b/gateway-js/src/executeQueryPlan.ts index 0a6eacfa0..2a8252ba2 100644 --- a/gateway-js/src/executeQueryPlan.ts +++ b/gateway-js/src/executeQueryPlan.ts @@ -30,6 +30,7 @@ import { } from '@apollo/query-planner'; import { deepMerge } from './utilities/deepMerge'; import { isNotNullOrUndefined } from './utilities/array'; +import { getOwn, hasOwn } from './utilities/own'; import { SpanStatusCode } from "@opentelemetry/api"; import { OpenTelemetryConfig, OpenTelemetrySpanNames, recordExceptions, tracer } from "./utilities/opentelemetry"; import { assert, defaultRootName, errorCodeDef, ERRORS, Operation, operationFromDocument, Schema } from '@apollo/federation-internals'; @@ -37,6 +38,7 @@ import { GatewayGraphQLRequestContext, GatewayExecutionResult } from '@apollo/se import { computeResponse } from './resultShaping'; import { applyRewrites, isObjectOfType } from './dataRewrites'; + export type ServiceMap = { [serviceName: string]: GraphQLDataSource; }; @@ -124,7 +126,12 @@ function executeIntrospection( () => `Introspection query for ${JSON.stringify(introspectionSelection)} should not have failed but got ${JSON.stringify(errors)}` ); assert(data, () => `Introspection query for ${JSON.stringify(introspectionSelection)} should not have failed`); - return data[introspectionSelection.alias?.value ?? introspectionSelection.name.value]; + // Note that it's not necessary to call getOwn() here since introspection + // fields can't currently error, and the callsite of executeIntrospection() + // ensures this field won't be skipped. However, if those constraints change + // or this function is reused, then the alias may refer to a non-own property, + // so we defensively use getOwn() here. + return getOwn(data, introspectionSelection.alias?.value ?? introspectionSelection.name.value); } export async function executeQueryPlan( @@ -449,6 +456,7 @@ async function executeFetch( const providedVariables = context.requestContext.request.variables; if ( providedVariables && + hasOwn(providedVariables, variableName) && typeof providedVariables[variableName] !== 'undefined' ) { variables[variableName] = providedVariables[variableName]; @@ -749,7 +757,7 @@ export function generateHydratedPaths( parent.pop(); } } else { - if (head in data) { + if (hasOwn(data, head)) { const value = data[head]; parent.push(head); generateHydratedPaths(parent, path.slice(1), value, result); @@ -786,7 +794,7 @@ function executeSelectionSet( const responseName = getResponseName(selection as QueryPlanFieldNode); const selections = (selection as QueryPlanFieldNode).selections; - if (typeof source[responseName] === 'undefined') { + if (!hasOwn(source, responseName) || typeof source[responseName] === 'undefined') { // This method is called to collect the inputs/requires of a fetch. So, assuming query plans are correct // (and we have not reason to assume otherwise here), all inputs should be fetched beforehand and the // only reason for not finding one of the inputs is that we had an error fetching it _and_ that field @@ -854,7 +862,7 @@ function flattenResultsAtPath(value: ResultCursor['data'] | undefined | null, pa // Note that this typecheck because `value[current]` is of type `any` and so the typechecker "trusts us", but in // practice this only work because we use this on path that do not point to leaf types, and the `value[current]` // is never a base type (non-object nor null/undefined). - return flattenResultsAtPath(value[current], rest); + return flattenResultsAtPath(getOwn(value, current), rest); } } diff --git a/gateway-js/src/resultShaping.ts b/gateway-js/src/resultShaping.ts index f171a5c69..691e325f7 100644 --- a/gateway-js/src/resultShaping.ts +++ b/gateway-js/src/resultShaping.ts @@ -21,6 +21,7 @@ import { } from "@apollo/federation-internals"; import { ResponsePath } from "@apollo/query-planner"; import { GraphQLError } from "graphql"; +import { getOwn } from './utilities/own'; /** * Performs post-query plan execution processing of internally fetched data to produce the final query response. @@ -71,6 +72,7 @@ export function computeResponse({ const parameters = { schema: operation.schema(), variables: { + __proto__: null, ...operation.collectDefaultedVariableValues(), // overwrite any defaulted variables if they are provided ...variables, @@ -178,7 +180,7 @@ function applySelectionSet({ continue; } - let inputValue = input[responseName] ?? null; + let inputValue = getOwn(input, responseName) ?? null; // We handle __typename separately because there is some cases where the internal data may either not have // the value (despite __typename always having a value), or an actually incorrect value that should not be diff --git a/gateway-js/src/utilities/deepMerge.ts b/gateway-js/src/utilities/deepMerge.ts index fb5504a38..039496647 100644 --- a/gateway-js/src/utilities/deepMerge.ts +++ b/gateway-js/src/utilities/deepMerge.ts @@ -1,10 +1,18 @@ +import { defineOwn } from './own'; import { isObject } from './predicates'; export function deepMerge(target: any, source: any): any { if (source === undefined || source === null) return target; for (const key of Object.keys(source)) { - if (source[key] === undefined || key === '__proto__') continue; + if (source[key] === undefined) continue; + + // While `Object.keys()` guarantees that `key` is an own property name in + // `source`, we do not have that same guarantee for `target`. In the event + // the property is absent from `target`, we don't want to accidentally + // fetch the property in `target`'s prototype chain (if present), and + // accordingly we define the property on `target` in such cases. + defineOwn(target, key); if (target[key] && isObject(source[key])) { deepMerge(target[key], source[key]); diff --git a/gateway-js/src/utilities/own.ts b/gateway-js/src/utilities/own.ts new file mode 100644 index 000000000..a00d326fe --- /dev/null +++ b/gateway-js/src/utilities/own.ts @@ -0,0 +1,24 @@ +export function hasOwn(obj: object, prop: PropertyKey): boolean { + return Object.prototype.hasOwnProperty.call(obj, prop) +} + +export function getOwn(obj: T, prop: K): T[K] | undefined { + return Object.prototype.hasOwnProperty.call(obj, prop) + ? obj[prop] + : undefined; +} + +export function defineOwn(obj: object, prop: PropertyKey) { + if (!hasOwn(obj, prop) && prop in obj) { + Object.defineProperty( + obj, + prop, + { + configurable: true, + enumerable: true, + value: undefined, + writable: true, + } + ); + } +} diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index 71660e93a..41c525777 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -1123,7 +1123,7 @@ export class Operation extends DirectiveTargetElement { } collectDefaultedVariableValues(): Record { - const defaultedVariableValues: Record = {}; + const defaultedVariableValues: Record = Object.create(null); for (const { variable, defaultValue } of this.variableDefinitions.definitions()) { if (defaultValue !== undefined) { defaultedVariableValues[variable.name] = defaultValue; diff --git a/query-planner-js/src/conditions.ts b/query-planner-js/src/conditions.ts index aaab4077b..b7d99ee9b 100644 --- a/query-planner-js/src/conditions.ts +++ b/query-planner-js/src/conditions.ts @@ -13,6 +13,7 @@ import { } from "@apollo/federation-internals"; import { extractOperationConditionals } from "@apollo/query-graphs"; import { ConditionNode } from "."; +import { getOwn } from './utilities/own'; export type VariableCondition = { variable: Variable, @@ -240,7 +241,7 @@ export function evaluateCondition( values: Record | undefined, ): boolean { const variable = condition.condition; - let val = values ? values[variable] : undefined; + let val = values ? getOwn(values, variable) : undefined; if (val === undefined) { val = variables.definition(variable)?.defaultValue; } diff --git a/query-planner-js/src/utilities/deepMerge.ts b/query-planner-js/src/utilities/deepMerge.ts index fb5504a38..039496647 100644 --- a/query-planner-js/src/utilities/deepMerge.ts +++ b/query-planner-js/src/utilities/deepMerge.ts @@ -1,10 +1,18 @@ +import { defineOwn } from './own'; import { isObject } from './predicates'; export function deepMerge(target: any, source: any): any { if (source === undefined || source === null) return target; for (const key of Object.keys(source)) { - if (source[key] === undefined || key === '__proto__') continue; + if (source[key] === undefined) continue; + + // While `Object.keys()` guarantees that `key` is an own property name in + // `source`, we do not have that same guarantee for `target`. In the event + // the property is absent from `target`, we don't want to accidentally + // fetch the property in `target`'s prototype chain (if present), and + // accordingly we define the property on `target` in such cases. + defineOwn(target, key); if (target[key] && isObject(source[key])) { deepMerge(target[key], source[key]); diff --git a/query-planner-js/src/utilities/own.ts b/query-planner-js/src/utilities/own.ts new file mode 100644 index 000000000..a00d326fe --- /dev/null +++ b/query-planner-js/src/utilities/own.ts @@ -0,0 +1,24 @@ +export function hasOwn(obj: object, prop: PropertyKey): boolean { + return Object.prototype.hasOwnProperty.call(obj, prop) +} + +export function getOwn(obj: T, prop: K): T[K] | undefined { + return Object.prototype.hasOwnProperty.call(obj, prop) + ? obj[prop] + : undefined; +} + +export function defineOwn(obj: object, prop: PropertyKey) { + if (!hasOwn(obj, prop) && prop in obj) { + Object.defineProperty( + obj, + prop, + { + configurable: true, + enumerable: true, + value: undefined, + writable: true, + } + ); + } +}