Skip to content

Commit 13bdaf9

Browse files
rmch91rmch91
authored
Revert "fix: fix unsanitized untrusted input v2" (#20129)
1 parent 993b903 commit 13bdaf9

File tree

8 files changed

+103
-95
lines changed

8 files changed

+103
-95
lines changed

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

-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
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",
3332
"@angular/router": "^19.1.8",
3433
"@ng-select/ng-select": "^14.1.0",
3534
"@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(/>/g, '');
211+
minVal = minVal.replace('>', '');
212212
}
213213

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

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

+57-43
Original file line numberDiff line numberDiff line change
@@ -3,22 +3,16 @@ 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';
76

87
describe('ConfiguratorShowMoreComponent', () => {
98
let component: ConfiguratorShowMoreComponent;
109
let fixture: ComponentFixture<ConfiguratorShowMoreComponent>;
1110
let htmlElem: HTMLElement;
12-
let sanitizerSpy: jasmine.SpyObj<DomSanitizer>;
1311

1412
beforeEach(waitForAsync(() => {
15-
sanitizerSpy = jasmine.createSpyObj<DomSanitizer>('DomSanitizer', [
16-
'bypassSecurityTrustHtml',
17-
]);
1813
TestBed.configureTestingModule({
1914
imports: [I18nTestingModule],
2015
declarations: [ConfiguratorShowMoreComponent],
21-
providers: [{ provide: DomSanitizer, useValue: sanitizerSpy }],
2216
})
2317
.overrideComponent(ConfiguratorShowMoreComponent, {
2418
set: {
@@ -41,59 +35,79 @@ describe('ConfiguratorShowMoreComponent', () => {
4135
expect(component).toBeTruthy();
4236
});
4337

44-
it('should render component', async () => {
38+
it('should render component', () => {
4539
fixture.detectChanges();
46-
await fixture.whenStable();
4740
CommonConfiguratorTestUtilsService.expectElementPresent(
4841
expect,
4942
htmlElem,
5043
'span'
5144
);
52-
});
53-
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>'
45+
CommonConfiguratorTestUtilsService.expectElementPresent(
46+
expect,
47+
htmlElem,
48+
'button'
6149
);
62-
expect(result).toEqual('Sanitized Text');
6350
});
6451

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('');
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));
6957
});
7058

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-
});
59+
it('should not set showMore after view init', () => {
60+
component.text = 'short text';
7661

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');
62+
component.ngAfterViewInit();
63+
fixture.detectChanges();
64+
CommonConfiguratorTestUtilsService.expectElementNotPresent(
65+
expect,
66+
htmlElem,
67+
'button'
68+
);
69+
expect(component.showMore).toBe(false);
70+
expect(component.textToShow).toBe(component.text);
8171
});
8272

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');
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);
8980
});
9081

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 ©');
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';
88+
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+
});
98+
99+
it('should happen on view', () => {
100+
component.text = suspiciousTextWithFormatting;
101+
component.ngAfterViewInit();
102+
fixture.detectChanges();
103+
104+
CommonConfiguratorTestUtilsService.expectElementToContainText(
105+
expect,
106+
htmlElem,
107+
'span',
108+
sanitizedText
109+
);
110+
});
97111
});
98112

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

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

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

1715
@Component({
1816
selector: 'cx-configurator-show-more',
@@ -26,8 +24,6 @@ export class ConfiguratorShowMoreComponent implements AfterViewInit {
2624
textToShow: string;
2725
textNormalized: string;
2826

29-
sanitizer = inject(DomSanitizer);
30-
3127
@Input() text: string;
3228
@Input() textSize = 60;
3329
@Input() productName: string;
@@ -57,8 +53,7 @@ export class ConfiguratorShowMoreComponent implements AfterViewInit {
5753
this.cdRef.detectChanges();
5854
}
5955

60-
normalize(text: string = ''): string {
61-
const safeHtml = this.sanitizer.bypassSecurityTrustHtml(text);
62-
return safeHtml ? safeHtml.toString().replace(/<[^>]*>/g, '') : '';
56+
protected normalize(text: string = ''): string {
57+
return text.replace(/<[^>]*>/g, '');
6358
}
6459
}

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

+38-31
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import { inject, TestBed } from '@angular/core/testing';
2+
import { Product } from '../../../../model/product.model';
23
import { OccConfig } from '../../../config/occ-config';
4+
import { Occ } from '../../../occ-models/occ.models';
35
import { ProductNameNormalizer } from './product-name-normalizer';
4-
import { DomSanitizer } from '@angular/platform-browser';
56

67
const MockOccModuleConfig: OccConfig = {
78
backend: {
@@ -17,17 +18,24 @@ const MockOccModuleConfig: OccConfig = {
1718

1819
describe('ProductNameNormalizer', () => {
1920
let service: ProductNameNormalizer;
20-
let sanitizerSpy: jasmine.SpyObj<DomSanitizer>;
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+
};
2133

2234
beforeEach(() => {
23-
sanitizerSpy = jasmine.createSpyObj<DomSanitizer>('DomSanitizer', [
24-
'bypassSecurityTrustHtml',
25-
]);
2635
TestBed.configureTestingModule({
2736
providers: [
2837
ProductNameNormalizer,
2938
{ provide: OccConfig, useValue: MockOccModuleConfig },
30-
{ provide: DomSanitizer, useValue: sanitizerSpy },
3139
],
3240
});
3341

@@ -41,35 +49,34 @@ describe('ProductNameNormalizer', () => {
4149
}
4250
));
4351

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');
52+
it('should convert product name', () => {
53+
const result = service.convert(product);
54+
expect(result).toEqual(convertedProduct);
5555
});
5656

57-
it('should sanitize the name', () => {
58-
sanitizerSpy.bypassSecurityTrustHtml.and.returnValue('Sanitized Name');
59-
60-
const result = service.convert({ name: '<b>Unsafe Name</b>' });
57+
describe('slug', () => {
58+
const reservedChars = ` !*'();:@&=+$,/?%#[]`;
6159

62-
expect(sanitizerSpy.bypassSecurityTrustHtml).toHaveBeenCalledWith(
63-
'<b>Unsafe Name</b>'
64-
);
65-
expect(result.name).toEqual('Sanitized Name'); // Ensure sanitized name is returned
66-
});
67-
68-
it('should handle empty names', () => {
69-
sanitizerSpy.bypassSecurityTrustHtml.and.returnValue('');
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+
});
7069

71-
const result = service.convert({ name: '' });
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+
});
7276

73-
expect(result.name).toEqual('');
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+
});
7481
});
7582
});

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

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

7-
import { inject, Injectable } from '@angular/core';
7+
import { 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';
1312

1413
@Injectable({ providedIn: 'root' })
1514
export class ProductNameNormalizer implements Converter<Occ.Product, Product> {
16-
sanitizer = inject(DomSanitizer);
17-
1815
constructor(protected config: OccConfig) {}
1916

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

4135
/**

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

-1
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,6 @@
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",
220219
"@angular/router": "^19.1.8",
221220
"@ng-select/ng-select": "^14.1.0",
222221
"@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, '\\\\').replace(/`/g, '\\`') + '`';
148+
return '`' + obj.replace(/`/g, '\\`') + '`';
149149
}
150150
if (obj.includes("'")) {
151-
return `"${obj.replace(/\\/g, '\\\\').replace(/"/g, '\\"')}"`;
151+
return `"${obj.replace(/"/g, '\\"')}"`;
152152
}
153153
return `'${obj}'`;
154154
} else if (typeof obj !== 'object' || obj === null) {

0 commit comments

Comments
 (0)