-
Notifications
You must be signed in to change notification settings - Fork 82
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: custom env linking text #828
Conversation
@noah-paige @dlpzx please review.. btw I can't set you as reviewers :) |
Hi @zsaltys, thanks for opening a PR :) We will review with @maryamkhidir and @itsmo-amzn after v2.1.0 is released |
@dlpzx thanks! |
frontend/src/modules/Environments/views/EnvironmentCreateForm.js
Outdated
Show resolved
Hide resolved
@itsmo-amzn @maryamkhidir I will make a PR that uses DOMPurify and a new react component? Will that be acceptable to merge this in? |
@zsaltys Yes, that works |
@maryamkhidir @itsmo-amzn I have added support for DOMPurify. I have close to no experience with React or NPM so apologies if I've made some mistakes. Please let me know if there's anything else I should fix. |
Changed the target branch to |
@zsaltys Taking a look |
export const SanitizedHTML = ({ dirtyHTML }) => { | ||
const defaultOptions = { | ||
ALLOWED_TAGS: ['b', 'i', 'em', 'strong', 'a'], | ||
ALLOWED_ATTR: ['href'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you expecting a link from the config? If not, I don't think you should allow href
. Otherwise, it looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maryamkhidir yes we are using links. That's the main reason I needed HTML support. We provide links for users to our internal systems so they know how to onboard data.all environments.
Thanks for picking it up @zsaltys, the PR also needs to be updates with the latest changes in |
Re-raised this PR to the main branch: https://github.com/awslabs/aws-dataall/pull/934/files .. To avoid solving a huge merge conflict by changing the base on this branch. |
Feature or Bugfix
Detail
Security
The only security concern is that there's an html value coming out of config.json which gets set as innerHTML. I don't think this would be an issue unless someone would find a way to override the config. value as an attack.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.