chore: merge cds-core tools to clr-angular (WIP) #1515
Conversation
Thank you, 🤖 Clarity Release Bot |
|
|
||
| ## Quick Start Install | ||
|
|
||
| 1. First, install the Clarity Core package from npm. |
There was a problem hiding this comment.
The info in this file is irrelevant now. It should either be updated, or the file deleted.
Jinnie
left a comment
Jinnie
left a comment
There was a problem hiding this comment.
The content of /projects/core/build should be reviewed. Some storybook and website related tools exist, that are not relevant to the new location.
✅ Deploy Preview for clarity-date-range ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| "@angular/platform-browser-dynamic": "15.2.2", | ||
| "@angular/router": "15.2.2", | ||
| "@cds/core": "6.9.2", | ||
| "@cds/core": "file:dist/@cds/core", |
There was a problem hiding this comment.
I don't think this should be in package.json. I think it would be better to use path aliases in tsconfig.json so that you don't have to have core built before staring ng-clarity.
There was a problem hiding this comment.
Another problem with this is that the cds-icon imports work in this project, but the icon component is not included in a published package. I think this needs to be restructured so that there is not a ./dist/@cds/core folder. It's not actually a dist folder anyway because it is not published. Instead, I think the styles/tokens should be in @clr/ui and the icon component should be in @clr/angular. Otherwise, there is no way for consumers of the published packages to use the things that used to be in @cds/core.
There was a problem hiding this comment.
It's still one of the WIP parts. And your feedback is very valuable.
The idea is to either have a separate package (but not named @cds/core, maybe @clr/core or something else), or, as you say, move tokens to @clr/ui and the rest to @clr/angular (or a smaller core package).
The problem with moving them to the existing packages is that many of the consumable artefacts should still be built before we're able to import and use them.
As a next step we can explore the merging option, and if unsuccessful, fall back to a @clr/core or similar package.
package.json
Outdated
| "@angular/router": "15.2.2", | ||
| "@cds/core": "6.9.2", | ||
| "@cds/core": "file:dist/@cds/core", | ||
| "@commitlint/cz-commitlint": "16.2.3", |
There was a problem hiding this comment.
This should be removed.
projects/demo/src/styles.scss
Outdated
| @import '../../../dist/@cds/core/styles/module.reset.min.css'; | ||
| @import '../../../dist/@cds/core/styles/module.tokens.min.css'; | ||
| @import '../../../dist/@cds/core/styles/module.layout.min.css'; | ||
| @import '../../../dist/@cds/core/styles/module.typography.min.css'; | ||
| @import '../../../dist/@cds/core/styles/theme.dark.min.css'; |
There was a problem hiding this comment.
How are consumer supposed to import these files? I think we need to build these files into the @clr/ui directory so that they are included in a published package.
There was a problem hiding this comment.
This will be handled once we make a choice and provide solution, as discussed in my reply to your first comment above.
There was a problem hiding this comment.
For now, changing the import to be through @cds/core/styles/....., so we don't rely on direct relative paths and dist.
Jinnie
force-pushed
the
jinnie/cds-core-to-clr
branch
2 times, most recently
from
415ea42 to
45b8ce1
Compare
Jinnie
force-pushed
the
jinnie/cds-core-to-clr
branch
2 times, most recently
from
d6c05d5 to
bc46b1c
Compare
|
This PR introduces visual changes: 57a4d47 |
Jinnie
force-pushed
the
jinnie/cds-core-to-clr
branch
from
b34b4aa to
4b800dd
Compare
|
This PR introduces visual changes: 8cb1534 |
Jinnie
force-pushed
the
jinnie/cds-core-to-clr
branch
from
4b800dd to
67e5b84
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const interpolatedString = template.replace(/\$\{.+?\}/g, match => { | ||
| const path = match.substr(2, match.length - 3).trim(); | ||
| const value = getFromObjectPath(path, dataObj, fallback); | ||
| return value; | ||
| }); |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
| ? sheet.cssRules[0].cssText | ||
| .replace(`${themeSelector} {`, '') | ||
| .replace('}', '') |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, we should replace the .replace(string, replacement) calls on lines 131 and 132 with versions that use a global regular expression so that all occurrences are replaced, not just the first. Specifically: Replace .replace(${themeSelector} {, '') with .replace(new RegExp(${escapeRegExp(themeSelector)}\s*{, 'g'), '') and .replace('}', '') with .replace(/}/g, ''), using an escapeRegExp utility function to ensure that themeSelector is safely embedded in the regex. We'll need to add an escapeRegExp helper to handle user-controlled regex input properly. All fixes are confined to projects/core/src/styles/tokens/tokens.stories.ts.
| @@ -121,6 +121,10 @@ | ||
| return value; | ||
| } | ||
|
|
||
| function escapeRegExp(string: string): string { | ||
| return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| } | ||
|
|
||
| function getThemeStyleMap(themeSelector: string) { | ||
| const sheet = Array.from(document.styleSheets) | ||
| .filter(sheet => sheet.href === null || sheet.href.startsWith(window.location.origin)) | ||
| @@ -128,8 +132,8 @@ | ||
|
|
||
| return sheet | ||
| ? sheet.cssRules[0].cssText | ||
| .replace(`${themeSelector} {`, '') | ||
| .replace('}', '') | ||
| .replace(new RegExp(`${escapeRegExp(themeSelector)}\\s*{`, 'g'), '') | ||
| .replace(/}/g, '') | ||
| .split(';') | ||
| .filter((r: any) => r.length > 1) | ||
| .reduce((prev: any, next: string) => { |
idonchev-vmw
force-pushed
the
jinnie/cds-core-to-clr
branch
from
67e5b84 to
0bc559e
Compare
| document.querySelector<HTMLElement>('.scale-typography-label').innerHTML = `--cds-global-scale-typography: ${ | ||
| dynamicTokens['--cds-global-scale-typography'] | ||
| } (${(event.target as HTMLInputElement).value}%)`; |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
The proper fix is to avoid assigning any user-generated or user-modifiable data directly to .innerHTML unless it’s been carefully encoded or sanitized. Since the content being set is just a text label, .textContent should be used instead of .innerHTML. This will ensure that any special characters are treated as plain text and not interpreted as HTML, closing the XSS avenue. The same string construction can be retained, but it should be assigned to .textContent. The fix should be applied to lines 220-221, and similar assignments elsewhere (see lines 228-229, 236-237).
No imports or additional dependencies are needed for this change.
| @@ -217,7 +217,7 @@ | ||
|
|
||
| function updateScaleTypography(event: Event) { | ||
| dynamicTokens['--cds-global-scale-typography'] = parseFloat((event.target as HTMLInputElement).value) / 100; | ||
| document.querySelector<HTMLElement>('.scale-typography-label').innerHTML = `--cds-global-scale-typography: ${ | ||
| document.querySelector<HTMLElement>('.scale-typography-label').textContent = `--cds-global-scale-typography: ${ | ||
| dynamicTokens['--cds-global-scale-typography'] | ||
| } (${(event.target as HTMLInputElement).value}%)`; | ||
| renderTokens(); | ||
| @@ -225,7 +225,7 @@ | ||
|
|
||
| function updateDensity(event: Event) { | ||
| dynamicTokens['--cds-global-scale-layout-space'] = parseFloat((event.target as HTMLInputElement).value) / 100; | ||
| document.querySelector<HTMLElement>('.density-label').innerHTML = `--cds-global-scale-layout-space: ${ | ||
| document.querySelector<HTMLElement>('.density-label').textContent = `--cds-global-scale-layout-space: ${ | ||
| dynamicTokens['--cds-global-scale-layout-space'] | ||
| } (${(event.target as HTMLInputElement).value}%)`; | ||
| renderTokens(); | ||
| @@ -233,7 +233,7 @@ | ||
|
|
||
| function updateScale(event: Event) { | ||
| dynamicTokens['--cds-global-scale-space'] = parseFloat((event.target as HTMLInputElement).value) / 100; | ||
| document.querySelector<HTMLElement>('.scale-label').innerHTML = `--cds-global-scale-space: ${ | ||
| document.querySelector<HTMLElement>('.scale-label').textContent = `--cds-global-scale-space: ${ | ||
| dynamicTokens['--cds-global-scale-space'] | ||
| } (${(event.target as HTMLInputElement).value}%)`; | ||
| renderTokens(); |
| document.querySelector<HTMLElement>('.density-label').innerHTML = `--cds-global-scale-layout-space: ${ | ||
| dynamicTokens['--cds-global-scale-layout-space'] | ||
| } (${(event.target as HTMLInputElement).value}%)`; |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix this DOM text reinterpreted as HTML vulnerability, the data from the input must not be inserted into the DOM using innerHTML unless it is properly escaped or, ideally, not treated as HTML at all. In this specific scenario, since the label update is purely textual and does not need HTML features, use textContent instead of innerHTML when updating the content of the .density-label element. This prevents any interpreted HTML and thus XSS, regardless of the contents of (event.target as HTMLInputElement).value.
Update document.querySelector<HTMLElement>('.density-label').innerHTML = ... to use .textContent instead. No imports or additional dependencies are needed. This change should be applied only where applicable—in this case, in the updateDensity function (lines 226-232).
| @@ -225,7 +225,7 @@ | ||
|
|
||
| function updateDensity(event: Event) { | ||
| dynamicTokens['--cds-global-scale-layout-space'] = parseFloat((event.target as HTMLInputElement).value) / 100; | ||
| document.querySelector<HTMLElement>('.density-label').innerHTML = `--cds-global-scale-layout-space: ${ | ||
| document.querySelector<HTMLElement>('.density-label').textContent = `--cds-global-scale-layout-space: ${ | ||
| dynamicTokens['--cds-global-scale-layout-space'] | ||
| } (${(event.target as HTMLInputElement).value}%)`; | ||
| renderTokens(); |
| document.querySelector<HTMLElement>('.scale-label').innerHTML = `--cds-global-scale-space: ${ | ||
| dynamicTokens['--cds-global-scale-space'] | ||
| } (${(event.target as HTMLInputElement).value}%)`; |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, we should avoid assigning untrusted input directly to innerHTML. Instead, use textContent to safely insert user-controlled values into the DOM, thereby preventing any input from being interpreted as HTML. Here's what to do:
- In all functions that currently assign to
.innerHTMLvia interpolated strings using user input, switch assignments to.textContentinstead. - Change any string formatting to build the appropriate text line (fully escaped) and set
.textContent = ...on the appropriate elements ([lines 220, 228, and 236 in code shown]). - No changes to imports or external dependencies are necessary.
| @@ -217,7 +217,7 @@ | ||
|
|
||
| function updateScaleTypography(event: Event) { | ||
| dynamicTokens['--cds-global-scale-typography'] = parseFloat((event.target as HTMLInputElement).value) / 100; | ||
| document.querySelector<HTMLElement>('.scale-typography-label').innerHTML = `--cds-global-scale-typography: ${ | ||
| document.querySelector<HTMLElement>('.scale-typography-label').textContent = `--cds-global-scale-typography: ${ | ||
| dynamicTokens['--cds-global-scale-typography'] | ||
| } (${(event.target as HTMLInputElement).value}%)`; | ||
| renderTokens(); | ||
| @@ -225,7 +225,7 @@ | ||
|
|
||
| function updateDensity(event: Event) { | ||
| dynamicTokens['--cds-global-scale-layout-space'] = parseFloat((event.target as HTMLInputElement).value) / 100; | ||
| document.querySelector<HTMLElement>('.density-label').innerHTML = `--cds-global-scale-layout-space: ${ | ||
| document.querySelector<HTMLElement>('.density-label').textContent = `--cds-global-scale-layout-space: ${ | ||
| dynamicTokens['--cds-global-scale-layout-space'] | ||
| } (${(event.target as HTMLInputElement).value}%)`; | ||
| renderTokens(); | ||
| @@ -233,7 +233,7 @@ | ||
|
|
||
| function updateScale(event: Event) { | ||
| dynamicTokens['--cds-global-scale-space'] = parseFloat((event.target as HTMLInputElement).value) / 100; | ||
| document.querySelector<HTMLElement>('.scale-label').innerHTML = `--cds-global-scale-space: ${ | ||
| document.querySelector<HTMLElement>('.scale-label').textContent = `--cds-global-scale-space: ${ | ||
| dynamicTokens['--cds-global-scale-space'] | ||
| } (${(event.target as HTMLInputElement).value}%)`; | ||
| renderTokens(); |
idonchev-vmw
force-pushed
the
jinnie/cds-core-to-clr
branch
from
9779674 to
e071ea5
Compare
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information