A warning should be issued when an invalid separator is passed to signing.Signer

[David Wolever]

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.

[Baptiste Mispelon]

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.

[Mark Lavin]

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.

[David 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)

[Sasha Romijn]

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.

[David 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).

[Tim Graham]

Marking as easy pickings since updating the initial ​pull request for the comments shouldn't be too difficult.

[Jaap Roes]

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.

[Tim Graham <timograham@…>]

In 0d71349:

Fixed #22804 -- Added warning for unsafe value of 'sep' in Signer

Thanks Jaap Roes for completing the patch.

[Tim Graham <timograham@…>]

In e6cfa08:

Refs #22804 -- Made an unsafe value of 'sep' in Signer an exception.

Per deprecation timeline.