Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[SECURITY] update in ipynb2md.py #21159

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DanMcInerney
Copy link

@DanMcInerney DanMcInerney commented Dec 23, 2022

Fixed command injection bug where a user could payload the Jupyter notebook name or md filename with something like "notebook.ipynb&&cat /etc/shadow>/public_html/index.html".

Description

(Brief description on what this PR is about)

Checklist

Essentials

  • PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented

Changes

  • Changes direct argument injection in a system command to a safely escaped one in case webserver ever gives access to this script

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

Fixed command injection bug where a user could payload the Jupyter notebook name or md filename with something like "notebook.ipynb&&cat /etc/shadow>/public_html/index.html".
@DanMcInerney DanMcInerney requested a review from szha as a code owner December 23, 2022 18:30
@mxnet-bot
Copy link

Hey @DanMcInerney , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [sanity, miscellaneous, clang, unix-cpu, website, centos-gpu, centos-cpu, edge, windows-gpu, windows-cpu, unix-gpu]


Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@mseth10 mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Dec 23, 2022
@DanMcInerney
Copy link
Author

This test is failing because an S3 bucket doesn't exist in the test cases. This PR shouldn't affect any usability or overhead.

@DanMcInerney DanMcInerney changed the title Security update in ipynb2md.py [SECURITY] update in ipynb2md.py Jan 3, 2023
@mseth10 mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Jan 3, 2023
@josephevans
Copy link
Contributor

josephevans commented Jan 16, 2023

Hi, thanks for your contribution. Could you please rebase this PR so the required CI checks will pass? Thanks.

@DanMcInerney
Copy link
Author

Yes, although I'm not clear on how to fix the fails. For example, this one seems to be failing because a bucket doesn't exist that I don't have control over?

ci/jenkins/mxnet-validation/website — Job failed
RuntimeError: Failed downloading url https://md-datasets-cache-zipfiles-prod.s3.eu-west-1.amazonaws.com/hb74ynkjcn-1.zip

@josephevans
Copy link
Contributor

Yes, although I'm not clear on how to fix the fails. For example, this one seems to be failing because a bucket doesn't exist that I don't have control over?

ci/jenkins/mxnet-validation/website — Job failed RuntimeError: Failed downloading url https://md-datasets-cache-zipfiles-prod.s3.eu-west-1.amazonaws.com/hb74ynkjcn-1.zip

Hi Dan, the failing tests have been fixed in #21162, so if you rebase your PR, it should then pass all CI pipelines.

@mseth10 mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Jan 26, 2023
@mseth10 mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Jan 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-work-in-progress PR is still work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants