Code

Opened 12 months ago

Closed 5 months ago

#20784 closed New feature (fixed)

RegexValidator should accept a new parameter to perform reversed validation

Reported by: devfeng Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords: RegexValidator
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In current implementation, RegexValidator only raises ValidationError when pattern DOES NOT match regex, while it is pretty common to use a "reversed" RegexValidator that raises ValidationError when pattern MATCHES regex. A typical scenario is to catch potential XSS inputs in form field validation (if it matches then the validator should panic).

Technically such reversed match could be performed by tweaking the regex itself, however in real world, not everybody is a regex master and there are a lot of people who may prefer a more straightforward solution as simple as "not some_condition".

In my own projects, I've written a ReversedRegexValidator by subclassing RegexValidator and overriding the call() method to change it's behavior (basically it just copied everything and then removed the "not" statement). It worked well, however there are some problems:

  1. RegexValidator uses some Django internal utils that are not documented during upgrades. For example, in Django 1.4.x, RegexValidator used smart_unicode() from utils.encoding, while in Django 1.5.x, it changed to force_text(). The custom ReversedRegexValidator will need to be upgraded as well for such internal change.
  2. Overriding the whole call() method in order to just remove (or have) a "not" operation seems to be too much. But that's the current problem with RegexValidator.

So my suggestion is to add a new parameter, say "reverse", to RegexValidator. By default it's False and won't change anything to existing codes, but a user can very easily change it's matching behavior by passing reverse=True.

I've had my changes ready for review. Test cases have been updated as well.

Attachments (0)

Change History (9)

comment:1 follow-up: Changed 12 months ago by devfeng

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 12 months ago by claudep

  • Needs documentation set
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 12 months ago by aaugustin

I'm not strongly against this idea in general, however, I'm very concerned about the rationale.

A blacklist implemented with a regex is a textbook example of the worst possible way to defend against XSS!

Last edited 12 months ago by aaugustin (previous) (diff)

comment:4 Changed 12 months ago by bmispelon

Hi,

I think reverse is a confusing name for this feature because it already has a different meaning for lists (consider the reversed builtin or the reverse argument to sorted). Maybe something like invert would work better?

I also wonder if a separate validator wouldn't be a better approach since the two are fundamentally different. What do you think?

Finally, as noted by claudep, your patch will need documentation too: a new entry in the ref/validators page as well as a mention in the release notes for 1.7.

Thanks

comment:5 in reply to: ↑ 1 Changed 12 months ago by devfeng

Replying to devfeng:

https://github.com/django/django/pull/1387

Updated, please review. :)

comment:6 follow-up: Changed 5 months ago by timo

  • Needs documentation unset
  • Patch needs improvement set

Patch needs updating to merge cleanly.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 5 months ago by devfeng

Replying to timo:

Patch needs updating to merge cleanly.

https://github.com/django/django/pull/2243

comment:8 in reply to: ↑ 7 Changed 5 months ago by devfeng

Replying to devfeng:

Replying to timo:

Patch needs updating to merge cleanly.

https://github.com/django/django/pull/2243

PR updated per Tim's comments.

comment:9 Changed 5 months ago by Tim Graham <timograham@…>

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

In b102c27ff4d21ea6262e600227530f75337a5df2:

Fixed #20784 -- Added inverse_match parameter to RegexValidator.

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.