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

Fix PyPI package source for v1.1.0 #1088

Merged
merged 10 commits into from
Jul 21, 2022

Conversation

sugatoray
Copy link
Contributor

@sugatoray sugatoray commented Jul 2, 2022

Use jinja2 autoescaping for security purposes

  • add select_autoescape(enabled_extensions=('html',)) to line-114.

    root = os.path.dirname(os.path.abspath(__file__))
    templates_dir = os.path.join(root, "html")
    env = Environment(loader=FileSystemLoader(templates_dir))
    template = env.get_template("template.html")

    # 🔥 include autoescaping for security purposes
    # sources:
    # - https://jinja.palletsprojects.com/en/3.0.x/api/#autoescaping
    # - https://stackoverflow.com/a/38642558/8474894 (see in comments)
    # - https://stackoverflow.com/a/68826578/8474894
  • Fix non-inclusion of flytekit/deck/html/template.html in v1.1.0: Add MANIFEST.in to fix this bug.

    • The setup.py file tries to package template.html from the root of the repository. But, it has been moved to flytekit/deck/html/template.html.

      package_data={"": ["template.html"]},

    • There is no template.html at the root of the package.

      image

    • The PyPI package (v1.1.0) does not have any template.html.

    image

    • The PyPI package (v1.1.0) also does not include any template.html inside flytekit/deck/html directory.

    image

Type

  • Bug Fix: 🟠
  • Security vulnerability

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

https://github.com/flyteorg/flyte/issues/

@welcome
Copy link

welcome bot commented Jul 2, 2022

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

sugatoray added 4 commits July 2, 2022 18:27
- explicitly control inclusion and exclusion of files and folders in the package source.
- update setup.py
Signed-off-by: Author Name <[email protected]>
@sugatoray sugatoray changed the title Use jinja2 autoescaping for security purposes Fix PyPI package source for v1.1.0 Jul 4, 2022
@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.93%. Comparing base (a968a5a) to head (032f1ad).
Report is 1027 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1088      +/-   ##
==========================================
+ Coverage   86.67%   86.93%   +0.25%     
==========================================
  Files         269      275       +6     
  Lines       25074    25448     +374     
  Branches     2826     2862      +36     
==========================================
+ Hits        21734    22123     +389     
+ Misses       2871     2847      -24     
- Partials      469      478       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pingsutw
Copy link
Member

pingsutw commented Jul 5, 2022

@sugatoray It seems like the dataframe doesn't be rendered correctly if using autoescaping, check the example below

import pandas as pd
from flytekit import task, workflow

@task
def t1() -> pd.DataFrame:
    return pd.DataFrame({"Name": ["Tom", "Joseph"], "Age": [20, 22]})

@workflow
def wf() -> pd.DataFrame:
    return t1()

wf()

image

The correct output should be like
image

@wild-endeavor
Copy link
Contributor

hey @sugatoray - would you mind taking a look at the issue kevin pointed out here please? There's no unit test to capture this yet unf. he was testing on a demo cluster i think.

@sugatoray
Copy link
Contributor Author

Interesting! Let me look into it.

@pingsutw
Copy link
Member

pingsutw commented Jul 8, 2022

@sugatoray You're able to reproduce it locally as well. check this example

@sugatoray
Copy link
Contributor Author

sugatoray commented Jul 14, 2022

Some Notes

The following is taken from Flask. It provides more clarity on what autoescaping means in Jinja2 and how to allow expected html tags safely.

I would like to try them in the following order:

  1. Use the {% autoescape %} block while rendering the template.

    {% autoescape false %}
        <p>autoescaping is disabled here
        <p>{{ will_not_be_escaped }}
    {% endautoescape %}
  2. Use {{ variable | safe }} inside the template (template.html) to specify each variable as safe.

The other alternative will be to pass the generated html template into IPython.display.HTML() function. (This is just a hunch; am not sure if this will work or not)

source: https://flask.palletsprojects.com/en/2.1.x/templating/#controlling-autoescaping

image

Other Notes

@wild-endeavor
Copy link
Contributor

did this work? tests are passing but lint is now complaining. would it be possible to codify the original issue pingsutw raised into a unit test btw?

@wild-endeavor
Copy link
Contributor

@sugatoray were you able to test this? does the safe render things correctly again?
Thank you.

@wild-endeavor
Copy link
Contributor

@sugatoray mind giving me write access to your fork so i can clean up the lint error?

@sugatoray
Copy link
Contributor Author

@wild-endeavor Sorry about the long silence. I have not had a chance to test if the same rendering error is still happening.

Please let me know what I need to do to give you access to clean up the linting errors.

Signed-off-by: Kevin Su <[email protected]>
Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

I just tested it, and render stuff looks good to me. Thank you @sugatoray

@wild-endeavor wild-endeavor merged commit 9d50502 into flyteorg:master Jul 21, 2022
@welcome
Copy link

welcome bot commented Jul 21, 2022

Congrats on merging your first pull request! 🎉

wild-endeavor pushed a commit that referenced this pull request Aug 2, 2022
Signed-off-by: Author Name <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

Co-authored-by: Kevin Su <[email protected]>
wild-endeavor pushed a commit that referenced this pull request Aug 2, 2022
Signed-off-by: Author Name <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

Co-authored-by: Kevin Su <[email protected]>
@wild-endeavor wild-endeavor mentioned this pull request Aug 4, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants