Skip to content

Commit

Permalink
Prevent incorrect result merging when aliasing __typename or id. (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
phryneas committed May 9, 2023
1 parent f247c7d commit 23a4e15
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 5 deletions.
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);

This comment has been minimized.

Copy link
@sordev

sordev Feb 8, 2024

I want to understand what exactly is happening here.
When this merge introduced our cache won't behave correctly, We have custom logic to use different id for caching.
And it seems to me that we've checked already in the beginning at line 372 whether we're going to use object or storeObject. But merging like this brings back storeObject it seems and breaks caching. Or am I wrong?

This comment has been minimized.

Copy link
@phryneas

phryneas Feb 9, 2024

Author Member

This is a while ago, and I'd have to dig very deep again to give a 100% sure answer, but from what I remember object did not contain the "real field names", so if you'd alias your field names in your queries, a keyFn would pick up those aliases instead of the real field names.

So a query would be able to do something like __typename: firstName and the keyFn would use firstName instead of __typename, calculating something like Max:23 instead of User:23. This could be used by a crafted query to inject arbitrary objects into the normalized cache into places where they would not belong.

What incorrect behaviour are you seeing now?

This comment has been minimized.

Copy link
@sordev

sordev Feb 9, 2024

Our cache uses custom id from non standard subfield. And it seems to me by doing this is trying to use id from the storeObject whereas we needs custom id thus failing cache. This is again theory since our client is under iframe I can't say for sure 100%. I'll try to replicate the issue some place.

if (isArray(specifierOrId)) {
keyFn = keyFieldsFnFromSpecifier(specifierOrId);
} else {
Expand Down
16 changes: 12 additions & 4 deletions src/utilities/graphql/storeUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
NameNode,
SelectionSetNode,
DocumentNode,
FragmentSpreadNode,
} from 'graphql';

import { isNonNullObject } from '../common/objects';
Expand Down Expand Up @@ -289,16 +290,23 @@ export function getTypenameFromResult(
selectionSet: SelectionSetNode,
fragmentMap?: FragmentMap,
): string | undefined {
if (typeof result.__typename === 'string') {
return result.__typename;
}

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;
}
if (fragments) {
for (const selection of fragments) {
const typename = getTypenameFromResult(
result,
getFragmentFromSelection(selection, fragmentMap)!.selectionSet,
Expand Down

0 comments on commit 23a4e15

Please sign in to comment.