Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings
This repository was archived by the owner on Nov 17, 2023. It is now read-only.

[SECURITY] update in ipynb2md.py #21159

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

Conversation

@DanMcInerney
Copy link

@DanMcInerney DanMcInerney commented Dec 23, 2022 •
edited
Loading

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".
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
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
Copy link
Contributor

josephevans commented Jan 16, 2023 •
edited
Loading

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

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

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.

Reviewers

1 more reviewer

@szha szha szha approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr-work-in-progress PR is still work in progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /