fix: validate negative offset and limit in graphql resolver#9203
Conversation
dgarros
left a comment
dgarros
left a comment
There was a problem hiding this comment.
Looks good, thank you
Ideally please can you add an entry in the changelog in https://github.com/opsmill/infrahub/tree/stable/changelog to include this fix in the release note
the format is describe here https://github.com/opsmill/infrahub/blob/stable/dev/guidelines/changelog.md
|
Hey, I added the changelog entry in |
There was a problem hiding this comment.
I suppose other resolvers would be affected by this too, the first coming to my mind is backend/infrahub/graphql/resolvers/ipam.py. Considering that, I would extract that tiny logic into a function that resolvers could call. That would also make the logic itself easier to test.
| from infrahub.graphql.resolvers.resolver import default_paginated_list_resolver | ||
|
|
||
|
|
||
| @pytest.mark.anyio |
There was a problem hiding this comment.
No need for those pytest.mark.anyio.
|
|
||
| @pytest.mark.anyio | ||
| async def test_negative_limit_raises() -> None: | ||
| info = MagicMock() |
There was a problem hiding this comment.
We tend to avoid mocks but that seems about right here in a unit test.
| if limit is not None and limit < 0: | ||
| raise GraphQLError("limit must be a non-negative integer") | ||
| if offset is not None and offset < 0: | ||
| raise GraphQLError("offset must be a non-negative integer") |
There was a problem hiding this comment.
If we look at the rest of the function this check comes a bit to late. For instance on line 208 we start a new database session and on line 226 there's a query to the database regarding permissions. So a check like this should be at the start of this function and fail early.
| await default_paginated_list_resolver(root=None, info=info, limit=-1) | ||
|
|
||
|
|
||
| @pytest.mark.anyio |
There was a problem hiding this comment.
These pytest markers should not be needed.
|
|
||
| @pytest.mark.anyio | ||
| async def test_negative_limit_raises() -> None: | ||
| info = MagicMock() |
There was a problem hiding this comment.
While we have mocking in this repo we're trying to phase it out, the reason being that it becomes quite muddy with regards to what is actually being testing when some interfaces doesn't behave as you would expect and with future changes bugs might be introduced when we thought that we had tests to cover a given scenario. If we move the check up as described above we can send in anything to the info parameter. If it's just about making the type checker happy a cast() of None into the correct type could do the trick here. Ideally we should not use cast() within the code either, however in this case it could be fine short of creating a dummy object.
After the change the main problem with having a MagicMock for something like this is that AI agents looking at other tests to figure out the structure can think that it's ok to continue using mocking.
| @@ -0,0 +1 @@ | |||
| Fixed GraphQL resolver returning an unhelpful database error when negative values were provided for limit or offset query arguments. | |||
There was a problem hiding this comment.
Unfortunately this doesn't fix all of the resolvers, could you please update this to say that it fixes the standard resolver for schema based objects?
|
Ah see that both me and @gmazoyer was reviewing at the same type. 😅 |
|
Thx for the input. I've refacto the logic into a tiny validation function, moved the check earlier, cleaned up the tests, and updated the changelog. |
|
The CI issue is not code related |
ogenstad
left a comment
ogenstad
left a comment
There was a problem hiding this comment.
Thank you for your contribution and the changes!
Approving, but also as an internal note as the release notes for Infrahub 1.9.4 has already been generated we should wait for that version to be released before merging this PR so that it gets included in Infrahub 1.9.5 instead.
4 participants
Closes #7474
What
Adds input validation in
default_paginated_list_resolverto reject negativelimitandoffsetvalues with a user-friendlyGraphQLErrorbefore reaching the database layer.Before
Negative values were passed through to Neo4j, resulting in a
CypherSyntaxErrorwith an unhelpful stack trace in the server logs.After
The resolver immediately raises a
GraphQLErrorwith a clear message (limit must be a non-negative integer/offset must be a non-negative integer).Tests
Added unit tests in
backend/tests/unit/graphql/resolvers/test_resolver.pycovering bothlimitandoffsetvalidation.