-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Restrict write18 #40
Restrict write18 #40
Conversation
Some notes from talking with @ian-r-rose in person: I think this should have three values that are strings validated by traitlets. I'm not entirely sure what the string values should be… but I think that we should have restricted write18 enabled as the default; that should probably be called "restricted", and could maybe call the complete enabled option "allow_all", and the completely disabled option as "disabled"? The way I think we're supposed to do this is to have a Unicode type traitlet, but have a validation function (using a |
This is ready for another look. |
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.
I think this makes the shell command harder to follow, can we keep it so we don’t need to conditionally add something and then finish the command? Or that we always add the empty string?
jupyterlab_latex/__init__.py
Outdated
if escape_flag != '': | ||
full_latex_sequence.append(escape_flag) | ||
|
||
full_latex_sequence.append(f"{tex_base_name}") |
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.
I don’t think I like this… I’d prefer for us to have the literal construction occur in one place and pass in the value even if it were to be the default anyway.
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.
The subprocess failed if it had an empty string in it (for some unknown reason). This is a workaround for that.
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.
Is there a value to pass in for restricted?
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.
Is there a value to pass in for restricted?
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.
No, that is the default.
Fixed. |
Does |
It does not appear to have any effect. Since restricted mode is default, it preserves that. Is that documented anywhere? |
jupyterlab_latex/__init__.py
Outdated
escape_flag = '-no-shell-escape' | ||
elif c.shell_escape == 'restricted': | ||
escape_flag = '-shell-restricted' | ||
full_latex_sequence = [ |
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.
Could we go back to this being a tuple?
It looks like most LaTeX distributions disable write18 by default, so the security risk for arbitrary code execution is already mitigated by that. This exposes an option for making that behavior explicit.