Skip to content

Commit 993b903

Browse files
fix: fix unsanitized untrusted input v2 (#20092)
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
1 parent 0e8a612 commit 993b903

File tree

8 files changed

+95
-103
lines changed

8 files changed

+95
-103
lines changed

Diff for: feature-libs/product-configurator/package.json

+1
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
"@angular/common": "^19.1.8",
3030
"@angular/core": "^19.1.8",
3131
"@angular/forms": "^19.1.8",
32+
"@angular/platform-browser": "^19.1.8",
3233
"@angular/router": "^19.1.8",
3334
"@ng-select/ng-select": "^14.1.0",
3435
"@ngrx/effects": "^19.0.1",

Diff for: feature-libs/product-configurator/rulebased/components/attribute/types/numeric-input-field/configurator-attribute-numeric-input-field.component.service.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -208,12 +208,12 @@ export class ConfiguratorAttributeNumericInputFieldService {
208208
interval.maxValueIncluded = true;
209209
if (minVal.includes('>')) {
210210
interval.minValueIncluded = false;
211-
minVal = minVal.replace('>', '');
211+
minVal = minVal.replace(/>/g, '');
212212
}
213213

214214
if (maxVal.includes('<')) {
215215
interval.maxValueIncluded = false;
216-
maxVal = maxVal.replace('<', '');
216+
maxVal = maxVal.replace(/</g, '');
217217
}
218218
return { minVal, maxVal };
219219
}

Diff for: feature-libs/product-configurator/rulebased/components/show-more/configurator-show-more.component.spec.ts

+43-57
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,22 @@ import { ComponentFixture, TestBed, waitForAsync } from '@angular/core/testing';
33
import { I18nTestingModule } from '@spartacus/core';
44
import { CommonConfiguratorTestUtilsService } from '../../../common/testing/common-configurator-test-utils.service';
55
import { ConfiguratorShowMoreComponent } from './configurator-show-more.component';
6+
import { DomSanitizer } from '@angular/platform-browser';
67

78
describe('ConfiguratorShowMoreComponent', () => {
89
let component: ConfiguratorShowMoreComponent;
910
let fixture: ComponentFixture<ConfiguratorShowMoreComponent>;
1011
let htmlElem: HTMLElement;
12+
let sanitizerSpy: jasmine.SpyObj<DomSanitizer>;
1113

1214
beforeEach(waitForAsync(() => {
15+
sanitizerSpy = jasmine.createSpyObj<DomSanitizer>('DomSanitizer', [
16+
'bypassSecurityTrustHtml',
17+
]);
1318
TestBed.configureTestingModule({
1419
imports: [I18nTestingModule],
1520
declarations: [ConfiguratorShowMoreComponent],
21+
providers: [{ provide: DomSanitizer, useValue: sanitizerSpy }],
1622
})
1723
.overrideComponent(ConfiguratorShowMoreComponent, {
1824
set: {
@@ -35,79 +41,59 @@ describe('ConfiguratorShowMoreComponent', () => {
3541
expect(component).toBeTruthy();
3642
});
3743

38-
it('should render component', () => {
44+
it('should render component', async () => {
3945
fixture.detectChanges();
46+
await fixture.whenStable();
4047
CommonConfiguratorTestUtilsService.expectElementPresent(
4148
expect,
4249
htmlElem,
4350
'span'
4451
);
45-
CommonConfiguratorTestUtilsService.expectElementPresent(
46-
expect,
47-
htmlElem,
48-
'button'
49-
);
50-
});
51-
52-
it('should set showMore after view init', () => {
53-
component.ngAfterViewInit();
54-
fixture.detectChanges();
55-
expect(component.showMore).toBe(true);
56-
expect(component.textToShow).toBe(component.text.substring(0, 60));
5752
});
5853

59-
it('should not set showMore after view init', () => {
60-
component.text = 'short text';
61-
62-
component.ngAfterViewInit();
63-
fixture.detectChanges();
64-
CommonConfiguratorTestUtilsService.expectElementNotPresent(
65-
expect,
66-
htmlElem,
67-
'button'
54+
it('should remove HTML tags from input text', () => {
55+
sanitizerSpy.bypassSecurityTrustHtml.and.returnValue(
56+
'Sanitized Text' as any
57+
); // Fake SafeHtml
58+
const result = component.normalize('<b>Sanitized Text</b>');
59+
expect(sanitizerSpy.bypassSecurityTrustHtml).toHaveBeenCalledWith(
60+
'<b>Sanitized Text</b>'
6861
);
69-
expect(component.showMore).toBe(false);
70-
expect(component.textToShow).toBe(component.text);
62+
expect(result).toEqual('Sanitized Text');
7163
});
7264

73-
it('should set showHiddenText after toggleShowMore action', () => {
74-
fixture.detectChanges();
75-
component.ngAfterViewInit();
76-
component.toggleShowMore();
77-
fixture.detectChanges();
78-
expect(component.showHiddenText).toBe(true);
79-
expect(component.textToShow).toBe(component.text);
65+
it('should return an empty string when input is null', () => {
66+
sanitizerSpy.bypassSecurityTrustHtml.and.returnValue(null);
67+
const result = component.normalize(null as unknown as string);
68+
expect(result).toEqual('');
8069
});
8170

82-
describe('Sanitization of suspicious input', () => {
83-
const suspiciousTextWithFormatting =
84-
'<h1>Digital camera</h1> is a great product <p> <script';
85-
const suspiciousTextWithoutFormatting =
86-
'Digital camera is a great product <script';
87-
const sanitizedText = 'Digital camera is a great product';
71+
it('should return an empty string when input is undefined', () => {
72+
sanitizerSpy.bypassSecurityTrustHtml.and.returnValue(undefined);
73+
const result = component.normalize(undefined as unknown as string);
74+
expect(result).toEqual('');
75+
});
8876

89-
it('does not happen through method normalize because that is meant for removing HTML tags for better readibility', () => {
90-
component.text = suspiciousTextWithFormatting;
91-
component.ngAfterViewInit();
92-
fixture.detectChanges();
93-
expect(component.textNormalized).toBe(suspiciousTextWithoutFormatting);
94-
expect(component['normalize'](suspiciousTextWithFormatting)).toBe(
95-
suspiciousTextWithoutFormatting
96-
);
97-
});
77+
it('should return the same text if there are no HTML elements', () => {
78+
sanitizerSpy.bypassSecurityTrustHtml.and.returnValue('Plain Text' as any);
79+
const result = component.normalize('Plain Text');
80+
expect(result).toEqual('Plain Text');
81+
});
9882

99-
it('should happen on view', () => {
100-
component.text = suspiciousTextWithFormatting;
101-
component.ngAfterViewInit();
102-
fixture.detectChanges();
83+
it('should remove script tags to prevent XSS', () => {
84+
sanitizerSpy.bypassSecurityTrustHtml.and.returnValue('Safe Content' as any);
85+
const result = component.normalize(
86+
'<script>alert("XSS")</script>Safe Content'
87+
);
88+
expect(result).toEqual('Safe Content');
89+
});
10390

104-
CommonConfiguratorTestUtilsService.expectElementToContainText(
105-
expect,
106-
htmlElem,
107-
'span',
108-
sanitizedText
109-
);
110-
});
91+
it('should handle special characters properly', () => {
92+
sanitizerSpy.bypassSecurityTrustHtml.and.returnValue(
93+
'Text & Special Chars ©' as any
94+
);
95+
const result = component.normalize('Text & Special Chars ©');
96+
expect(result).toEqual('Text & Special Chars ©');
11197
});
11298

11399
describe('Accessibility', () => {

Diff for: feature-libs/product-configurator/rulebased/components/show-more/configurator-show-more.component.ts

+7-2
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,10 @@ import {
99
ChangeDetectionStrategy,
1010
ChangeDetectorRef,
1111
Component,
12+
inject,
1213
Input,
1314
} from '@angular/core';
15+
import { DomSanitizer } from '@angular/platform-browser';
1416

1517
@Component({
1618
selector: 'cx-configurator-show-more',
@@ -24,6 +26,8 @@ export class ConfiguratorShowMoreComponent implements AfterViewInit {
2426
textToShow: string;
2527
textNormalized: string;
2628

29+
sanitizer = inject(DomSanitizer);
30+
2731
@Input() text: string;
2832
@Input() textSize = 60;
2933
@Input() productName: string;
@@ -53,7 +57,8 @@ export class ConfiguratorShowMoreComponent implements AfterViewInit {
5357
this.cdRef.detectChanges();
5458
}
5559

56-
protected normalize(text: string = ''): string {
57-
return text.replace(/<[^>]*>/g, '');
60+
normalize(text: string = ''): string {
61+
const safeHtml = this.sanitizer.bypassSecurityTrustHtml(text);
62+
return safeHtml ? safeHtml.toString().replace(/<[^>]*>/g, '') : '';
5863
}
5964
}
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
import { inject, TestBed } from '@angular/core/testing';
2-
import { Product } from '../../../../model/product.model';
32
import { OccConfig } from '../../../config/occ-config';
4-
import { Occ } from '../../../occ-models/occ.models';
53
import { ProductNameNormalizer } from './product-name-normalizer';
4+
import { DomSanitizer } from '@angular/platform-browser';
65

76
const MockOccModuleConfig: OccConfig = {
87
backend: {
@@ -18,24 +17,17 @@ const MockOccModuleConfig: OccConfig = {
1817

1918
describe('ProductNameNormalizer', () => {
2019
let service: ProductNameNormalizer;
21-
22-
const product: Occ.Product = {
23-
name: '<div>Product1</div>',
24-
code: 'testCode',
25-
};
26-
27-
const convertedProduct: Product = {
28-
name: 'Product1',
29-
nameHtml: '<div>Product1</div>',
30-
code: 'testCode',
31-
slug: 'product1',
32-
};
20+
let sanitizerSpy: jasmine.SpyObj<DomSanitizer>;
3321

3422
beforeEach(() => {
23+
sanitizerSpy = jasmine.createSpyObj<DomSanitizer>('DomSanitizer', [
24+
'bypassSecurityTrustHtml',
25+
]);
3526
TestBed.configureTestingModule({
3627
providers: [
3728
ProductNameNormalizer,
3829
{ provide: OccConfig, useValue: MockOccModuleConfig },
30+
{ provide: DomSanitizer, useValue: sanitizerSpy },
3931
],
4032
});
4133

@@ -49,34 +41,35 @@ describe('ProductNameNormalizer', () => {
4941
}
5042
));
5143

52-
it('should convert product name', () => {
53-
const result = service.convert(product);
54-
expect(result).toEqual(convertedProduct);
44+
it('should sanitize the name', () => {
45+
sanitizerSpy.bypassSecurityTrustHtml.and.returnValue('Sanitized Name');
46+
47+
const result = service.convert({
48+
name: '<script>alert("XSS")</script>Product',
49+
});
50+
51+
expect(sanitizerSpy.bypassSecurityTrustHtml).toHaveBeenCalledWith(
52+
'<script>alert("XSS")</script>Product'
53+
);
54+
expect(result.name).toEqual('Sanitized Name');
5555
});
5656

57-
describe('slug', () => {
58-
const reservedChars = ` !*'();:@&=+$,/?%#[]`;
57+
it('should sanitize the name', () => {
58+
sanitizerSpy.bypassSecurityTrustHtml.and.returnValue('Sanitized Name');
5959

60-
// try all chars separately
61-
reservedChars.split('').forEach((char) => {
62-
it(`should replace "${char}"`, () => {
63-
const result = service.convert({
64-
name: `a product with ${char} included`,
65-
});
66-
expect(result.slug).toEqual('a-product-with-included');
67-
});
68-
});
60+
const result = service.convert({ name: '<b>Unsafe Name</b>' });
6961

70-
it(`should replace multiple occasions of the slug char (-)`, () => {
71-
const result = service.convert({
72-
name: ` a product with multiple --- symbols `,
73-
});
74-
expect(result.slug).toEqual('a-product-with-multiple-symbols');
75-
});
62+
expect(sanitizerSpy.bypassSecurityTrustHtml).toHaveBeenCalledWith(
63+
'<b>Unsafe Name</b>'
64+
);
65+
expect(result.name).toEqual('Sanitized Name'); // Ensure sanitized name is returned
66+
});
7667

77-
it('should not alter the original name', () => {
78-
const result = service.convert({ name: 'my product title' });
79-
expect(result.name).toEqual('my product title');
80-
});
68+
it('should handle empty names', () => {
69+
sanitizerSpy.bypassSecurityTrustHtml.and.returnValue('');
70+
71+
const result = service.convert({ name: '' });
72+
73+
expect(result.name).toEqual('');
8174
});
8275
});

Diff for: projects/core/src/occ/adapters/product/converters/product-name-normalizer.ts

+8-2
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,17 @@
44
* SPDX-License-Identifier: Apache-2.0
55
*/
66

7-
import { Injectable } from '@angular/core';
7+
import { inject, Injectable } from '@angular/core';
88
import { Product } from '../../../../model/product.model';
99
import { Converter } from '../../../../util/converter.service';
1010
import { OccConfig } from '../../../config/occ-config';
1111
import { Occ } from '../../../occ-models/occ.models';
12+
import { DomSanitizer } from '@angular/platform-browser';
1213

1314
@Injectable({ providedIn: 'root' })
1415
export class ProductNameNormalizer implements Converter<Occ.Product, Product> {
16+
sanitizer = inject(DomSanitizer);
17+
1518
constructor(protected config: OccConfig) {}
1619

1720
convert(source: Occ.Product, target?: Product): Product {
@@ -29,7 +32,10 @@ export class ProductNameNormalizer implements Converter<Occ.Product, Product> {
2932
* Sanitizes the name so that the name doesn't contain html elements.
3033
*/
3134
protected normalize(name: string): string {
32-
return name.replace(/<[^>]*>/g, '');
35+
return (
36+
this.sanitizer.bypassSecurityTrustHtml(name).toString() ||
37+
''.replace(/<[^>]*>/g, '')
38+
);
3339
}
3440

3541
/**

Diff for: projects/schematics/src/dependencies.json

+1
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,7 @@
216216
"@angular/common": "^19.1.8",
217217
"@angular/core": "^19.1.8",
218218
"@angular/forms": "^19.1.8",
219+
"@angular/platform-browser": "^19.1.8",
219220
"@angular/router": "^19.1.8",
220221
"@ng-select/ng-select": "^14.1.0",
221222
"@ngrx/effects": "^19.0.1",

Diff for: scripts/i18n/convert-translations-json-2-ts.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -145,10 +145,10 @@ function handleExistingJsonFile(
145145
function stringify(obj: any, indent: string = ''): string {
146146
if (typeof obj === 'string') {
147147
if (obj.includes('\n')) {
148-
return '`' + obj.replace(/`/g, '\\`') + '`';
148+
return '`' + obj.replace(/\\/g, '\\\\').replace(/`/g, '\\`') + '`';
149149
}
150150
if (obj.includes("'")) {
151-
return `"${obj.replace(/"/g, '\\"')}"`;
151+
return `"${obj.replace(/\\/g, '\\\\').replace(/"/g, '\\"')}"`;
152152
}
153153
return `'${obj}'`;
154154
} else if (typeof obj !== 'object' || obj === null) {

0 commit comments

Comments
 (0)