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

[Data masking] Fix bug where masked field could be returned in parent query #12014

Merged
merged 6 commits into from
Aug 22, 2024

Conversation

jerelmiller
Copy link
Member

Because we try and retain object identities as much as possible when running the data masking algorithm, this causes an issue where would-be masked fields are returned in the query when a named fragment selects additional fields on the same child selection as the query.

For example, take this query:

query {
  user {
    profile {
      id
    }
    ...UserFields
  }
}

fragment UserFields on User {
  profile {
    id
    age
  }
}

Here the parent asks only for the id on the profile field while the UserFields fragment selects age on profile as well. Prior to this fix, both id and age were returned as part of the parent query even though age should have been masked. This fix ensures age would be masked here as expected.

@jerelmiller jerelmiller added 🐞 bug 🎭 data-masking Issues/PRs related to data masking labels Aug 21, 2024
Copy link

changeset-bot bot commented Aug 21, 2024

⚠️ No Changeset found

Latest commit: 6a2a365

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@jerelmiller jerelmiller changed the base branch from main to data-masking August 21, 2024 18:13
Copy link
Contributor

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 39.89 KB (+1.44% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 49.05 KB (+2.2% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 46.18 KB (+1.34% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 35.45 KB (+3.05% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 32.88 KB (+1.87% 🔺)
import { ApolloProvider } from "dist/react/index.js" 1.26 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production) 1.24 KB (+0.16% 🔺)
import { useQuery } from "dist/react/index.js" 5.21 KB (-0.02% 🔽)
import { useQuery } from "dist/react/index.js" (production) 4.3 KB (+0.03% 🔺)
import { useLazyQuery } from "dist/react/index.js" 5.7 KB (+0.07% 🔺)
import { useLazyQuery } from "dist/react/index.js" (production) 4.77 KB (+0.03% 🔺)
import { useMutation } from "dist/react/index.js" 3.62 KB (+0.03% 🔺)
import { useMutation } from "dist/react/index.js" (production) 2.84 KB (+0.04% 🔺)
import { useSubscription } from "dist/react/index.js" 4.41 KB (+0.05% 🔺)
import { useSubscription } from "dist/react/index.js" (production) 3.46 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 5.49 KB (+0.02% 🔺)
import { useSuspenseQuery } from "dist/react/index.js" (production) 4.15 KB (+0.03% 🔺)
import { useBackgroundQuery } from "dist/react/index.js" 4.99 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.64 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" 5.07 KB (+0.04% 🔺)
import { useLoadableQuery } from "dist/react/index.js" (production) 3.72 KB (+0.08% 🔺)
import { useReadQuery } from "dist/react/index.js" 3.39 KB (+0.06% 🔺)
import { useReadQuery } from "dist/react/index.js" (production) 3.33 KB (0%)
import { useFragment } from "dist/react/index.js" 2.32 KB (0%)
import { useFragment } from "dist/react/index.js" (production) 2.27 KB (+0.05% 🔺)

Copy link

netlify bot commented Aug 21, 2024

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit 9a8ef88
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/66c62e5575e1e20008b56e07
😎 Deploy Preview https://deploy-preview-12014--apollo-client-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines 162 to 167
if (
!childChanged &&
Object.keys(masked).length !== Object.keys(data[keyName]).length
) {
childChanged = true;
}
Copy link
Member

@phryneas phryneas Aug 22, 2024

Choose a reason for hiding this comment

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

Suggested change
if (
!childChanged &&
Object.keys(masked).length !== Object.keys(data[keyName]).length
) {
childChanged = true;
}
childChanged ||= Object.keys(masked).length !== Object.keys(data[keyName]).length;

Just a suggestion, though - whatever you like better :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with your other suggestion. Thanks a bunch!

) {
childChanged = true;
}

if (childChanged) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (childChanged) {
if (childChanged || Object.keys(masked).length !== Object.keys(data[keyName]).length) {

or maybe this. Both of these should save a bunch of bytes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh ya I like this one a lot. Don't know why I didn't think of this initially 🤦. Great suggestion!

Copy link
Member Author

@jerelmiller jerelmiller Aug 22, 2024

Choose a reason for hiding this comment

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

@jerelmiller jerelmiller force-pushed the jerel/fix-masking-bug branch from efc0e64 to f5b021a Compare August 22, 2024 14:44
@jerelmiller jerelmiller merged commit 8e73c32 into data-masking Aug 22, 2024
36 checks passed
@jerelmiller jerelmiller deleted the jerel/fix-masking-bug branch August 22, 2024 15:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐞 bug 🎭 data-masking Issues/PRs related to data masking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants