Conversation
Cleared previous selection
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on 2fec878..9aadb87
| Severity | Location | Issue | Delete |
|---|---|---|---|
 |
recon/src/components/JsonToReact.js:13 | Null pointer dereference |  |
|
recon/src/components/JsonToReact.js:43 | Undefined destructuring crash | |
|
recon/src/components/JsonToReact.js:48 | Cross-site scripting vulnerability | |
| if (foundEl.current) | ||
| foundEl.current.style.backgroundColor = ''; | ||
| var el = document.querySelector(`span[data-slice='${tokenSlice}']`); | ||
| el.style.backgroundColor = 'yellow'; |
There was a problem hiding this comment.
Null pointer dereference: Line 12 calls document.querySelector() which returns null if no matching element is found. Line 13 immediately accesses el.style.backgroundColor without checking if el is null. This will throw a runtime error: 'TypeError: Cannot read property "style" of null' when a user enters a tokenSlice value that doesn't match any span element in the DOM. The fix should add a null check: if (el) el.style.backgroundColor = 'yellow';
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| const {key, tag, val = '', attrs: {src} = {}, children = []} = el; | ||
| var text = []; | ||
| if (val) { | ||
| var {span: {start, end}} = reconKeyToSlice.find(({uuid}) => uuid === key); |
There was a problem hiding this comment.
Undefined destructuring error: The Array.find() method returns undefined when no matching element is found. This code attempts to destructure the result without checking if it exists, which will throw a runtime error: 'TypeError: Cannot read property "span" of undefined' when the key parameter doesn't match any uuid in the reconKeyToSlice array. This will crash the application when rendering elements with keys that don't exist in the nlp data structure. The fix should check for undefined: const match = reconKeyToSlice.find(({uuid}) => uuid === key); if (!match) return ''; var {span: {start, end}} = match;
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| } | ||
| let html = `<${tag}${src ? ' src=' + src : ''}>`; | ||
| text.forEach(({textWithWs, sentIdx}, i) => { | ||
| html += `<span data-slice=${start + i} data-sentence=${sentIdx}>${textWithWs}</span>` |
There was a problem hiding this comment.
Cross-Site Scripting (XSS) vulnerability: This line constructs HTML with unquoted attributes and directly inserts textWithWs content without escaping. The generated HTML is then rendered via dangerouslySetInnerHTML on line 33. If textWithWs contains malicious HTML or JavaScript (e.g., <script>alert('xss')</script>, <img src=x onerror=alert('xss')>, or even attribute injection via unquoted attributes), it will be executed in the browser, allowing attackers to inject arbitrary code. React's documentation explicitly warns that dangerouslySetInnerHTML can lead to XSS attacks when used with unsanitized content. The fix should: 1) Quote all HTML attributes, 2) HTML-escape the textWithWs content before insertion, or 3) Use React's virtual DOM instead of dangerouslySetInnerHTML.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
1 participant
As discussed in interview, here's a PR for my technical test. Thanks for your time