Opened 10 months ago

Last modified 8 months ago

#22804 new Cleanup/optimization

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

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

Description

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.

Attachments (1)

0001-Warn-on-unsafe-value-of-sep-in-Signer.patch (2.9 KB) - added by wolever 8 months ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 10 months ago by wolever

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from A warning should be issued when an invalid separator is passed to crypto.Signer to A warning should be issued when an invalid separator is passed to signing.Signer

comment:2 Changed 10 months ago by bmispelon

  • Has patch set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

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.

comment:3 Changed 10 months ago 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.

comment:4 Changed 10 months ago 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)

comment:5 Changed 10 months ago 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.

comment:6 Changed 8 months ago 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).

Note: See TracTickets for help on using tickets.
Back to Top