Add metadata, filtering, and message formatting features#6
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces a new public API function Changes
Sequence DiagramsequenceDiagram
participant Caller
participant formatDeclineMessage
participant getDeclineMessage
participant Result
Caller->>formatDeclineMessage: formatDeclineMessage(code, locale, variables)
formatDeclineMessage->>getDeclineMessage: getDeclineMessage(code, locale)
getDeclineMessage-->>formatDeclineMessage: base message or undefined
alt message not found
formatDeclineMessage-->>Result: undefined
else message found & no variables
formatDeclineMessage-->>Result: base message
else message found & variables provided
formatDeclineMessage->>formatDeclineMessage: replace {{key}} placeholders
formatDeclineMessage-->>Result: formatted message
end
Result-->>Caller: final result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @hideokamoto, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality by adding metadata to decline codes, along with filtering functions and a message formatting utility. The implementation is clean and well-documented in the README. The new types and data are well-structured.
My review focuses on two main points:
- A potential vulnerability in the
formatDeclineMessagefunction where using a regular expression with an unescaped key could lead to unexpected behavior. I've suggested a safer alternative. - A gap in test coverage for the new
formatDeclineMessagefunction. The existing test does not actually verify the variable replacement logic, leaving a key part of the new feature untested.
Overall, this is a great addition to the library. Addressing these points will improve the robustness and reliability of the new features.
| it('should replace variables in message template', () => { | ||
| const message = formatDeclineMessage('insufficient_funds', 'en', { | ||
| merchantName: 'Acme Store', | ||
| }); | ||
| // Base message doesn't have placeholders, so should remain unchanged | ||
| expect(message).toBe('Please try again using an alternative payment method.'); | ||
| }); |
There was a problem hiding this comment.
This test case's title is misleading. It claims to test variable replacement, but the message for insufficient_funds has no placeholders. As a result, the test only verifies that the function doesn't alter a string with no placeholders, leaving the actual replacement logic untested. This is a significant gap in test coverage for the new formatDeclineMessage feature.
I recommend two changes:
- Rename this test to reflect what it actually does, for example:
it('should return the base message if no placeholders are present in the template'). - Add a new test case that properly verifies the variable replacement. Since there are no messages with placeholders in the current dataset, you might need to temporarily add one to a decline code for testing purposes, or mock the
getDeclineMessagefunction to return a templated string.
Example of a new, effective test:
it('should correctly replace placeholders in a message template', () => {
// This test requires a decline code whose message is a template string.
// For example, if 'generic_decline' had a message "Action required for {{merchantName}}."
const message = formatDeclineMessage('code_with_template', 'en', {
merchantName: 'Acme Store',
});
expect(message).toBe('Action required for Acme Store.');
});| const placeholder = new RegExp(`\\{\\{${key}\\}\\}`, 'g'); | ||
| formattedMessage = formattedMessage.replace(placeholder, value); |
There was a problem hiding this comment.
Constructing a regular expression from a variable key can be unsafe if the key contains special regex characters (e.g., ., *, +). This could lead to incorrect replacements or even break the regex. A safer and more modern approach is to use String.prototype.replaceAll(), which handles this correctly without needing a regular expression for simple string substitutions.
| const placeholder = new RegExp(`\\{\\{${key}\\}\\}`, 'g'); | |
| formattedMessage = formattedMessage.replace(placeholder, value); | |
| formattedMessage = formattedMessage.replaceAll(`{{${key}}}`, value); | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/data/decline-codes.ts (1)
460-465: Consider the semantics ofrequiresUserActionfor merchant_blacklist.The
merchant_blacklistdecline hasrequiresUserAction: false, yet thenextUserActioninstructs users to contact their card issuer. This creates a semantic ambiguity: doesrequiresUserActionmean "the user can resolve this" or "the user should take action"?For merchant_blacklist, the user cannot directly resolve the block (only the merchant can update their blocklist), so
falsemay be correct. However, consider documenting this semantic distinction to clarify thatrequiresUserAction: falsemeans "user cannot directly resolve" rather than "no action message exists."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(5 hunks)src/data/decline-codes.ts(43 hunks)src/index.ts(2 hunks)src/types.ts(2 hunks)tests/index.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/types.ts (1)
src/index.ts (3)
DeclineCodeCategory(255-255)DeclineSeverity(259-259)DeclineCodeMetadata(257-257)
tests/index.test.ts (1)
src/index.ts (6)
getDeclineCodeMetadata(134-140)getDeclineCodesByCategory(155-157)getDeclineCodesBySeverity(172-174)getRetryableDeclineCodes(188-190)getDeclineCodesRequiringUserAction(203-205)formatDeclineMessage(225-248)
src/index.ts (1)
src/types.ts (5)
DeclineCodeMetadata(34-43)DeclineCodeCategory(17-24)DeclineCode(74-117)DeclineSeverity(29-29)Locale(4-4)
🪛 ast-grep (0.40.0)
src/index.ts
[warning] 242-242: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\{\\{${key}\\}\\}, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (7)
src/types.ts (1)
14-59: LGTM! Well-structured type definitions.The new metadata types are clearly defined with helpful inline documentation. The extension of
DeclineCodeInfoto include metadata integrates smoothly with the existing structure.src/data/decline-codes.ts (1)
28-786: Comprehensive metadata coverage looks good.The metadata has been systematically added to all 44 decline codes with thoughtful categorization. The assignments generally align well with the decline code semantics (e.g., authentication errors are retryable, insufficient funds is high severity and not retryable).
README.md (1)
18-302: Excellent documentation updates.The README has been thoroughly updated with clear examples for all new features. The reorganized API Reference with dedicated sections for metadata, filtering, and formatting functions makes the documentation easy to navigate. The usage examples are practical and demonstrate real-world integration patterns.
tests/index.test.ts (1)
119-267: Comprehensive test coverage for new features.The test suite thoroughly covers all new metadata and filtering functions with appropriate edge case handling (invalid codes, undefined returns, etc.). Good practice testing both positive and negative cases.
src/index.ts (3)
116-140: LGTM! Clean metadata accessor.The
getDeclineCodeMetadatafunction is straightforward with proper validation and clear JSDoc documentation.
142-205: Well-implemented filtering functions.All four filtering functions (
getDeclineCodesByCategory,getDeclineCodesBySeverity,getRetryableDeclineCodes,getDeclineCodesRequiringUserAction) follow a consistent pattern with clear documentation and examples.
253-262: LGTM! Type exports properly expanded.The type exports correctly include all new public types:
DeclineCodeCategory,DeclineCodeMetadata, andDeclineSeverity.
| export function formatDeclineMessage( | ||
| declineCode: string, | ||
| locale: Locale = 'en', | ||
| variables?: Record<string, string>, | ||
| ): string | undefined { | ||
| const baseMessage = getDeclineMessage(declineCode, locale); | ||
|
|
||
| if (!baseMessage) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if (!variables) { | ||
| return baseMessage; | ||
| } | ||
|
|
||
| // Replace variables in the format {{variableName}} | ||
| let formattedMessage = baseMessage; | ||
| for (const [key, value] of Object.entries(variables)) { | ||
| const placeholder = new RegExp(`\\{\\{${key}\\}\\}`, 'g'); | ||
| formattedMessage = formattedMessage.replace(placeholder, value); | ||
| } | ||
|
|
||
| return formattedMessage; | ||
| } |
There was a problem hiding this comment.
Address potential ReDoS vulnerability in regex construction.
The function constructs a regular expression from user-supplied variable keys without sanitization (line 243). While the keys come from the variables object keys rather than values, a malicious caller could still provide keys with regex special characters or catastrophic backtracking patterns, leading to Regular Expression Denial of Service (ReDoS).
Recommended fix: Escape the key before constructing the regex:
// Replace variables in the format {{variableName}}
let formattedMessage = baseMessage;
for (const [key, value] of Object.entries(variables)) {
- const placeholder = new RegExp(`\\{\\{${key}\\}\\}`, 'g');
+ const escapedKey = key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+ const placeholder = new RegExp(`\\{\\{${escapedKey}\\}\\}`, 'g');
formattedMessage = formattedMessage.replace(placeholder, value);
}Alternatively, use a simpler string replacement approach:
// Replace variables in the format {{variableName}}
let formattedMessage = baseMessage;
for (const [key, value] of Object.entries(variables)) {
- const placeholder = new RegExp(`\\{\\{${key}\\}\\}`, 'g');
- formattedMessage = formattedMessage.replace(placeholder, value);
+ formattedMessage = formattedMessage.replaceAll(`{{${key}}}`, value);
}📝 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.
| export function formatDeclineMessage( | |
| declineCode: string, | |
| locale: Locale = 'en', | |
| variables?: Record<string, string>, | |
| ): string | undefined { | |
| const baseMessage = getDeclineMessage(declineCode, locale); | |
| if (!baseMessage) { | |
| return undefined; | |
| } | |
| if (!variables) { | |
| return baseMessage; | |
| } | |
| // Replace variables in the format {{variableName}} | |
| let formattedMessage = baseMessage; | |
| for (const [key, value] of Object.entries(variables)) { | |
| const placeholder = new RegExp(`\\{\\{${key}\\}\\}`, 'g'); | |
| formattedMessage = formattedMessage.replace(placeholder, value); | |
| } | |
| return formattedMessage; | |
| } | |
| export function formatDeclineMessage( | |
| declineCode: string, | |
| locale: Locale = 'en', | |
| variables?: Record<string, string>, | |
| ): string | undefined { | |
| const baseMessage = getDeclineMessage(declineCode, locale); | |
| if (!baseMessage) { | |
| return undefined; | |
| } | |
| if (!variables) { | |
| return baseMessage; | |
| } | |
| // Replace variables in the format {{variableName}} | |
| let formattedMessage = baseMessage; | |
| for (const [key, value] of Object.entries(variables)) { | |
| const escapedKey = key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| const placeholder = new RegExp(`\\{\\{${escapedKey}\\}\\}`, 'g'); | |
| formattedMessage = formattedMessage.replace(placeholder, value); | |
| } | |
| return formattedMessage; | |
| } |
| export function formatDeclineMessage( | |
| declineCode: string, | |
| locale: Locale = 'en', | |
| variables?: Record<string, string>, | |
| ): string | undefined { | |
| const baseMessage = getDeclineMessage(declineCode, locale); | |
| if (!baseMessage) { | |
| return undefined; | |
| } | |
| if (!variables) { | |
| return baseMessage; | |
| } | |
| // Replace variables in the format {{variableName}} | |
| let formattedMessage = baseMessage; | |
| for (const [key, value] of Object.entries(variables)) { | |
| const placeholder = new RegExp(`\\{\\{${key}\\}\\}`, 'g'); | |
| formattedMessage = formattedMessage.replace(placeholder, value); | |
| } | |
| return formattedMessage; | |
| } | |
| export function formatDeclineMessage( | |
| declineCode: string, | |
| locale: Locale = 'en', | |
| variables?: Record<string, string>, | |
| ): string | undefined { | |
| const baseMessage = getDeclineMessage(declineCode, locale); | |
| if (!baseMessage) { | |
| return undefined; | |
| } | |
| if (!variables) { | |
| return baseMessage; | |
| } | |
| // Replace variables in the format {{variableName}} | |
| let formattedMessage = baseMessage; | |
| for (const [key, value] of Object.entries(variables)) { | |
| formattedMessage = formattedMessage.replaceAll(`{{${key}}}`, value); | |
| } | |
| return formattedMessage; | |
| } |
🧰 Tools
🪛 ast-grep (0.40.0)
[warning] 242-242: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\{\\{${key}\\}\\}, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In src/index.ts around lines 225 to 248, the code builds a RegExp from variable
keys (`new RegExp(\`\\{\\{${key}\\}\\}\`, 'g')`) which can be abused to cause
ReDoS; fix this by escaping any regex metacharacters in `key` before
constructing the RegExp (implement a small escape function that replaces
characters like [\^$.|?*+(){}\\] with backslash-escaped versions and use that
escapedKey in the pattern), or replace the regex approach entirely with a safe
string-based replacement (e.g., construct the placeholder string `{{key}}` and
use split/join or indexOf/substring replacement to perform global replacements)
so user-supplied keys cannot inject regex patterns.
This commit adds only Stripe-official data based features: - formatDeclineMessage() for custom message templates with variable substitution - Comprehensive tests for message formatting - Updated README with usage examples Note: Removed previously added metadata features (category, retryable, severity, requiresUserAction) as they are not part of official Stripe API and could be considered unreliable. Focus on providing official Stripe decline code data with localization support.
hideokamoto
force-pushed
the
claude/rename-library-features-01L8GCPmpRaFDSeXQmzE7VCa
branch
from
d42c56f to
d9a63d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/index.ts (1)
145-146: Address the ReDoS vulnerability flagged in previous reviews.This regex construction issue has already been identified by previous reviewers. As suggested, use
String.prototype.replaceAll()for safe string replacement:- const placeholder = new RegExp(`\\{\\{${key}\\}\\}`, 'g'); - formattedMessage = formattedMessage.replace(placeholder, value); + formattedMessage = formattedMessage.replaceAll(`{{${key}}}`, value);
🧹 Nitpick comments (2)
src/index.ts (1)
117-125: Enhance the JSDoc example to demonstrate variable substitution.The example shows the
variablesparameter being passed but doesn't demonstrate how placeholders in the message template are replaced. Consider showing a message that contains{{merchantName}}or{{supportEmail}}placeholders in the output to clearly illustrate the variable substitution feature.README.md (1)
173-181: Clarify the Message Formatting example.The example shows passing
merchantNameandsupportEmailvariables, but the decline message for'insufficient_funds'doesn't contain corresponding{{merchantName}}or{{supportEmail}}placeholders. This may mislead users into thinking these variables will be substituted when they won't.Consider either:
- Showing a hypothetical message with placeholders in the comment, or
- Noting that this demonstrates the API signature and that actual messages would need to contain the corresponding placeholders
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(4 hunks)src/index.ts(1 hunks)tests/index.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/index.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (1)
src/types.ts (1)
Locale(4-4)
🪛 ast-grep (0.40.0)
src/index.ts
[warning] 144-144: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\{\\{${key}\\}\\}, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
hideokamoto
deleted the
claude/rename-library-features-01L8GCPmpRaFDSeXQmzE7VCa
branch
2 participants
Summary by CodeRabbit
New Features
formatDeclineMessage()function to format decline messages with locale support and optional variable substitution for dynamic messaging.Documentation
Translationtype to public exports.Tests