Stored constraint plugin #700
Conversation
![flatfile-nullify[bot]](https://avatars.githubusercontent.com/u/31830338?s=60&v=4){kind=small}
Nullify Code Vulnerabilities3 findings found in this pull request
You can find a list of all findings here |
meritmalling
force-pushed
the
feat/stored-constraints-plugin
branch
from
197edfe to
f6f67f1
Compare
WalkthroughThis pull request introduces the Changes
Possibly related PRs
Suggested reviewers
🪧 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: 7
🧹 Outside diff range and nitpick comments (7)
plugins/stored-constraints/README.md (1)
1-4: Fix typo in plugin descriptionThere's a grammatical error in the description.
-This plugin enabled running stored constraints. +This plugin enables running stored constraints.plugins/stored-constraints/src/utils.ts (2)
1-6: Consider dependency injection for API clientInstead of creating a global API client instance, consider injecting it as a parameter to functions that need it. This would improve testability and make dependencies explicit.
-const api = new FlatfileClient() +export const createUtils = (api: FlatfileClient) => ({ + getSheet: (event: FlatfileEvent) => api.sheets.get(event.context.sheetId), + getAppConstraints: (a) => api.apps.getConstraints(a, { includeBuiltins: true }) +})
48-52: Add documentation and improve type safety for utility functionsThe array manipulation utilities would benefit from:
- JSDoc documentation explaining their purpose and usage
- Type annotations for parameters and return values
- Input validation
+/** + * Computes the Cartesian product of multiple arrays + * @param arrays - Arrays to compute the product of + * @returns Array of all possible combinations + */ -export const crossProduct = (...a) => +export const crossProduct = <T>(...arrays: T[][]): T[][] => a.reduce((u, c) => _.flatMap(u, (a) => c.map((b) => a.concat(b))), [[]]) +/** + * Applies a callback to each combination in the Cartesian product + * @param arrays - Arrays to compute combinations from + * @param callback - Function to apply to each combination + */ -export const crossEach = (a, cb) => +export const crossEach = <T>(arrays: T[][], callback: (...args: T[]) => void): void => crossProduct(...a).forEach((p) => cb(...p)) +/** + * Fetches application constraints including built-ins + * @param appId - Application identifier + * @returns Promise resolving to constraints + */ -export const getAppConstraints = (a) => +export const getAppConstraints = (appId: string) => api.apps.getConstraints(a, { includeBuiltins: true })plugins/stored-constraints/src/stored.constraint.ts (4)
13-14: Remove Unused Imports for Cleaner CodeThe imports
validatorfrom'validator',countryStateCityfrom'country-state-city', andDateTimefrom'luxon'are assigned to thedepsobject but are not directly used within this file.Consider removing these imports if they are not utilized elsewhere to keep the codebase clean and maintainable.
18-18: Clarify the Purpose of thedepsObjectThe
depsobject is created but not used within this file.If
depsis intended for future use or to be passed to other functions, please ensure it is utilized appropriately. Otherwise, consider removing it to avoid confusion.
49-49: Specify Field Type for Better Type DefinitionsThe parameter
field: anylacks a specific type, which can lead to type inconsistencies.Define a type or interface for
fieldto ensure clarity and maintainability. For example:import type { FlatfileRecord } from '@flatfile/hooks'; +import type { SheetField } from '@flatfile/api'; // Assuming this is the correct import path // ... [records, storedConstraintFields], -(record: FlatfileRecord, field: any) => { +(record: FlatfileRecord, field: SheetField) => {
20-24: Document theConstraintInterface for ClarityAdding comments to the
Constraintinterface properties will improve code readability and maintainability.For example:
export interface Constraint { /** The name of the validator function */ validator: string; /** The function code as a string */ function: string; /** Optional type information */ type?: string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonplugins/stored-constraints/package.jsonis excluded by!**/*.json
📒 Files selected for processing (7)
plugins/stored-constraints/CHANGELOG.md(1 hunks)plugins/stored-constraints/README.md(1 hunks)plugins/stored-constraints/jest.config.cjs(1 hunks)plugins/stored-constraints/src/index.ts(1 hunks)plugins/stored-constraints/src/stored.constraint.ts(1 hunks)plugins/stored-constraints/src/utils.ts(1 hunks)plugins/stored-constraints/tsup.config.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- plugins/stored-constraints/CHANGELOG.md
- plugins/stored-constraints/jest.config.cjs
- plugins/stored-constraints/src/index.ts
- plugins/stored-constraints/tsup.config.mjs
🧰 Additional context used
🪛 Biome
plugins/stored-constraints/src/utils.ts
[error] 33-33: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 38-38: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 39-39: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (2)
plugins/stored-constraints/src/stored.constraint.ts (2)
26-34: Handle Potential Null or Undefined Values in Constraints
When mapping over constraints.data, ensure that c.validator and c.function are defined to prevent runtime errors.
Add checks or default values in case these properties are missing.
55-60: Ensure Dependencies Are Passed Correctly
Verify that the deps object contains all necessary dependencies required by applyConstraintToRecord.
Run the following script to check if all dependencies used in applyConstraintToRecord are included in deps:
✅ Verification successful
Let me try to locate the correct file and analyze the dependencies.
Let me try a different approach to locate and analyze the function.
Let me check how these dependencies are used within the applyConstraintToRecord function.
From the code analysis, I can now provide a final response:
Dependencies Are Correctly Passed
The deps object containing validator, countryStateCity, and luxon is correctly passed through to the constraint function. The implementation shows that these dependencies are forwarded to the evaluated constraint function as part of the options object, allowing constraints to access these utilities as needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all dependencies required by applyConstraintToRecord are present in deps.
# Find all usages of deps properties in applyConstraintToRecord
rg -A 5 'function applyConstraintToRecord' ./utils.ts | rg 'deps\.'
# List properties of deps
echo "Properties in deps:"
echo "validator, countryStateCity, luxon"
Length of output: 248
Script:
#!/bin/bash
# Find the file containing applyConstraintToRecord function
fd applyConstraintToRecord
# Once we find the file, let's examine its content
ast-grep --pattern 'function applyConstraintToRecord($$$) {
$$$
}'
# Also search for any usage of deps within the function
rg -A 2 'deps\.'
Length of output: 118
Script:
#!/bin/bash
# First, let's find the file containing applyConstraintToRecord using ripgrep
rg -l "applyConstraintToRecord"
# Then let's examine the function implementation and its usage of deps
rg -B 5 -A 15 "applyConstraintToRecord"
# Also search for any type definitions or interfaces related to deps
rg "interface.*Deps" -A 5
Length of output: 4938
Script:
#!/bin/bash
# Get the full implementation of applyConstraintToRecord to see how deps are used
rg -B 5 -A 30 "export const applyConstraintToRecord = \(" plugins/stored-constraints/src/utils.ts
# Also search for any usage of deps.validator, deps.countryStateCity, or deps.luxon
rg "deps\.(validator|countryStateCity|luxon)" plugins/stored-constraints/src/utils.ts
Length of output: 1288
Signed-off-by: Merit <[email protected]>
Signed-off-by: Merit <[email protected]>
carlbrugger
force-pushed
the
feat/stored-constraints-plugin
branch
from
ec5d2a8 to
33b7a96
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
plugins/stored-constraints/src/utils.ts (2)
1-6: Consider making the API client configurableThe FlatfileClient instance is created as a global constant. Consider making it configurable to:
- Support different environments (development, production)
- Enable dependency injection for better testability
- Allow custom configuration options
-const api = new FlatfileClient() +export const createClient = (config?: FlatfileClientConfig) => new FlatfileClient(config) +export const api = createClient()
48-52: Improve helper functions with types and error handlingThe helper functions need:
- Type definitions for parameters and return values
- Error handling for the API call
- Documentation for complex operations
-export const crossProduct = (...a) => +export const crossProduct = <T>(...arrays: T[][]): T[][] => a.reduce((u, c) => _.flatMap(u, (a) => c.map((b) => a.concat(b))), [[]]) -export const crossEach = (a, cb) => +export const crossEach = <T>(arrays: T[][], callback: (...args: T[]) => void): void => crossProduct(...arrays).forEach((p) => callback(...p)) -export const getAppConstraints = (a) => +export const getAppConstraints = async (appId: string) => { + try { + return await api.apps.getConstraints(appId, { includeBuiltins: true }); + } catch (error) { + throw new Error(`Failed to fetch app constraints: ${error.message}`); + } +}plugins/stored-constraints/src/stored.constraint.ts (1)
20-24: Add JSDoc documentation for the Constraint interface.Adding documentation would help other developers understand the purpose of each field and how to use them correctly.
+/** + * Represents a constraint configuration for validation. + * @interface Constraint + * @property {string} validator - The identifier of the validator to be used + * @property {string} function - The validation function to be executed + * @property {string} [type] - Optional type specification for the constraint + */ export interface Constraint { validator: string function: string type?: string }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonplugins/stored-constraints/package.jsonis excluded by!**/*.json
📒 Files selected for processing (7)
plugins/stored-constraints/CHANGELOG.md(1 hunks)plugins/stored-constraints/README.md(1 hunks)plugins/stored-constraints/src/index.ts(1 hunks)plugins/stored-constraints/src/stored.constraint.ts(1 hunks)plugins/stored-constraints/src/utils.ts(1 hunks)plugins/stored-constraints/tsup.config.mjs(1 hunks)plugins/stored-constraints/vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugins/stored-constraints/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/stored-constraints/CHANGELOG.md
- plugins/stored-constraints/README.md
- plugins/stored-constraints/src/index.ts
- plugins/stored-constraints/tsup.config.mjs
🧰 Additional context used
🪛 Biome (1.9.4)
plugins/stored-constraints/src/utils.ts
[error] 33-33: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 38-38: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 39-39: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (3)
plugins/stored-constraints/src/utils.ts (1)
20-47:
Critical: Replace unsafe eval() with secure alternatives
The current implementation has several critical issues:
- Use of eval() creates severe security vulnerabilities
- No return value is captured from constraintFn
- Lack of error handling for function execution
Consider using a secure alternative like vm2 for sandboxed execution:
import { VM } from 'vm2';
export const applyConstraintToRecord = (
constraint: Constraint,
record: any,
field: { key: string; constraints?: Constraint[] },
deps: any,
sheet: any
): any => {
try {
const storedConstraint = field.constraints?.find(
(fieldConstraint) => fieldConstraint.validator === constraint.validator
);
const { config = {} } = storedConstraint || {};
const vm = new VM({
timeout: 1000,
sandbox: {
record: record,
field: field,
deps: deps,
sheet: sheet,
config: config
}
});
// Prepare the function in a safe context
const fnWrapper = `
(function(value, key, context) {
${constraint.function}
})
`;
const constraintFn = vm.run(fnWrapper);
return constraintFn(record.get(field.key), field.key, {
config,
record,
deps,
sheet,
});
} catch (error) {
throw new Error(`Failed to execute constraint: ${error.message}`);
}
}Let's verify the constraint function usage patterns:
🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 38-38: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 39-39: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
plugins/stored-constraints/src/stored.constraint.ts (2)
12-14: Consider pinning external dependency versions.
The external dependencies (validator, country-state-city, luxon) should have their versions pinned to ensure consistent behavior and avoid potential breaking changes.
2-11: Verify utility function exports.
Ensure all required utility functions are properly exported from './utils'.
✅ Verification successful
All utility functions are properly exported
All functions imported in the stored.constraint.ts file are correctly exported from utils.ts:
applyConstraintToRecord: exported as functioncrossEach: exported as functiongetStoredConstraints: exported as functiongetFields: exported as functiongetSheet: exported as functiongetValidator: exported as functionhasStoredConstraints: exported as functiongetAppConstraints: exported as function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all required utilities are exported from utils
ast-grep --pattern 'export function $UTIL' ./src/utils.ts
Length of output: 121
Script:
#!/bin/bash
# First, let's find the correct path to utils file
fd utils.ts
# Then check the exports
ast-grep --pattern 'export function $_'
Length of output: 565
Script:
#!/bin/bash
# Now that we found the correct path, let's check the exports in the utils file
cat plugins/stored-constraints/src/utils.ts
# Also check if these functions are exported as named exports
ast-grep --pattern 'export { $_ }' plugins/stored-constraints/src/utils.ts
Length of output: 1800
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
plugins/stored-constraints/src/utils.ts (1)
6-6: Consider adding configuration options to FlatfileClient initializationThe client initialization could benefit from configuration options such as timeout, retries, or custom headers for better error handling and reliability.
-const api = new FlatfileClient() +const api = new FlatfileClient({ + timeout: 30000, + retries: 3, + headers: { + 'X-Client-Name': '@flatfile/plugin-stored-constraints' + } +})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonplugins/stored-constraints/package.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
plugins/stored-constraints/src/stored.constraint.ts(1 hunks)plugins/stored-constraints/src/utils.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
plugins/stored-constraints/src/utils.ts
[error] 41-41: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 46-46: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 47-47: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (4)
plugins/stored-constraints/src/stored.constraint.ts (3)
1-20: LGTM! Well-organized imports and dependency management.
The imports are properly structured with clear type definitions, and the deps object provides a clean way to manage external dependencies.
21-25: LGTM! Clear and concise interface definition.
The Constraint interface provides a well-defined structure for constraint objects.
37-61:
Critical: Fix async operation handling in the constraint processing.
The current implementation has several critical issues that could lead to race conditions and unexpected behavior:
- Async operations in forEach loop won't be awaited
- Missing await for crossEach execution
- Lack of proper error handling
The issues identified in the previous reviews are still valid. Here's a comprehensive fix that addresses all the problems:
export function storedConstraint() {
return bulkRecordHook(
'**',
async (records: FlatfileRecord[], event: FlatfileEvent) => {
+ try {
const sheet: Flatfile.SheetResponse = await getSheet(event)
const storedConstraintFields =
getFields(sheet).filter(hasStoredConstraints)
const validators = await getValidators(event)
- crossEach(
+ await crossEach(
[records, storedConstraintFields],
- (record: FlatfileRecord, field: Flatfile.Property) => {
- getStoredConstraints(field.constraints).forEach(
- async ({ validator }: { validator: string }) => {
- const constraint = await getValidator(validators, validator)
- if (constraint) {
- applyConstraintToRecord(constraint, record, field, deps, sheet)
- }
- }
- )
+ async (record: FlatfileRecord, field: Flatfile.Property) => {
+ const constraints = getStoredConstraints(field.constraints)
+ for (const { validator } of constraints) {
+ try {
+ const constraint = await getValidator(validators, validator)
+ if (constraint) {
+ await applyConstraintToRecord(constraint, record, field, deps, sheet)
+ }
+ } catch (error) {
+ record.addError([field.key], `Constraint validation failed: ${error.message}`)
+ }
+ }
}
)
+ } catch (error) {
+ event.logger.error('Failed to process stored constraints:', error)
+ throw error
+ }
}
)
}plugins/stored-constraints/src/utils.ts (1)
28-55:
Critical: Replace unsafe eval() with secure function execution
The current implementation using eval() poses a severe security risk as it allows execution of arbitrary code. This needs immediate attention.
I notice this issue was previously flagged. Here's a secure implementation using the VM2 sandbox:
+import { VM } from 'vm2'
+
export const applyConstraintToRecord = (
constraint: Constraint,
record: FlatfileRecord,
field: Flatfile.Property,
deps: any,
sheet: Flatfile.SheetResponse
) => {
const storedConstraint = getStoredConstraints(field.constraints || []).find(
(fieldConstraint) => fieldConstraint.validator === constraint.validator
)
const { config = {} } = storedConstraint || {}
- const constraintFn = constraint.function.startsWith('function')
- ? constraint.function.includes('function constraint')
- ? eval(
- '(' +
- constraint.function.replace('function constraint', 'function') +
- ')'
- )
- : eval('(' + constraint.function + ')')
- : eval(constraint.function)
+ const vm = new VM({
+ timeout: 1000, // Prevent infinite loops
+ sandbox: {
+ value: record.get(field.key),
+ key: field.key,
+ context: {
+ config,
+ record,
+ deps,
+ sheet
+ }
+ }
+ })
+
+ try {
+ const wrappedFn = `
+ (function() {
+ const fn = ${constraint.function};
+ return fn(value, key, context);
+ })()
+ `
+ return vm.run(wrappedFn)
+ } catch (error) {
+ throw new Error(`Failed to execute constraint: ${error.message}`)
+ }
}🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 46-46: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 47-47: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
plugins/stored-constraints/src/utils.ts (2)
21-27: Improve type safety for constraint-related functionsAdd proper type definitions to improve code maintainability and prevent potential runtime errors.
+interface Field { + constraints?: Flatfile.Constraint[] +} -export const hasStoredConstraints = (field) => +export const hasStoredConstraints = (field: Field): boolean => getStoredConstraints(field.constraints || []).length > 0 -export const getValidator: ( - v: Constraint[], - n: string -) => Constraint | undefined = (v, n) => v.find((w) => w.validator === n) +export const getValidator = (validators: Constraint[], name: string): Constraint | undefined => + validators.find((v) => v.validator === name)
59-59: Add type safety to crossEach functionThe function should include proper type definitions for its parameters.
-export const crossEach = (a, cb) => crossProduct(...a).forEach((p) => cb(...p)) +export const crossEach = <T>(arrays: T[][], callback: (...args: T[]) => void): void => + crossProduct(...arrays).forEach((params) => callback(...params))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
plugins/stored-constraints/src/utils.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
plugins/stored-constraints/src/utils.ts
[error] 42-42: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 47-47: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 48-48: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (3)
plugins/stored-constraints/src/utils.ts (3)
1-6: LGTM: Imports and client initialization are well-structured
The necessary types and clients are properly imported, and the FlatfileClient is correctly initialized.
57-58: LGTM: Well-implemented cross product function
The implementation is type-safe and efficiently uses array operations.
29-56:
CRITICAL: Replace unsafe eval() with secure alternatives
The current implementation using eval() poses a severe security risk as it allows execution of arbitrary code.
I notice this issue was previously flagged. Here's a secure implementation using VM2:
import { VM } from 'vm2';
export const applyConstraintToRecord = (
constraint: Constraint,
record: FlatfileRecord,
field: Flatfile.Property,
deps: any,
sheet: Flatfile.SheetResponse
) => {
const storedConstraint = getStoredConstraints(field.constraints || []).find(
(fieldConstraint) => fieldConstraint.validator === constraint.validator
);
const vm = new VM({
timeout: 1000, // Prevent infinite loops
sandbox: {
value: record.get(field.key),
key: field.key,
config: storedConstraint?.config ?? {},
record,
deps,
sheet
}
});
try {
return vm.run(`(${constraint.function})(value, key, { config, record, deps, sheet })`);
} catch (error) {
throw new Error(`Failed to execute constraint: ${error.message}`);
}
}🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 47-47: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 48-48: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
carlbrugger
force-pushed
the
feat/stored-constraints-plugin
branch
from
008ec2a to
b42ea42
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
plugins/stored-constraints/src/utils.ts (2)
6-6: Consider making the API client configurableThe FlatfileClient is initialized without any configuration options. Consider making it configurable to support different environments and testing scenarios.
-const api = new FlatfileClient() +const api = new FlatfileClient({ + token: process.env.FLATFILE_API_KEY, + environment: process.env.FLATFILE_ENV +})
16-28: Enhance type safety and null handlingThe constraint management functions could be improved with better type safety and null handling.
export const getStoredConstraints = ( - constraints: Flatfile.Constraint[] + constraints: Flatfile.Constraint[] | undefined | null ): Flatfile.StoredConstraint[] => - constraints?.filter((c) => c.type === 'stored') + (constraints || []).filter((c): c is Flatfile.StoredConstraint => c.type === 'stored') export const hasStoredConstraints = (field: Flatfile.Property): boolean => getStoredConstraints(field.constraints || []).length > 0 export const getValidator = ( - v: Constraint[], + validators: Constraint[] | undefined | null, - n: string + name: string ): Constraint | undefined => - v.find((w) => w.validator === n) + (validators || []).find((v) => v.validator === name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonplugins/stored-constraints/package.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
plugins/stored-constraints/src/stored.constraint.ts(1 hunks)plugins/stored-constraints/src/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/stored-constraints/src/stored.constraint.ts
🧰 Additional context used
🪛 Biome (1.9.4)
plugins/stored-constraints/src/utils.ts
[error] 42-42: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 47-47: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 48-48: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
carlbrugger
force-pushed
the
feat/stored-constraints-plugin
branch
from
b42ea42 to
7ea2c95
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonplugins/stored-constraints/package.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
plugins/stored-constraints/src/stored.constraint.ts(1 hunks)plugins/stored-constraints/src/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/stored-constraints/src/stored.constraint.ts
🧰 Additional context used
🪛 Biome (1.9.4)
plugins/stored-constraints/src/utils.ts
[error] 42-42: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 47-47: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 48-48: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (4)
plugins/stored-constraints/src/utils.ts (4)
1-7: LGTM: Imports and API client setup
The imports are well-organized and the API client is properly initialized.
16-28: LGTM: Well-implemented constraint utilities
The constraint management utilities are well-typed, handle null cases appropriately, and implement their logic correctly.
57-60: LGTM: Well-implemented helper functions
The crossProduct and crossEach functions are well-typed and correctly implemented.
29-56:
Critical: Replace eval() with secure function execution
The current implementation using eval() poses a significant security risk as it allows execution of arbitrary code. This could lead to remote code execution vulnerabilities.
I notice this issue was raised in previous reviews. The suggested solution using vm2 for sandboxed execution is the correct approach. Additionally:
- Add proper return type annotation
- Add error handling for constraint execution
- Consider caching compiled functions for performance
+import { VM } from 'vm2'
export const applyConstraintToRecord = (
constraint: Constraint,
record: FlatfileRecord,
field: Flatfile.Property,
deps: any,
sheet: Flatfile.SheetResponse
-) => {
+): any => {
const storedConstraint = getStoredConstraints(field.constraints || []).find(
(fieldConstraint) => fieldConstraint.validator === constraint.validator
)
const { config = {} } = storedConstraint || {}
- const constraintFn = constraint.function.startsWith('function')
- ? constraint.function.includes('function constraint')
- ? eval(
- '(' +
- constraint.function.replace('function constraint', 'function') +
- ')'
- )
- : eval('(' + constraint.function + ')')
- : eval(constraint.function)
+ try {
+ const vm = new VM({
+ timeout: 1000,
+ sandbox: {
+ value: record.get(field.key),
+ key: field.key,
+ config,
+ record,
+ deps,
+ sheet
+ }
+ })
+
+ const wrappedFunction = `
+ (function() {
+ ${constraint.function}
+ return constraint(value, key, { config, record, deps, sheet });
+ })()
+ `
+
+ return vm.run(wrappedFunction)
+ } catch (error) {
+ throw new Error(`Failed to execute constraint: ${error.message}`)
+ }
- constraintFn(record.get(field.key), field.key, {
- config,
- record,
- deps,
- sheet,
- })
}🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 47-47: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 48-48: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
carlbrugger
force-pushed
the
feat/stored-constraints-plugin
branch
from
7ea2c95 to
67a18fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
plugins/stored-constraints/src/utils.ts (3)
33-33: Consider adding proper type definition for deps parameterReplace the
anytype with a more specific interface or type definition to improve type safety and code maintainability.
16-27: Add input validation to constraint management functionsConsider adding validation for null/undefined inputs:
export const getStoredConstraints = ( constraints: Flatfile.Constraint[] ): Flatfile.StoredConstraint[] => - constraints?.filter((c) => c.type === 'stored') + constraints?.filter((c) => c?.type === 'stored') ?? [] export const getValidator = ( v: Constraint[], n: string -): Constraint | undefined => v.find((w) => w.validator === n) +): Constraint | undefined => { + if (!v || !n) return undefined; + return v.find((w) => w?.validator === n) +}
57-60: Add input validation to utility functionsThe generic implementations look good, but consider adding input validation:
export const crossProduct = <T>(...a: T[][]): T[][] => { + if (!a.length) return [[]]; + if (a.some(arr => !Array.isArray(arr))) { + throw new Error('All arguments must be arrays'); + } return a.reduce((u, c) => u.flatMap((x) => c.map((b) => [...x, b])), [[]]) } export const crossEach = <T>(a: T[][], cb: (...args: T[]) => void): void => { + if (!Array.isArray(a) || !cb || typeof cb !== 'function') { + throw new Error('Invalid arguments: expected array and callback function'); + } crossProduct(...a).forEach((p) => cb(...p)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonplugins/stored-constraints/package.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
plugins/stored-constraints/src/stored.constraint.ts(1 hunks)plugins/stored-constraints/src/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/stored-constraints/src/stored.constraint.ts
🧰 Additional context used
🪛 Biome (1.9.4)
plugins/stored-constraints/src/utils.ts
[error] 42-42: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 47-47: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 48-48: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
| export const applyConstraintToRecord = ( | ||
| constraint: Constraint, | ||
| record: FlatfileRecord, | ||
| field: Flatfile.Property, | ||
| deps: any, | ||
| sheet: Flatfile.SheetResponse | ||
| ) => { | ||
| const storedConstraint = getStoredConstraints(field.constraints || []).find( | ||
| (fieldConstraint) => fieldConstraint.validator === constraint.validator | ||
| ) | ||
| const { config = {} } = storedConstraint || {} | ||
| const constraintFn = constraint.function.startsWith('function') | ||
| ? constraint.function.includes('function constraint') | ||
| ? eval( | ||
| '(' + | ||
| constraint.function.replace('function constraint', 'function') + | ||
| ')' | ||
| ) | ||
| : eval('(' + constraint.function + ')') | ||
| : eval(constraint.function) | ||
|
|
||
| constraintFn(record.get(field.key), field.key, { | ||
| config, | ||
| record, | ||
| deps, | ||
| sheet, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Critical: Replace eval() with secure function execution
The current implementation using eval() is vulnerable to code injection attacks. While the previous suggestion to use vm2 is better, here's an enhanced solution with additional security measures:
import { VM } from 'vm2';
import { memoize } from 'lodash';
// Cache compiled functions for better performance
const createConstraintFunction = memoize((functionStr: string) => {
// Validate function string format
if (!/^(function\s+constraint|\(.*\)\s*=>)\s*\{[\s\S]*\}$/.test(functionStr)) {
throw new Error('Invalid constraint function format');
}
// Create a new VM instance for each function
const vm = new VM({
timeout: 1000,
sandbox: {},
eval: false,
wasm: false
});
// Wrap function to ensure it only returns primitive values
const wrappedFn = `
(function() {
${functionStr}
return function wrapper(value, key, context) {
const result = constraint(value, key, context);
if (result && typeof result === 'object') {
throw new Error('Constraint must return primitive values only');
}
return result;
}
})()
`;
try {
return vm.run(wrappedFn);
} catch (error) {
throw new Error(`Failed to compile constraint: ${error.message}`);
}
});
export const applyConstraintToRecord = (
constraint: Constraint,
record: FlatfileRecord,
field: Flatfile.Property,
deps: any,
sheet: Flatfile.SheetResponse
) => {
const storedConstraint = getStoredConstraints(field.constraints || [])
.find((fc) => fc.validator === constraint.validator);
const { config = {} } = storedConstraint || {};
try {
const constraintFn = createConstraintFunction(constraint.function);
return constraintFn(record.get(field.key), field.key, {
config,
record,
deps,
sheet
});
} catch (error) {
throw new Error(`Constraint execution failed: ${error.message}`);
}
}Key improvements:
- Function compilation is memoized for better performance
- Input validation ensures proper function format
- Prevents object return values to avoid prototype pollution
- Disables eval and WebAssembly in the sandbox
- Proper error handling and messages
🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 47-47: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 48-48: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.changeset/witty-books-turn.md(1 hunks)
🔇 Additional comments (1)
.changeset/witty-books-turn.md (1)
1-3: 🛠️ Refactor suggestion
Consider using a minor version bump instead of patch
Since this introduces new functionality ("Runs the stored constraints"), it might be more appropriate to use a minor version bump instead of a patch. According to semver, new backwards-compatible functionality should increment the minor version.
-'@flatfile/plugin-stored-constraints': patch
+'@flatfile/plugin-stored-constraints': minor✅ Verification successful
The patch version bump is appropriate
This is actually a patch version since this is the initial release (version 0.0.0) of the plugin. For first releases, the initial version bump can be a patch as the functionality itself isn't "new" relative to any existing version. The stored constraints implementation already exists in the codebase, and this appears to be the first packaging of it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this is truly a new feature by looking for previous implementations
rg -l "storedConstraint"
Length of output: 153
Script:
#!/bin/bash
# Let's check the git history of these files to see if they existed before
git log --diff-filter=A -- plugins/stored-constraints/src/stored.constraint.ts
# Also check the actual implementation to understand if this is new functionality
rg -A 10 "storedConstraint" plugins/stored-constraints/src/stored.constraint.ts
Length of output: 910
Script:
#!/bin/bash
# Let's check the package.json to see if this is a new package
cat plugins/stored-constraints/package.json
# Also check git history of the entire plugin directory
git log --diff-filter=A -- plugins/stored-constraints/
Length of output: 2208
| '@flatfile/plugin-stored-constraints': patch | ||
| --- | ||
|
|
||
| Runs the stored constraints for an app |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the changeset description
The current description is too brief and doesn't provide enough context for changelog readers. Consider expanding it to include:
- What stored constraints are
- What problem this solves
- Key benefits or use cases
- Any notable implementation details or limitations
Example enhancement:
-Runs the stored constraints for an app
+Introduces a new plugin for executing stored constraints in Flatfile apps. This plugin:
+- Allows running predefined data validation rules stored in your Flatfile workspace
+- Automatically applies constraints during data processing
+- Supports both synchronous and asynchronous constraint execution
+- Integrates seamlessly with existing Flatfile listeners📝 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.
| Runs the stored constraints for an app | |
| Introduces a new plugin for executing stored constraints in Flatfile apps. This plugin: | |
| - Allows running predefined data validation rules stored in your Flatfile workspace | |
| - Automatically applies constraints during data processing | |
| - Supports both synchronous and asynchronous constraint execution | |
| - Integrates seamlessly with existing Flatfile listeners |
Adds plugin that runs stored constraints.
Please explain how to summarize this PR for the Changelog:
Tell code reviewer how and what to test: