-
Notifications
You must be signed in to change notification settings - Fork 229
Demo Rough Draft PR for Arg Validation #7315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there might be some misunderstanding of my previous comment that I'll try to clarify again 🙂
There is no safe way to validate these unsanitized arguments by just running the functions directly with those arguments in the same thread. You are executing this code in the same process that is rendering the page, meaning that if you are syncronously "locking" it to execute the function (that's what faker will do), you are locking the whole page.
If you want to validate the arguments by running them, they need to be moved out from the main thread ("sandboxed" as @ncarbon already correctly noted, there are ways of doing this via WebWorker API for example, but it's a pretty involved process) or by actually validating the argument values for every function instead of executing them.
It might be helpful to understand on a high level how processes in the browser work. There is a lot of good resources on this topic, a few resources that you might want to read through: 1, 2
method: 'string.alpha', | ||
args: [Number.MAX_SAFE_INTEGER], | ||
description: 'String with excessive length', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I provided this example in my comment for a specific reason:
}, | |
}, | |
{ | |
method: 'string.alpha', | |
args: [1_000_000_000], | |
description: 'String with excessive length 2', | |
}, |
This case crashes node process with an OOM, which means it will crash Compass or browser tab with an OOM
npx ts-node packages/compass-collection/src/components/mock-data-generator-modal/faker-validation-demo.ts
=== Faker Argument Validation Demo ===
1. Simple method with no args
Method: person.firstName
Args: []
[DEBUG] Faker validation: person.firstName took 0ms
✅ VALID (0ms)
2. String generation with valid length
Method: string.alpha
Args: [10]
[DEBUG] Faker validation: string.alpha took 0ms
✅ VALID (0ms)
3. Number with valid range
Method: number.int
Args: [{"min":1,"max":100}]
[DEBUG] Faker validation: number.int took 0ms
✅ VALID (0ms)
4. Date with valid range
Method: date.between
Args: [{"from":"2020-01-01","to":"2025-01-01"}]
[DEBUG] Faker validation: date.between took 0ms
✅ VALID (0ms)
5. String with excessive length
Method: string.alpha
Args: [9007199254740991]
❌ INVALID: Invalid arguments for string.alpha: Invalid array length (0ms)
6. String with excessive length 2
Method: string.alpha
Args: [1000000000]
<--- Last few GCs --->
[47870:0x140008000] 3629 ms: Scavenge (interleaved) 963.6 (1001.1) -> 963.6 (1001.1) MB, pooled: 0 MB, 72.21 / 0.00 ms (average mu = 0.988, current mu = 0.990) allocation failure;
[47870:0x140008000] 4387 ms: Mark-Compact 1731.6 (1769.1) -> 1346.7 (1391.0) MB, pooled: 0 MB, 371.12 / 0.00 ms (+ 0.0 ms in 0 steps since start of marking, biggest step 0.0 ms, walltime since start of marking 2297 ms) (average mu = 0.910, current mu
<--- JS stacktrace --->
FATAL ERROR: invalid table size Allocation failed - JavaScript heap out of memory
----- Native stack trace -----
1: 0x1028c5cac node::OOMErrorHandler(char const*, v8::OOMDetails const&) [/Users/sergey.petushkov/.nvm/versions/node/v22.17.1/bin/node]
2: 0x102a9b998 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [/Users/sergey.petushkov/.nvm/versions/node/v22.17.1/bin/node]
3: 0x102cab0c0 v8::internal::Heap::stack() [/Users/sergey.petushkov/.nvm/versions/node/v22.17.1/bin/node]
Even if you reduce the number slightly to the point where it's not immeditaely crashing the browser, you will be locking the browser window for seconds, this is unacceptable level of perfromance impact for something like this
); | ||
} | ||
|
||
if (executionTime > configuredTimeoutMs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure it's clear: in the current implementation the "price" of this validation is that Compass app / Atlas Cloud tab was unresponsive for executionTime
(or crashed completely), this is unreasonably high price for this validation.
08b758f
to
a43ebf2
Compare
#7295