[Django] #20784: RegexValidator should accept a new parameter to perform reversed validation
Django
------------------------------+----------------------------
Reporter: devfeng | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Keywords: RegexValidator
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
------------------------------+----------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20784>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: devfeng | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: master
Keywords: RegexValidator | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by devfeng):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
https://github.com/django/django/pull/1387
--
Ticket URL: <https://code.djangoproject.com/ticket/20784#comment:1>
Django
Reporter: devfeng | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: master
Keywords: RegexValidator | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Changes (by claudep):
* needs_docs: 0 => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/20784#comment:2>
Django
Reporter: devfeng | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: master
Keywords: RegexValidator | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Comment (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 defending against XSS!
--
Ticket URL: <https://code.djangoproject.com/ticket/20784#comment:3>
Django
Reporter: devfeng | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: master
Keywords: RegexValidator | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/20784#comment:4>
Django
Reporter: devfeng | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: master
Keywords: RegexValidator | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Comment (by devfeng):
Replying to [comment:1 devfeng]:
> https://github.com/django/django/pull/1387
Updated, please review. :)
--
Ticket URL: <https://code.djangoproject.com/ticket/20784#comment:5>
Django
Reporter: devfeng | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: master
Keywords: RegexValidator | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: 0 => 1
* needs_docs: 1 => 0
Comment:
Patch needs updating to merge cleanly.
--
Ticket URL: <https://code.djangoproject.com/ticket/20784#comment:6>
Django
Reporter: devfeng | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: master
Keywords: RegexValidator | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by devfeng):
Replying to [comment:6 timo]:
> Patch needs updating to merge cleanly.
https://github.com/django/django/pull/2243
--
Ticket URL: <https://code.djangoproject.com/ticket/20784#comment:7>
Django
Reporter: devfeng | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: master
Keywords: RegexValidator | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by devfeng):
Replying to [comment:7 devfeng]:
> Replying to [comment:6 timo]:
> > Patch needs updating to merge cleanly.
>
> https://github.com/django/django/pull/2243
PR updated per Tim's comments.
--
Ticket URL: <https://code.djangoproject.com/ticket/20784#comment:8>
Django
Reporter: devfeng | Owner: nobody
Type: New feature | Status: closed
Component: Core (Other) | Version: master
Keywords: RegexValidator | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"b102c27ff4d21ea6262e600227530f75337a5df2"]:
{{{
#!CommitTicketReference repository=""
revision="b102c27ff4d21ea6262e600227530f75337a5df2"
Fixed #20784 -- Added inverse_match parameter to RegexValidator.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20784#comment:9>