[Components] fiserv - new components #14687
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
@jcortes is attempting to deploy a commit to the Pipedreamers Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces several new modules for handling checkout processes in the Fiserv component. It includes the creation of a checkout module, a module for retrieving checkout details, and a constants module for environment settings and API paths. Additionally, utility functions for JSON parsing and enhancements to the main application file for authentication and request handling are included. The Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 8
🧹 Outside diff range and nitpick comments (6)
components/fiserv/common/utils.mjs (3)
1-2: Add JSDoc documentation and consider renaming for clarity.Since this is a utility function that will be used across Fiserv components, it should have comprehensive documentation. Also, consider a more specific name like
parseJsonRecursiveto better indicate its behavior.+/** + * Recursively parses JSON strings within an object or string input + * @param {Object|string} input - The input value to parse + * @returns {Object|string} The parsed result + */ -const parseJson = (input) => { +const parseJsonRecursive = (input) => {
18-22: Add type validation for payment-related fields.Since this utility is used for processing payment data, consider adding validation for specific fields to ensure data integrity.
+const PAYMENT_FIELD_TYPES = { + transactionAmount: 'number', + currency: 'string', + // Add other payment-specific fields +}; + const parseJson = (input) => { - const parse = (value) => { + const parse = (value, field = null) => { + // Validate payment-specific fields + if (field && PAYMENT_FIELD_TYPES[field]) { + const expectedType = PAYMENT_FIELD_TYPES[field]; + if (typeof value !== expectedType) { + throw new TypeError(`Invalid type for ${field}: expected ${expectedType}`); + } + }
24-26: Add explicit type definitions.Consider adding TypeScript or JSDoc type definitions to improve type safety and developer experience.
+/** + * @typedef {Object} Utils + * @property {function(any): any} parseJson - Recursively parses JSON strings + */ + +/** @type {Utils} */ export default { parseJson, };components/fiserv/actions/create-checkout/create-checkout.mjs (1)
26-26: Typo in 'transactionOrigin' description: 'recieved' should be 'received'.Please correct the spelling in the description.
Apply this diff:
- description: "This parameter is used to flag the transaction source correctly. The possible values are `ECOM` (if the order was recieved from online shop), `MAIL` & `PHONE`.", + description: "This parameter is used to flag the transaction source correctly. The possible values are `ECOM` (if the order was received from online shop), `MAIL` & `PHONE`.",components/fiserv/fiserv.app.mjs (2)
64-71: Maintain consistent access to authentication propertiesIn the
getHeaders(headers)method, the API key is accessed directly fromthis.$auth.api_keyon line 69. For consistency with other methods that usethis.getAuth(), consider accessing the API key viathis.getAuth().apiKey.Apply this diff for consistency:
getHeaders(headers) { return { ...headers, "Content-Type": "application/json", "Accept": "application/json", - "API-Key": this.$auth.api_key, + "API-Key": this.getAuth().apiKey, }; },
75-85: Make thedebugflag configurable or remove itIn the
_makeRequestmethod, thedebugflag is set totrueon line 77. Leaving debugging enabled in a production environment can expose sensitive information and affect performance. Consider making this flag configurable or removing it entirely.Apply this diff to parameterize the
debugflag:_makeRequest({ $ = this, path, headers, data, apiPath, debug = false, ...args } = {}) { return axios($, { ...args, - debug: true, + debug, url: this.getUrl(path, apiPath), data, headers: { ...this.getHeaders(headers), ...this.getSignatureHeaders(data), }, }); },This allows you to control the debug setting when calling
_makeRequestor default it tofalse.
📜 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 (6)
components/fiserv/actions/create-checkout/create-checkout.mjs(1 hunks)components/fiserv/actions/retrieve-checkout-details/retrieve-checkout-details.mjs(1 hunks)components/fiserv/common/constants.mjs(1 hunks)components/fiserv/common/utils.mjs(1 hunks)components/fiserv/fiserv.app.mjs(1 hunks)components/fiserv/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/fiserv/common/constants.mjs
🔇 Additional comments (8)
components/fiserv/package.json (1)
18-18:
Fix uuid version to latest stable release
The specified version ^11.0.3 is invalid as the latest major version of uuid is 9.x. This will cause installation failures.
Apply this diff to use the latest stable version:
"dependencies": {
"@pipedream/platform": "3.0.3",
- "uuid": "^11.0.3"
+ "uuid": "^9.0.1"
}Let's verify the latest version and any security advisories:
components/fiserv/common/utils.mjs (1)
1-26: Verify compatibility with Fiserv API requirements.
Let's ensure this utility can handle all data types required by the Fiserv API endpoints.
✅ Verification successful
Let me gather more specific information about how this utility is being used in the Fiserv checkout process.
Based on the create-checkout.mjs implementation, I can now generate a final response since I have enough context about how the parseJson utility is used.
The parseJson utility implementation is compatible with Fiserv API requirements
The utility correctly handles all data types required by the Fiserv checkout process:
- Recursively parses nested JSON strings
- Preserves non-string primitive values
- Handles null values appropriately
- Used correctly for parsing transactionAmount, order, checkoutSettings and paymentMethodDetails objects
The implementation safely handles potential JSON parsing errors by returning the original value if parsing fails, which is appropriate for this use case since some inputs may already be objects rather than JSON strings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Fiserv API usage patterns to verify data type handling
echo "Checking Fiserv API usage patterns..."
# Look for checkout creation calls
ast-grep --pattern 'parseJson($checkout)' || \
rg -A 5 'parseJson.*checkout'
# Look for specific Fiserv fields that need parsing
rg -l 'transactionAmount|checkoutSettings|paymentMethodDetails' components/fiserv/
# Check for any JSON schema definitions
fd -e json -e schema.json -x cat {} \; | jq . || true
Length of output: 135262
Script:
#!/bin/bash
# Look for usage of parseJson in create-checkout.mjs
echo "Checking create-checkout.mjs for parseJson usage..."
cat components/fiserv/actions/create-checkout/create-checkout.mjs
# Look for any test files that might validate the parsing
echo -e "\nChecking for test files..."
fd -e test.js -e test.mjs -e spec.js -e spec.mjs -x cat {} \; components/fiserv/
# Look for any schema or type definitions
echo -e "\nChecking for type definitions..."
fd -e d.ts -e types.ts -x cat {} \; components/fiserv/
Length of output: 5494
components/fiserv/actions/retrieve-checkout-details/retrieve-checkout-details.mjs (3)
9-17: LGTM! Props are well-defined
The props are properly typed and documented. The checkoutId prop is appropriately marked as required with a clear description.
1-8: Verify the imported app module exists
The import path seems correct, but let's verify the existence and exports of the Fiserv app module.
✅ Verification successful
Import path and app module are correctly implemented
The Fiserv app module exists at the correct location and exports all necessary methods including _makeRequest. The module is properly structured with authentication, URL handling, and request methods that will be needed by the retrieve-checkout-details action.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and structure of the Fiserv app module
# Expected: Should find the app module and confirm it exports required methods
# Check if the app file exists and contains necessary exports
rg -l "export default.*{" "components/fiserv/fiserv.app.mjs"
# Verify if the app module contains _makeRequest method
ast-grep --pattern 'export default {
$$$
_makeRequest($_) {
$$$
}
$$$
}' "components/fiserv/fiserv.app.mjs"
Length of output: 278
Script:
#!/bin/bash
# Let's try a simpler approach to verify the app module
# Check if the app file exists
ls -l components/fiserv/fiserv.app.mjs
# Check the content of the app file
cat components/fiserv/fiserv.app.mjs
# Alternative search for exports using ripgrep with fixed string
rg -F "export default" components/fiserv/fiserv.app.mjs
Length of output: 2532
23-23: Verify API endpoint matches documentation
Let's ensure the API endpoint matches the one specified in the Fiserv documentation.
✅ Verification successful
Let me gather more specific information about the API endpoint usage.
API endpoint is correctly implemented and documented
The endpoint /checkouts/${checkoutId} is correctly implemented as it matches the official Fiserv documentation referenced in the component's description: https://docs.fiserv.dev/public/reference/get-checkouts-id. The documentation link is embedded directly in the component, providing clear traceability to the API specification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify API endpoint consistency across the codebase
# Expected: Should find consistent usage of the checkouts endpoint
# Check for any constants defining the API path
rg -l "checkouts/" "components/fiserv/"
# Check for other usages of the checkouts endpoint
ast-grep --pattern 'path: `/checkouts/$_`'
Length of output: 403
Script:
#!/bin/bash
# Search for the complete file content to understand the context
cat components/fiserv/actions/retrieve-checkout-details/retrieve-checkout-details.mjs
# Look for any API documentation or configuration files
fd -e md -e json -e yaml -e yml . components/fiserv/ --exec grep -l "checkout"
# Search for any test files that might validate the endpoint
fd -e test.js -e test.mjs -e spec.js -e spec.mjs . components/fiserv/ --exec grep -l "checkout"
Length of output: 1315
components/fiserv/fiserv.app.mjs (3)
10-23: LGTM
The getAuth() method correctly retrieves and returns the necessary authentication details from this.$auth. The structure and variable assignments are clear and well-organized.
24-33: LGTM
The getUrl() method accurately constructs the full URL based on the environment and provided path. It correctly handles the sandbox environment and appends the appropriate API paths.
86-90: LGTM
The post(args = {}) method correctly abstracts the POST request by utilizing the _makeRequest method with the HTTP method set to "POST".
| }, | ||
| "dependencies": { | ||
| "@pipedream/platform": "3.0.3", | ||
| "crypto": "^1.0.1", |
There was a problem hiding this comment.
Remove unnecessary crypto dependency
The crypto module is a built-in Node.js module and should not be listed as an external dependency. It's available by default in Node.js environments.
Apply this diff to remove the unnecessary dependency:
"dependencies": {
"@pipedream/platform": "3.0.3",
- "crypto": "^1.0.1",
"uuid": "^11.0.3"
}📝 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.
| "crypto": "^1.0.1", |
| if (typeof(value) === "string") { | ||
| try { | ||
| return parseJson(JSON.parse(value)); | ||
| } catch (e) { | ||
| return value; | ||
| } | ||
| } else if (typeof(value) === "object" && value !== null) { |
There was a problem hiding this comment.
Add safety measures to prevent recursion-related issues.
The current implementation has potential security and stability issues:
- No maximum recursion depth limit could lead to stack overflow with deeply nested JSON.
- Silent error handling could mask important parsing issues.
Consider implementing these safety measures:
+const MAX_DEPTH = 100; // Prevent excessive recursion
-const parse = (value) => {
+const parse = (value, depth = 0) => {
+ if (depth > MAX_DEPTH) {
+ throw new Error('Maximum parsing depth exceeded');
+ }
if (typeof(value) === "string") {
try {
- return parseJson(JSON.parse(value));
+ return parse(JSON.parse(value), depth + 1);
} catch (e) {
+ // Log parsing errors in development
+ if (process.env.NODE_ENV === 'development') {
+ console.warn('JSON parsing failed:', e.message);
+ }
return 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.
| if (typeof(value) === "string") { | |
| try { | |
| return parseJson(JSON.parse(value)); | |
| } catch (e) { | |
| return value; | |
| } | |
| } else if (typeof(value) === "object" && value !== null) { | |
| const MAX_DEPTH = 100; // Prevent excessive recursion | |
| if (typeof(value) === "string") { | |
| try { | |
| return parse(JSON.parse(value), depth + 1); | |
| } catch (e) { | |
| // Log parsing errors in development | |
| if (process.env.NODE_ENV === 'development') { | |
| console.warn('JSON parsing failed:', e.message); | |
| } | |
| return value; | |
| } | |
| } else if (typeof(value) === "object" && value !== null) { |
| } else if (typeof(value) === "object" && value !== null) { | ||
| return Object.entries(value) | ||
| .reduce((acc, [ | ||
| key, | ||
| val, | ||
| ]) => Object.assign(acc, { | ||
| [key]: parse(val), | ||
| }), {}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve object handling robustness and efficiency.
The current implementation has several areas for improvement:
- No explicit handling of arrays
- No protection against circular references
- Object reconstruction could be more efficient
Consider this enhanced implementation:
-} else if (typeof(value) === "object" && value !== null) {
+} else if (value && typeof(value) === "object") {
+ // Handle circular references
+ if (seen.has(value)) {
+ throw new Error('Circular reference detected');
+ }
+ seen.add(value);
+
+ // Handle arrays
+ if (Array.isArray(value)) {
+ return value.map(item => parse(item, depth + 1));
+ }
+
+ // Handle objects more efficiently
+ const result = {};
+ for (const [key, val] of Object.entries(value)) {
+ result[key] = parse(val, depth + 1);
+ }
+ seen.delete(value);
+ return result;
- return Object.entries(value)
- .reduce((acc, [
- key,
- val,
- ]) => Object.assign(acc, {
- [key]: parse(val),
- }), {});Committable suggestion skipped: line range outside the PR's diff.
| async run({ $ }) { | ||
| const { | ||
| getCheckoutDetails, | ||
| checkoutId, | ||
| } = this; | ||
|
|
||
| const response = await getCheckoutDetails({ | ||
| $, | ||
| checkoutId, | ||
| }); | ||
|
|
||
| $.export("$summary", `Successfully retrieved details for checkout ID ${this.checkoutId}`); | ||
| return response; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and fix inconsistent property access
The run function needs several improvements:
- Fix inconsistent use of
this.checkoutIdvscheckoutId - Add error handling for API calls
- Add response validation
async run({ $ }) {
const {
getCheckoutDetails,
checkoutId,
} = this;
+ try {
const response = await getCheckoutDetails({
$,
checkoutId,
});
- $.export("$summary", `Successfully retrieved details for checkout ID ${this.checkoutId}`);
+ if (!response || typeof response !== 'object') {
+ throw new Error('Invalid response received from API');
+ }
+
+ $.export("$summary", `Successfully retrieved details for checkout ID ${checkoutId}`);
return response;
+ } catch (error) {
+ throw new Error(`Failed to retrieve checkout details: ${error.message}`);
+ }
},📝 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.
| async run({ $ }) { | |
| const { | |
| getCheckoutDetails, | |
| checkoutId, | |
| } = this; | |
| const response = await getCheckoutDetails({ | |
| $, | |
| checkoutId, | |
| }); | |
| $.export("$summary", `Successfully retrieved details for checkout ID ${this.checkoutId}`); | |
| return response; | |
| }, | |
| async run({ $ }) { | |
| const { | |
| getCheckoutDetails, | |
| checkoutId, | |
| } = this; | |
| try { | |
| const response = await getCheckoutDetails({ | |
| $, | |
| checkoutId, | |
| }); | |
| if (!response || typeof response !== 'object') { | |
| throw new Error('Invalid response received from API'); | |
| } | |
| $.export("$summary", `Successfully retrieved details for checkout ID ${checkoutId}`); | |
| return response; | |
| } catch (error) { | |
| throw new Error(`Failed to retrieve checkout details: ${error.message}`); | |
| } | |
| }, |
| methods: { | ||
| getCheckoutDetails({ | ||
| checkoutId, ...args | ||
| } = {}) { | ||
| return this.app._makeRequest({ | ||
| path: `/checkouts/${checkoutId}`, | ||
| ...args, | ||
| }); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling
The current implementation could be more robust. Consider these improvements:
- Validate checkoutId before making the request
- Add error handling for edge cases
- Be more explicit about which parameters can be overridden via args
methods: {
getCheckoutDetails({
checkoutId, ...args
} = {}) {
+ if (!checkoutId || typeof checkoutId !== 'string') {
+ throw new Error('Invalid checkoutId provided');
+ }
+ const allowedParams = ['$'];
+ const invalidParams = Object.keys(args).filter(key => !allowedParams.includes(key));
+ if (invalidParams.length > 0) {
+ throw new Error(`Invalid parameters provided: ${invalidParams.join(', ')}`);
+ }
return this.app._makeRequest({
path: `/checkouts/${checkoutId}`,
- ...args,
+ $: args.$,
});
},
},📝 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.
| methods: { | |
| getCheckoutDetails({ | |
| checkoutId, ...args | |
| } = {}) { | |
| return this.app._makeRequest({ | |
| path: `/checkouts/${checkoutId}`, | |
| ...args, | |
| }); | |
| }, | |
| }, | |
| methods: { | |
| getCheckoutDetails({ | |
| checkoutId, ...args | |
| } = {}) { | |
| if (!checkoutId || typeof checkoutId !== 'string') { | |
| throw new Error('Invalid checkoutId provided'); | |
| } | |
| const allowedParams = ['$']; | |
| const invalidParams = Object.keys(args).filter(key => !allowedParams.includes(key)); | |
| if (invalidParams.length > 0) { | |
| throw new Error(`Invalid parameters provided: ${invalidParams.join(', ')}`); | |
| } | |
| return this.app._makeRequest({ | |
| path: `/checkouts/${checkoutId}`, | |
| $: args.$, | |
| }); | |
| }, | |
| }, |
| async run({ $ }) { | ||
| const { | ||
| createCheckout, | ||
| storeId, | ||
| merchantTransactionId, | ||
| transactionOrigin, | ||
| transactionType, | ||
| transactionAmount, | ||
| order, | ||
| checkoutSettings, | ||
| paymentMethodDetails, | ||
| } = this; | ||
|
|
||
| const response = await createCheckout({ | ||
| $, | ||
| data: { | ||
| storeId, | ||
| merchantTransactionId, | ||
| transactionOrigin, | ||
| transactionType, | ||
| transactionAmount: utils.parseJson(transactionAmount), | ||
| order: utils.parseJson(order), | ||
| checkoutSettings: utils.parseJson(checkoutSettings), | ||
| paymentMethodDetails: utils.parseJson(paymentMethodDetails), | ||
| }, | ||
| }); | ||
|
|
||
| $.export("$summary", `Successfully created checkout. Redirect URL: ${response.redirectionUrl}`); | ||
| return response; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure proper error handling for JSON parsing and API requests.
When parsing JSON inputs (transactionAmount, order, checkoutSettings, paymentMethodDetails), consider handling potential parsing errors to prevent the application from crashing due to invalid JSON input. Additionally, ensure that errors from the createCheckout API request are properly caught and handled to inform the user appropriately.
Consider wrapping the parsing calls with try-catch blocks or using a safe parsing method that returns meaningful errors if parsing fails.
| transactionAmount: { | ||
| type: "object", | ||
| label: "Transaction Amount", | ||
| description: "Object contains `total` transaction amount, `currency`, tax and discount details. Example: `{\"total\":123,\"currency\":\"EUR\",\"components\":{\"subtotal\":115,\"vat\":3,\"shipping\":2.5}}`", | ||
| optional: true, | ||
| }, |
There was a problem hiding this comment.
Inconsistent handling of 'transactionAmount' prop type and unnecessary JSON parsing.
The transactionAmount prop is declared as type object, but in the run method (line 116), it is parsed using utils.parseJson(transactionAmount). Since transactionAmount is already an object, parsing it as JSON is unnecessary and may cause errors if the object is not serializable. Consider either changing the prop type to string if you expect JSON input as a string, or removing the JSON parsing if an object is expected.
If you expect transactionAmount to be an object, apply this diff:
- transactionAmount: utils.parseJson(transactionAmount),
+ transactionAmount: transactionAmount,Alternatively, if you expect input as a JSON string, change the prop type to string:
- transactionAmount: {
- type: "object",
+ transactionAmount: {
+ type: "string",Committable suggestion skipped: line range outside the PR's diff.
| getSignatureHeaders(data) { | ||
| const { | ||
| apiKey, | ||
| secretKey, | ||
| environment, | ||
| } = this.getAuth(); | ||
|
|
||
| if (environment === constants.ENVIRONMENT.SANDBOX) { | ||
| return; | ||
| } | ||
|
|
||
| const clientRequestId = uuid(); | ||
| const timestamp = Date.now().toString(); | ||
| const requestBody = JSON.stringify(data) || ""; | ||
| const rawSignature = apiKey + clientRequestId + timestamp + requestBody; | ||
|
|
||
| const computedHmac = | ||
| crypto.createHmac("sha256", secretKey) | ||
| .update(rawSignature) | ||
| .digest("base64"); | ||
|
|
||
| return { | ||
| "Client-Request-Id": clientRequestId, | ||
| "Message-Signature": computedHmac, | ||
| "Timestamp": timestamp, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Ensure getSignatureHeaders returns an object to prevent errors
In the getSignatureHeaders(data) method, when the environment is set to SANDBOX, the function returns undefined due to return; on line 45. Later, in the _makeRequest method (line 82), spreading undefined into the headers object will cause a runtime error. To prevent this, ensure that getSignatureHeaders always returns an object, even if it's empty.
Apply this diff to fix the issue:
if (environment === constants.ENVIRONMENT.SANDBOX) {
- return;
+ return {};
}📝 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.
| getSignatureHeaders(data) { | |
| const { | |
| apiKey, | |
| secretKey, | |
| environment, | |
| } = this.getAuth(); | |
| if (environment === constants.ENVIRONMENT.SANDBOX) { | |
| return; | |
| } | |
| const clientRequestId = uuid(); | |
| const timestamp = Date.now().toString(); | |
| const requestBody = JSON.stringify(data) || ""; | |
| const rawSignature = apiKey + clientRequestId + timestamp + requestBody; | |
| const computedHmac = | |
| crypto.createHmac("sha256", secretKey) | |
| .update(rawSignature) | |
| .digest("base64"); | |
| return { | |
| "Client-Request-Id": clientRequestId, | |
| "Message-Signature": computedHmac, | |
| "Timestamp": timestamp, | |
| }; | |
| }, | |
| getSignatureHeaders(data) { | |
| const { | |
| apiKey, | |
| secretKey, | |
| environment, | |
| } = this.getAuth(); | |
| if (environment === constants.ENVIRONMENT.SANDBOX) { | |
| return {}; | |
| } | |
| const clientRequestId = uuid(); | |
| const timestamp = Date.now().toString(); | |
| const requestBody = JSON.stringify(data) || ""; | |
| const rawSignature = apiKey + clientRequestId + timestamp + requestBody; | |
| const computedHmac = | |
| crypto.createHmac("sha256", secretKey) | |
| .update(rawSignature) | |
| .digest("base64"); | |
| return { | |
| "Client-Request-Id": clientRequestId, | |
| "Message-Signature": computedHmac, | |
| "Timestamp": timestamp, | |
| }; | |
| }, |
jcortes
force-pushed
the
fiserv-new-components
branch
from
0dc4ec7
to
0b992cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
components/fiserv/fiserv.app.mjs (2)
24-33: Add path validation to prevent URL injection.While the URL construction is correct, it's recommended to validate the path parameter to prevent potential URL injection attacks.
Consider adding path validation:
getUrl(path, apiPath = constants.API_PATH.DEFAULT) { + if (!path || typeof path !== 'string' || path.includes('..')) { + throw new Error('Invalid path parameter'); + } const { url, environment, } = this.getAuth(); const baseUrl = environment === constants.ENVIRONMENT.SANDBOX ? `${url}${constants.SANDBOX_PATH}${apiPath}` : `${url}${apiPath}`; return `${baseUrl}${path}`; },
72-85: Consider enhancing request handling robustness.While the request implementation is functional, consider these improvements:
- Make debug mode configurable rather than always enabled
- Add retry mechanism for failed requests
- Add request timeout configuration
Consider this enhancement:
_makeRequest({ - $ = this, path, headers, data, apiPath, ...args + $ = this, + path, + headers, + data, + apiPath, + debug = false, + timeout = 10000, + retries = 3, + ...args } = {}) { - return axios($, { + const config = { ...args, - debug: true, + debug, url: this.getUrl(path, apiPath), data, + timeout, headers: { ...this.getHeaders(headers), ...this.getSignatureHeaders(data), }, - }); + }; + + return axios($, config).catch((error) => { + if (retries > 0 && error.response?.status >= 500) { + return this._makeRequest({ + $, + path, + headers, + data, + apiPath, + debug, + timeout, + retries: retries - 1, + ...args, + }); + } + throw error; + }); },components/fiserv/actions/create-checkout/create-checkout.mjs (1)
123-123: Enhance the summary message with more transaction details.The current summary only includes the redirect URL. Consider adding more relevant details like the transaction ID or amount to make it more informative.
Apply this diff:
- $.export("$summary", `Successfully created checkout. Redirect URL: ${response.redirectionUrl}`); + $.export("$summary", `Successfully created checkout for ${transactionType} transaction. Amount: ${transactionAmount?.total} ${transactionAmount?.currency}. Redirect URL: ${response.redirectionUrl}`);
📜 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 (6)
components/fiserv/actions/create-checkout/create-checkout.mjs(1 hunks)components/fiserv/actions/retrieve-checkout-details/retrieve-checkout-details.mjs(1 hunks)components/fiserv/common/constants.mjs(1 hunks)components/fiserv/common/utils.mjs(1 hunks)components/fiserv/fiserv.app.mjs(1 hunks)components/fiserv/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- components/fiserv/actions/retrieve-checkout-details/retrieve-checkout-details.mjs
- components/fiserv/common/constants.mjs
- components/fiserv/common/utils.mjs
- components/fiserv/package.json
🔇 Additional comments (7)
components/fiserv/fiserv.app.mjs (4)
1-4: LGTM! Appropriate imports for secure API integration.
The imports are well-chosen for the required functionality:
uuidfor generating unique request IDscryptofor secure HMAC signature generationaxiosfrom the platform for HTTP requests
44-46: Return empty object in sandbox mode to prevent runtime errors.
When the environment is set to SANDBOX, the function returns undefined. This will cause a runtime error when spread into headers object.
48-62: LGTM! Secure implementation of message signing.
The HMAC signature generation follows Fiserv's documentation correctly:
- Uses UUID v4 for unique request IDs
- Includes all required components in the signature
- Properly handles empty request bodies
1-93: Verify secure coding practices for payment processing.
As this is a payment processing integration, let's verify that secure coding practices are followed throughout the codebase.
components/fiserv/actions/create-checkout/create-checkout.mjs (3)
1-8: LGTM! Well-structured component definition.
The component is properly defined with clear documentation and version numbering appropriate for a new component.
88-95: LGTM! Clean method implementation.
The createCheckout method is well-implemented with proper parameter handling.
96-125: Verify required fields against the Fiserv API documentation.
Let's ensure all required fields from the API documentation are properly handled in the implementation.
| getAuth() { | ||
| const { | ||
| url, | ||
| api_key: apiKey, | ||
| secret_key: secretKey, | ||
| environment = constants.ENVIRONMENT.SANDBOX, | ||
| } = this.$auth; | ||
| return { | ||
| url, | ||
| apiKey, | ||
| secretKey, | ||
| environment, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required authentication properties.
While the method correctly extracts auth properties, it should validate that required properties (url, api_key, secret_key) are present to fail fast and provide clear error messages.
Consider adding validation:
getAuth() {
const {
url,
api_key: apiKey,
secret_key: secretKey,
environment = constants.ENVIRONMENT.SANDBOX,
} = this.$auth;
+ if (!url || !apiKey || !secretKey) {
+ throw new Error('Missing required authentication properties: url, api_key, and secret_key must be provided');
+ }
return {
url,
apiKey,
secretKey,
environment,
};
},📝 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.
| getAuth() { | |
| const { | |
| url, | |
| api_key: apiKey, | |
| secret_key: secretKey, | |
| environment = constants.ENVIRONMENT.SANDBOX, | |
| } = this.$auth; | |
| return { | |
| url, | |
| apiKey, | |
| secretKey, | |
| environment, | |
| }; | |
| }, | |
| getAuth() { | |
| const { | |
| url, | |
| api_key: apiKey, | |
| secret_key: secretKey, | |
| environment = constants.ENVIRONMENT.SANDBOX, | |
| } = this.$auth; | |
| if (!url || !apiKey || !secretKey) { | |
| throw new Error('Missing required authentication properties: url, api_key, and secret_key must be provided'); | |
| } | |
| return { | |
| url, | |
| apiKey, | |
| secretKey, | |
| environment, | |
| }; | |
| }, |
| order: { | ||
| type: "string", | ||
| label: "Order", | ||
| description: "Object contains order related details. [See the documentation](https://docs.fiserv.dev/public/reference/postcheckouts).", | ||
| optional: true, | ||
| }, |
There was a problem hiding this comment.
Inconsistent type definition for order prop.
The order prop is defined as type string but the documentation indicates it should be an object. This creates confusion and requires unnecessary JSON parsing. Consider changing the type to object to maintain consistency with the API expectations.
Apply this diff:
order: {
- type: "string",
+ type: "object",
label: "Order",
description: "Object contains order related details. [See the documentation](https://docs.fiserv.dev/public/reference/postcheckouts).",
optional: true,
},📝 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.
| order: { | |
| type: "string", | |
| label: "Order", | |
| description: "Object contains order related details. [See the documentation](https://docs.fiserv.dev/public/reference/postcheckouts).", | |
| optional: true, | |
| }, | |
| order: { | |
| type: "object", | |
| label: "Order", | |
| description: "Object contains order related details. [See the documentation](https://docs.fiserv.dev/public/reference/postcheckouts).", | |
| optional: true, | |
| }, |
| const response = await createCheckout({ | ||
| $, | ||
| data: { | ||
| storeId, | ||
| merchantTransactionId, | ||
| transactionOrigin, | ||
| transactionType, | ||
| transactionAmount: utils.parseJson(transactionAmount), | ||
| order: utils.parseJson(order), | ||
| checkoutSettings: utils.parseJson(checkoutSettings), | ||
| paymentMethodDetails: utils.parseJson(paymentMethodDetails), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add response validation to ensure required fields are present.
The Fiserv API response should be validated to ensure it contains the expected fields before using them.
Add validation before using the response:
const response = await createCheckout({
$,
data: {
storeId,
merchantTransactionId,
transactionOrigin,
transactionType,
transactionAmount: utils.parseJson(transactionAmount),
order: utils.parseJson(order),
checkoutSettings: utils.parseJson(checkoutSettings),
paymentMethodDetails: utils.parseJson(paymentMethodDetails),
},
});
+
+if (!response?.redirectionUrl) {
+ throw new Error('Invalid response: missing redirectionUrl');
+}Committable suggestion skipped: line range outside the PR's diff.
WHY
Resolves #14562
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Dependencies
package.jsonto include new dependencies for enhanced functionality.