Django

Code

Ticket #4131 (closed: fixed)

Opened 1 year ago

Last modified 7 months ago

addslashes isn't sufficient to protect literal strings in embedded JavaScript code

Reported by: Ned Batchelder <ned@nedbatchelder.com> Assigned to: jacob
Milestone: Component: Template system
Version: SVN Keywords:
Cc: metajack@gmail.com, me@andy.durdin.net, sam@robots.org.uk Triage Stage: Ready for checkin
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

When creating literal strings in embedded JavaScript? code, the addslashes filter is used to escape characters significant to JavaScript?:

<script> var x = "{{x|addslashes}}"; blah(x); </script>

But if the variable x includes the string "</script>", then this script block is ended too early, and the page is broken.

Attached is a patch that also escapes the </ sequence to ensure that this can't happen.

Attachments

patch_addslashes.txt (1.1 kB) - added by ned@nedbatchelder on 04/23/07 10:26:33.
The patch!
addslash_escape.diff (0.8 kB) - added by Jeremy Dunck <jdunck@gmail.com> on 04/30/07 14:17:04.
patch_4131.diff (3.0 kB) - added by durdinator on 10/17/07 17:06:12.
escapejs with tests and docs (2nd attempt)

Change History

04/23/07 10:26:33 changed by ned@nedbatchelder

  • attachment patch_addslashes.txt added.

The patch!

04/24/07 22:08:57 changed by Collin Grady <cgrady@the-magi.us>

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

But should this really be in addslashes? It's a pretty specific edge-case, and may not be desirable anywhere else - e.g. non-Javascript strings.

Seems like a custom filter made to escape </script> would be better, IMO :)

04/25/07 00:54:13 changed by Collin Grady <cgrady@the-magi.us>

  • has_patch set to 1.
  • summary changed from [patch] addslashes isn't sufficient to protect literal strings in embedded JavaScript code to addslashes isn't sufficient to protect literal strings in embedded JavaScript code.

04/25/07 06:07:25 changed by Simon G. <dev@simon.net.nz>

  • needs_better_patch set to 1.
  • stage changed from Unreviewed to Design decision needed.

It does sound like a corner case, but isn't escaping things for javascript the primary usage for addslashes?

04/25/07 07:34:32 changed by anonymous

It is a corner case, but there's no harm in escaping </ even in non-javascript settings. Given the current difficulty of making Django apps XSS-safe, I'd think we'd want to take an advantage we could.

The ticket has been marked needs_better_patch. Is there some action I should be taking?

04/25/07 10:15:22 changed by ned@nedbatchelder

(edited, with attribution) It is a corner case, but there's no harm in escaping </ even in non-javascript settings. Given the current difficulty of making Django apps XSS-safe, I'd think we'd want to take any advantage we could.

The ticket has been marked needs_better_patch. Is there some action I should be taking?

04/25/07 17:31:56 changed by SmileyChris

There potentially could be harm in escaping </ in a non-javascript setting where your escaped \/ doesn't resolve to /.

However, I agree that XSS-safety should be a priority. Perhaps a specific filter for javascript "escaping"?

04/30/07 14:16:40 changed by Jeremy Dunck <jdunck@gmail.com>

I'm attaching another patch which escapes all things not allowed in a JS string literal per ECMA 262 7.8.4 and also incorporates Ned's change for </script>.

Please do consider changing addslashes to include this or consider adding another filter to handle all embedded strings in JS.

04/30/07 14:17:04 changed by Jeremy Dunck <jdunck@gmail.com>

  • attachment addslash_escape.diff added.

06/03/07 14:20:13 changed by max.rabkin@gmail.com

If this change is not made to addslashes, a warning should be added to its documentation to the effect that it is insufficient to protect strings in javascript.

08/25/07 00:58:05 changed by Jack Moffitt <metajack@gmail.com>

  • cc set to metajack@gmail.com.

I just ran into this bug today (specifically not escaping \n) and this patch works great. Would love to see this in the main tree.

10/16/07 19:18:42 changed by jdunck

  • needs_better_patch deleted.

Unflagging "Patch needs improvement" since I attached a new patch quite a while ago, and the ticket has had no action on it since then.

10/16/07 20:51:50 changed by SmileyChris

  • needs_tests set to 1.

10/17/07 13:36:33 changed by durdinator

  • needs_tests deleted.

addslashes as it stands is useful for things other than javascript, which may not accept \/, \n; csv for example (see http://www.djangoproject.com/documentation/outputting_csv/ for a prominent example).

I'm attaching a patch which adds this code as a new filter "escapejs", along with docs and tests.

Also, I question the removal in Jeremy's patch of carriage returns, so they're being converted to \r.

10/17/07 13:37:46 changed by durdinator

  • cc changed from metajack@gmail.com to metajack@gmail.com, me@andy.durdin.net.

10/17/07 15:12:31 changed by SmileyChris

Looks good, Andy.

Small change required to documentation: *form feed* ``\f`` to ``\f`` should read *form feed* to ``\f``

Perhaps you could bring this up in django-dev to get a design decision.

10/17/07 15:37:01 changed by jdunck

Sweet, Andy.

FWIW, ECMA 262 7.8.4, to paraphrase, says you can't have a LineTerminator? inside a string: 

SourceCharacter but not double-quote " or backslash \ or LineTerminator \ EscapeSequence

I'm not sure what to make of that last "\ EscapeSequence?".

LineTerminator? is defined in 7.3 as LF, CR, LS, PS: 

\u000A Line Feed <LF> 
\u000D Carriage Return <CR> 
\u2028 Line separator <LS> 
\u2029 Paragraph separator <PS>

In any case, I agree it's a bit ambiguous, and probably isn't a common issue. I'd much rather see escapejs go in as-is than debate whether it's completely correct according to a mis-worded spec. At the end of the day, this is about not breaking web pages and avoiding security problems, which I'm sure the patch does better than current trunk. :)

Will you bring it up on django-dev, or should I?

10/17/07 15:59:59 changed by Collin Grady <cgrady@the-magi.us>

Isn't that talking about the literal characters though, not the escapes?

So the literal character represented by \u000D or \n wouldn't be allowed, yet '\n' is?

10/17/07 16:08:40 changed by jdunck

Oh. Hah. Yeah, Andy's right, that's just a bug in my patch. Andy's is correct. I just read his comment and mistook his meaning.

10/17/07 17:06:12 changed by durdinator

  • attachment patch_4131.diff added.

escapejs with tests and docs (2nd attempt)

10/17/07 17:08:42 changed by durdinator

@SmileyChris?: Patch updated, docs fixed.

@jdunck: Can you bring it up on dj-d? I was just trying my hand at a bit of triaging on a handful of tickets, and saw that this one could do with a quick (and easy) tidy.

10/17/07 17:24:34 changed by jdunck

Thread here: http://groups.google.com/group/django-developers/browse_thread/thread/f0fb763a94c32e22

11/05/07 11:26:00 changed by anonymous

  • cc changed from metajack@gmail.com, me@andy.durdin.net to metajack@gmail.com, me@andy.durdin.net, sam@robots.org.uk.

12/01/07 13:05:23 changed by jacob

  • owner changed from nobody to jacob.
  • status changed from new to assigned.
  • stage changed from Design decision needed to Ready for checkin.

12/04/07 15:08:30 changed by jacob

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [6892]) Fixed #4131: added an "escapejs" filter for use in JavaScript? strings, and updated the documentation on addslashes to point to the new ticket. Featuring contributions from Ned Batchelder, Jeremy Dunck, and Andy Durdin.

Add/Change #4131 (addslashes isn't sufficient to protect literal strings in embedded JavaScript code)




Change Properties
Action