Fix error handling, type safety, security vulnerabilities, and add test coverage

[Copilot]

Pull Request Description

Comprehensive codebase analysis identified and fixed critical bugs, type inconsistencies, and security vulnerabilities. Fixes issues with network error handling, unsafe string operations, and inconsistent response code validation. Added comprehensive test cases to achieve 100% patch coverage.

Type of Change

• Bugfix
• New feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

Changes Made

Error Handling (src/vnpay.ts)

• getBankList(): Added HTTP response validation before JSON parsing
• logo_link processing: Safe path handling to prevent crashes on empty strings or missing leading slash

// Before: Crashes if response fails or logo_link is empty
const bankList = (await response.json()) as Bank[];
bank.logo_link = resolveUrlString(host, bank.logo_link.slice(1));

// After: Validates response and handles edge cases
if (!response.ok) {
    throw new Error(`Failed to fetch bank list: HTTP ${response.status}`);
}
const logoPath = bank.logo_link?.startsWith('/') 
    ? bank.logo_link.slice(1) 
    : bank.logo_link;

Type Safety (src/services/verification.service.ts)

• Fixed vnp_ResponseCode comparison to handle both string | number per API spec
• Now consistent with query.service.ts and refund methods

// Before: Only handles string
isSuccess: cloneQuery.vnp_ResponseCode === '00'

// After: Handles both types
isSuccess: cloneQuery.vnp_ResponseCode === '00' || cloneQuery.vnp_ResponseCode === 0

Security

• Resolved glob (high): Command injection vulnerability
• Resolved js-yaml (moderate): Prototype pollution
• undici (low): Remains in dev dependency release-it, no production impact

Code Quality

• Fixed Prettier formatting in test files

Test Coverage

Added 4 comprehensive test cases to achieve 100% statement coverage for all changes:

1. getBankList - HTTP error handling: Tests error throwing when fetch fails
2. getBankList - logo_link without leading slash: Tests safe path handling
3. getBankList - empty logo_link: Tests edge case handling
4. verifyReturnUrl - vnp_ResponseCode as number 0: Tests type consistency for response code validation

Testing

• All 164 tests pass (160 existing + 4 new)
• 100% statement coverage for changed files
• Linter passes with zero warnings
• Build succeeds
• CodeQL security scan clean

Coverage Results:

• Before: 66.67% patch coverage
• After: 100% statement coverage (src/vnpay.ts & src/services/verification.service.ts)

Does this introduce a breaking change?

• Yes
• No

All changes are backward compatible. Error handling improvements throw descriptive errors only on failures that would have crashed before.

Related Issues or References

Addresses codebase analysis request for bug identification and code cleanliness verification.

Original prompt

Read codebase, analysis to find bugs, issues, and make sure code clean.

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Summary by CodeRabbit

• Bug Fixes

  • Improved error handling when retrieving bank information from external services
  • Enhanced response code verification to recognize additional response formats
  • Fixed bank logo URL path normalization for proper asset loading

• Tests

  • Expanded test coverage for fetch error scenarios and URL path handling

[coderabbitai]

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The PR modifies verification logic in VNPay service to accept numeric 0 response code as success alongside existing '00' string check, adds defensive HTTP response validation for bank list retrieval, improves logo URL path handling, and introduces corresponding test coverage for new scenarios.

Changes

Cohort / File(s)	Summary
Verification Response Handling src/services/verification.service.ts	Modified verifyReturnUrl to treat numeric VNPay response code 0 as success, complementing existing '00' string validation
Bank List Fetch & Logo Resolution src/vnpay.ts	Added HTTP response validation after bank list fetch; improved logo_link path normalization by conditionally slicing leading slash instead of unconditional slice(1)
Bank List Integration Tests test/integration/vnpay/get-bank-list.test.ts	Added test cases for fetch error handling with non-ok HTTP response, logo_link normalization without leading slash, and empty logo_link fallback behavior
Verify Return URL Integration Tests test/integration/vnpay/verify-return-url.test.ts	Added test case validating verifyReturnUrl correctly handles numeric 0 response code with secure hash recomputation

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• src/vnpay.ts may warrant extra attention for the logo_link path normalization logic to ensure edge cases (empty strings, various slash combinations) are covered
• src/services/verification.service.ts condition addition should be verified against all possible VNPay response code formats and ensure no existing string-based checks are affected

Possibly related PRs

• Fix incorrect signature hash during verify return url #16 — Related to verify-return-url signature verification flow; both PRs adjust handling within the VNPay response code validation pathway

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title lists multiple distinct improvements (error handling, type safety, security, test coverage) but the changeset focuses primarily on HTTP validation and logo_link processing in getBankList, vnp_ResponseCode type handling, and test coverage—not comprehensively covering all stated areas equally.	Clarify the title to prioritize the main changes, such as 'Fix getBankList validation, vnp_ResponseCode type handling, and add test coverage' to better reflect the actual changeset scope.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
vnpay	Ready	Preview	Comment	Nov 19, 2025 5:09am

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

[lehuygiang28]

@copilot Please add test cases to cover the uncovered changes in the branch copilot/analyze-codebase-for-issues of the repository lehuygiang28/vnpay. The current patch coverage is 66.67%, and we need to raise it to 100%.

[Copilot]

@copilot Please add test cases to cover the uncovered changes in the branch copilot/analyze-codebase-for-issues of the repository lehuygiang28/vnpay. The current patch coverage is 66.67%, and we need ...

Added comprehensive test cases to achieve 100% statement coverage for the changes. Commit: b4203bc

New test cases added:

1. getBankList - HTTP error response handling
2. getBankList - logo_link without leading slash
3. getBankList - empty logo_link edge case
4. verifyReturnUrl - vnp_ResponseCode as number 0

Coverage improved:

• Before: 66.67% patch coverage
• After: 100% statement coverage for changed files (src/vnpay.ts, src/services/verification.service.ts)

All 164 tests passing ✅

[lehuygiang28]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[lehuygiang28]

LGTM