Skip to content

Conversation

@jczaplew
Copy link
Contributor

@jczaplew jczaplew commented Mar 1, 2025

Closes #738

There is a GraphQL vulnerability related to allowing an unlimited number of aliases of __typename - an attacker can craft a query that consists of tens of thousands of aliases, like {"query": "query aliasOverLoad { alias0: __typename alias1: __typename alias2: __typename alias3: __typename alias4: __typename alias5: __typename <...thousands more> }"} and can be a way to bypass authentication on the API because it does not hit any resolvers. With enough simultaneous queries like this it can fairly easily bring down a server.

This PR adds ignoreFields to the options of max-aliases and sets the default value to ["__typename"] to ensure backwards compatibility. By passing an empty array the plugin will count aliases of __typename against the max alias limit.

@changeset-bot
Copy link

changeset-bot bot commented Mar 1, 2025

⚠️ No Changeset found

Latest commit: 6700265

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

@jczaplew
Copy link
Contributor Author

jczaplew commented Mar 1, 2025

I'm very open to changes in the nomenclature and specific API here - suggestions are most welcome!

@iCarossio
Copy link
Member

Hey @jczaplew , thanks for contributing!

I am not the creator of the package, but I am here to help! How does this differ from allowList?

@nullswan
Copy link
Contributor

nullswan commented Mar 3, 2025

Hey @jczaplew , thanks for contributing!

I am not the creator of the package, but I am here to help! How does this differ from allowList?

We exclude alias with a __typename from the count

node.name.value !== '__typename' &&

cf @jczaplew coment: #738 (comment)
and Stellate team PR: #468

});
});

it('counts __typename aliases against limit when ignoreFields is passed', async () => {
Copy link
Contributor

@nullswan nullswan Mar 3, 2025

Choose a reason for hiding this comment

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

Additionally, we can add a test to make sure the change is backward and forward compatible by checking if __typename is a default in the ignoreFields list and avoid breaking Stellate workflows.

@jczaplew
Copy link
Contributor Author

jczaplew commented Mar 3, 2025

Hi @iCarossio!

How does this differ from allowList?

Thinking about it some more I don't think it does, and I think this PR would be much cleaner if the default value of allowList was simply ['__typename']. This issue IMO with #468 is that there is no way to opt out of the behavior, and I don't think I realized that that is the purpose of allowList. I've created a different PR #767 that takes this approach. Lemme know what you think!

@LMaxence
Copy link
Member

LMaxence commented Apr 4, 2025

I've created a different PR #767 that takes this approach. Lemme know what you think!

I do prefer the approach you used in #767, is it okay if I close this PR ?

@jczaplew
Copy link
Contributor Author

jczaplew commented Apr 4, 2025

@LMaxence yes, definitely!

@jczaplew jczaplew closed this Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The 'max-aliases' plugin doesn't limit '__typename' aliases

4 participants