Skip to content

Add a new GraphQL mutation to sync with app token #202

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michelletran-codecov
Copy link
Collaborator

This change makes the sync task use an app token rather than the user's token. It's a separate mutation, so it shouldn't conflict with the existing sync trigger.

This change makes the sync task use an app token rather than the user's token.
It's a separate mutation, so it shouldn't conflict with the existing sync trigger.
Copy link

seer-by-sentry bot commented Jun 2, 2025

Sentry detected 4 potential issues in your recent changes

Suspicion: The `syncRepos` mutation skips authentication validation but still accesses `self.current_owner`, potentially causing an `AttributeError` for unauthenticated users.
  • Description: The syncRepos GraphQL mutation calls the owner command's trigger_sync method with using_integration=True. This parameter was recently added to trigger_sync to conditionally skip the self.validate() call. However, the subsequent call to RefreshService().trigger_refresh() still attempts to access self.current_owner.ownerid and self.current_owner.username. The BaseInteractor (parent of the owner command) initializes self.current_owner based on the request context, which can be None for unauthenticated users. If an unauthenticated user calls syncRepos, validation is skipped, self.current_owner is None, and accessing self.current_owner.ownerid or self.current_owner.username will raise an AttributeError. This unhandled exception will likely result in a 500 error or unexpected behavior for the user and potentially impact server stability.
  • Code location: apps/codecov-api/graphql_api/types/mutation/sync_repos/sync_repos.py:5~8
  • Suggested fix: Ensure that self.current_owner is not None before accessing its attributes within trigger_sync when using_integration=True, or reconsider skipping validation for this specific use case.
The `syncRepos` GraphQL resolver uses an error handling decorator that does not catch built-in Python exceptions, risking unhandled `KeyError` or `AttributeError` if context access or method calls fail.
  • Description: The @wrap_error_handling_mutation decorator used on GraphQL resolvers is designed to catch custom exceptions.BaseException but explicitly does not catch built-in Python exceptions like KeyError, AttributeError, or TypeError. The new resolve_sync_repos mutation resolver accesses info.context["executor"] and calls get_command("owner") directly without specific try...except blocks for these built-in exceptions. If info.context is missing the "executor" key (KeyError), if info.context["executor"] is None or missing the get_command method (AttributeError), or if get_command fails in a way that raises a built-in exception, these exceptions will propagate unhandled by the decorator. This behavior is confirmed by existing tests for the decorator. Unhandled built-in exceptions in a resolver can lead to unexpected termination of the request handling process.
  • Code location: apps/codecov-api/graphql_api/types/mutation/sync_repos/sync_repos.py:4~8
  • Suggested fix: Add specific try...except blocks within the resolve_sync_repos function to handle potential built-in exceptions like KeyError or AttributeError when accessing info.context or calling methods, or modify the decorator to catch a broader range of exceptions if appropriate for all mutations.
Suspicion of an `AttributeError` in the new `syncRepos` mutation when `self.current_owner` is `None`, potentially causing unexpected behavior due to bypassed validation.
  • Description: The new syncRepos GraphQL mutation in resolve_sync_repos calls command.trigger_sync(using_integration=True). This bypasses the standard validation in TriggerSyncInteractor.execute(). The bypassed code path then directly calls RefreshService().trigger_refresh using self.current_owner.ownerid and self.current_owner.username. The current_owner can be None for unauthenticated requests, as set by middleware. If self.current_owner is None when this code is reached, accessing self.current_owner.ownerid will raise an AttributeError: 'NoneType' object has no attribute 'ownerid'. This would result in a server-side error for the client request. For example, if an unauthenticated user attempts to use this mutation, self.current_owner would be None, leading to the AttributeError when attempting to access its attributes.
  • Code location: apps/codecov-api/graphql_api/types/mutation/sync_repos/sync_repos.py:5~7
  • Suggested fix: Ensure self.current_owner is not None before accessing its attributes within the code path triggered by using_integration=True, or reconsider bypassing validation if an authenticated user is required.
A type annotation change for the token refresh callback suggests a potential mismatch where a dictionary might be passed when an `OauthConsumerToken` is expected, risking runtime errors like `AttributeError` or `KeyError`.
  • Description: The get_token_refresh_callback function's type annotation was changed from Callable[[dict], None] to Callable[[OauthConsumerToken], None] | None. While the API implementation's callback now expects OauthConsumerToken, the worker implementation still expects a dict. The code calling the callback (e.g., in torngit adapters) appears to pass a dictionary. If the callback implementation attempts to access attributes or methods specific to OauthConsumerToken that are not present on a standard dictionary, this will result in a runtime AttributeError or KeyError. Although OauthConsumerToken is a TypedDict and behaves like a dict, relying on this compatibility without explicit handling or ensuring the correct type is passed could lead to unexpected behavior or errors if the callback's usage of the token object changes. This could potentially lead to unhandled exceptions during token refresh operations.
  • Code location: apps/codecov-api/services/repo_providers.py:38
  • Suggested fix: Ensure consistency in the expected parameter type for the token refresh callback across all implementations (API and worker). Update the code that calls the callback to explicitly pass an OauthConsumerToken instance, or adjust the callback implementation to safely handle either type or explicitly check the type.

Did you find this useful? React with a 👍 or 👎

Copy link

codecov bot commented Jun 2, 2025

Codecov Report

Attention: Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.

Project coverage is 94.19%. Comparing base (2db5bcd) to head (4ffc809).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...raphql_api/types/mutation/sync_repos/sync_repos.py 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #202      +/-   ##
==========================================
- Coverage   94.20%   94.19%   -0.01%     
==========================================
  Files        1203     1205       +2     
  Lines       44991    45006      +15     
  Branches     1431     1431              
==========================================
+ Hits        42383    42395      +12     
- Misses       2305     2308       +3     
  Partials      303      303              
Flag Coverage Δ
apiunit 96.38% <84.21%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codecov-notifications
Copy link

codecov-notifications bot commented Jun 2, 2025

Codecov Report

Attention: Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...raphql_api/types/mutation/sync_repos/sync_repos.py 50.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

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.

1 participant