Redact private key material in errors#3327
Open
cybele-ripple wants to merge 2 commits intomainfrom
Open
Conversation
When key validation failed, keyError() interpolated the full hex key into the thrown Error.message. For private keys this propagated the secret to application logs, error-tracking services, and HTTP responses that echo error messages. Replace the key with "[redacted]" when type === 'private'. Public keys are still rendered since they are not sensitive. Add a test asserting the private-key hex is absent from the error message and a regression test pinning the redaction contract.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR redacts private key material in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ripple-keypairs/src/utils/getAlgorithmFromKey.ts`:
- Around line 82-88: The error message in getAlgorithmFromKey.ts builds a
template string that redacts the key via keyRepr but still renders
prefixRepr(prefix), which can leak key-derived bytes for private-key errors;
update the return logic in the function (the block that builds the
`invalid_key:` message) to detect when `type === 'private'` and use a redacted
placeholder for the Prefix (e.g., the same `[redacted]` used for `keyRepr`)
instead of calling `prefixRepr(prefix)` so private-key-derived prefixes are
never included in the Error.message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1fc5022b-11bf-489a-bb9d-4f568f8bdd33
📒 Files selected for processing (2)
packages/ripple-keypairs/src/utils/getAlgorithmFromKey.tspackages/ripple-keypairs/test/getAlgorithmFromKey.test.ts
Comment on lines
+82
to
+88
| const keyRepr = type === 'private' ? '[redacted]' : key | ||
|
|
||
| return `invalid_key: | ||
|
|
||
| Type: ${type} | ||
| Key: ${key} | ||
| Prefix: ${prefixRepr(prefix)} | ||
| Key: ${keyRepr} | ||
| Prefix: ${prefixRepr(prefix)} |
Contributor
There was a problem hiding this comment.
Redact private-key-derived prefixes too.
Key: is redacted, but Prefix: can still expose the first byte of an invalid unprefixed private key. For private-key errors, avoid rendering key-derived prefix bytes in Error.message.
🛡️ Proposed fix
const validFormats = getValidFormatsTable(type)
const keyRepr = type === 'private' ? '[redacted]' : key
+ const prefixReprForMessage =
+ type === 'private' ? '[redacted]' : prefixRepr(prefix)
return `invalid_key:
Type: ${type}
Key: ${keyRepr}
-Prefix: ${prefixRepr(prefix)}
+Prefix: ${prefixReprForMessage}
Length: ${len} bytes📝 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
| const keyRepr = type === 'private' ? '[redacted]' : key | |
| return `invalid_key: | |
| Type: ${type} | |
| Key: ${key} | |
| Prefix: ${prefixRepr(prefix)} | |
| Key: ${keyRepr} | |
| Prefix: ${prefixRepr(prefix)} | |
| const keyRepr = type === 'private' ? '[redacted]' : key | |
| const prefixReprForMessage = | |
| type === 'private' ? '[redacted]' : prefixRepr(prefix) | |
| return `invalid_key: | |
| Type: ${type} | |
| Key: ${keyRepr} | |
| Prefix: ${prefixReprForMessage} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ripple-keypairs/src/utils/getAlgorithmFromKey.ts` around lines 82 -
88, The error message in getAlgorithmFromKey.ts builds a template string that
redacts the key via keyRepr but still renders prefixRepr(prefix), which can leak
key-derived bytes for private-key errors; update the return logic in the
function (the block that builds the `invalid_key:` message) to detect when `type
=== 'private'` and use a redacted placeholder for the Prefix (e.g., the same
`[redacted]` used for `keyRepr`) instead of calling `prefixRepr(prefix)` so
private-key-derived prefixes are never included in the Error.message.
The Karma (Jasmine) browser runner doesn't support expect.assertions or Jest-style expect(fn).toThrow(regex) message matching. Replace both with try/catch + expect(thrown).toBeDefined() and .toContain / .not .toContain on the error message, which work identically in Jest and Jasmine. Fixes ripple-keypairs:test:browser failures.
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
1 participant
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.
High Level Overview of Change
Private keys now appear as [redacted] in error messages
Context of Change
When key validation failed, keyError() interpolated the full hex key into the thrown Error.message. For private keys this propagated the secret to application logs, error-tracking services, and HTTP responses that echo error messages.
Replace the key with "[redacted]" when type === 'private'. Public keys are still rendered since they are not sensitive. Add a test asserting the private-key hex is absent from the error message and a regression test pinning the redaction contract.
Type of Change
Did you update HISTORY.md?
Test Plan
Added tests to demonstrate security fix