[Django] #22804: A warning should be issued when an invalid separator is passed to crypto.Signer
Django
crypto.Signer
-------------------------------+--------------------
Reporter: wolever | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Other) | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
Currently there is no warning or error when an invalid value (ex, `-`) is
passed to `django.core.signing.Signer`.
See https://github.com/django/django/pull/2784 for a patch which adds a
warning.
An exception might be even better, but that would break backwards
compatibility.
--
Ticket URL: <https://code.djangoproject.com/ticket/22804>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------+--------------------------------------
Reporter: wolever | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by wolever):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22804#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: wolever | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
Hi,
I'll mark this as accepted on the basis that the current behavior leaves
the user open to serious issues.
However, wouldn't it be better to go through the deprecation process and
raise an error eventually? I can't think of a reason why a user would want
to pass a separator that has a chance to appear in the encoded data.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/22804#comment:2>
Django
--------------------------------------+------------------------------------
Reporter: wolever | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by mlavin):
If you can't distinguish the data from the signature then the signature
can't be verified. A warning is not strong enough. This should be an
exception.
--
Ticket URL: <https://code.djangoproject.com/ticket/22804#comment:3>
Django
--------------------------------------+------------------------------------
Reporter: wolever | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by wolever):
Yep - would definitely agree it should go through the deprecation process
and raise an error eventually. I'd be -0 on raising an error immediately
because invalid separators can appear to work (I was using "-" as a
separator on a low traffic internal thing), and halting application
entirely could be even less helpful. Proposal for now:
1) Change warning to deprecation warning
2) If a bad signature error is raised, check to see if the sep is invalid
and augment the error message if it is (ex, `InavlidSignature("the
signature was invalid; NOTE: the separator '-' is not valid; this may be
the cause of the invalid signature")`)
3) Raise an exception in the future (as per whatever deprecation timeline
is sensible)
--
Ticket URL: <https://code.djangoproject.com/ticket/22804#comment:4>
Django
--------------------------------------+------------------------------------
Reporter: wolever | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by erikr):
I agree with the latest comment by wolever. We should not allow broken
behaviour, so raising an exception is right in the long term, but a little
harsh to do in 1.8 straight away. I suggest an accelerated deprecation, so
DeprecationWarning in 1.8, exception in 1.9. This will then also have to
be added to the 1.8 release notes, and the deprecation timeline.
--
Ticket URL: <https://code.djangoproject.com/ticket/22804#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: wolever | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by wolever):
Okay, attached is a patch which correctly implements and tests invalid
separators. Please forgive the small micro-optimization (`sep != ":' and
...`). I don't know the correct procedure for adding to the release notes
or deprecation timeline, so if someone could give me a hand with that it
would be great (and, specifically: if someone just wants to go ahead and
commit this, I don't care if my name isn't in the commit log).
--
Ticket URL: <https://code.djangoproject.com/ticket/22804#comment:6>
Django
--------------------------------------+------------------------------------
Reporter: wolever | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
--------------------------------------+------------------------------------
Changes (by timgraham):
* easy: 0 => 1
Comment:
Marking as easy pickings since updating the initial
[https://github.com/django/django/pull/2784 pull request] for the comments
shouldn't be too difficult.
--
Ticket URL: <https://code.djangoproject.com/ticket/22804#comment:7>
Django
--------------------------------------+------------------------------------
Reporter: wolever | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by jaap3):
I've rebased the original patch and then applied the patch in this ticket,
with some minor changes by myself. If this patch is acceptable I'll update
the release notes.
--
Ticket URL: <https://code.djangoproject.com/ticket/22804#comment:8>
Django
--------------------------------------+------------------------------------
Reporter: wolever | Owner: jaap3
Type: Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* owner: nobody => jaap3
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/22804#comment:9>
Django
--------------------------------------+------------------------------------
Reporter: wolever | Owner: jaap3
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22804#comment:10>
Django
--------------------------------------+------------------------------------
Reporter: wolever | Owner: jaap3
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"0d71349773f8d2e8160d76b5c91a1896ece772db" 0d71349]:
{{{
#!CommitTicketReference repository=""
revision="0d71349773f8d2e8160d76b5c91a1896ece772db"
Fixed #22804 -- Added warning for unsafe value of 'sep' in Signer
Thanks Jaap Roes for completing the patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22804#comment:11>
Django
--------------------------------------+------------------------------------
Reporter: wolever | Owner: jaap3
Type: Cleanup/optimization | Status: closed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"e6cfa08f2d478cb962528188dff5cf4daf5e5f9b" e6cfa08]:
{{{
#!CommitTicketReference repository=""
revision="e6cfa08f2d478cb962528188dff5cf4daf5e5f9b"
Refs #22804 -- Made an unsafe value of 'sep' in Signer an exception.
Per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22804#comment:12>