Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested script tags are not properly escaped #1379

Closed
jbrichau opened this issue Sep 27, 2023 · 2 comments · Fixed by #1380 or #1429
Closed

Nested script tags are not properly escaped #1379

jbrichau opened this issue Sep 27, 2023 · 2 comments · Fixed by #1380 or #1429

Comments

@jbrichau
Copy link
Member

Adding the following snippet to a Seaside render method breaks the generated html because the closing script tag in the jQuery append expression is not properly escaped.

html anchor script:
	((html jQuery this closest: 'div') append: [ :r |
		 r div script: (html jQuery this
				  on: 'click'
				  selector: '.class'
				  do: (JSStream on: 'alert(''nested script''')) ])
Screenshot 2023-09-27 at 17 35 08

In Seaside 3.0, the method https://github.com/SeasideSt/Seaside/blob/61f25aa0e8b820cf1e3d554ef8bcceb12e307233/Javascript-Core.package/JSStream.class/class/encodeString.on..st used to contain code that escapes closing of nested tags:

"avoid that browsers mistakenly take the output as a closing tag"
(last = $< and: [ char = $/ ])
	ifTrue: [ aStream nextPutAll: '\/' ]  
	ifFalse: [ aStream nextPut: char ] ]

#726, the commit comment that removed this from Seaside 3.0 code mentions:

do we really have to encode </ as <\/?

  • if it's inside a <script> it should work fine without it
  • if it's inside anything other and <script> or <style> HTML escaping should kick in
  • if it's inside an attribute value HTML escaping should kick in
@jbrichau
Copy link
Member Author

jbrichau commented Oct 8, 2023

Comment by @Rinzwind in the PR for the temporary fix: #1380 (comment)

There’s a note in section ‘4.12.1.3 Restrictions for contents of script elements’ in the HTML Living Standard which recommends escaping <!-- and <script as well. The following snippet is based on the example in the section, the page does not show ‘Test Paragraph’ as would be expected:

@jbrichau jbrichau reopened this Oct 8, 2023
@jbrichau
Copy link
Member Author

jbrichau commented Oct 8, 2023

Temporary fix was in #1380
Opened draft PR #1381 to work on proper fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants