Code

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#11362 closed (invalid)

CSRF middleware XHTML conformance

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

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 loren 5 years ago.
make_clean_csrf_token_html.diff (732 bytes) - added by andriijas 4 years ago.

Download all attachments as: .zip

Change History (8)

Changed 5 years ago by loren

comment:1 follow-up: Changed 5 years ago by lukeplant

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

Single quotes for attributes are valid XHTML.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 4 years ago by andriijas

  • Resolution invalid deleted
  • Status changed from closed to reopened

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.

Changed 4 years ago by andriijas

comment:3 Changed 4 years ago by andriijas

  • Cc andreas@… added
  • milestone set to 1.2

comment:4 in reply to: ↑ 2 ; follow-up: Changed 4 years ago by lukeplant

  • Resolution set to invalid
  • Status changed from reopened to closed

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.

comment:5 in reply to: ↑ 4 Changed 4 years ago by andriijas

  • 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 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.