Conversation
There was a problem hiding this comment.
Pull request overview
Adds regression test coverage to ensure large secrets (e.g., JWT-sized credentials) can be stored and retrieved via the keychain store, including exercising Windows’ Credential Manager blob-size limit behavior.
Changes:
- Add Windows-specific encode/decode roundtrip tests that exceed
maxBlobSizeand validatechunkBlobreassembly. - Add a cross-platform integration test that saves/gets a large JWT credential via the keychain store API.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| store/keychain/keychain_windows_test.go | Adds UTF-16 encode/decode and oversized-blob chunking roundtrip tests on Windows. |
| store/keychain/keychain_test.go | Adds an end-to-end save/get test for a large JWT-like credential to cover storage limits behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
0a87cda to
9fa678c
Compare
There was a problem hiding this comment.
Review Summary
Assessment: 🟢 APPROVE
This PR adds comprehensive test coverage for keychain storage limits, specifically testing large JWT credentials that exceed Windows' 2560-byte blob limit. The tests verify that credentials are properly chunked, stored, retrieved, and reassembled.
The new tests are well-structured and provide valuable coverage for edge cases. I found one minor opportunity to strengthen test validation in the third test case.
Findings
1 LOW severity issue
LOW — Missing chunk validation in third test
The test "roundtrip multiple large JWTs as separate credentials" calls chunkBlob at line 136 but doesn't verify the chunking behavior. Unlike the second test (which includes assert.Greater(t, len(chunks), 1)), this test skips chunk validation entirely and only verifies the final reassembled output.
Consider adding assertions to verify:
len(chunks) > 1to confirm chunking occurred- Each chunk size is
<= maxBlobSizeto validate chunking correctness - Optional: expected chunk count based on the blob size
This would make the test more thorough and catch potential chunking bugs that might still allow correct reassembly.
| } { | ||
| cred := &mocks.MockCredential{Username: tc.username, Password: tc.password} | ||
| blob, err := encodeSecret(cred) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Missing chunk validation in third test
Unlike the second test at line 110 (which includes assert.Greater(t, len(chunks), 1)), this test doesn't verify the chunking behavior. Consider adding assertions to check:
len(chunks) > 1to confirm chunking occurred- Each chunk is
<= maxBlobSizeto validate correctness - Optional: expected chunk count based on blob size
This would strengthen the test and catch potential chunking bugs.
Add additional tests for the keychain store - especially windows. This checks if we can store large blobs in the store.