Code

Opened 3 years ago

Closed 3 years ago

#16157 closed New feature (wontfix)

Make sure that fields that are presented as single-line are validated as such

Reported by: tkolar Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords: CharField multiline validator
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

This applies to fields like CharField that are presented as <input type="text"> by default.

As a developer, it is easy to overlook the fact that it is nonetheless possible to submit multiline data to such a field, for instance by creating a custom form, or by manipulating the original form. If the assumption that entries for this field will always be single-line is erroneously made, this is a hard-to-find bug at best, and a vulnerability at worst.

My proposal is to add a validator (for instance single_line) that checks that an input value doesn't contain a newline, and to add it to all the fields that are presented as <input type="text">, and (optionally) to add a field option (for instance, allow_multiline) to override this behavior.

I'm proposing that this become part of django for the following reasons:

  • If the user uses the default form field produced by such a field, they cannot enter a multiline value anyway, so my proposal fixes the problem that validation on the server is "weaker" than on the client.
  • Although it's a corner case, this could, in fact, create actual vulnerabilities (Use case: a simple protocol that has DSV with the field as the last entry per line).
  • People who want multiline will use TextField anyway. If someone out there has customized CharField to act like TextField, they need not complain if they have to fix that. For the other field types, "no multiline" is implicit on their respective validation (haven't checked, but if that isn't the case, that's arguable a bug in itself). Therefore, compatibility is not a problem.

I volunteer to write a patch that implements this if this ticket is accepted.

Attachments (0)

Change History (3)

comment:1 Changed 3 years ago by melinath

  • Component changed from Database layer (models, ORM) to Forms
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to New feature

I've marked this as Accepted, but I do have slight misgivings. It seems like your intent is to add the validator to every CharField - but a CharField won't necessarily be rendered as an <input type="text">. I'm also not sure whether you're planning on patching the model field or the form field. Since this is related to what the html input box looks like, it seems more like a form field patch, especially since I don't know that databases would actually have a problem storing newlines as long as the string was short enough.

comment:2 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset

tl;dr This proposal adds an implicit, non-obvious behavior; and explicit is better than implicit.


I'm skeptical, to say the least.

1) Currently, Django does not apply any restrictions to the data it receives in form fields, and we should keep it consistent. If \n are undesirable in some POST data, it is the developer's responsibility to validate it at the level of the form.

2) Linking a validation to the fact that the widget will be rendered as <input type="text"> is magic and prone to unexpected side effects. What if Javascript turns the <input> into a full blow WYSIWYG editor? This makes the proposed change backwards incompatible.

3) If \n in strings create a vulnerability, it is the developer's responsibility to validate it at the level of the model and . And \n isn't more dangerous in itself than \r, \t, \v and other hard-to-type characters, depending on the context.

4) (This is debatable.) I don't like the idea of comforting developers in the belief that their app won't receive anything else than data entered by users in form fields. IMO, that helps them to shoot themselves into the foot, from a security point of view. Web apps receive all kinds of crap and should validate external data, period.

comment:3 Changed 3 years ago by lukeplant

  • Resolution set to wontfix
  • Status changed from new to closed

Closing as wontfix for all the reasons given by aaugustin. It is also similar to #4960 which was closed as wontfix.

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.