Skip to content

Conversation

@pmathew92
Copy link
Contributor

Changes

This PR fixes the issue caused when the Initialisation Vector (IV) used to encrypt the Credentials in the SecureCredentialsManager gets overwritten with another IV when used along the multiple resource refresh token flow.

Cause

The IV generated while encrypting one set of credentials are stored in the shared preference using a key. When used along with the mrrt flow, the same shared preference key is used to store the new IV which replaces the existing one used to encrypt the first set of credentials. This results in a CryptoException when the new IV is used to decrypt the original set of credentials.

Fix

Instead of storing the IV in the shared preference , The IV is encrypted alongside the Credentials and stored. This ensures all set of different credentials would have their own IV which can later be extracted to decrypt them hence avoiding any exception. The Fix also ensures seamless migration for existing set of stored Credentials using the old manner.

Testing

Manual tests has been conducted for migration scenario and along with multiple set of credentials

  • This change adds unit test coverage

  • This change adds integration test coverage

  • This change has been tested on the latest version of the platform/language or why not

Checklist

@pmathew92
Copy link
Contributor Author

@claude

@github-actions

This comment has been minimized.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

@pmathew92
Copy link
Contributor Author

@claude

@github-actions
Copy link

github-actions bot commented Nov 23, 2025

Claude finished @pmathew92's task —— View job


Code Review Complete

Overall Assessment

❌ This PR addresses a critical IV collision bug in the credential encryption system, but has several security and code quality issues that should be addressed before merging.

Strengths

  • Correctly identifies and fixes the root cause of IV collision in MRRT flow
  • Implements backward compatibility for legacy format credentials
  • Comprehensive test coverage including migration scenarios
  • Well-documented format specification in comments

Critical Issues Requiring Attention

Security Concerns:

  1. Potential integer overflow when casting IV length to byte (line 588)
  2. Insufficient validation of IV length from untrusted input could allow values up to 255
  3. Missing bounds validation in array copy operations could cause crashes with malformed data
  4. Aggressive credential clearing on CryptoException may hide transient errors

Code Quality:

  1. Magic number "2" in length calculations reduces readability
  2. Variable naming inconsistency ("decoded" contains encrypted data)
  3. Test coverage gaps for IV length edge cases (0, 255, 13-15)

Performance:
No significant performance concerns identified. The new format adds minimal overhead (2 bytes per encryption).

Recommendations

The cryptographic approach is sound, but the implementation needs hardening against edge cases and malformed input. Please review the inline comments for specific suggestions to improve input validation, error handling, and code clarity.


@pmathew92 pmathew92 marked this pull request as ready for review November 23, 2025 14:46
@pmathew92 pmathew92 requested a review from a team as a code owner November 23, 2025 14:47
@utkrishtsahu utkrishtsahu self-requested a review November 24, 2025 08:00
@pmathew92 pmathew92 merged commit 854fbc9 into main Nov 24, 2025
15 checks passed
@pmathew92 pmathew92 deleted the mrrt_error branch November 24, 2025 09:54
@pmathew92 pmathew92 mentioned this pull request Nov 24, 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.

5 participants