Skip to content

feat: Allow dangerous html based on config variable #1459

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 20, 2022
Merged

feat: Allow dangerous html based on config variable #1459

merged 6 commits into from
May 20, 2022

Conversation

MrwanBaghdad
Copy link
Contributor

Summary of Changes

Added configuration variable to allow rendering of dangerous HTML in the programmatic description for table detail pages

Tests

Documentation

The use case is that we'd like to enable embedding in the programmatic descriptions. As the programmatic descriptions are populated by an ingestion pipeline that can validate its safety and is not editable by any web user; a requirement missing in the table description were editing may be allowed and it won't be safe to enable such functionality of rendering dangerous HTML

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does

@boring-cyborg boring-cyborg bot added area:frontend From the Frontend folder category:ui labels Aug 25, 2021
@MrwanBaghdad MrwanBaghdad force-pushed the DCAT-209_allow_dangerous_html_based_on_config_variable branch from 683ef32 to 61ecb76 Compare August 25, 2021 11:25
@MrwanBaghdad MrwanBaghdad changed the title feat-allow dangerous html based on config variable feat: Allow dangerous html based on config variable Aug 25, 2021
@MrwanBaghdad
For table page's programmatic descriptions, allow dangerous html base…
b6e3c8a 
…d on configuration variable

Signed-off-by: MrwanBaghdad <[email protected]>
@MrwanBaghdad MrwanBaghdad force-pushed the DCAT-209_allow_dangerous_html_based_on_config_variable branch from 61ecb76 to b6e3c8a Compare August 25, 2021 12:46
@feng-tao
Copy link
Member

@allisonsuarez @danwom @Golodhros

@feng-tao feng-tao added the keep fresh Disables stalebot from closing an issue label Aug 27, 2021
@Golodhros
Copy link
Member

I am a bit weary of this change as we use that <EditableText /> for a lot of things.

I would love to hear the opinion of others on this matter. WDYT @danwom @dorianj?

@dorianj
Copy link
Contributor

dorianj commented Sep 13, 2021

Do we even need to use EditableText for this, given that editable is false? Can we include ReactMarkdown directly? I think that would assuage the concern of turning EditableText into a footgun?

@Golodhros
Copy link
Member

Do we even need to use EditableText for this, given that editable is false? Can we include ReactMarkdown directly? I think that would assuage the concern of turning EditableText into a footgun?

That makes sense to me!

@boring-cyborg boring-cyborg bot added area:common From common folder area:all Related to all the project area:helm labels Apr 28, 2022
@ozandogrultan ozandogrultan force-pushed the DCAT-209_allow_dangerous_html_based_on_config_variable branch from 3edbd64 to 9c04ef2 Compare April 28, 2022 14:42
@ozandogrultan ozandogrultan force-pushed the DCAT-209_allow_dangerous_html_based_on_config_variable branch from 9c04ef2 to b6e3c8a Compare April 28, 2022 14:43
@ozandogrultan
Copy link
Contributor

@Golodhros @dorianj Can you please take a look at this again? I've implemented the suggestion of including ReactMarkdown directly

Golodhros
Golodhros requested changes Apr 28, 2022
View reviewed changes
frontend/amundsen_application/static/js/config/config-utils.ts Outdated
export function isProgrammaticDescAllowDangerousHtml(): boolean {
return AppConfig.programmaticDescriptionAllowDangerousHtml;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a basic tests for this

frontend/amundsen_application/static/js/config/config-utils.ts Outdated
/**
* Returns whether dangerous html should be allowed in programmatic descriptions
*/
export function isProgrammaticDescAllowDangerousHtml(): boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a mouthful of name. It is tricky, but I think 'hasHTMLDescription' would work better. Same for the config object field.

frontend/amundsen_application/static/js/pages/TableDetailPage/index.tsx Outdated
@@ -232,7 +233,15 @@ export class TableDetail extends React.Component<

return descriptions.map((d) => (
<EditableSection key={`prog_desc:${d.source}`} title={d.source} readOnly>
<EditableText maxLength={999999} value={d.text} editable={false} />
<div className="editable-text">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what are we doing here.
You modified to allow for the 'allowDangerousHtml' prop, why using ReactMarkdown here directly?

@ozandogrultan
Copy link
Contributor

@Golodhros I have changed the approach slightly; instead of checking a frontend config variable, now only programmatic descriptions always allow dangerous html. This would allow the usage of tags like iframe in the programmatic descriptions.

@ozandogrultan
feat: Remove unrelated changes
29696ed 
Signed-off-by: Ozan Dogrultan <[email protected]>
@Golodhros
Copy link
Member

We talked about this before @dorianj, what do you think about this change?

@ozandogrultan
Copy link
Contributor

@dorianj @Golodhros Any updates on this pull request? It is a small change which would help our team (and potentially teams which require similar functionality) a lot and would be great to see it merged.

@ozandogrultan
Copy link
Contributor

@dorianj @Golodhros Just bumping this up, our team is still looking forward to seeing this PR getting merged!

Golodhros
Golodhros approved these changes May 20, 2022
View reviewed changes
Copy link
Member

@Golodhros Golodhros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Golodhros Golodhros merged commit 95e5458 into amundsen-io:main May 20, 2022
allisonsuarez pushed a commit that referenced this pull request Jun 8, 2022
@MrwanBaghdad @ozandogrultan @allisonsuarez
feat: Allow dangerous html based on config variable (#1459)
171e7ff 
* For table page's programmatic descriptions, allow dangerous html based on configuration variable

Signed-off-by: MrwanBaghdad <[email protected]>

* feat: PR review

Signed-off-by: Ozan Dogrultan <[email protected]>

* feat: Always allowDangerousHtml in programmatic descriptions

Signed-off-by: Ozan Dogrultan <[email protected]>

* feat: Remove unused imports

Signed-off-by: Ozan Dogrultan <[email protected]>

* feat: Remove unrelated changes

Signed-off-by: Ozan Dogrultan <[email protected]>

Co-authored-by: Ozan Dogrultan <[email protected]>
hansadriaans pushed a commit to DataChefHQ/amundsen that referenced this pull request Jun 30, 2022
@MrwanBaghdad @ozandogrultan
feat: Allow dangerous html based on config variable (amundsen-io#1459)
b2d2efe 
* For table page's programmatic descriptions, allow dangerous html based on configuration variable

Signed-off-by: MrwanBaghdad <[email protected]>

* feat: PR review

Signed-off-by: Ozan Dogrultan <[email protected]>

* feat: Always allowDangerousHtml in programmatic descriptions

Signed-off-by: Ozan Dogrultan <[email protected]>

* feat: Remove unused imports

Signed-off-by: Ozan Dogrultan <[email protected]>

* feat: Remove unrelated changes

Signed-off-by: Ozan Dogrultan <[email protected]>

Co-authored-by: Ozan Dogrultan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Golodhros Golodhros Golodhros approved these changes

@allisonsuarez allisonsuarez Awaiting requested review from allisonsuarez allisonsuarez is a code owner

@dorianj dorianj Awaiting requested review from dorianj

@feng-tao feng-tao Awaiting requested review from feng-tao feng-tao is a code owner

@mgorsk1 mgorsk1 Awaiting requested review from mgorsk1 mgorsk1 is a code owner

@sewardgw sewardgw Awaiting requested review from sewardgw sewardgw is a code owner

@ttannis ttannis Awaiting requested review from ttannis ttannis is a code owner

@verdan verdan Awaiting requested review from verdan verdan is a code owner

@youngyjd youngyjd Awaiting requested review from youngyjd youngyjd is a code owner

@dechoma dechoma Awaiting requested review from dechoma dechoma is a code owner

@jinhyukchang jinhyukchang Awaiting requested review from jinhyukchang

@bolkedebruin bolkedebruin Awaiting requested review from bolkedebruin

@dkunitsk dkunitsk Awaiting requested review from dkunitsk

Assignees
No one assigned
Labels
area:all Related to all the project area:common From common folder area:frontend From the Frontend folder keep fresh Disables stalebot from closing an issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MrwanBaghdad @feng-tao @Golodhros @dorianj @ozandogrultan