google / neuroglancer

WebGL-based viewer for volumetric data
Apache License 2.0
1.02k stars 283 forks source link

Update status.ts DOM text reinterpreted as HTML {Patch} #575

Closed Shivam7-1 closed 2 months ago

Shivam7-1 commented 2 months ago

By using innerText, it will avoid the risk of HTML injection, as these properties automatically escape any HTML special characters in the provided text. This helps prevent cross-site scripting (XSS) vulnerabilities by treating the input as plain text rather than interpreted HTML. Always be cautious when dealing with user input or dynamic content to prevent security risks

Shivam7-1 commented 2 months ago

Hi @jbms Could You Please Review Above PR Thanks

Shivam7-1 commented 2 months ago

Hi @jbms I noticed my PR was closed. Could you please provide some feedback on why it was closed? I'm eager to learn and improve. Thank you

NeilFraser commented 2 months ago

@Shivam7-1 If you look at the code, you'll see that there are two functions, setText and setHTML:

  setText(text: string, makeVisible?: boolean) {
    this.element.textContent = text;
    if (makeVisible) {
      this.setVisible(true);
    }
  }
  setHTML(text: string, makeVisible?: boolean) {
    this.element.innerHTML = text;
    if (makeVisible) {
      this.setVisible(true);
    }
  }

The setText function sets .textContent which is a safe way to assign text to a node, whereas the setHTML function sets .innerHTML specifically because it intends to creat HTML tags. Your proposed change would make setHTML behave the same as setText with any HTML tags printed as raw sources on the screen for the user to see.

In general it's not a good idea to submit code changes to a product you don't use, without testing it, and without reading and understanding the surrounding code. Blindly submitting changes breaks things.