Opened 15 months ago

Closed 8 weeks ago

#22804 closed Cleanup/optimization (fixed)

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 13 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 15 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 15 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 15 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 15 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 15 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 13 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 2 months 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 2 months 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 2 months ago by jaap3

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

comment:10 Changed 2 months ago by timgraham

  • Patch needs improvement unset

comment:11 Changed 8 weeks ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 0d71349:

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

Thanks Jaap Roes for completing the patch.

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