feat(backend): handle Suunto JWT signatures#618
Open
HuziHoic wants to merge 5 commits intothe-momentum:mainfrom
Open
feat(backend): handle Suunto JWT signatures#618HuziHoic wants to merge 5 commits intothe-momentum:mainfrom
HuziHoic wants to merge 5 commits intothe-momentum:mainfrom
Conversation
HuziHoic
requested review from
KaliszS,
bartmichalak and
czajkub
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughVerifies Suunto OAuth JWTs using the provider's Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Suunto OAuth
participant Service as Backend
participant Creds as Credentials Store
participant Redis as Redis State
Client->>Service: OAuth callback (access_token JWT)
Service->>Redis: fetch state by oauth_state
Service->>Creds: request client_id & client_secret
Creds-->>Service: return client_id, client_secret
Service->>Service: decode & verify JWT (HS256, aud=client_id)
alt verification success
Service->>Service: extract sub as user_id, user as username
Service->>Redis: persist linked user
else verification failure
Service-->>Service: raise ValueError("JWT signature verification failed")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/providers/suunto/test_suunto_oauth.py (1)
132-144:⚠️ Potential issue | 🟠 MajorTest missing credentials mock - behavior depends on environment.
This test doesn't mock
credentials, so it relies on real settings. Ifclient_idorclient_secretare empty in the test environment,_get_provider_user_inforeturns{"user_id": None, "username": None}instead of raisingValueError, causing the test to fail unexpectedly.🐛 Proposed fix: add credentials mock
def test_extract_user_info_from_invalid_jwt(self, suunto_oauth: SuuntoOAuth) -> None: """Should raise ValueError for invalid JWT.""" # Arrange token_response = OAuthTokenResponse( access_token="invalid.jwt.token", refresh_token="test_refresh_token", expires_in=3600, token_type="Bearer", ) # Act - with pytest.raises(ValueError, match="JWT signature verification failed"): - suunto_oauth._get_provider_user_info(token_response, "test_user_id") + with patch.object(type(suunto_oauth), "credentials", new_callable=PropertyMock) as mock_credentials: + mock_credentials.return_value.client_secret = "test_secret" + mock_credentials.return_value.client_id = "test_client_id" + with pytest.raises(ValueError, match="JWT signature verification failed"): + suunto_oauth._get_provider_user_info(token_response, "test_user_id")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/providers/suunto/test_suunto_oauth.py` around lines 132 - 144, The test test_extract_user_info_from_invalid_jwt relies on actual credentials and can return early; update the test to mock valid non-empty credentials so SuuntoOAuth._get_provider_user_info attempts JWT verification and raises the expected ValueError: ensure the test supplies or patches the SuuntoOAuth instance (or its credentials property) with a valid client_id and client_secret before calling _get_provider_user_info with the OAuthTokenResponse access_token "invalid.jwt.token" so pytest.raises(ValueError, match="JWT signature verification failed") reliably triggers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/providers/suunto/oauth.py`:
- Around line 34-35: The current check returns {"user_id": None, "username":
None} when credentials.client_secret or credentials.client_id are missing, which
silently allows a broken OAuth flow; update this branch to fail fast by either
logging a clear warning and then raising an exception or directly raising a
ValueError; specifically replace the return of {"user_id": None, "username":
None} with raising ValueError("Suunto OAuth credentials not configured") (or log
via your logger before raising) so credentials.client_secret and
credentials.client_id misconfiguration is surfaced immediately.
In `@backend/tests/providers/suunto/test_suunto_oauth.py`:
- Around line 112-116: The test's JWT lacks the 'aud' claim so audience
validation fails before the intended missing-field checks; update the
test_payload (used with jwt.encode and test_secret) to include an 'aud' set to
the same audience value your validator expects (the constant or client_id used
in the code under test) while still omitting 'sub' and 'user', so the token
passes audience validation and the test actually exercises the missing-field
logic.
- Around line 84-89: The test JWT payload created in test_suunto_oauth.py
(test_payload used to build access_token via jwt.encode) lacks the "aud" claim,
which causes jwt.decode(..., audience=credentials.client_id) in oauth.py to
raise JWTClaimsError; update the test_payload to include an "aud" value that
matches the client id used by the code under test (e.g., the test's client id
variable or credentials.client_id) so the audience check passes when jwt.decode
is called.
---
Outside diff comments:
In `@backend/tests/providers/suunto/test_suunto_oauth.py`:
- Around line 132-144: The test test_extract_user_info_from_invalid_jwt relies
on actual credentials and can return early; update the test to mock valid
non-empty credentials so SuuntoOAuth._get_provider_user_info attempts JWT
verification and raises the expected ValueError: ensure the test supplies or
patches the SuuntoOAuth instance (or its credentials property) with a valid
client_id and client_secret before calling _get_provider_user_info with the
OAuthTokenResponse access_token "invalid.jwt.token" so pytest.raises(ValueError,
match="JWT signature verification failed") reliably triggers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b378cd94-d738-4629-b8fe-4e8c0fa92571
📒 Files selected for processing (3)
backend/app/services/providers/suunto/oauth.pybackend/tests/integrations/test_suunto_import.pybackend/tests/providers/suunto/test_suunto_oauth.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/providers/suunto/test_suunto_oauth.py (1)
134-146:⚠️ Potential issue | 🟠 MajorMissing credentials mock causes unreliable test behavior.
This test doesn't mock
credentials, unliketest_extract_user_info_from_jwt_successandtest_extract_user_info_from_jwt_missing_fields. Looking at the implementation inoauth.py:if not credentials.client_secret or not credentials.client_id: return {"user_id": None, "username": None}If
suunto_client_id/suunto_client_secretare not configured in the test environment (typical), the function returns early withNonevalues instead of attempting JWT decode, causing the test to fail with an unexpected result rather than aValueError.🐛 Proposed fix: add credentials mock for consistency
def test_extract_user_info_from_invalid_jwt(self, suunto_oauth: SuuntoOAuth) -> None: """Should raise ValueError for invalid JWT.""" # Arrange + test_secret = "test_secret" token_response = OAuthTokenResponse( access_token="invalid.jwt.token", refresh_token="test_refresh_token", expires_in=3600, token_type="Bearer", ) # Act - with pytest.raises(ValueError, match="JWT signature verification failed"): - suunto_oauth._get_provider_user_info(token_response, "test_user_id") + with patch.object(type(suunto_oauth), "credentials", new_callable=PropertyMock) as mock_credentials: + mock_credentials.return_value.client_secret = test_secret + mock_credentials.return_value.client_id = "test_client_id" + with pytest.raises(ValueError, match="JWT signature verification failed"): + suunto_oauth._get_provider_user_info(token_response, "test_user_id")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/providers/suunto/test_suunto_oauth.py` around lines 134 - 146, The test test_extract_user_info_from_invalid_jwt fails to reliably exercise _get_provider_user_info because it doesn't mock credentials (suunto_client_id/suunto_client_secret) so the function may return early; update the test to mock credentials the same way as test_extract_user_info_from_jwt_success and test_extract_user_info_from_jwt_missing_fields (ensure credentials.client_id and credentials.client_secret are set) before calling suunto_oauth._get_provider_user_info so the JWT decode path runs and the ValueError for signature verification is raised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/tests/providers/suunto/test_suunto_oauth.py`:
- Around line 134-146: The test test_extract_user_info_from_invalid_jwt fails to
reliably exercise _get_provider_user_info because it doesn't mock credentials
(suunto_client_id/suunto_client_secret) so the function may return early; update
the test to mock credentials the same way as
test_extract_user_info_from_jwt_success and
test_extract_user_info_from_jwt_missing_fields (ensure credentials.client_id and
credentials.client_secret are set) before calling
suunto_oauth._get_provider_user_info so the JWT decode path runs and the
ValueError for signature verification is raised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 612bda34-b195-4bdf-807c-abbfff5c4106
📒 Files selected for processing (1)
backend/tests/providers/suunto/test_suunto_oauth.py
KaliszS
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/providers/suunto/test_suunto_oauth.py (1)
100-103: Extract repeated mocked-credentials setup into a helper.This setup is duplicated three times; a small helper will reduce maintenance friction.
♻️ Suggested refactor
class TestSuuntoOAuth: """Test suite for SuuntoOAuth.""" + + `@staticmethod` + def _set_mock_credentials( + mock_credentials: PropertyMock, client_secret: str = "test_secret", client_id: str = "test_client_id" + ) -> None: + mock_credentials.return_value.client_secret = client_secret + mock_credentials.return_value.client_id = client_id @@ with patch.object(type(suunto_oauth), "credentials", new_callable=PropertyMock) as mock_credentials: - mock_credentials.return_value.client_secret = test_secret - mock_credentials.return_value.client_id = "test_client_id" + self._set_mock_credentials(mock_credentials, client_secret=test_secret, client_id="test_client_id") user_info = suunto_oauth._get_provider_user_info(token_response, "test_user_id") @@ with patch.object(type(suunto_oauth), "credentials", new_callable=PropertyMock) as mock_credentials: - mock_credentials.return_value.client_secret = test_secret - mock_credentials.return_value.client_id = "test_client_id" + self._set_mock_credentials(mock_credentials, client_secret=test_secret, client_id="test_client_id") with pytest.raises(ValueError, match="JWT signature verification failed"): suunto_oauth._get_provider_user_info(token_response, "test_user_id") @@ with patch.object(type(suunto_oauth), "credentials", new_callable=PropertyMock) as mock_credentials: - mock_credentials.return_value.client_secret = "test_secret" - mock_credentials.return_value.client_id = "test_client_id" + self._set_mock_credentials(mock_credentials) with pytest.raises(ValueError, match="JWT signature verification failed"): suunto_oauth._get_provider_user_info(token_response, "test_user_id")Also applies to: 128-131, 144-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/providers/suunto/test_suunto_oauth.py` around lines 100 - 103, Extract the repeated PropertyMock setup for suunto_oauth.credentials into a single helper to reduce duplication: create a helper (e.g., setup_mock_credentials or a pytest fixture) that accepts the suunto_oauth object and secret/client_id values and applies patch.object(type(suunto_oauth), "credentials", new_callable=PropertyMock) and sets mock.return_value.client_secret and mock.return_value.client_id; then replace the three duplicated blocks around calls to suunto_oauth._get_provider_user_info with calls to that helper to keep tests DRY and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/tests/providers/suunto/test_suunto_oauth.py`:
- Around line 100-103: Extract the repeated PropertyMock setup for
suunto_oauth.credentials into a single helper to reduce duplication: create a
helper (e.g., setup_mock_credentials or a pytest fixture) that accepts the
suunto_oauth object and secret/client_id values and applies
patch.object(type(suunto_oauth), "credentials", new_callable=PropertyMock) and
sets mock.return_value.client_secret and mock.return_value.client_id; then
replace the three duplicated blocks around calls to
suunto_oauth._get_provider_user_info with calls to that helper to keep tests DRY
and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73944783-0f43-4804-b5e6-13c896d6fe03
📒 Files selected for processing (1)
backend/tests/providers/suunto/test_suunto_oauth.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/providers/suunto/test_suunto_oauth.py (1)
83-103: Consider extracting shared JWT/credentials test setup into a helper fixture.
test_secret,test_client_id, token creation, andPropertyMockcredentials patching are repeated in multiple tests; a fixture/helper would reduce duplication and future drift.Also applies to: 111-132, 144-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/providers/suunto/test_suunto_oauth.py` around lines 83 - 103, Extract the repeated JWT and credentials setup into a reusable pytest fixture (e.g., suunto_jwt_fixture) that builds test_secret, test_client_id, creates the signed access_token (jwt.encode with HS256) and returns an OAuthTokenResponse plus the raw payload values; update tests that currently create test_secret/test_payload/access_token and patch type(suunto_oauth).credentials via PropertyMock to instead use the fixture and a small per-test override when needed, and keep calls to suunto_oauth._get_provider_user_info and references to OAuthTokenResponse, PropertyMock and suunto_oauth unchanged so locating changes is trivial.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/tests/providers/suunto/test_suunto_oauth.py`:
- Around line 83-103: Extract the repeated JWT and credentials setup into a
reusable pytest fixture (e.g., suunto_jwt_fixture) that builds test_secret,
test_client_id, creates the signed access_token (jwt.encode with HS256) and
returns an OAuthTokenResponse plus the raw payload values; update tests that
currently create test_secret/test_payload/access_token and patch
type(suunto_oauth).credentials via PropertyMock to instead use the fixture and a
small per-test override when needed, and keep calls to
suunto_oauth._get_provider_user_info and references to OAuthTokenResponse,
PropertyMock and suunto_oauth unchanged so locating changes is trivial.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cab99366-ca59-4e30-a8b0-108deff20d8f
📒 Files selected for processing (3)
backend/app/services/providers/suunto/oauth.pybackend/tests/integrations/test_suunto_import.pybackend/tests/providers/suunto/test_suunto_oauth.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/services/providers/suunto/oauth.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/services/providers/suunto/oauth.py (1)
39-39: Move logger to module level.Creating the logger inside the method on every call is unnecessary overhead. Define it once at module level.
♻️ Proposed fix
Add at module level (after imports):
logger = logging.getLogger(__name__)Then remove line 39 and use the module-level
loggerdirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/providers/suunto/oauth.py` at line 39, Move the logger creation out of the function: add a module-level declaration logger = logging.getLogger(__name__) immediately after the imports, then remove the local logger = logging.getLogger(__name__) inside the function (the instance created on line 39) and update the function to use the module-level logger variable instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/providers/suunto/oauth.py`:
- Around line 54-55: The current check that raises ValueError when
decoded.get("sub") is falsy uses a misleading message ("JWT signature
verification failed"); update the exception message in the block that checks
decoded.get("sub") to accurately state the problem (e.g., raise
ValueError("Missing required 'sub' claim in JWT") or similar) and optionally
include the decoded payload or claim name for debugging; locate the check on
decoded.get("sub") in backend/app/services/providers/suunto/oauth.py to make the
change.
---
Nitpick comments:
In `@backend/app/services/providers/suunto/oauth.py`:
- Line 39: Move the logger creation out of the function: add a module-level
declaration logger = logging.getLogger(__name__) immediately after the imports,
then remove the local logger = logging.getLogger(__name__) inside the function
(the instance created on line 39) and update the function to use the
module-level logger variable instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f73d4ea-5f40-4315-95f0-0a9dbc44e58c
📒 Files selected for processing (1)
backend/app/services/providers/suunto/oauth.py
Comment on lines
+54
to
+55
| if not decoded.get("sub"): | ||
| raise ValueError("JWT signature verification failed") |
There was a problem hiding this comment.
Misleading error message for missing claim.
When sub is absent, the JWT signature was actually verified successfully—only the required claim is missing. Consider a more accurate message.
🔧 Suggested fix
if not decoded.get("sub"):
- raise ValueError("JWT signature verification failed")
+ raise ValueError("JWT missing required 'sub' claim")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not decoded.get("sub"): | |
| raise ValueError("JWT signature verification failed") | |
| if not decoded.get("sub"): | |
| raise ValueError("JWT missing required 'sub' claim") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/providers/suunto/oauth.py` around lines 54 - 55, The
current check that raises ValueError when decoded.get("sub") is falsy uses a
misleading message ("JWT signature verification failed"); update the exception
message in the block that checks decoded.get("sub") to accurately state the
problem (e.g., raise ValueError("Missing required 'sub' claim in JWT") or
similar) and optionally include the decoded payload or claim name for debugging;
locate the check on decoded.get("sub") in
backend/app/services/providers/suunto/oauth.py to make the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #511
Description
Fixes a security vulnerability in the Suunto OAuth integration where JWT tokens were being accepted without signature verification.
Checklist
General
Backend Changes
You have to be in
backenddirectory to make it work:uv run pre-commit run --all-filespassesFrontend Changes
pnpm run lintpassespnpm run format:checkpassespnpm run buildsucceedsTesting Instructions
Steps to test:
Expected behavior:
Screenshots
Additional Notes
Summary by CodeRabbit
Bug Fixes
Tests