XWIKI-23433: Provide frontend check of links #4645
Conversation
* Add new properties to enable or not frontend link checks and
whitelist of links
* Provide script services to access the data from the configuration
* Add new mechanism allowing to check links when clicking on them
based on the configuration
| <script type="application/json" id="trusted-domains-configuration"> | ||
| { | ||
| "trustedDomains": $jsontool.serialize($services.security.url.getTrustedDomains()), | ||
| "whitelistedUrls": $jsontool.serialize($services.security.url.getWhitelistedFrontendUrls()), |
There was a problem hiding this comment.
Comment here but I'll also add it to other places: It would be nice to avoid the term "white" and use "allowed" instead as it is much clearer and doesn't have any bad connotations. See also https://en.wikipedia.org/wiki/Whitelist#Controversy_regarding_name
| "trustedDomains": $jsontool.serialize($services.security.url.getTrustedDomains()), | ||
| "whitelistedUrls": $jsontool.serialize($services.security.url.getWhitelistedFrontendUrls()), | ||
| "confirmationText": $jsontool.serialize($services.localization.render('url.api.followLinkConfirmationText', | ||
| ['__CURRENT_DOMAIN__', '__EXTERNAL_DOMAIN__'])) |
There was a problem hiding this comment.
Any reason for not using the standard translations mechanism in JavaScript? If yes, it might be good to add a comment.
| @Unstable | ||
| public List<String> getWhitelistedFrontendUrls() | ||
| { | ||
| return this.urlConfiguration.getWhitelistedFrontendUrls(); |
There was a problem hiding this comment.
As mentioned before, I would prefer to avoid the term "white" and use "allowed" instead.
| * @since 17.9.0RC1 | ||
| */ | ||
| @Unstable | ||
| default List<String> getWhitelistedFrontendUrls() |
There was a problem hiding this comment.
Same as before, I would prefer "allowed" instead of "white".
| # Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA | ||
| # 02110-1301 USA, or see the FSF site: http://www.fsf.org. | ||
| # --------------------------------------------------------------------------- | ||
| url.api.followLinkConfirmationText=You are about to leave the domain "{0}" to follow a link to "{1}". Are you sure to \ |
There was a problem hiding this comment.
"Are you sure you want to continue?" sounds better as second sentence for me.
| require(['jquery', 'xwiki-events-bridge'], function($) { | ||
|
|
||
| let protectLinks = function(elements) { | ||
| let trustedDomainConfigElement = $('#trusted-domains-configuration'); |
There was a problem hiding this comment.
I would make this more explicit with
| let trustedDomainConfigElement = $('#trusted-domains-configuration'); | |
| let trustedDomainConfigElement = $('script#trusted-domains-configuration'); |
to avoid that somebody creates another element in the document with the same ID - selecting for script elements acts as a protection here as we don't allow creating script elements easily. It's probably unlikely as you would probably still take the first one, and it shouldn't be possible to insert another element before the correct one.
| }; | ||
|
|
||
| $(document).on('xwiki:dom:updated', function(event, data) { | ||
| protectLinks($(data.elements)); |
There was a problem hiding this comment.
As mentioned above, just listening on document is probably easier and would avoid this.
| $(document).on('xwiki:dom:updated', function(event, data) { | ||
| protectLinks($(data.elements)); | ||
| }); | ||
| protectLinks($('body')); |
There was a problem hiding this comment.
Shouldn't this wait for the document to be ready?
| let customizedMessage = configuration.confirmationText | ||
| .replace('__CURRENT_DOMAIN__', currentHostname) | ||
| .replace('__EXTERNAL_DOMAIN__', anchorHostname); | ||
| return confirm(customizedMessage); |
There was a problem hiding this comment.
I'm wondering if it wouldn't be nicer to use a custom styled <dialog>. But it would probably interfere with the nice property here that the original event continues instead of having a new one where all open in new tab properties etc. would be lost.
| #-# https://github.com/xwiki/xwiki-platform,https://www.xwiki.org/xwiki/bin/view/Main/WebHome | ||
| #-# | ||
| #-# By default this property is empty: | ||
| # url.whitelistedFrontendUrls= |
There was a problem hiding this comment.
Same comment as above regarding "whitelisted" vs. "allowed".
| let protectLinks = function(elements) { | ||
| let trustedDomainConfigElement = $('#trusted-domains-configuration'); |
There was a problem hiding this comment.
Can't those let be const?
| let protectLinks = function(elements) { | ||
| let trustedDomainConfigElement = $('#trusted-domains-configuration'); | ||
| if (trustedDomainConfigElement.length > 0) { | ||
| let configuration = JSON.parse(trustedDomainConfigElement.text()); |
There was a problem hiding this comment.
How do you handle parsing issues?
Asking since we encountered some issues where a failed parsing was breaking unrelated pieces of JS.
| } | ||
| }; | ||
|
|
||
| let askIfLinkNotTrusted = function (event, anchor, configuration) { |
There was a problem hiding this comment.
Why assign an anonymous function to a mutable variable?
| let askIfLinkNotTrusted = function (event, anchor, configuration) { | |
| function askIfLinkNotTrusted (event, anchor, configuration) { |
3 participants
Jira URL
https://jira.xwiki.org/browse/XWIKI-23433
Changes
Description
Clarifications
Screenshots & Video
Executed Tests
Manual test only, it's probably missing some javascript tests and integration test maybe.
Expected merging strategy