Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#9490 closed defect (fixed)

[patch][ccla] dojox.layout.ContentPane: Problems with certain external content.

Reported by: Jared Jurkiewicz Owned by: Karl Tiedt
Priority: high Milestone: 1.4
Component: DojoX Layout Version: 1.3.0
Keywords: Cc: Sam Foster
Blocked By: Blocking:

Description

There are two problems discovered at my place of employment:

1.) If the src="" of an embedded script tag in a file loaded via the href option of content pane has entity characters in it (such as &), the download of the script will often fail. this is because that in normal cases, the browser itself will resolve those back to & when accessed, but since the content is parsed as text, this does not happen. The solution is to update the dojox.html._base code for script downloading to convert back entity characters in the URL.

2.) Second problem is in the way scripts in remote content are handled. Namely, the scarfScripts function in dojox.html._base will grab script tags that are commented out. This causes errors as comment out script tags should not be executed. This too can be easily fixed in the dojox.html._base code. Testcases for each (updated ContentPane?.html tests), along with a patch are forthcoming.

Attachments (3)

dojox.layout.ContentPane.patch (6.4 KB) - added by Jared Jurkiewicz 10 years ago.
Patch + UT for these issues.
dojox.layout.ContentPane.2.patch (6.9 KB) - added by Jared Jurkiewicz 10 years ago.
Moved UT to dojox.html, applied suggested edits.
dojox.html._base.patch (610 bytes) - added by Karl Tiedt 10 years ago.
patched for DTL templates in text/html script tags

Download all attachments as: .zip

Change History (20)

Changed 10 years ago by Jared Jurkiewicz

Patch + UT for these issues.

comment:1 Changed 10 years ago by Jared Jurkiewicz

The patch, for some reason, doesn't view well from track. Download the original form to see it. It's small, the size increases are mainly from the UT.

comment:2 Changed 10 years ago by Jared Jurkiewicz

Summary: dojox.layout.ContentPane: Problems with certain external content.[PATCH][CCLA] dojox.layout.ContentPane: Problems with certain external content.

comment:3 Changed 10 years ago by Sam Foster

Couple comments. First on the implementation itself:

src = src.replace(/>/g, ">")
	.replace(/>/g, ">")
	.replace(/&lt;/g, "<")
	.replace(/&#60;/g, "<")
	.replace(/&amp;/g, "&")
	.replace(/&#38;/g, "&");

I'd like to propose instead:

src = src.replace(/&([a-z0-9#]+);/g, function(m, name) {
	switch(name) {
		case "amp"	: return "&";
		case "gt"	: return ">";
		case "lt"	: return "<";
		case "#62"	: return ">";
		case "#60"	: return "<";
		case "#38"	: return "<";
		default		: return "&"+name+";";
	}
});

Which is a little faster by my reckoning.

I'd rather see the new UT added to dojox/html/tests/test_set.html (scriptTests group) and dojox/html/tests/remote/getResponse.php .. this is functionality that lives in dojox.html._ContentSetter, so its tests should be there first. The test for this is a little thin - it just checks that the ampersand entity gets replaced. But some test is better than no test.

On the commented scripts - good catch.

comment:4 Changed 10 years ago by Adam Peller

it might be nice to break out the unescape method as a separate method... it's used often enough, though this may not be the ideal place to share it

also, the #nnn cases can be resolved something like

 default:
  return name.charAt(0)=="#" ? String.fromCharCode(name.substring(1)) : "&"+name+";";

Changed 10 years ago by Jared Jurkiewicz

Moved UT to dojox.html, applied suggested edits.

comment:5 Changed 10 years ago by Jared Jurkiewicz

Tested on:

IE 7

Safari 3.2.3

FireFox? 2

Google Chrome 1.0

comment:6 Changed 10 years ago by Jared Jurkiewicz

Milestone: tbd1.4
Resolution: fixed
Status: newclosed

Fixed in changeset [18623]

comment:7 Changed 10 years ago by Jared Jurkiewicz

(In [18624]) Forgot the default. Ack. \!strict refs #9490

comment:8 Changed 10 years ago by Karl Tiedt

Cc: Sam Foster added
Resolution: fixed
Status: closedreopened

This did not cover all cases for this. Basically snarfScripts also tries to run script nodes with type="text/html" which is used with DTL to inject more complex templates w/o having to escape strings and what not.

Simple fix is to update the regex in dojox.html._base with

/<script\s*(?![>]*type=['"]?dojo)(?![>]*type=['"]?text\/html)(?:[>]*?(?:src=(['"]?)([>]*?)\1[>]*)?)*>([\s\S]*?)<\/script>/gi

Basically adding (?![>]*type=['"]?text\/html) which allows HTML templates to be passed through script tags for use in dojox.dtl...... I made this change to a local copy for work and it appears to be working 100% no hangs up so far (we use all dojox cpanes)

The only reason I am reopening this is I noticed this fix never made it in so I'm guessing the initial discussion to get this fixed was overlooked. Figured best way to make sure it gets fixed (possibly in time for 1.4 final) is to reopen this bug with more details.

Note: I would commit the change but I'm not a regex expert and it was suggested that there may be a better way to handle this,

comment:9 Changed 10 years ago by Adam Peller

Owner: changed from Adam Peller to Jared Jurkiewicz
Status: reopenednew

comment:10 Changed 10 years ago by Jared Jurkiewicz

Owner: changed from Jared Jurkiewicz to Karl Tiedt

Karl,

Can you attach a patch you made of the change? I'm not sure which RegExpo? you refer to in _base, there are several.

Changed 10 years ago by Karl Tiedt

Attachment: dojox.html._base.patch added

patched for DTL templates in text/html script tags

comment:11 Changed 10 years ago by Karl Tiedt

Added.

comment:12 Changed 10 years ago by dante

Milestone: 1.41.5
Summary: [PATCH][CCLA] dojox.layout.ContentPane: Problems with certain external content.[patch][ccla] dojox.layout.ContentPane: Problems with certain external content.

comment:13 Changed 10 years ago by Jared Jurkiewicz

That patch looks okay, best I can tell. It just skips the text/html tagged scripts. If Pete will let it in, commit it!

comment:14 Changed 10 years ago by Karl Tiedt

Resolution: fixed
Status: newclosed

[20832] fixes this -- I typo'd the ticket #

comment:15 Changed 10 years ago by Karl Tiedt

Milestone: 1.51.4

comment:16 Changed 10 years ago by Karl Tiedt

(In [20833]) refs #9490 better regex provided by doughays !strict

comment:17 Changed 9 years ago by bill

Component: DojoxDojoX Layout
Note: See TracTickets for help on using tickets.