fix(davinci-client): add validation props to collectors #413
Conversation
🦋 Changeset detectedLatest commit: 9213bb9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds validation metadata to collector input shapes (validation arrays and a phone-number rule), broadens validator parameter/types and store validation checks to include object and multi-value collectors, updates related types and tests, and exposes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Collector factory / caller
participant CU as CollectorUtils
participant RV as returnValidator
participant RE as RegexEngine
Note over Caller,CU #E8F5E9: Build public collector input with validation metadata
Caller->>CU: build collector (field config)
activate CU
CU->>CU: collect rules (required, regex, validatePhoneNumber)
CU-->>Caller: return collector.input (includes optional `validation` array)
Note over Caller,RV #FFF3E0: Validation executed at runtime
Caller->>RV: validate(value, collector)
activate RV
alt regex rule present
RV->>RE: test regex
RE-->>RV: match / fail
end
RV-->>Caller: validation result (messages/empty array)
deactivate RV
deactivate CU
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
|
View your CI Pipeline Execution ↗ for commit 9213bb9
☁️ Nx Cloud last updated this comment at |
ancheetah
force-pushed
the
update-collector-validation
branch
from
7641a33 to
75c2e7b
Compare
|
Deployed 0c876da to https://ForgeRock.github.io/ping-javascript-sdk/pr-413/0c876dacdaf01dba8ced521945b2dc5e06cf08bd branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis📊 Minor Changes📈 @forgerock/davinci-client - 34.5 KB (+0.2 KB) ➖ No Changes➖ @forgerock/sdk-utilities - 4.0 KB 11 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/davinci-client/src/lib/collector.utils.test.ts (2)
603-605: Also assert absence of validation when flags are falseAdd expectations that input.validation is undefined when required=false and validatePhoneNumber=false to lock in schema.
Example:
expect(result.input).not.toHaveProperty('validation');Also applies to: 639-641, 681-682, 719-721
572-583: Add validator behavior tests for validatePhoneNumberYou assert the validation metadata exists (see returnPhoneNumberCollector) but don't verify returnValidator enforces it — add tests in packages/davinci-client/src/lib/collector.utils.test.ts (inside the "Return collector validator" describe) that call returnValidator with a phone collector and assert errors for invalid phone objects and no errors for valid ones.
packages/davinci-client/src/lib/collector.utils.ts (2)
387-395: Type the validation array (minor)Validation arrays are currently inferred as any[]. Consider using a concrete Validation* union from collector.types for stronger type safety.
- const validationArray = []; + const validationArray: Array< + { type: 'required'; message: string; rule: true } + > = [];Also applies to: 406-407
450-457: Same note: prefer concrete validation rule typeApplies to Object collectors too; keep schema consistent and self-documenting.
Also applies to: 532-532
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/fine-windows-search.md(1 hunks)packages/davinci-client/src/lib/client.store.ts(1 hunks)packages/davinci-client/src/lib/collector.types.ts(5 hunks)packages/davinci-client/src/lib/collector.utils.test.ts(8 hunks)packages/davinci-client/src/lib/collector.utils.ts(8 hunks)packages/davinci-client/src/lib/node.reducer.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/davinci-client/src/lib/node.reducer.test.ts
- .changeset/fine-windows-search.md
- packages/davinci-client/src/lib/collector.types.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/davinci-client/src/lib/client.store.ts (1)
packages/davinci-client/src/lib/client.store.utils.ts (1)
handleUpdateValidateError(51-66)
packages/davinci-client/src/lib/collector.utils.ts (1)
packages/davinci-client/src/lib/collector.types.ts (3)
ValidatedTextCollector(184-184)ObjectValueCollectors(387-392)MultiValueCollectors(249-251)
🪛 ast-grep (0.38.6)
packages/davinci-client/src/lib/collector.utils.ts
[warning] 629-629: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(next.rule)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr
🔇 Additional comments (7)
packages/davinci-client/src/lib/collector.utils.test.ts (3)
413-414: LGTM: asserts validation presence for MultiSelect when requiredExpectation reflects new validation metadata for MultiValue collectors.
442-442: LGTM: DeviceAuthentication required=false baselineKeeps test neutral; OK given we don’t attach validation for this collector here.
528-534: LGTM: DeviceRegistration adds required rule to input.validationMatches implementation and public shape.
packages/davinci-client/src/lib/collector.utils.ts (3)
28-30: LGTM: widened collector unions for downstream validationImporting MultiValueCollectors and ObjectValueCollectors here is correct for the broadened validator API.
213-219: LGTM: required rule for validated single-value collectorsAdds consistent required semantics.
506-513: Enforce validatePhoneNumber in returnValidatorvalidatePhoneNumber is pushed into validationArray (packages/davinci-client/src/lib/collector.utils.ts near lines 506–511) but returnValidator doesn't implement it, so the rule is a no‑op — update returnValidator to accept string | string[] | {countryCode?: string; phoneNumber?: string} and add a branch that normalizes digits, composes an E.164 value and validates it with /^+?[1-9]\d{1,14}$/ (push next.message on failure); also keep the regex-length guard for regex rules.
packages/davinci-client/src/lib/client.store.ts (1)
320-324: Fix validate() allowed categories, error message, and signatureChange the error string and expand the validate() JSDoc/signature to accept ValidatedSingleValueCollector | ObjectValueCollector | MultiValueCollector (see diffs). Verification: ripgrep found no .validate() callsites under packages/davinci-client; Validator is declared in packages/davinci-client/src/lib/client.types.ts and re-exported in packages/davinci-client/src/types.ts.
- 'Collector is not a SingleValueCollector, ObjectValueCollector, or MultiValueCollector and cannot be validated', + 'Collector is not a ValidatedSingleValueCollector, ObjectValueCollector, or MultiValueCollector and cannot be validated',- * @param {SingleValueCollector} collector - the collector to validate - * @throws {Error} - if the collector is not a SingleValueCollector + * @param {ValidatedSingleValueCollector | ObjectValueCollector | MultiValueCollector} collector - the collector to validate + * Note: Returns an error function for unsupported categories. @@ - validate: (collector: SingleValueCollectors): Validator => { + validate: ( + collector: SingleValueCollectors | ObjectValueCollectors | MultiValueCollectors, + ): Validator => {
| return (value: string) => { | ||
| return rules.reduce((acc, next) => { | ||
| if (next.type === 'required' && !value) { | ||
| acc.push(next.message); | ||
| } else if (next.type === 'regex') { | ||
| try { | ||
| const result = new RegExp(next.rule).test(value); | ||
| if (!result) { | ||
| acc.push(next.message); | ||
| return ( | ||
| rules?.reduce((acc, next) => { | ||
| if (next.type === 'required' && !value) { | ||
| acc.push(next.message); | ||
| } else if (next.type === 'regex') { | ||
| try { | ||
| const result = new RegExp(next.rule).test(value); | ||
| if (!result) { | ||
| acc.push(next.message); | ||
| } | ||
| } catch (err) { | ||
| const error = err as Error; | ||
| acc.push(error.message); | ||
| } | ||
| } catch (err) { | ||
| const error = err as Error; | ||
| acc.push(error.message); | ||
| } | ||
| } | ||
| return acc; | ||
| }, [] as string[]); | ||
| return acc; | ||
| }, [] as string[]) ?? [] | ||
| ); |
There was a problem hiding this comment.
Guard dynamic regex against ReDoS (server-controlled patterns)
Compiling variable regex directly is risky. Add safety checks (length, known-bad constructs), or vet with a lib (e.g., safe-regex2/recheck) or switch to RE2 where possible. You already catch syntax errors; that won’t prevent catastrophic backtracking.
If you prefer not to add a dependency now, keep the length cap (as above) and consider rejecting patterns with nested quantifiers:
// simplistic guard (not a full solution)
const dangerous = /(\+|\*|\{[^}]*\})\s*(\+|\*|\{[^}]*\})/;
if (dangerous.test(pattern)) {
acc.push('Invalid regular expression: potentially unsafe pattern');
return acc;
}🧰 Tools
🪛 ast-grep (0.38.6)
[warning] 629-629: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(next.rule)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In packages/davinci-client/src/lib/collector.utils.ts around lines 623 to 641,
the code compiles server-controlled regexes directly which risks ReDoS; modify
the regex-handling branch to first validate the pattern before constructing
RegExp: impose a reasonable max pattern length (e.g., 200 chars), reject empty
or excessively long patterns by pushing a clear "Invalid regular expression: too
long" message to acc, perform a simple heuristic check for nested quantifiers
(e.g., detect two adjacent quantifier tokens like + * or {...} separated only by
optional whitespace) and push "Invalid regular expression: potentially unsafe
pattern" if matched, and only then attempt new RegExp in try/catch;
alternatively prefer switching to a safe regex engine (RE2) or importing a
vetted safety lib when possible.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #413 +/- ##
==========================================
+ Coverage 55.63% 56.19% +0.56%
==========================================
Files 32 32
Lines 2051 2091 +40
Branches 344 353 +9
==========================================
+ Hits 1141 1175 +34
- Misses 910 916 +6
🚀 New features to boost your workflow:
|
| * @returns {function} - A "validator" function that validates the input value | ||
| */ | ||
| export function returnValidator(collector: ValidatedTextCollector) { | ||
| export function returnValidator( |
There was a problem hiding this comment.
Are we providing a "validator" method for this phone number collector? If so, what does it actually do?
There was a problem hiding this comment.
We do but it only supports validation for required. There is no validation regex that can be provided in the P1 Form for the phone number. validatePhoneNumber is simply a checkbox. So I don't think there's anything for us to do other than provide this information in the collector. I am still unclear whether this means the user will do client side validation or the DaVinci server will validate automatically upon form submission.
There was a problem hiding this comment.
Got ya. Yeah, I'm just unsure what to do here as well related to validating the actual value itself. I think we should bring this up to the team.
| ) { | ||
| return handleUpdateValidateError( | ||
| 'Collector is not a SingleValueCollector and cannot be validated', | ||
| 'Collector is not a SingleValueCollector, ObjectValueCollector, or MultiValueCollector and cannot be validated', |
There was a problem hiding this comment.
I'm wondering if we're reaching a tipping point with these error messages. We may want to consider rewording these as its possible we have to add more collectors that will fit this error.
There was a problem hiding this comment.
I was thinking this too. Maybe something like Collector does not fall into a category that can be validated
There was a problem hiding this comment.
Let's hope we don't have to invent new collector categories :D
ancheetah
force-pushed
the
update-collector-validation
branch
from
75c2e7b to
1939ec3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/davinci-client/src/lib/client.store.ts (1)
323-333: Error text is inaccurate; prefer concise, future-proof wordingYou check for ValidatedSingleValueCollector, ObjectValueCollector, or MultiValueCollector, but the error says “SingleValueCollector”. Also matches prior suggestion to avoid enumerating every category.
- 'Collector is not a SingleValueCollector, ObjectValueCollector, or MultiValueCollector and cannot be validated', + 'Collector does not fall into a category that can be validated',packages/davinci-client/src/lib/collector.utils.ts (1)
506-512: ‘validatePhoneNumber’ rule is metadata only todayreturnValidator doesn’t handle
validatePhoneNumber, so this won’t produce any client-side validation. Either:
- Document that it’s informational and enforced server-side, or
- Add minimal client evaluation (e.g., basic length/character checks) behind a guard.
Do we intend the SDK to enforce this, or just surface it? If enforcement is desired, see the returnValidator refactor below.
🧹 Nitpick comments (6)
packages/davinci-client/src/lib/client.store.ts (1)
335-341: ‘validation’ presence check is brittleYou currently gate on the existence of the property via the in-operator. Because some builders set
validation: undefined, this condition passes even when there are no rules. Check for a non-empty array.- if (!('validation' in collectorToUpdate.input)) { + const rules = (collectorToUpdate.input as { validation?: unknown }).validation; + if (!Array.isArray(rules) || rules.length === 0) { return handleUpdateValidateError( 'Collector has no validation rules', 'state_error', log.error, ); - } + }packages/davinci-client/src/lib/collector.utils.test.ts (3)
572-583: Phone number validation metadata asserted; consider test for evaluator once implementedYou check both required and validatePhoneNumber metadata. After we expand returnValidator to handle objects, add a test to ensure empty phoneNumber triggers the required error.
603-605: Defaults flipped to false: OKThese make the tests explicit and avoid incidental validation rules. Consider also asserting that
input.validationis absent when both flags are false to prevent accidental regressions.Also applies to: 639-641, 681-682, 719-721
824-836: Regex error message assertion may be brittle across runtimesNode/V8 can vary the exact message. To reduce flakiness, assert it contains a stable prefix like “Invalid regular expression” rather than the full engine-produced text.
packages/davinci-client/src/lib/collector.utils.ts (2)
387-395: Emit ‘validation’ only when non-emptyRight now you set
validation: undefined, which still defines the property and breaks consumers that check for absence. Prefer conditional object spread so the key is omitted entirely when there are no rules.input: { key: field.key, value: data || [], type: field.type, - validation: validationArray.length ? validationArray : undefined, + ...(validationArray.length > 0 && { validation: validationArray }), },Also applies to: 402-407
450-457: Same conditional ‘validation’ emission for Object collectorsMirror the MultiValue pattern so client.store validate can reliably detect rule presence.
input: { key: field.key, value: defaultValue, type: field.type, - validation: validationArray.length ? validationArray : undefined, + ...(validationArray.length > 0 && { validation: validationArray }), },Also applies to: 529-533
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/fine-windows-search.md(1 hunks)packages/davinci-client/src/lib/client.store.ts(3 hunks)packages/davinci-client/src/lib/collector.types.ts(5 hunks)packages/davinci-client/src/lib/collector.utils.test.ts(8 hunks)packages/davinci-client/src/lib/collector.utils.ts(8 hunks)packages/davinci-client/src/lib/node.reducer.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/davinci-client/src/lib/node.reducer.test.ts
- .changeset/fine-windows-search.md
- packages/davinci-client/src/lib/collector.types.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/davinci-client/src/lib/collector.utils.ts (1)
packages/davinci-client/src/lib/collector.types.ts (3)
ValidatedTextCollector(184-184)ObjectValueCollectors(387-392)MultiValueCollectors(249-251)
packages/davinci-client/src/lib/client.store.ts (2)
packages/davinci-client/src/lib/collector.types.ts (3)
SingleValueCollectors(174-179)ObjectValueCollectors(387-392)MultiValueCollectors(249-251)packages/davinci-client/src/types.ts (1)
Validator(23-23)
🪛 ast-grep (0.38.6)
packages/davinci-client/src/lib/collector.utils.ts
[warning] 629-629: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(next.rule)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr
🔇 Additional comments (6)
packages/davinci-client/src/lib/client.store.ts (1)
36-37: Import widening looks goodIncluding MultiValueCollectors aligns with the broadened validation surface.
packages/davinci-client/src/lib/collector.utils.test.ts (3)
413-414: Nice: asserting validation metadata is present on MultiSelectConfirms the new input shape without over-specifying rule details.
442-442: Required=false change is consistent with PR intentMatches the decision to shift “required” into validation metadata when true.
528-535: Good coverage of required rule on DeviceRegistrationValidates that required -> validation array is emitted.
packages/davinci-client/src/lib/collector.utils.ts (2)
28-30: Import expansion aligns with new unionsBrings MultiValueCollectors and ObjectValueCollectors into scope where needed.
213-219: Adding required -> validation for validated text is correctEnsures parity with Multi/Object collectors.
| * @param {ValidatedTextCollector | ObjectValueCollectors | MultiValueCollectors} collector - The collector to which the value will be validated | ||
| * @returns {function} - A "validator" function that validates the input value | ||
| */ | ||
| export function returnValidator(collector: ValidatedTextCollector) { | ||
| export function returnValidator( | ||
| collector: ValidatedTextCollector | ObjectValueCollectors | MultiValueCollectors, | ||
| ) { |
There was a problem hiding this comment.
Validator currently only accepts strings; arrays/objects won’t validate as intended; also add ReDoS guard
- The returned function signature is
(value: string), but validate() is now exposed for MultiValue and Object collectors. ‘required’ will never fire for[]or{ phoneNumber: '' }, and regex on non-strings is ill-defined. - Compiling server-provided regex directly risks catastrophic backtracking.
Apply this refactor to widen the accepted value and add safe handling and a basic regex safety check:
- export function returnValidator(
- collector: ValidatedTextCollector | ObjectValueCollectors | MultiValueCollectors,
- ) {
- const rules = collector.input.validation;
- return (value: string) => {
- return (
- rules?.reduce((acc, next) => {
- if (next.type === 'required' && !value) {
- acc.push(next.message);
- } else if (next.type === 'regex') {
- try {
- const result = new RegExp(next.rule).test(value);
- if (!result) {
- acc.push(next.message);
- }
- } catch (err) {
- const error = err as Error;
- acc.push(error.message);
- }
- }
- return acc;
- }, [] as string[]) ?? []
- );
- };
- }
+ export function returnValidator(
+ collector: ValidatedTextCollector | ObjectValueCollectors | MultiValueCollectors,
+ ) {
+ const rules = collector.input.validation;
+ return (value: unknown) => {
+ if (!Array.isArray(rules) || rules.length === 0) return [];
+ const isString = (v: unknown): v is string => typeof v === 'string';
+ const isArrayOfStrings = (v: unknown): v is string[] =>
+ Array.isArray(v) && v.every((x) => typeof x === 'string');
+ const isPhoneNumber = (
+ v: unknown,
+ ): v is { countryCode?: unknown; phoneNumber?: unknown } =>
+ v !== null && typeof v === 'object' && 'phoneNumber' in (v as Record<string, unknown>);
+
+ return rules.reduce((acc, next) => {
+ if (next.type === 'required') {
+ const emptyString = isString(value) && value.trim() === '';
+ const emptyArray = isArrayOfStrings(value) && value.length === 0;
+ const emptyPhone =
+ isPhoneNumber(value) &&
+ (value.phoneNumber === undefined || String(value.phoneNumber).trim() === '');
+ if (emptyString || emptyArray || emptyPhone) {
+ acc.push(next.message);
+ }
+ } else if (next.type === 'regex') {
+ try {
+ const pattern = String((next as any).rule ?? '');
+ if (pattern.length > 200) {
+ acc.push('Invalid regular expression: too long');
+ return acc;
+ }
+ const dangerous = /(\+|\*|\{[^}]*\})\s*(\+|\*|\{[^}]*\})/;
+ if (dangerous.test(pattern)) {
+ acc.push('Invalid regular expression: potentially unsafe pattern');
+ return acc;
+ }
+ const re = new RegExp(pattern);
+ if (isString(value)) {
+ if (!re.test(value)) acc.push(next.message);
+ } else if (isArrayOfStrings(value)) {
+ if (!value.every((v) => re.test(v))) acc.push(next.message);
+ }
+ } catch (err) {
+ acc.push((err as Error).message);
+ }
+ } else if (next.type === 'validatePhoneNumber') {
+ // Basic client-side check; treat as metadata if undesired
+ if (isPhoneNumber(value) && value.phoneNumber) {
+ const digits = String(value.phoneNumber).replace(/\D+/g, '');
+ if (digits.length < 6 || digits.length > 15) {
+ acc.push(next.message || 'Invalid phone number');
+ }
+ }
+ }
+ return acc;
+ }, [] as string[]);
+ };
+ }After this change, please add/adjust tests to cover:
- required on MultiSelect: validator([]) returns the “Value cannot be empty” message.
- required on PhoneNumber: validator({ countryCode: 'US', phoneNumber: '' }) returns the message.
- regex on arrays: validator(['a', 'b']) passes/fails coherently.
Also applies to: 623-641
🤖 Prompt for AI Agents
In packages/davinci-client/src/lib/collector.utils.ts around lines 616-621 (and
also affecting 623-641): the validator currently types its input as string and
blindly runs required/regex logic, so arrays/objects never trigger required and
non-string regex runs are ill-defined and unsafe. Update the returned validator
to accept unknown (string | any[] | Record<string, any>), implement required
checks that treat [] as empty and treat an object as empty if all its relevant
string fields are empty (e.g., for phone: {phoneNumber: ''} should be "empty"),
and for multi-value arrays apply validation per element (or treat presence of
any non-empty entry as satisfied). Only execute regex matching on actual
strings; for arrays/objects either map to their string elements or skip regex
with a coherent pass/fail policy. Add a basic regex safety guard before
compiling: limit pattern length (e.g., <= 200), disallow highly risky patterns
(e.g., nested catastrophic quantifiers like (.+)+) or at minimum wrap RegExp
construction/matching in try/catch to avoid ReDoS crashing the process. Finally,
adjust tests to assert required triggers on [] and object-with-empty-fields and
that regex behavior on arrays is consistent.
There was a problem hiding this comment.
@cerebrl @ryanbas21 I've addressed this issue by updating the returnValidator utility and adding unit tests. Ready for re-review.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
ancheetah
force-pushed
the
update-collector-validation
branch
from
1939ec3 to
9213bb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/fine-windows-search.md(1 hunks)packages/davinci-client/src/lib/client.store.ts(3 hunks)packages/davinci-client/src/lib/collector.types.ts(6 hunks)packages/davinci-client/src/lib/collector.utils.test.ts(11 hunks)packages/davinci-client/src/lib/collector.utils.ts(8 hunks)packages/davinci-client/src/lib/node.reducer.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/davinci-client/src/lib/node.reducer.test.ts
- .changeset/fine-windows-search.md
🧰 Additional context used
🧬 Code graph analysis (3)
packages/davinci-client/src/lib/client.store.ts (4)
packages/davinci-client/src/lib/collector.types.ts (3)
SingleValueCollectors(174-179)ObjectValueCollectors(388-393)MultiValueCollectors(250-252)packages/davinci-client/src/lib/client.types.ts (1)
Validator(25-33)packages/davinci-client/src/types.ts (1)
Validator(23-23)packages/davinci-client/src/lib/client.store.utils.ts (1)
handleUpdateValidateError(51-66)
packages/davinci-client/src/lib/collector.utils.ts (2)
packages/davinci-client/src/lib/collector.types.ts (3)
ValidatedTextCollector(184-184)ObjectValueCollectors(388-393)MultiValueCollectors(250-252)packages/davinci-client/src/types.ts (1)
ValidatedTextCollector(44-44)
packages/davinci-client/src/lib/collector.utils.test.ts (2)
packages/davinci-client/src/lib/collector.types.ts (3)
ValidatedTextCollector(184-184)PhoneNumberCollector(408-412)MultiSelectCollector(258-258)packages/davinci-client/src/lib/collector.utils.ts (1)
returnValidator(619-649)
🪛 ast-grep (0.39.5)
packages/davinci-client/src/lib/collector.utils.ts
[warning] 635-635: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(next.rule)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr
🔇 Additional comments (17)
packages/davinci-client/src/lib/collector.types.ts (3)
40-44: LGTM - New ValidationPhoneNumber typeThe new
ValidationPhoneNumberinterface follows the same pattern as existing validation types (ValidationRequired,ValidationRegex) and is well-structured for phone number validation.
205-206: LGTM - Validation arrays added to multi-value and object collectorsThe addition of optional
validation?: ValidationRequired[]properties toMultiValueCollectorWithValue,MultiValueCollectorNoValue,ObjectOptionsCollectorWithStringValue, andObjectOptionsCollectorWithObjectValueis consistent and follows the established pattern.Also applies to: 226-227, 318-319, 342-343, 367-368
367-367: LGTM - Enhanced validation for ObjectValueCollectorWithObjectValueThe union type
validation?: (ValidationRequired | ValidationPhoneNumber)[]appropriately allows both required validation and phone number validation for object value collectors, which makes sense for phone number fields.packages/davinci-client/src/lib/collector.utils.test.ts (5)
33-38: LGTM - Added required imports for enhanced validationThe imports for
MultiSelectCollector,PhoneNumberCollector,PhoneNumberOutputValue, andValidatedTextCollectorare necessary for the expanded test coverage of the validation functionality.
418-418: Good test coverage for validation propertiesThe test correctly verifies that the input contains the validation property for MultiSelectCollector.
533-540: LGTM - Validation arrays properly testedThe tests correctly verify that validation arrays are added to collector inputs with the expected structure for required and validatePhoneNumber rules.
Also applies to: 577-588
831-855: Enhanced validator test coverageThe expanded tests now properly validate multi-value and object collectors, which aligns with the broadened validation support. The test scenarios cover:
- Required validation on empty objects and arrays
- Valid inputs returning empty error arrays
447-447: Confirm intent for inconsistent 'required' / 'validatePhoneNumber' flagspackages/davinci-client/src/lib/collector.utils.test.ts has a PHONE_NUMBER case with required: true and validatePhoneNumber: true (lines 557–561) and multiple PHONE_NUMBER cases with both false (lines 608–609, 644–645, 685–686, 724–725). The DeviceAuthenticationField mock at lines 445–448 sets required: false while earlier related fields are required: true (e.g. lines 346–348, 371, 401–402). Confirm these differences are intentional (distinct test scenarios) and not accidental copy/paste errors.
packages/davinci-client/src/lib/client.store.ts (3)
36-36: LGTM - Import additionAdding
MultiValueCollectorsimport aligns with the expanded validation support.
294-300: Update parameter and documentationThe parameter type has been correctly broadened to accept the union of collector types, and the JSDoc has been updated to reflect this change.
However, there's a discrepancy in the documentation:
323-333: Update validation eligibility checkThe validation logic correctly checks for the three supported collector categories:
ValidatedSingleValueCollector,ObjectValueCollector, andMultiValueCollector. The error message appropriately reflects the supported types.packages/davinci-client/src/lib/collector.utils.ts (6)
28-30: LGTM - Import additions for expanded validation supportThe imports for
MultiValueCollectorsandObjectValueCollectorsare necessary for the broadened validation functionality.
213-219: LGTM - Required validation logicThe condition
field.required === trueproperly checks for required fields and adds the appropriate validation rule.
387-406: LGTM - Multi-value validation implementationThe validation array logic for multi-value collectors correctly:
- Initializes an empty validation array
- Adds required validation when
field.required === true- Conditionally includes validation in input only when array has content
450-457: LGTM - Object collector validation with phone number supportThe validation implementation for object collectors properly:
- Handles required validation
- Adds phone number validation when
field.validatePhoneNumber === true- Uses conditional validation inclusion
Also applies to: 506-532
616-621: Update function signature for broader validation supportThe function signature has been correctly updated to accept the expanded union of collector types.
623-647: Address ReDoS vulnerability and improve validation logicThe current implementation has several issues:
- ReDoS Risk: Direct compilation of server-controlled regex patterns without validation
- Type Handling: The function accepts broader input types but validation logic needs enhancement
- Missing Phone Validation: No handling for
validatePhoneNumberrule typeApply this enhanced implementation with safety checks:
export function returnValidator( collector: ValidatedTextCollector | ObjectValueCollectors | MultiValueCollectors, ) { const rules = collector.input.validation; - return (value: string | string[] | Record<string, string>) => { + return (value: string | string[] | Record<string, string | undefined>) => { + if (!rules?.length) return []; + return ( rules?.reduce((acc, next) => { if (next.type === 'required') { - if ( - !value || - (Array.isArray(value) && !value.length) || - (typeof value === 'object' && !Array.isArray(value) && !Object.keys(value).length) - ) { + const isEmpty = !value || + (typeof value === 'string' && value.trim() === '') || + (Array.isArray(value) && value.length === 0) || + (typeof value === 'object' && !Array.isArray(value) && + (!Object.keys(value).length || + Object.values(value).every(v => !v || String(v).trim() === ''))); + if (isEmpty) { acc.push(next.message); } } else if (next.type === 'regex' && typeof value === 'string') { try { + const pattern = String(next.rule || ''); + if (pattern.length > 200) { + acc.push('Invalid regular expression: pattern too long'); + return acc; + } + // Basic ReDoS guard - reject nested quantifiers + if (/(\+|\*|\{[^}]*\})\s*(\+|\*|\{[^}]*\})/.test(pattern)) { + acc.push('Invalid regular expression: potentially unsafe pattern'); + return acc; + } - const result = new RegExp(next.rule).test(value); + const result = new RegExp(pattern).test(value); if (!result) { acc.push(next.message); } } catch (err) { const error = err as Error; acc.push(error.message); } + } else if (next.type === 'validatePhoneNumber') { + // Basic phone number validation for client-side feedback + if (typeof value === 'object' && !Array.isArray(value) && value.phoneNumber) { + const phoneStr = String(value.phoneNumber).replace(/\D/g, ''); + if (phoneStr.length < 6 || phoneStr.length > 15) { + acc.push(next.message || 'Invalid phone number format'); + } + } } return acc; }, [] as string[]) ?? [] ); }; }This addresses:
- ReDoS protection with length limits and basic pattern detection
- Proper empty value detection for strings, arrays, and objects
- Phone number validation implementation
- Improved type safety
| input: { | ||
| validation: [ | ||
| { type: 'required', message: 'This field is required' }, | ||
| { type: 'required', message: 'This field is required', rule: true }, |
There was a problem hiding this comment.
🧩 Analysis chain
Test data change needs verification
The validation rule definition has changed from { type: 'required', message: 'This field is required', rule: true } to include additional test collectors. Please verify that these changes align with the new validation structure.
Also applies to: 812-825
🏁 Script executed:
#!/bin/bash
# Verify the validation rule structure in collector types
echo "=== Check ValidationRequired interface ==="
rg -n -A5 -B2 'ValidationRequired|ValidationPhoneNumber' packages/davinci-client/src/lib/collector.types.ts
echo "=== Check returnValidator implementation ==="
rg -n -A10 -B5 'returnValidator.*collector.*{' packages/davinci-client/src/lib/collector.utils.tsLength of output: 1598
🏁 Script executed:
#!/bin/bash
set -euo pipefail
types="packages/davinci-client/src/lib/collector.types.ts"
utils="packages/davinci-client/src/lib/collector.utils.ts"
test="packages/davinci-client/src/lib/collector.utils.test.ts"
echo "=== Checking types file ==="
if [ -f "$types" ]; then
echo "FOUND: $types"
rg -n -C3 -e 'interface ValidationRequired' -e 'interface ValidationRegex' -e 'interface ValidationPhoneNumber' "$types" || true
echo "--- validation property occurrences ---"
rg -n -C3 'validation' "$types" || true
ln=$(rg -n 'interface ValidationRequired' "$types" | sed -n '1p' | cut -d: -f1 || true)
if [ -n "$ln" ]; then
echo "--- ValidationRequired definition ---"
sed -n "${ln},$((ln+50))p" "$types"
fi
else
echo "MISSING: $types"
fi
echo
echo "=== Checking utils file for returnValidator ==="
if [ -f "$utils" ]; then
echo "FOUND: $utils"
rg -n -C5 'returnValidator' "$utils" || true
lnu=$(rg -n 'returnValidator' "$utils" | sed -n '1p' | cut -d: -f1 || true)
if [ -n "$lnu" ]; then
start=$((lnu>30?lnu-30:1))
echo "--- returnValidator surrounding snippet ---"
sed -n "${start},$((lnu+200))p" "$utils"
fi
else
echo "MISSING: $utils"
fi
echo
echo "=== Showing test file snippet around lines 780-840 ==="
if [ -f "$test" ]; then
sed -n '780,840p' "$test"
else
echo "MISSING: $test"
fiLength of output: 8235
returnValidator missing handling for 'validatePhoneNumber'
collector.types.ts defines ValidationPhoneNumber and the test adds { type: 'validatePhoneNumber', ... }, but returnValidator (packages/davinci-client/src/lib/collector.utils.ts) only implements 'required' and 'regex' — implement phone-number validation in returnValidator or remove/adjust the test data.
Locations: packages/davinci-client/src/lib/collector.types.ts (ValidationPhoneNumber), packages/davinci-client/src/lib/collector.utils.ts (returnValidator ~line 619), packages/davinci-client/src/lib/collector.utils.test.ts (~lines 800–820).
4 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4373
https://pingidentity.atlassian.net/browse/SDKS-4371
Description
Adds
requiredandvalidatePhoneNumberproperties tocollector.input.validationarray forObjectValueCollectorsandMultiValueCollectors.Patch changeset added.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor