Prevent hyperlink handler for potential dangerous URIs #850
Conversation
|
Can one of the admins verify this patch? |
|
ok to test |
|
Build succeeded. |
| window.open(url); | ||
| // Ask the browser to open the link in a new window. `javascript` and `data` URIs are disabled for | ||
| // security reasons. | ||
| if (url.toLowerCase().indexOf('javascript') !== 0 && url.toLowerCase().indexOf('data') !== 0) { |
There was a problem hiding this comment.
This can be done via regex:
/(javascript|data):/i.test(url)
There was a problem hiding this comment.
Also, it's worth logging when the url is ignored:
runtime.log("WARN:", "potentially malicious URL ignored");
There was a problem hiding this comment.
Good points. Updated the PR. Though I went with /^(javascript|data):/i.test(url)
LukasReschke
force-pushed
the
allow-data-and-js-uris
branch
from
55d71e3
to
805a110
Compare
|
Build succeeded. |
|
I'm happy with this, so 👍 from me. @kossebau, are you wanting further reviews on this? |
| window.open(url); | ||
| // Ask the browser to open the link in a new window. `javascript` and `data` URIs are disabled for | ||
| // security reasons. | ||
| if(/^(javascript|data):/i.test(url)) { |
There was a problem hiding this comment.
At least firefox 33.0 also deals with urls that are prepended with whitespaces, so evil persons could hack this by prepending whitespaces. So perhaps better /^\s*(javascript|data):/?
There was a problem hiding this comment.
Gotta love browser's forgiveness … - Adjusted. THX.
LukasReschke
force-pushed
the
allow-data-and-js-uris
branch
from
805a110
to
0f4190a
Compare
|
Build succeeded. |
LukasReschke
force-pushed
the
allow-data-and-js-uris
branch
from
0f4190a
to
934cb91
Compare
This prevents the user from clicking on URIs starting with `javascript:` or `data:`. The reason behind this is that this may be used to trick users in executing dangerous JS when viewing an untrusted document. (which is the case in our deployment for ownCloud) I'm not absolutely happy with that patch since it uses a blacklisting instead a whitelisting approach, but I consider it a feasible approach. Especially, considering all the possible values. (`mailto:foo@bar.com`, `ftp://`, `skype://`, etc...) Conflicts: ChangeLog.md
LukasReschke
force-pushed
the
allow-data-and-js-uris
branch
from
934cb91
to
5bd39ab
Compare
|
I rebased this - can I please get some momentum on this? I also fixed c29f77c#commitcomment-9190875 |
…ments If there are only floating elements, the Dojo toolbar shrinks to 0 height. To prevent this for left-aligned elements, do not set them as float:left. (Also reorder generation to keep current tool ordering) And for elements floating to the right, clear any floating for the toolbar element with "clear:both" on the pseudo :after element.
|
Build succeeded. |
|
Build succeeded. |
|
@kossebau this is waiting for your +1 😸 |
|
Time to work the switches to have this roll into master. There is a button with a switch symbol, perhaps that does it... |
Prevent hyperlink handler for potential dangerous URIs
|
@kossebau THX - I'll coordinate some patches on our side - please don't share this too prominently yet. |
This prevents the user from clicking on URIs starting with
javascriptordata. The reason behind this is that this may be used to trick users in executing dangerous JS when viewing an untrusted document. (which is the case in our deployment for ownCloud)I'm not absolutely happy with that patch for multiple reasons, but I consider it a feasible approach:
mailto:foo@bar.comorftp://. That's why I went with this route instead.javascript:instead but this fails due to the JSLint policy which then complains aboutlib/gui/HyperlinkClickHandler.js:119:28: error: JavaScript URL.