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.

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

Prevent incorrect result merging when aliasing __typename or id. #10789

Merged
merged 10 commits into from
May 9, 2023
5 changes: 5 additions & 0 deletions .changeset/forty-zebras-smash.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Fix a bug where other fields could be aliased to `__typename` or `id`, in which case an incoming result would be merged into the wrong cache entry.
73 changes: 73 additions & 0 deletions src/cache/inmemory/__tests__/writeToStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3784,4 +3784,77 @@ describe('writing to the store', () => {
});
});
});

test("prevent that a crafted query can overwrite Post:1 with what should be User:5", () => {
const postFragment = gql`
fragment PostFragment on Post {
id
title
}
`;

const cache = new InMemoryCache();
cache.writeFragment({
fragment: postFragment,
data: {
__typename: "Post",
id: "1",
title: "Hello",
},
});

expect(cache.extract()["Post:1"]).toMatchInlineSnapshot(`
Object {
"__typename": "Post",
"id": "1",
"title": "Hello",
}
`);

const injectingQuery = gql`
query ($id: String) {
user(id: $id) {
__typename: firstName
id: lastName
title
ignore: __typename
ignore2: id
}
}
`;
cache.write({
query: injectingQuery,
variables: { id: 5 },
dataId: "ROOT_QUERY",
result: {
user: {
__typename: "Post",
id: "1",
title: "Incorrect!",
ignore: "User",
ignore2: "5",
},
},
});

expect(cache.extract()["Post:1"]).toMatchInlineSnapshot(`
Object {
"__typename": "Post",
"id": "1",
"title": "Hello",
}
`);


expect(cache.extract()["User:5"]).toMatchInlineSnapshot(`
Object {
"__typename": "User",
"firstName": "Post",
"id": "5",
"lastName": "1",
"title": "Incorrect!",
}
`);
});

});
2 changes: 1 addition & 1 deletion src/cache/inmemory/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ export class Policies {
const policy = typename && this.getTypePolicy(typename);
let keyFn = policy && policy.keyFn || this.config.dataIdFromObject;
while (keyFn) {
const specifierOrId = keyFn(object, context);
const specifierOrId = keyFn({...object, ...storeObject}, context);
Copy link
Member

Choose a reason for hiding this comment

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

Great catch @phryneas!

For future reference, I think this should have been keyFn(storeObject, context) from the beginning, but our test suite has a few test cases where properties of a result object were important (like id) even if not mentioned as fields in the query document, so keyFn({...object, ...storeObject}, context) ensures backwards compatibility for that edge case, while always preferring storeObject properties over object properties.

if (isArray(specifierOrId)) {
keyFn = keyFieldsFnFromSpecifier(specifierOrId);
} else {
Expand Down
12 changes: 7 additions & 5 deletions src/utilities/graphql/storeUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -289,16 +289,18 @@ export function getTypenameFromResult(
selectionSet: SelectionSetNode,
fragmentMap?: FragmentMap,
): string | undefined {
if (typeof result.__typename === 'string') {
return result.__typename;
}

for (const selection of selectionSet.selections) {
if (isField(selection)) {
if (selection.name.value === '__typename') {
return result[resultKeyNameFromField(selection)];
}
} else {
}
}
if (typeof result.__typename === 'string') {
return result.__typename;
}
for (const selection of selectionSet.selections) {
if (!isField(selection)) {
Copy link
Member

Choose a reason for hiding this comment

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

I have a feeling getTypenameFromResult is (or could be) hot code, since we need to know the __typename for every result object, and each call might involve recursive fragment traversal.

Instead of iterating over the selections again here, here's one way to avoid the whole second loop when there are no fragments in a given selection set:

diff --git a/src/utilities/graphql/storeUtils.ts b/src/utilities/graphql/storeUtils.ts
index f995b41d9..f2ddec155 100644
--- a/src/utilities/graphql/storeUtils.ts
+++ b/src/utilities/graphql/storeUtils.ts
@@ -18,6 +18,7 @@ import {
   NameNode,
   SelectionSetNode,
   DocumentNode,
+  FragmentSpreadNode,
 } from 'graphql';
 
 import { isNonNullObject } from '../common/objects';
@@ -289,18 +290,26 @@ export function getTypenameFromResult(
   selectionSet: SelectionSetNode,
   fragmentMap?: FragmentMap,
 ): string | undefined {
+  let fragments: undefined | Array<InlineFragmentNode | FragmentSpreadNode>;
+
   for (const selection of selectionSet.selections) {
     if (isField(selection)) {
       if (selection.name.value === '__typename') {
         return result[resultKeyNameFromField(selection)];
       }
+    } else if (fragments) {
+      fragments.push(selection);
+    } else {
+      fragments = [selection];
     }
   }
+
   if (typeof result.__typename === 'string') {
     return result.__typename;
   }
-  for (const selection of selectionSet.selections) {
-    if (!isField(selection)) {
+
+  if (fragments) {
+    for (const selection of fragments) {
       const typename = getTypenameFromResult(
         result,
         getFragmentFromSelection(selection, fragmentMap)!.selectionSet,

const typename = getTypenameFromResult(
result,
getFragmentFromSelection(selection, fragmentMap)!.selectionSet,
Expand Down