Opened 4 years ago

Closed 4 years ago

#12215 closed Bug (fixed)

Basepath resolution doesn't recognize semicolon as a query separator

Reported by: makhiel Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.4.4
Component: General Version:
Keywords: Cc:

Description

ckeditor.js, line 5:

/(^|.*[\\\/])ckeditor(?:_basic)?(?:_source)?.js(?:\?.*)?$/i

This regex unfortunately doesn't recognize URLs with semicolons like this one:

/path/to/webapp/resources/ckeditor.js;jsessionid=xGgih0owBq2-mcqzqyyh-WmE

which can result in the correct basePath not being found and the editor not being displayed.

I propose changing the regex to:

/(^|.*[\\\/])ckeditor(?:_basic)?(?:_source)?.js(?:\?.*|;.*)?$/i

Change History (6)

comment:1 Changed 4 years ago by Wiktor Walc

Status: newconfirmed

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

Milestone: CKEditor 4.4.4
Owner: set to Piotrek Koszuliński
Status: confirmedreview

Pushed branch:t/12215. I made some additional cleanup there because that code was outdated.

comment:3 Changed 4 years ago by Marek Lewandowski

Status: reviewreview_failed

Branch t/12215 rebased onto master.

Cool thing that the code is getting more DRY with that code.

ckeditor-dev/tests/core/ckeditor/basepathproperty.js

test CKEDITOR.basePath when used src="ckeditor.js;foo&bar"

  • I think that we should leave a feature request in Bender tracker, and link this request in the comment, marking it with a @todo:.
  • assert.areSame( CKEDITOR.basePath, CKEDITOR.basePath ); Source - Does it test anything?

test CKEDITOR.basePath when used src="ckeditor.js?foo&bar"

  • These vars looks like leftovers: defaultPath, iCKEDITOR not used in the code.
  • I feel like you wanted to use iCKEDITOR in this assertion

test CKEDITOR.basePath ckeditor.js src pattern

  • Question: How about renaming a() to something meaningful, like urlMatches() ?

ckeditor-dev/ckeditor.js

  • What's the point of splitting these lines? It's a minified code anyway, it's not readable in any case.
  • We should remember about keeping comments in sync. There is one in edited function refering to basePath which you've removed in git:53d3032380
  • Question: Any idea why minified code was kept at the begining of the file? Maybe we can remove these comments too if this code is not needed?

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

Status: review_failedreview
  1. In both tests there should be iCKEDITOR on the right side - I fixed this.
  2. Next Bender version will be able to run the commented test.
  3. I removed the outdated comment - nice catch.
  4. The lines weren't split by me - Google Closure Compiler outputs that code in this format - I didn't want to change this, because it's time consuming.
  5. I don't know why there's that minified code and it's not part of the ticket to change this. These are leftovers from the v3 and they must be changed carefully. I even looked at this, but resigned because of unnecessary risk.
  6. I left the a() function - we use t() and a() functions in many places in tests to keep some obvious parts short.

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

Version: 4.4.3

comment:6 Changed 4 years ago by Marek Lewandowski

Resolution: fixed
Status: reviewclosed

Fixed with git:ed68866e87 (merged to master) at dev.

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