Opened 13 months ago

Last modified 23 hours ago

#22804 assigned Cleanup/optimization

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

Reported by: wolever Owned by: jaap3
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: no
Easy pickings: yes 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 11 months ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 13 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 13 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 13 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 13 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 13 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 11 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).

comment:7 Changed 10 days ago by timgraham

  • Easy pickings set

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

comment:8 Changed 5 days ago 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.

comment:9 Changed 5 days ago by jaap3

  • Owner changed from nobody to jaap3
  • Status changed from new to assigned

comment:10 Changed 23 hours ago by timgraham

  • Patch needs improvement unset
Note: See TracTickets for help on using tickets.
Back to Top