pawarprasad123 commented on code in PR #732: URL: https://github.com/apache/atlas/pull/732#discussion_r3840603729
########## dashboard/src/utils/__tests__/Utils.test.ts: ########## @@ -22,6 +22,7 @@ * Coverage Target: 100% for Statements, Branches, Functions, and Lines */ + Review Comment: The PR description states: `All 189 test suites and 4,791 tests pass with a 100% success rate. ` Local run 189 total suites → 42 failed, 147 passed Tests that did run: 3,730 passed (suites that failed never executed) Please re-run npm run test on a clean install and update the PR description, or fix the mock before claiming green CI. ########## dashboard/src/__mocks__/sanitize-html.ts: ########## @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const actualSanitizeModule = jest.requireActual('sanitize-html/index.js') as any; Review Comment: Jest mock breaks the test suite: jest.requireActual() runs at module load time, not lazily when options are passed. Any file importing Utils.ts (which imports sanitize-html) triggers this immediately. [email protected] pulls in htmlparser2@12 (pure ESM). Jest fails with: `SyntaxError: Cannot use import statement outside a module ` Impact: 42 test suites fail to load (anything transitively importing @utils/Utils). ########## dashboard/package.json: ########## @@ -3,6 +3,9 @@ "private": true, "version": "0.0.0", "type": "module", + "engines": { Review Comment: This aligns with root pom.xml (node-for-v1.version = v22.22.1), so the Maven build is consistent. However: No update to a dashboard README or dev docs Developers on Node 20 will get install/runtime warnings ########## dashboard/src/utils/__tests__/Utils.test.ts: ########## @@ -878,11 +879,52 @@ describe('Utils', () => { }); describe('sanitizeHtmlContent', () => { - it('should sanitize HTML content', () => { - const html = '<script>alert("xss")</script><p>Safe content</p>'; - const result = sanitizeHtmlContent(html); + it('should allow configured positive HTML tags and attributes', () => { Review Comment: Add edge-case tests: non-string input, empty string, mailto: links, and remaining allowed tags (strong, u, ol, h2–h4). ########## dashboard/src/__mocks__/sanitize-html.ts: ########## @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const actualSanitizeModule = jest.requireActual('sanitize-html/index.js') as any; + +const getSanitizeFn = () => { + if (typeof actualSanitizeModule === 'function') return actualSanitizeModule; + if (actualSanitizeModule && typeof actualSanitizeModule.default === 'function') return actualSanitizeModule.default; + return null; +}; + +const sanitizeHtml = (html: string, _options?: Record<string, unknown>) => { Review Comment: Rename _options → options since it is used. ########## dashboard/src/__mocks__/sanitize-html.ts: ########## @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const actualSanitizeModule = jest.requireActual('sanitize-html/index.js') as any; + +const getSanitizeFn = () => { Review Comment: line 20-24 Fix indentation to match project style; replace as any with a proper type. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
