Opened 15 years ago

Closed 14 years ago

Last modified 12 years ago

#11362 closed (invalid)

CSRF middleware XHTML conformance

Reported by: Patryk Lorenowicz Owned by: nobody
Component: Contrib apps Version: dev
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

HTML code inserted by CSRF middleware should conform XHTML standard. Attributes should be double quoted, which is OK for HTML and XHTML.

Attachments (2)

csrf_double_quotes.diff (1.3 KB ) - added by Patryk Lorenowicz 15 years ago.
make_clean_csrf_token_html.diff (732 bytes ) - added by andriijas 14 years ago.

Download all attachments as: .zip

Change History (8)

by Patryk Lorenowicz, 15 years ago

Attachment: csrf_double_quotes.diff added

comment:1 by Luke Plant, 15 years ago

Resolution: invalid
Status: newclosed

Single quotes for attributes are valid XHTML.

in reply to:  1 ; comment:2 by andriijas, 14 years ago

Resolution: invalid
Status: closedreopened

Replying to lukeplant:

Single quotes for attributes are valid XHTML.

Lame excuse for closing. Double quoting should be used since
1) Consistency, Its used everywhere else in django
2) Single quoted html comes from the world of php idiocy ( print "<foo id='$bar'>"; )
3) Why on earth wrap a display none div around an input hidden field? If you are afraid of margins and paddings added by user style sheet, just put display none on the input.
4) Anyone closing this issue again without the patch being apply obviously don't care about consistency and clean markup, which does matter to some people. So lets not make something big of this. It's a quick job to review and apply the patch, though I understand it will take some minutes of someones precious spare time.

It's all about the semantics.

by andriijas, 14 years ago

comment:3 by andriijas, 14 years ago

Cc: andreas@… added
milestone: 1.2

in reply to:  2 ; comment:4 by Luke Plant, 14 years ago

Resolution: invalid
Status: reopenedclosed

Replying to andriijas:

Lame excuse for closing. Double quoting should be used since

1) Consistency, Its used everywhere else in django

This is not true.

2) Single quoted html comes from the world of php idiocy ( print "<foo id='$bar'>"; )

HTML did not inherit its use of single quotes from PHP practices. Equivalent things are done in the Django code base i.e. single quotes used to avoid problems with nesting quotations. See http://code.djangoproject.com/browser/django/tags/releases/1.1/django/views/debug.py#L489 for example.

3) Why on earth wrap a display none div around an input hidden field? If you are afraid of margins and paddings added by user style sheet, just put display none on the input.

Without the div, you have invalid HTML. The div has "display:none" to defensively protect against older browsers and their rendering quirks. (I'm not sure if there is a specific bug with any, but I'm not going to fire up a Windows VM and try X versions of Internet Explorer just to check, and it can't harm, and I've run across related bugs in the past).

4) Anyone closing this issue again without the patch being apply obviously don't care about consistency and clean markup, which does matter to some people. So lets not make something big of this. It's a quick job to review and apply the patch, though I understand it will take some minutes of someones precious spare time.

If I changed the quoting style, I'd could well have another bug filed from someone else who was relying on the previous style for some reason (e.g. if they had a regression test that checked the exact output of a page), and they would actually have a better case — why did I change something that wasn't broken? What am I going to put in the commit message - "Changed some valid HTML to some other valid HTML because andriijas told me so"?

And if I applied your patch as is, I'd have HTML errors immediately. Perhaps you haven't thought this through as well as you thought?

Yes, reviewing this patch did waste some minutes of my precious spare time. I don't begrudge them, but I do object to the attitude that says that you have a right to them, or the idea that if the review does not turn out how you want then obviously I have no valid reasons for doing turning you down.

It's all about the semantics.

The semantics are not affected, I have no idea what you mean here.

The policy in Django is that you don't re-open a bug that is closed by a core committer without discussion on django-devs. Please do not re-open.

in reply to:  4 comment:5 by andriijas, 14 years ago

Cc: andreas@… removed

The policy in Django is that you don't re-open a bug that is closed by a core committer without discussion on django-devs. Please do not re-open.

Didnt know about that (the ticket didnt say it was closed by a "core comitter". Sorry.

And about the rest... I don't have the energy to argue about it. Though it's a shame there's no "html guidlines" or something to maintain consistency.

comment:6 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top