plots: fix wrong link #6693
plots: fix wrong link #6693
Conversation
| <img src="{src}"> | ||
| </div>""".format( | ||
| title=revision, src=rel_img_path | ||
| title=revision, src=(relpath(img_path, page_dir_path)) |
There was a problem hiding this comment.
Not sure what this refactoring does 😋
My feedback (#6431 (review)) was that you could inject HTML code via revision, which is a CLI input I think (can be any string). Of course it get's prepended with static/ so maybe that neutralizes any script, not sure.
Still the best practice would be to validate the input or at least sanitize the HTML output? Or maybe I'm exaggerating here.
There was a problem hiding this comment.
@jorgeorpinel I am not sure I understand. Do you mean to pass some HTML via revision CLI argument? What would this HTML be and what would be its purpose?
There was a problem hiding this comment.
Sorry for the delay @pared .
Do you mean to pass some HTML via revision CLI argument?
Yes. Well, probably JS mainly.
What would this HTML be and what would be its purpose?
A malicious <script>.
There was a problem hiding this comment.
Ok, so in the beginning, I did not understand the problem. Hmm, it seems that you are right here.
On the other hand, I think that if one is able to use dvc plots show, he/she can edit the webpage in-place (since they have write permission anyway), so the modification can be done without help of DVC.
There was a problem hiding this comment.
OK @pared ! Up to you
There was a problem hiding this comment.
If #6693 (review) is not a vulnerability you should probably either roll back the refactored chunk and merge, or merge as it is now if you prefer. Thanks
|
@jorgeorpinel let's leave it as is. @jbencook can I get approval? cannot merge without it |
|
Thanks @skshetry! |
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏
Fixes @jorgeorpinel review comments at #6431