Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: make html sanitization optional #6432

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

laurence-hudson-mindfoundry

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Tests for the changes have been added/updated (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • Backend (Database)
  • Backend (API)
  • Frontend

Describe the reason for change

We inject supplementary details into the labeler's interface that they need when making informed labeling decisions (but which isn't the object being labelled). This is usually data types which are not currently supported by LS (for example an interactive scrollable/zoomable map showing some geojson). Additionally this lets us smooth over any small gaps between what LS offers and what we need for any specific use case, without needing to fork LS.

Until #5232, we could use the HyperText tag for this, injecting elements & scripts. However that no longer works. It is pretty reasonable that you want to limit peoples ability to mess with the DOM, but in practice this code injection approach can work well.

What does this fix?

See above. Here's a few other people encountering similar issues #5860 #5688

What is the new behavior?

The <HyperText> tag now has an optional sanitizeHtml argument (default true; keep the old behavior), which can be used to bypass sanitization.

What is the current behavior?

When using the <HyperText> tag, the specified html is always sanitized, removing several tag types (script etc). There is no way to opt out of this.

What libraries were added/updated?

None

Does this change affect performance?

Not measurably

Does this change affect security?

Code injection if misused, but its opt-in, user is accepting that risk. Partially mitigated via CSP.

What alternative approaches were there?

Currently we are using v1.10.01 as its the last version before html sanitization was added.

A better overall approach might be to support a html/iframe "visual & experience tag" as the use case for this is not as part of the object being labeled.

What feature flags were used to cover this change?

None, its already opt-in

Does this PR introduce a breaking change?

(check only one)

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

What level of testing was included in the change?

(check all that apply)

  • e2e
  • integration
  • unit

I'd happily take some guidance on where there relevant tests are, and fill the gap. I struggled to orient myself within the mobx-statetree stuff.

Which logical domain(s) does this change affect?

frontend

Copy link

netlify bot commented Sep 24, 2024

👷 Deploy request for heartex-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 9d8d02e

Copy link

netlify bot commented Sep 24, 2024

👷 Deploy request for label-studio-docs-new-theme pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 9d8d02e

@github-actions github-actions bot added the feat label Sep 24, 2024
@laurence-hudson-mindfoundry
Copy link
Author

Bumping this for review / discussion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant