JavaScript escape call breaks string rendering

[marisn]

PR #1354 introduced a call to escape function on text to display with an intent of fixing some kind of XSS vector. Unfortunately it also broke rendering of text with non-ascii chars, spaces etc.

Symptoms: widgets with already selected value render text as an escape sequence

Workaround: comment this line:

django-autocomplete-light/src/dal_select2/static/autocomplete_light/select2.js

Line 34 in 3410996

if (item.selected) text = escape(text);

Test data: "āšņļ ēčžģ"

[jpic]

Turns out I'm going to agree with you and actually you've opened a pandora box that goes beyond that, it's the number 1 drama of Django in my opinion, the security model they suggest is completely backwards and against OWASP recommendations. Ever eard about that drama? OWASP/CheatSheetSeries#472

Where there is this quote:

Seems related to something I learned early in my web career... about not storing values escaped, because you don't know which medium it needs escaping for -- e.g. HTML needs different escaping than URLs.

Funny, something I learned early in my web career is to validate and sanitize user inputs, which Django doesn't want to do by default (lol), which means we always have to patch default django CharFields such as User first_name/last_name and every single CharField with custom validators which pass our own security tests as we follow OWASP recommendations: a whitelist of acceptable input characters, all Django sites we audit fall into this

But Django is scared that something outside of Django users codebases injects code in the database because, like Django, they "forget" to validate/sanitize user input, or decide to not go that way, in case someone else doesn't ... What a vicious circle.

Anyway, as I'm not trying to waste your time but just wanted your effort to pay off with a bit of excitement, let's talk about that patch again.

To my understanding, it was not really about fixing the XSS but instead because of a bug, @elapouya:

Actually, for field forwarding, there is a mismatch between the div id given at python side and the id at javascript side. At python side the widget id is not read at the right place.
In the PR there is a very quick fix for that.

I wouldn't be surprised if the XSS fix wasn't just an additional argument to push the said PR to actually fix the bug (who hasn't ever done that can throw the first PHP script at my face)

Can you take a look at that bug and perhaps suggest a patch that both fixes the bug he intended to fix and lets you use the cars you want?

Otherwise I will revert the commit because BC is a fundamental priority to me - again I'm not caring about the XSS fix for user who don't follow OWASP recommendations and common sense which is validating and sanitizing (escaping as last resort) input, not output lol

[jpic]

Meanwhile, I deleted 3.12 release to stop the bleeding until we figure out what we do.

Published 3.12.1-rc1 so users can still get the Django 5.1 fixes

[izimobil]

Hi, maybe I misunderstood the purpose of the 3.12.1rc1 release, but this escaping issue is still there in 3.12.1-rc1.
Do you plan to revert the commit and do another release @jpic ?

Thanks.

[marisn]

The problem with PR #1354 is that it mixes two things in to one. Only the JavaScript part needs to be reverted, Python – should stay. They should have been two separate PRs.

I don't get the Django rant as CharField in different models / apps can have different sets of valid input (think different languages, their mix, special chars etc.). I work on projects where single field can appear in HTML, JavaScript, e-mails, PDFs, Excel files, CAD drawings etc. – each of them with different escape rules and thus far Django approach has worked just fine.

I'm not an expert in JavaScript and thus can not judge if this should be handled in JS or Python side.

[izimobil]

Yep, that's the commit I was thinking about: 71d75f3

I won't argue on the Django rant either ;) one thing is sure is that it's not DAL's role to escape (maybe user entered or not) input, not to mention it's a BC break !

Cheers.