feat: added query params injection to api plugin url & headers #2848
Conversation
|
WalkthroughThis pull request introduces several updates across multiple packages, primarily focusing on version increments and dependency updates. The Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (14)
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.ts (2)
3-3: Consider a more robust browser environment check.While the current check works for basic cases, consider adding additional checks for a more robust browser environment detection.
-export const isBrowser = () => typeof window !== 'undefined'; +export const isBrowser = () => typeof window !== 'undefined' && window.document !== undefined;
1-41: Add comprehensive documentation.Consider adding detailed JSDoc documentation for the module and each exported function, including:
- Purpose and usage examples
- Integration guidelines with the API plugin
- Error handling strategies
- Browser compatibility notes
Example documentation:
/** * Helper functions for API plugin URL and header manipulation. * @module api-plugin/helpers */ /** * Checks if the code is running in a browser environment. * @returns {boolean} True if running in a browser context */ export const isBrowser = ... /** * Retrieves query parameters from the current URL. * @returns {Record<string, string | string[]>} Object containing query parameters * @throws {Error} When URLSearchParams is not supported */ export const getGlobalQueryParams = ... /** * Formats a string by replacing placeholders with corresponding values. * @example * formatStringValues("Hello {user.name}!", { user: { name: "John" } }) * // Returns: "Hello John!" * * @param {string} str - Template string with {placeholder} syntax * @param {Record<string, any>} data - Data object for placeholder replacement * @returns {string} Formatted string * @throws {Error} When input validation fails */ export const formatStringValues = ...packages/workflow-core/src/lib/plugins/external-plugin/kyb-plugin.ts (1)
Line range hint
11-53: Consider enhancing type safety and error handlingWhile the implementation is functional, there are several opportunities for improvement:
- The
payloadparameter could benefit from a more specific type instead ofAnyRecord- Error messages could be more descriptive and include the actual invalid values
- Consider adding input sanitization for
companyNumberandjurisdictionCodeHere's a suggested improvement:
+ interface KybPayload { + countryOfIncorporation?: string; + state?: string; + companyNumber?: string; + vendor?: string; + } - async makeApiRequest( - url: string, - method: ApiPlugin['method'], - payload: AnyRecord, - headers: HeadersInit, - ) { + async makeApiRequest( + url: string, + method: ApiPlugin['method'], + payload: KybPayload, + headers: HeadersInit, + ) { const { countryOfIncorporation = '', state: stateCode = '', companyNumber = '', vendor = 'open-corporates', } = payload; let countryCode: string | undefined; if ( typeof countryOfIncorporation === 'string' && countryOfIncorporation.length === 2 && countryOfIncorporation === countryOfIncorporation.toUpperCase() ) { countryCode = countryOfIncorporation; } else { countryCode = countries.getAlpha2Code(countryOfIncorporation as string, 'en'); } if (typeof countryCode !== 'string') - throw new Error('Invalid countryOfIncorporation for KYB process'); + throw new Error(`Invalid countryOfIncorporation for KYB process: ${countryOfIncorporation}`); if (typeof companyNumber !== 'string') - throw new Error('Invalid companyNumber for KYB process'); + throw new Error(`Invalid companyNumber for KYB process: ${companyNumber}`); + // Sanitize inputs + const sanitizedCompanyNumber = encodeURIComponent(companyNumber.trim()); const jurisdictionCode = stateCode && typeof stateCode == 'string' && stateCode.length == 2 ? `${countryCode}-${stateCode}` : countryCode; - const formattedUrl = `${url}/${jurisdictionCode}/${companyNumber}`; + const formattedUrl = `${url}/${jurisdictionCode}/${sanitizedCompanyNumber}`; return await super.makeApiRequest(formattedUrl, method, { vendor }, headers); }packages/workflow-core/src/lib/plugins/external-plugin/email-plugin.ts (1)
Line range hint
17-54: Consider improving type safety, separation of concerns, and security.While the implementation works, there are several areas for improvement:
- Type Safety: Consider adding TypeScript interfaces for the email payload structure.
- Separation of Concerns: The adapter logic could be moved to a separate class/factory.
- Security: Filter sensitive fields from dynamic_template_data to prevent information leakage.
Here's a suggested refactor:
interface IEmailPayload { from: { email: string; name?: string; }; subject?: string; preheader?: string; receivers: string[]; templateId: string; [key: string]: unknown; } interface IEmailTemplateData { // Define allowed template fields [key: string]: string; } class EmailPayloadBuilder { static async build(payload: IEmailPayload): Promise<Record<string, unknown>> { // ... existing payload construction logic // with type safety and field filtering } } class EmailAdapterFactory { static create(adapter: string = 'sendgrid') { switch(adapter) { case 'log': return new LogEmailAdapter(); default: return new SendgridAdapter(); } } }packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.test.ts (2)
20-41: Consider adding edge cases to URL parameter tests.The current tests cover the basic scenarios well. Consider adding tests for:
- Empty search string (
?)- URL-encoded special characters
- Duplicate parameters
Example test cases:
it('should handle empty search string', () => { global.window = { location: { search: '?' }, } as Window & typeof globalThis; expect(getGlobalQueryParams()).toEqual({}); }); it('should handle URL-encoded characters', () => { global.window = { location: { search: '?key=hello%20world&special=%26%3D' }, } as Window & typeof globalThis; expect(getGlobalQueryParams()).toEqual({ key: 'hello world', special: '&=' }); });🧰 Tools
🪛 Biome
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
43-77: LGTM! Consider adding more edge cases.The test suite is well-structured and covers the main scenarios effectively. Consider adding these edge cases:
- Empty string input
- Empty data object
- Multiple occurrences of the same placeholder
Example additional test:
it('should handle multiple occurrences of same placeholder', () => { const str = 'Hello {name}! How are you {name}?'; const data = { name: 'John' }; expect(formatStringValues(str, data)).toBe('Hello John! How are you John?'); });packages/workflow-core/src/lib/plugins/external-plugin/ballerine-plugin.ts (2)
Line range hint
13-52: Consider improving type safety in the plugin factory.The function uses
anytype in several places which reduces type safety:let optionsFactoryFn: any = BALLERINE_API_PLUGIN_FACTORY[params.pluginKind]; const pluginOptionFactoryFn = BALLERINE_API_PLUGIN_FACTORY[params.pluginKind] as any; return optionsFactoryFn(params as any);Consider creating proper type definitions for the factory functions and their return types to maintain type safety throughout the plugin system.
Error handling improvement looks good!
The enhanced error message with JSON stringified params will help with debugging:
throw new Error(`Unknown plugin kind: ${params.pluginKind}, params: ${JSON.stringify(params)}`);
Line range hint
54-76: Consider improving type definitions for request/response configurations.The constructor uses type assertions to bypass TypeScript checks:
request: { transformers: requestTransformer, schemaValidator: requestValidator } as any, response: { transformers: responseTransformer, schemaValidator: responseValidator } as any,Consider creating proper interfaces for request and response configurations to avoid type assertions:
interface PluginRequestConfig { transformers: typeof requestTransformer; schemaValidator: typeof requestValidator; } interface PluginResponseConfig { transformers: typeof responseTransformer; schemaValidator: typeof responseValidator; }Constructor implementation looks good!
The use of spread operator for merging configurations is clean and maintainable.
packages/workflow-core/src/lib/plugins/external-plugin/mastercard-merchant-screening-plugin.ts (2)
Line range hint
77-116: Improve error handling and response processing.The error handling could be more robust and informative:
- API errors should be properly structured
- Response validation could be more detailed
- Error messages could be more specific
Consider this enhanced implementation:
- return this.returnErrorResponse( - 'Request Failed: ' + apiResponse.statusText + ' Error: ' + JSON.stringify(errorResponse), - ); + const errorMessage = errorResponse?.error?.message || errorResponse?.message || apiResponse.statusText; + const errorCode = errorResponse?.error?.code || apiResponse.status; + return this.returnErrorResponse( + `Mastercard API Error (${errorCode}): ${errorMessage}`, + errorResponse + );
Address transformation needs validation and error handling improvements
The verification reveals several concerns with the current implementation:
- No validation or error handling exists for address transformation
- The country subdivision support is limited to US and CA, but there's no validation when the country is invalid or missing
- No test coverage exists for this critical address transformation logic
- The arbitrary fallback to "Singapore" for city is not justified by any validation or business logic
Consider:
- Adding input validation for required fields
- Implementing proper error handling for invalid country codes
- Adding test coverage for address transformation scenarios
- Removing the arbitrary city fallback to "Singapore"
- Adding validation for country subdivision matching
🔗 Analysis chain
Line range hint
41-57: Consider adding address validation and sanitization.The address construction logic handles multiple fields but could benefit from additional validation:
- The street/streetNumber concatenation could result in "undefined undefined"
- The city fallback to "Singapore" seems arbitrary
- The countrySubdivision logic might fail silently if state names don't match exactly
Let's verify the state matching logic:
Consider this safer implementation:
const address = { - line1: [entity?.data?.address?.street, entity?.data?.address?.streetNumber] - .filter(Boolean) - .join(' '), + line1: [entity?.data?.address?.street, entity?.data?.address?.streetNumber] + .filter(Boolean) + .join(' ') || undefined, - city: entity?.data?.address?.city || 'Singapore', + city: entity?.data?.address?.city, country: alpha2ToAlpha3(entity?.data?.address?.country), postalCode: entity?.data?.address?.postalCode, countrySubdivision: countrySubdivisionSupportedCountries.includes( entity?.data?.address?.country, ) ? statesOfCountry.find( - state => state.name.toLowerCase() === entity?.data?.address?.state?.toLowerCase(), + state => state.name.toLowerCase().trim() === (entity?.data?.address?.state || '').toLowerCase().trim(), )?.isoCode : undefined, };🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any tests covering the address transformation rg -l "address.*state.*toLowerCase" --type=testLength of output: 78
Script:
#!/bin/bash # Search for test files related to mastercard-merchant-screening fd -e test.ts -e spec.ts mastercard # Search for any usage of the address transformation logic rg -A 5 "const address = \{" # Check for any validation utilities or address-related functions rg -l "address.*validation|validateAddress"Length of output: 1876
Script:
#!/bin/bash # Look for the definition of countrySubdivisionSupportedCountries rg "const.*countrySubdivisionSupportedCountries.*=" -A 3 # Check for any error handling or validation around address transformation rg "throw.*address" -B 2 -A 2 # Look for any tests related to this plugin fd "mastercard.*\.(spec|test)\.(ts|js)"Length of output: 782
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/api-plugin.test.ts (3)
Line range hint
52-54: Consider using a more reliable mock data sourceThe test is using a public S3 bucket URL (
https://simple-kyb-demo.s3.eu-central-1.amazonaws.com/mock-data/business_test_us.json) for mock data. This could make tests unreliable if the URL becomes unavailable.Consider using one of these alternatives:
- Mock the HTTP calls using a library like
nockormsw- Include the mock data directly in the test file
- Store mock data in a local test fixtures directory
Line range hint
127-128: Document the reason for @ts-expect-errorThe usage of
@ts-expect-errorcomments should be documented to explain why the type assertion is necessary and what the actual expected type should be.Add a more detailed comment explaining the type mismatch:
- // @ts-expect-error - `schema` type is wrong + // @ts-expect-error - The schema type in ISerializableHttpPluginParams doesn't include JSON Schema properties yetAlso applies to: 166-167
Line range hint
126-146: Enhance schema validation test coverageThe schema validation tests could be more comprehensive. Currently, they only test for required fields, but JSON Schema supports many other validation rules.
Consider adding test cases for:
- Type validation (string, number, boolean)
- Pattern matching
- Enum values
- Array validation
- Nested object validation
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/api-plugin.ts (1)
120-120: Consider adding query parameter validationWhile the query parameter injection works as intended, consider adding validation to prevent potential security issues:
- Validate parameter names and values
- Consider whitelisting allowed parameters
- Add sanitization if needed
Example validation approach:
const queryParams = getGlobalQueryParams(); +// Add validation +const allowedParams = ['param1', 'param2']; // configure as needed +const validatedParams = Object.fromEntries( + Object.entries(queryParams).filter(([key, value]) => { + return allowedParams.includes(key) && typeof value === 'string'; + }) +); -_url = await this.replaceVariablesFromContext(_url, { query: queryParams }); +_url = await this.replaceVariablesFromContext(_url, { query: validatedParams });Also applies to: 144-145
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
apps/backoffice-v2/CHANGELOG.md(1 hunks)apps/backoffice-v2/package.json(2 hunks)apps/kyb-app/CHANGELOG.md(1 hunks)apps/kyb-app/package.json(2 hunks)examples/headless-example/CHANGELOG.md(1 hunks)examples/headless-example/package.json(2 hunks)packages/workflow-core/CHANGELOG.md(1 hunks)packages/workflow-core/package.json(3 hunks)packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/api-plugin.test.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/api-plugin.ts(4 hunks)packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.test.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/index.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/ballerine-email-plugin.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/ballerine-plugin.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/email-plugin.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/index.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/kyb-plugin.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/kyc-plugin.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/kyc-session-plugin.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/mastercard-merchant-screening-plugin.ts(1 hunks)packages/workflow-core/src/lib/plugins/types.ts(1 hunks)packages/workflow-core/src/lib/workflow-runner.ts(2 hunks)sdks/workflow-browser-sdk/CHANGELOG.md(1 hunks)sdks/workflow-browser-sdk/package.json(2 hunks)sdks/workflow-node-sdk/CHANGELOG.md(1 hunks)sdks/workflow-node-sdk/package.json(2 hunks)services/workflows-service/CHANGELOG.md(1 hunks)services/workflows-service/package.json(2 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
✅ Files skipped from review due to trivial changes (14)
- apps/backoffice-v2/CHANGELOG.md
- apps/backoffice-v2/package.json
- apps/kyb-app/CHANGELOG.md
- apps/kyb-app/package.json
- examples/headless-example/package.json
- packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/index.ts
- packages/workflow-core/src/lib/plugins/types.ts
- sdks/workflow-browser-sdk/CHANGELOG.md
- sdks/workflow-browser-sdk/package.json
- sdks/workflow-node-sdk/CHANGELOG.md
- sdks/workflow-node-sdk/package.json
- services/workflows-service/CHANGELOG.md
- services/workflows-service/package.json
- services/workflows-service/prisma/data-migrations
🧰 Additional context used
🪛 Biome
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.test.ts
[error] 12-12: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (39)
packages/workflow-core/src/lib/plugins/external-plugin/index.ts (3)
3-9: LGTM! Well-organized type exports
The type exports are properly grouped in a single export statement, improving code organization and maintainability.
10-10: LGTM! Clean export statement
The WebhookPlugin export is cleanly separated, maintaining consistency with the module structure.
1-1: Verify the new ApiPlugin import path
The ApiPlugin export path has been updated to reflect a new directory structure.
Let's verify the new path exists:
✅ Verification successful
The ApiPlugin path is valid and correctly structured
The file exists at the expected location: packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/api-plugin.ts, confirming that the export path change is correct and matches the actual file structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new ApiPlugin path exists
# Expected: The file should exist at the new location
fd --type f "api-plugin.ts" --exec ls -l {}
Length of output: 177
packages/workflow-core/src/lib/plugins/external-plugin/kyb-plugin.ts (2)
3-3: LGTM: Import path update is consistent
The updated import path aligns with the broader codebase changes and provides a more specific module location.
Line range hint 11-53: Verify query params injection implementation
The PR title mentions "query params injection to api plugin url & headers", but this file doesn't show direct changes related to that functionality. The query params handling might be implemented in the parent ApiPlugin class.
Let's verify the implementation in the parent class:
✅ Verification successful
Query params injection is properly implemented
The ApiPlugin class correctly handles query parameters in two ways:
- GET requests automatically convert the payload into URL query parameters:
_url = ${_url}?${queryParams} - Global query parameters from the browser's URL are injected via
getGlobalQueryParams()helper
The KybPlugin implementation in the review extends ApiPlugin and correctly uses this functionality by passing the vendor parameter in the payload, which will be properly handled by the parent class.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for query params handling in ApiPlugin
ast-grep --pattern 'class ApiPlugin {
$$$
makeApiRequest($$$) {
$$$
}
$$$
}'
# Search for any query params related changes
rg -A 5 'queryParams|query_params|params' packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/
Length of output: 4182
Script:
#!/bin/bash
# Let's get the full implementation of ApiPlugin's makeApiRequest
ast-grep --pattern 'makeApiRequest($$$) {
$$$
}'
# Also get the full implementation of getGlobalQueryParams helper
ast-grep --pattern 'getGlobalQueryParams($$$) {
$$$
}'
Length of output: 111
Script:
#!/bin/bash
# Let's try a different approach to get the implementation
rg -A 20 "async makeApiRequest" packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/
# And get the helper implementation
rg -A 10 "getGlobalQueryParams" packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/
Length of output: 11019
packages/workflow-core/src/lib/plugins/external-plugin/kyc-session-plugin.ts (1)
3-4: LGTM! Verify the new directory structure.
The import path changes look correct, but let's verify the directory structure to ensure all related files are properly organized.
✅ Verification successful
Import paths are correctly updated and match the directory structure
The verification confirms:
- The
api-plugindirectory exists at the correct location with expected files - All imports of
ApiPluginacross the codebase are consistent with the new structure - The import path change in
kyc-session-plugin.tsaligns with the repository's organization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the directory structure and related files
# Check if the api-plugin directory and files exist
fd --type d "api-plugin" --exec tree {}
# Verify imports in related files
rg --type ts "import.*ApiPlugin" -g "!node_modules"
Length of output: 3304
packages/workflow-core/src/lib/plugins/external-plugin/email-plugin.ts (2)
3-3: LGTM! Import path updated to reflect new directory structure.
The import path change aligns with the codebase restructuring mentioned in the summary.
Line range hint 1-67: Verify integration with updated ApiPlugin functionality.
Since the PR adds query params injection to the ApiPlugin, we should verify that the EmailPlugin properly inherits and utilizes this new functionality.
✅ Verification successful
Let me gather more information about the ApiPlugin implementation to better understand the query params functionality.
Let me get the complete ApiPlugin implementation to understand how the query params functionality is integrated with the makeApiRequest method.
Based on the gathered information, I can now provide the final response:
EmailPlugin correctly inherits query params functionality from ApiPlugin
The EmailPlugin extends ApiPlugin and doesn't override the makeApiRequest method's query params handling. The base ApiPlugin class properly injects query parameters in two ways:
- Global query params from URL for all requests via
getGlobalQueryParams() - Request payload conversion to query params for GET requests
Since EmailPlugin only customizes the email-specific payload structure and maintains the super call to makeApiRequest, it automatically inherits this functionality without any needed changes:
return await super.makeApiRequest(url, method, emailPayload, headers);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ApiPlugin changes and their impact on EmailPlugin
# Check the ApiPlugin implementation for query params injection
echo "Checking ApiPlugin implementation:"
ast-grep --pattern 'class ApiPlugin {
$$$
makeApiRequest($$$) {
$$$
}
$$$
}'
# Look for any query params related tests
echo -e "\nChecking for related tests:"
rg -l "queryParams|query params" --type ts --glob "**/test/**"
# Check if other plugins have been updated for query params
echo -e "\nChecking other plugins for query params usage:"
rg "queryParams" --type ts --glob "**/plugins/**"
Length of output: 1049
Script:
#!/bin/bash
# Get the full implementation of ApiPlugin's makeApiRequest method
ast-grep --pattern 'class ApiPlugin {
$$$
async makeApiRequest($$$) {
$$$
}
$$$
}'
# Check if EmailPlugin overrides any query params related methods
echo -e "\nChecking EmailPlugin for query params handling:"
rg -A 5 "queryParams|getGlobalQueryParams|replaceVariablesFromContext" packages/workflow-core/src/lib/plugins/external-plugin/email-plugin.ts
# Check the global query params implementation
echo -e "\nChecking global query params implementation:"
rg "getGlobalQueryParams" -A 5
Length of output: 5373
Script:
#!/bin/bash
# Get the full content of api-plugin.ts to see the complete implementation
echo "ApiPlugin implementation:"
cat packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/api-plugin.ts
# Check if there are any query params related interfaces or types
echo -e "\nChecking related interfaces and types:"
rg "interface.*ApiPlugin|type.*ApiPlugin" --type ts
Length of output: 13439
packages/workflow-core/src/lib/plugins/external-plugin/ballerine-email-plugin.ts (3)
3-4: LGTM: Import statements are properly organized.
The imports are correctly structured and include all necessary dependencies for the plugin functionality.
Line range hint 84-86: Verify the impact of returning an empty object.
The returnSuccessResponse method always returns an empty object regardless of the response body. This might lead to loss of important response data.
#!/bin/bash
# Search for usages of returnSuccessResponse to understand the impact
rg -A 5 "returnSuccessResponse" Line range hint 19-21: Verify the necessity of combining all parameter types.
The constructor combines IBallerineEmailPluginParams, IBallerineApiPluginParams, and IApiPluginParams. Please ensure that all these parameters are necessary and being utilized within the class.
✅ Verification successful
Let me gather more information about the parameter interfaces and their usage.
Based on the gathered information, I can now generate the final response:
The combination of parameter types in the constructor is necessary and correctly utilized.
Each interface serves a specific purpose:
IBallerineEmailPluginParams: Provides email-specific parameters (template, pluginKind)IBallerineApiPluginParams: Provides Ballerine API specific parameters (pluginKind, vendor, displayName)IApiPluginParams: Provides base API parameters (url, method, headers)
These parameters are used throughout the class hierarchy for API requests, email template handling, and plugin configuration. The inheritance chain and parameter combination follow a proper design pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of parameters from these interfaces
ast-grep --pattern 'interface IBallerineEmailPluginParams {
$$$
}'
ast-grep --pattern 'interface IBallerineApiPluginParams {
$$$
}'
ast-grep --pattern 'interface IApiPluginParams {
$$$
}'
Length of output: 3562
Script:
#!/bin/bash
# Search for interface definitions and their usage
rg -A 10 "interface (IBallerineEmailPluginParams|IBallerineApiPluginParams|IApiPluginParams)"
# Search for the class implementation
rg -A 20 "class BallerineEmailPlugin"
Length of output: 5935
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.test.ts (2)
1-2: LGTM! Clean test setup with clear imports.
5-18: LGTM! Well-structured browser detection tests.
The use of delete global.window is appropriate here despite the static analysis warning. It's necessary for proper test isolation and cleanup of the global scope.
Let's verify there are no other tests depending on window object:
✅ Verification successful
Since the rg search shows that helpers.test.ts is the only TypeScript file in the codebase that uses global.window, we can confirm that the cleanup with delete global.window is safe and won't affect other tests. Let me generate the final response:
Test cleanup is properly isolated
The delete global.window cleanup is safe as this is the only test file manipulating the global window object, ensuring proper test isolation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other tests that might be affected by window object
rg -l "global.window" --type ts
Length of output: 113
🧰 Tools
🪛 Biome
[error] 12-12: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/workflow-core/src/lib/plugins/external-plugin/ballerine-plugin.ts (2)
2-3: LGTM! Clean import organization.
The imports are well-organized and follow good practices by separating concerns into different modules.
Line range hint 78-85: LGTM! Clean implementation of variable replacement.
The method properly:
- Handles asynchronous operations with async/await
- Extends parent class functionality while maintaining plugin-specific logic
- Uses clear variable naming
packages/workflow-core/src/lib/plugins/external-plugin/kyc-plugin.ts (2)
3-3: LGTM: Import change aligns with the class dependencies
The JsonSchemaValidator import is correctly positioned and necessary for the validateContent method implementation.
Line range hint 89-108: Verify query params injection functionality
While the class extends BallerineApiPlugin, we should verify that the query params injection functionality (mentioned in PR title) is properly inherited and works as expected with KYC-specific endpoints.
Consider adding KYC-specific test cases to verify the query params injection works correctly with your API endpoints.
✅ Verification successful
Query params injection functionality is properly inherited and working
The verification shows that query params injection is properly implemented in the base ApiPlugin class and inherited by the KycPlugin. The functionality is handled in two ways:
- Global query params are automatically injected via
getGlobalQueryParams()in the_getPluginUrlmethod - Request-specific query params are handled for GET requests via
URLSearchParams
Since KycPlugin extends BallerineApiPlugin which extends ApiPlugin, it automatically inherits this functionality without any additional implementation needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of query params injection in BallerineApiPlugin
# and its usage in other plugins
# Check BallerineApiPlugin implementation
echo "Checking BallerineApiPlugin implementation..."
ast-grep --pattern 'class BallerineApiPlugin {
$$$
queryParams($$$) {
$$$
}
$$$
}'
# Check for other plugins using query params
echo "Checking other plugins using query params..."
rg -l "queryParams" --type ts
# Check for tests covering this functionality
echo "Checking for related tests..."
fd -e test.ts -e spec.ts | xargs rg "queryParams"
Length of output: 998
Script:
#!/bin/bash
# Let's check the implementation of BallerineApiPlugin and its query params handling
# First, let's find and check the BallerineApiPlugin implementation
echo "=== Checking BallerineApiPlugin implementation ==="
fd "api-plugin.ts" | xargs cat
# Check if KYC plugin extends BallerineApiPlugin and its implementation
echo -e "\n=== Checking KYC plugin implementation ==="
fd "kyc-plugin.ts" | xargs cat
# Check for any API plugin related tests
echo -e "\n=== Checking API plugin tests ==="
fd -e test.ts -e spec.ts | xargs rg -l "ApiPlugin|queryParams"
Length of output: 15207
Script:
#!/bin/bash
# Let's check the BallerineApiPlugin implementation since KYC plugin extends it
echo "=== Checking BallerineApiPlugin implementation ==="
fd "ballerine-plugin.ts" | xargs cat
# Let's also check the helper function that handles query params
echo -e "\n=== Checking query params helper implementation ==="
fd "helpers.ts" --full-path --base-directory packages/workflow-core/src/lib/plugins/external-plugin/api-plugin | xargs cat
Length of output: 2952
Script:
#!/bin/bash
# Let's check the helpers implementation in the correct location
echo "=== Checking helpers implementation ==="
fd "helpers.ts" --type f --hidden --no-ignore | xargs cat
# Let's also check if there are any references to query params in the API plugin
echo -e "\n=== Checking query params usage in API plugin ==="
rg -A 5 -B 5 "queryParams" packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/
Length of output: 26436
packages/workflow-core/package.json (3)
4-4: LGTM: Version bump follows semver
The patch version increment is appropriate and consistent with the changes being made across related packages.
Line range hint 1-99: Verify scope of changes matches dependencies
The added lodash utilities suggest object manipulation for query params and headers, but let's verify the implementation scope.
#!/bin/bash
# Description: Verify the implementation scope matches the added dependencies
# Look for files that might contain the API plugin implementation
echo "Searching for API plugin related files..."
fd -e ts -e js "api.*plugin|plugin.*api"
# Search for query parameter and header manipulation in the codebase
echo "Checking for query parameter and header manipulation..."
rg -g '*.{ts,js}' "(?i)(query.*param|headers?)\s*[=:]" --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}'64-65: LGTM: Dependencies align with feature requirements
The addition of lodash.get and lodash.set with their type definitions is appropriate for safely handling nested object properties, which aligns with the query params injection feature.
Let's verify these are the only new lodash utilities needed:
Also applies to: 81-82
✅ Verification successful
Let me search for specific lodash utility usage to verify if we need additional lodash packages.
LGTM: Dependencies are correctly scoped and used
The codebase scan confirms that lodash.get and lodash.set are actively used in the codebase, particularly in API plugin helpers and rule engine operators. No additional lodash utilities are required beyond what's already included in the dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing or potential lodash utility usage in the codebase
# that might suggest we need additional lodash packages.
# Search for potential get/set operations on objects that might need these utilities
rg -g '*.{ts,js}' "(\.|get|set)\(.*\)" --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}'
Length of output: 13592
Script:
#!/bin/bash
# Search for lodash utility usage patterns
echo "Searching for lodash imports and usage..."
rg -g '*.{ts,js}' "import.*lodash" --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}'
echo -e "\nSearching for potential lodash get/set usage patterns..."
rg -g '*.{ts,js}' "(lodash\.|\b_\.)(get|set)\b" --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}'
echo -e "\nSearching for other lodash utility patterns..."
rg -g '*.{ts,js}' "(lodash\.|\b_\.)(merge|cloneDeep|pick|omit|has|isEqual)" --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}'
Length of output: 5831
packages/workflow-core/src/lib/plugins/external-plugin/mastercard-merchant-screening-plugin.ts (3)
2-2: LGTM! Import organization looks good.
The import statements are well-organized with the State import at the top and the updated ApiPlugin import path reflecting the new structure.
Also applies to: 6-6
Line range hint 92-94: Verify API token security.
The code uses an environment variable for API authentication. Ensure this token is properly secured and rotated.
Let's check for token usage patterns:
#!/bin/bash
# Search for UNIFIED_API_TOKEN usage
rg "UNIFIED_API_TOKEN" --type=tsConsider implementing:
- Token rotation mechanism
- Token validation before use
- Fallback mechanism for token refresh
Line range hint 59-75: Enhance security for sensitive data handling.
The code includes sensitive data (consumerKey, privateKey) in the request payload. While these are fetched from a secrets manager, we should ensure they're not logged or exposed in error messages.
Let's check for any sensitive data logging:
Consider these security enhancements:
- Redact sensitive fields in logs
- Validate secrets exist before use
- Add input validation for secrets
+ if (!secrets?.consumerKey || !secrets?.privateKey || !secrets?.acquirerId) {
+ return this.returnErrorResponse('Missing required authentication credentials');
+ }
requestPayload = {
...requestPayload,
consumerKey: secrets?.consumerKey,
privateKey: secrets?.privateKey,
acquirerId: secrets?.acquirerId,packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/api-plugin.test.ts (2)
2-4: LGTM: Import paths are properly updated
The import path changes correctly reflect the new directory structure while maintaining clean organization.
Line range hint 1-190: Overall: Well-structured and comprehensive test suite
The test file is well-organized with good coverage of various scenarios including happy paths and error cases. The nested describe blocks provide clear context for each test case, and the assertions are thorough.
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/api-plugin.ts (2)
2-5: LGTM: Import changes align with functionality addition
The import changes, including the new getGlobalQueryParams helper, are well-organized and support the new query parameter injection feature.
Line range hint 1-391: Verify test coverage and security implications
Please ensure:
- Test coverage for query parameter injection in URLs and headers
- Security testing for potential injection vulnerabilities
- Documentation of allowed query parameters
Let's check for existing tests and potential security implications:
✅ Verification successful
Query parameter handling is well-tested and safely implemented
The verification shows:
- Query parameters are properly tested with unit tests covering browser/non-browser environments and parameter parsing
- Implementation in
helpers.tsuses nativeURLSearchParamsfor safe parameter handling - Query parameters are only used in GET requests with proper URL construction
- Parameters are handled client-side only, reducing server-side injection risks
The code follows security best practices by:
- Using native browser APIs for parameter parsing
- Implementing environment-aware handling (browser vs non-browser)
- Maintaining proper separation between URL construction and parameter handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for test coverage
echo "Checking for test coverage..."
fd -e test.ts -e spec.ts . | xargs rg -l "getGlobalQueryParams|query params"
# Check for potential security implications
echo "Checking for potential security risks..."
rg -l "headers.*query|url.*query" .
# Check for existing parameter validation
echo "Checking for parameter validation..."
rg -l "allowedParams|validateParams|sanitize.*params" .
Length of output: 780
Script:
#!/bin/bash
# Check the test implementation for query params
echo "Checking test implementation..."
cat packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.test.ts
# Check the implementation of getGlobalQueryParams
echo "Checking query params implementation..."
cat packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.ts
# Check URL handling in the files that use query params
echo "Checking URL handling in backoffice..."
rg -A 5 "headers.*query|url.*query" apps/backoffice-v2/src/domains/transactions/fetchers.ts apps/backoffice-v2/src/domains/alerts/fetchers.ts
Length of output: 4788
packages/workflow-core/CHANGELOG.md (1)
3-8: LGTM! Version bump and changelog entry are appropriate.
The changelog entry accurately describes the addition of query parameters to the apiPlugin's URL and headers. The patch version increment (0.6.64 → 0.6.65) is appropriate for this backward-compatible enhancement.
Let's verify the version bump across related packages:
✅ Verification successful
Version bump is consistent across all dependent packages
All packages referencing @ballerine/workflow-core are using the latest version 0.6.65:
- sdks/workflow-node-sdk
- sdks/workflow-browser-sdk
- services/workflows-service
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify version consistency across related packages
# Expected: All packages should reference [email protected]
# Check package.json files for workflow-core dependency
rg -A 1 '"@ballerine/workflow-core":\s*"[^"]*"' --type json
Length of output: 524
examples/headless-example/CHANGELOG.md (1)
3-7: LGTM! Changes are well-documented and consistent.
The changelog updates follow the proper format and accurately reflect:
- Version bump to 0.3.64 (patch)
- Dependency update to @ballerine/[email protected]
packages/workflow-core/src/lib/workflow-runner.ts (10)
3-3: Approved: Added necessary import for search from jmespath
The import of search from jmespath is appropriate and necessary for implementing JMESPath queries within the workflow runner.
7-7: Approved: Imported pluginsRegistry from ./constants
This import is required for accessing the plugins registry, enhancing the plugin management functionality.
9-13: Approved: Added essential imports for built-in events and plugins
The added imports for BUILT_IN_EVENT, logger, ChildWorkflowPlugin, IterativePlugin, and RiskRulePlugin are necessary for the updated plugin architecture and event handling within the workflow runner.
15-17: Approved: Imported TransformerPlugin and TransformerPluginParams
These imports are needed for incorporating transformer plugins, which enhance data transformation capabilities.
29-35: Approved: Added imports for various external plugins
The imports for ApiPlugin, BallerineEmailPlugin, BallerineApiPlugin, DispatchEventPlugin, KycPlugin, and KycSessionPlugin are appropriate additions to support the extended plugin functionalities.
36-39: Approved: Imported plugin parameter types
The imports of IApiPluginParams, IDispatchEventPluginParams, and ISerializableHttpPluginParams are necessary for type definitions related to plugins.
45-45: Approved: Imported WebhookPlugin
The import of WebhookPlugin is appropriate for webhook support in the workflow.
47-54: Approved: Imported plugin-related types
The added imports for ActionablePlugins, ChildPlugins, CommonPlugin, CommonPlugins, HttpPlugin, HttpPlugins, and StatePlugin from ./plugins/types are necessary for defining plugin interfaces and types.
55-64: Approved: Imported various utility types and enums
The imports of ErrorEnum, ObjectValues, SecretsManager, WorkflowEvent, WorkflowEvents, WorkflowEventWithoutState, WorkflowExtensions, and WorkflowRunnerArgs from ./types enhance type safety and error handling.
65-65: Approved: Imported utilities for deep merging and context management
The import of ArrayMergeOption, deepMergeWithOptions, and TContext from ./utils is appropriate for context management and deep merging functionalities.
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.ts
Outdated
Show resolved
Hide resolved
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.ts
Outdated
Show resolved
Hide resolved
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/api-plugin.ts
Show resolved
Hide resolved
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.ts
Outdated
Show resolved
Hide resolved
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.ts
Outdated
Show resolved
Hide resolved
| @@ -251,7 +255,12 @@ export class ApiPlugin { | |||
| return { [returnArgKey]: isValid, errorMessage }; | |||
| } | |||
|
|
|||
| async composeRequestHeaders(headers: HeadersInit, context: TContext) { | |||
| async composeRequestHeaders(headers: HeadersInit, _context: TContext) { | |||
| const context = { | |||
There was a problem hiding this comment.
Why do we need to add the query params to the context?
There was a problem hiding this comment.
Method uses context as data source to replace tags. Just updating single data source here so query values also could be included. No mutations so should not affect context outside of this method call.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.ts (1)
12-34: Consider adding DoS protection for multiple parameter values.While the function correctly handles multiple values and implements sanitization, it could benefit from limiting the number of values per parameter to prevent potential DoS attacks.
Consider adding a limit check:
+const MAX_VALUES_PER_PARAM = 100; // Adjust based on requirements + export const getGlobalQueryParams = (): Record<string, string | string[]> => { if (!isBrowser()) return {}; const searchParams = new URLSearchParams(window.location.search); const paramsObject: Record<string, string | string[]> = {}; // Handle multiple values for the same parameter for (const [key, value] of searchParams.entries()) { const sanitizedValue = sanitizeQueryParam(value); const sanitizedKey = sanitizeQueryParam(key); if (sanitizedKey in paramsObject) { const existing = paramsObject[sanitizedKey]; + // Check if we've hit the limit for this parameter + if (Array.isArray(existing) && existing.length >= MAX_VALUES_PER_PARAM) { + console.warn(`Exceeded maximum values for parameter: ${sanitizedKey}`); + continue; + } paramsObject[sanitizedKey] = Array.isArray(existing) ? [...existing, sanitizedValue] : [existing as string, sanitizedValue]; } else { paramsObject[sanitizedKey] = sanitizedValue; } } return paramsObject; };packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.test.ts (2)
4-18: Consider using beforeEach/afterEach for better test isolationWhile the tests are well-structured and cover the necessary cases, consider using Vitest's
beforeEachandafterEachhooks for window object cleanup to improve test isolation.describe('isBrowser', () => { + afterEach(() => { + global.window = undefined; + }); + it('should return true when window is defined', () => { // Mock window object global.window = {} as Window & typeof globalThis; expect(isBrowser()).toBe(true); - delete (global as any).window; }); it('should return false when window is undefined', () => { expect(isBrowser()).toBe(false); }); });🧰 Tools
🪛 Biome
[error] 12-12: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
20-25: Consider adding more edge cases to sanitizeQueryParam testsWhile the current tests cover basic functionality, consider adding tests for:
- null/undefined inputs
- empty string
- non-string inputs (numbers, objects, etc.)
describe('sanitizeQueryParam', () => { it('should trim and escape special characters', () => { expect(sanitizeQueryParam(' test<script> ')).toBe('test<script>'); expect(sanitizeQueryParam(' hello&world ')).toBe('hello&world'); }); + + it('should handle edge cases', () => { + expect(sanitizeQueryParam('')).toBe(''); + expect(sanitizeQueryParam(null)).toBe(''); + expect(sanitizeQueryParam(undefined)).toBe(''); + expect(sanitizeQueryParam(123)).toBe('123'); + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/workflow-core/package.json(4 hunks)packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.test.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/workflow-core/package.json
🧰 Additional context used
🪛 Biome
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.test.ts
[error] 12-12: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 46-46: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 62-62: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (4)
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.ts (2)
3-3: LGTM: Browser detection implementation is correct.
The function correctly checks for browser environment using the standard typeof window !== 'undefined' pattern.
1-34: Verify integration with API plugin.
The implementation provides the necessary utilities for query parameter injection. Let's verify the integration with the API plugin.
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have sufficient information about the integration:
Query parameter injection is properly integrated with API plugin
The implementation is correctly integrated and used within the API plugin:
- Query parameters are injected in both URL construction and headers:
_getPluginUrlmethod usesgetGlobalQueryParams()to include query parameters in URLscomposeRequestHeadersmethod includes query parameters in the request context
- Parameters are properly sanitized using
sanitizeQueryParamwithin thegetGlobalQueryParamsimplementation - The functionality is used in a type-safe manner with proper error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of these utility functions in the API plugin
echo "Searching for getGlobalQueryParams usage..."
rg "getGlobalQueryParams" --type ts
echo "Searching for sanitizeQueryParam usage..."
rg "sanitizeQueryParam" --type ts
echo "Checking API plugin implementation..."
ast-grep --pattern 'class ApiPlugin {
$$$
invoke($$$) {
$$$
}
$$$
}'
Length of output: 47486
packages/workflow-core/src/lib/plugins/external-plugin/api-plugin/helpers.test.ts (2)
1-2: LGTM: Test setup and imports are appropriate
The test file correctly imports the necessary testing utilities from Vitest and the helper functions being tested.
27-64: Improve test isolation and verify URL parameter security
- Similar to the isBrowser tests, consider using beforeEach/afterEach hooks for cleanup.
- The tests should verify that the function properly handles potentially malicious URL parameters.
describe('getGlobalQueryParams', () => {
+ afterEach(() => {
+ global.window = undefined;
+ });
+
it('should return empty object when not in browser', () => {
const result = getGlobalQueryParams();
expect(result).toEqual({});
});
it('should parse and sanitize URL search params into object', () => {
// Mock window with location and URLSearchParams
global.window = {
location: {
search: '?key1=<script>&key2= value2 ',
},
} as Window & typeof globalThis;
expect(getGlobalQueryParams()).toEqual({
key1: '<script>',
key2: 'value2',
});
- delete (global as any).window;
});
+ it('should handle potentially malicious URL parameters', () => {
+ global.window = {
+ location: {
+ search: '?key1=javascript:alert(1)&key2=data:text/html,<script>',
+ },
+ } as Window & typeof globalThis;
+
+ const result = getGlobalQueryParams();
+ expect(result.key1).not.toContain('javascript:');
+ expect(result.key2).not.toContain('<script>');
+ });
+
// ... rest of the tests
});✅ Verification successful
Based on the shell script results, I can now generate the final response since I have sufficient information about the URL parameter handling implementation. The results show that:
- The
sanitizeQueryParamfunction uses validator.escape() for sanitization - Both keys and values are sanitized in
getGlobalQueryParams - The implementation is consistently used across the codebase
- The existing tests already verify HTML entity encoding for special characters
Test cleanup suggestion is valid, but security concerns are already addressed
The URL parameter sanitization is properly implemented using validator.escape() which handles HTML encoding. The existing tests already verify this behavior with cases like <script> being converted to <script>. The only improvement needed is the test cleanup suggestion.
describe('getGlobalQueryParams', () => {
+ afterEach(() => {
+ delete (global as any).window;
+ });
+
it('should return empty object when not in browser', () => {
const result = getGlobalQueryParams();
expect(result).toEqual({});
});
it('should parse and sanitize URL search params into object', () => {
global.window = {
location: {
search: '?key1=<script>&key2= value2 ',
},
} as Window & typeof globalThis;
expect(getGlobalQueryParams()).toEqual({
key1: '<script>',
key2: 'value2',
});
- delete (global as any).window;
});🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper URL parameter sanitization in the codebase
# Search for URL parameter handling in the helper implementation
echo "Checking URL parameter sanitization implementation:"
rg -A 5 "sanitizeQueryParam|getGlobalQueryParams" --type ts
# Look for other URL parameter handling to ensure consistent sanitization
echo "Checking other URL parameter handling in the codebase:"
ast-grep --pattern 'URLSearchParams($$$)'
Length of output: 8076
🧰 Tools
🪛 Biome
[error] 46-46: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 62-62: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| /** | ||
| * Sanitizes query parameter values to prevent XSS and injection attacks | ||
| */ | ||
| export const sanitizeQueryParam = (param: string): string => { | ||
| return validator.escape(param.trim()); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for edge cases.
While the function correctly sanitizes strings, it should handle null/undefined inputs and non-string types.
Consider this enhanced implementation:
export const sanitizeQueryParam = (param: string): string => {
+ if (param == null) return '';
+ if (typeof param !== 'string') {
+ return validator.escape(String(param).trim());
+ }
return validator.escape(param.trim());
};📝 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.
| /** | |
| * Sanitizes query parameter values to prevent XSS and injection attacks | |
| */ | |
| export const sanitizeQueryParam = (param: string): string => { | |
| return validator.escape(param.trim()); | |
| }; | |
| /** | |
| * Sanitizes query parameter values to prevent XSS and injection attacks | |
| */ | |
| export const sanitizeQueryParam = (param: string): string => { | |
| if (param == null) return ''; | |
| if (typeof param !== 'string') { | |
| return validator.escape(String(param).trim()); | |
| } | |
| return validator.escape(param.trim()); | |
| }; |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Refactor
Tests