Opened 5 years ago

Closed 4 years ago

#9661 closed Bug (fixed)

javascript:void(0) Invalid Value for Link Href

Reported by: TomNM Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.4.1
Component: General Version: 4.0
Keywords: Cc:

Description

Is javascript:void(0) intended to be an invalid value for the link dialog? My view is that it should be allowed in the href field. It was formerly with FCKeditor.

You can reproduce in the http://nightly-v4.ckeditor.com/3928/samples/replacebycode.html.

  1. Paste <a href="javascript:void(0);">test</a> in source mode.
  2. Go to Wysiwyg.
  3. Edit link, and just try to save. You'll get "invalid value".

So, again, I believe there should be a way to allow this to support this method.

Thanks, Tom

Change History (20)

comment:1 Changed 5 years ago by Jakub Ś

Status: newconfirmed
Version: 4.0 Beta4.0 (GitHub - master)

comment:2 Changed 5 years ago by Jakub Ś

Please note that it has worked on CKE 4 beta.

comment:3 Changed 5 years ago by Piotrek Koszuliński

Resolution: invalid
Status: confirmedclosed

JS links aren't allowed any more. See #9289.

We may consider allowing javascript:void(0); value since it is a well known pattern and it's harmless (but I'm not 100% sure if on some browser void can't be overwritten). But I think it doesn't make sense, because eventually someone can ask for next pattern.

Therefore, you can modify link's url validator (check dialog sample).

comment:4 Changed 5 years ago by TomNM

void(0) would be fine.

JS links are very common. Am I missing how to include the "onclick" attribute? Without these, one could not fire google analytics event tracking on a link, for example.

Yes, whatever the next widely supported pattern becomes, it would be good to support that too, I suppose, IF it is as widely supported as this. However, for now, this is the pattern that has existed for 10+ years(?) is widely supported, popular and widely used and even provided in link code by certain third party api's, which is why it was brought to us by a user. I agree it's probably harmless.

I do think it makes sense to include, and is easier to have it as an option in the standard build rather than requiring developers to add it. I believe it's common enough that all serious users would need it at some point.

Also, in the dialog, I don't appear to be able to instead use 'onclick="return false;"' Am I missing that?

What would you say the options are to fire js from a link entered through the dialog? Google Analytics requires it.

I really think it makes sense to include this.

comment:5 Changed 5 years ago by Piotrek Koszuliński

We need to discuss this topic with Fred. He has closed #9289 so he knows about that change. So he probably has more to say in this case.

Anyway - we can't have editor suitable for all needs. Of course we're trying to do so, but the more flexibility we add the harder the project is to maintain for us but also to configure for users. Modifying dialogs is already possible. We can add another configuration option for links dialog allowing/disallowing JS links or more flexible one - defining disallowed patterns. But there are hundreds of feature requests and we have to filter them wisely.

But for now let's wait for Fred (he'll be back next week). I'm leaving this issue closed, but don't worry - it'll stay in my TODO list :).

Last edited 5 years ago by Piotrek Koszuliński (previous) (diff)

comment:6 Changed 5 years ago by Alfonso Martínez de Lizarrondo

Yes, it's quite incredible why that change was made as no one else would think that the change should be done on CKEditor instead of correctly filtering the data at the server.

That's so basic that seeing that request (as well as some other ones) makes one think that the product from Oracle must be a Swiss cheese full of security holes because they only looked at first layer instead of applying a deep security review of their systems.

And the bad part is that the only way to disable it is by modifying the source code and recompiling. One would think that as they are using an old version of CKEditor they should be the ones that make that change to their version, or use the CKEditor API to apply filtering on the data<->HTML transformation instead of forcing it to everyone with some code that it's inside a closure.

comment:7 Changed 5 years ago by Jakub Ś

Before you guys hang us:) I will just say that I agree with you (perhaps others will agree but we have to wait for Fred to get back) and I think:

  1. #9289 - this should be made e.g. configurable with config option
  2. This ticket (#9661) should be confirmed and fixed.
  3. #9908 should be invalidated.

Just please be patient :)

comment:8 Changed 5 years ago by Piotrek Koszuliński

Resolution: invalid
Status: closedreopened

Alfonso - you're right in 98% :). Data validation has to be done on the server side and if Oracle doesn't do that... That would be unwise ;)

  • 1% - #9289 isn't related to server side validation - it was about preview.
  • 1% - temporary fix isn't so complicate and you don't have to modify source:
CKEDITOR.on( 'dialogDefinition', function( evt ) {
	if ( evt.data.name == 'link' ) {
		var field = evt.data.definition.getContents( 'info' ).get( 'url' ),
			validate = field.validate;

		field.validate = function() {
			if ( ( /javascript\:/ ).test( this.getValue() ) )
				return true;
			return validate.call( this );
		};
	}
} );

I'm reopening this ticket. Not because I instantly liked JS links in CKEditor (I'm a purist ;)), but because I lost confidence in the #9298. I was (naively) thinking that editor completely disables scripts (tags and attrs) and that link dialog was the only "security hole" (ideologically, because it accepts JS when editor strips it), but I was completely wrong. In this case patch for #9289 looks strange.

comment:9 Changed 5 years ago by TomNM

Understood. Thank you for requesting a a new look at it.

comment:10 Changed 5 years ago by Frederico Caldeira Knabben

Status: reopenedconfirmed

#9289 is something we didn't want to have, because we all know that this doesn't solve any security issue. Still the fix was so simple that we decided to bring it in, just to make our Oracle friends happy. It was my mistake to think that it was a safe solution.

We may consider reverting [7603], explaining that if any kind of custom validation is necessary, it is to be done by custom code.

comment:11 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:12 Changed 4 years ago by Piotrek Koszuliński

The idea is simple - we should make it configurable whether javascript links are allowed. The option should be disabled by default (js links are not allowed by default).

comment:13 Changed 4 years ago by Artur Delura

Status: assignedreview

comment:14 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_failed

The code looks good. Now:

  1. It's "JavaScript", not "javascript" ;)
  2. Explain on an example what is "JavaScript code in a href attribute".
  3. You can try to write a tests for this. The problem with validator is that it will call alert() right away. AFAIK, that will cause automatic test fail in YUI Test. The tests should:
    1. Create editor bot with the default configuration, open link dialog and check if alert is executed (if validator fails) if you entered JS link. You can use assert.throwsError to verify that alert was executed what, thanks to YUI Test, should throw an error.
    2. Create second editor bot with the option set to true and verify the behaviour.
Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

comment:15 Changed 4 years ago by Artur Delura

Status: review_failedreview
Last edited 4 years ago by Artur Delura (previous) (diff)

comment:16 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_failed
  • I outdented script in the tests - we start tests with 0 indentation to avoid indenting too much.
  • I pushed a commit with docs clarification.
  • Tests pass on branch:t/9661, but when ran on master we can see that something is wrong with them - one test fails as it should but not in the right place. The reason of that is that you used wrong editor for wrong test. Check the editor names and their configs - the editor named "allowedJS" uses the default option (which is "not allowed") and the editor called "notAllowedJS" uses option set to true.
  • We always start variables from lowercase (JSContent, TextContent are incorrect).
  • You can move this part:
    				dialog.setValueOf( 'info', 'url', TextContent );
    				dialog.getButton( 'ok' ).click();
    
    				assert.areEqual( null, CKEDITOR.dialog.getCurrent() );
    

To a tearDown callback, because if test fails this code must be executed to clean up after it. And since assertions throw errors if they fail this code will not be executed at all, leaving a dialog open and breaking following tests.

  • In first test you assume that the content of the editor is empty. What if someone adds a test before it? It will break. Therefore you need to set content to empty at the beginning of the test.
  • Use some proper link's href rather than a sentence.
  • notAllowedJSCfg - no need to create this variable - set the config straight.
  • Write messages for assertions that could not be clear. For example when you check data, you are really checking whether link wasn't created.

comment:17 in reply to:  16 Changed 4 years ago by Artur Delura

Status: review_failedreview

Replying to Reinmar:

  • Tests pass on branch:t/9661, but when ran on master we can see that something is wrong with them - one test fails as it should but not in the right place. The reason of that is that you used wrong editor for wrong test. Check the editor names and their configs - the editor named "allowedJS" uses the default option (which is "not allowed") and the editor called "notAllowedJS" uses option set to true.

Second test still fail after corrections but I think this is because ckeditor/master has no applied changes.

  • You can move this part:
    				dialog.setValueOf( 'info', 'url', TextContent );
    				dialog.getButton( 'ok' ).click();
    
    				assert.areEqual( null, CKEDITOR.dialog.getCurrent() );
    

To a tearDown callback, because if test fails this code must be executed to clean up after it. And since assertions throw errors if they fail this code will not be executed at all, leaving a dialog open and breaking following tests.

Just close dialog in tearDown when present.

comment:18 Changed 4 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.1
Status: reviewreview_passed

Looks great now.

I rebased both branches on master and pushed additional commit to tests. It fixes some minor issues - check it and let me know if something is unclear.

I'm setting 4.4.1 milestone, so you can merge branches to master.

comment:19 Changed 4 years ago by Piotrek Koszuliński

I also pushed one small commit to dev. Doc was missing the @since tag.

comment:20 Changed 4 years ago by Artur Delura

Resolution: fixed
Status: review_passedclosed

Fixed on master with ​git:d971319f04 on dev and a92b5ccaa6 on tests.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy