Skip to content

Loading…

Fixed #22804 -- Warned on unsafe value of 'sep=' in Signer #2784

Closed
wants to merge 1 commit into from

2 participants

@wolever

Issues a warning if the the separator passed to django.core.signing.Signer is an invalid value.

See also: https://code.djangoproject.com/ticket/22804

@timgraham
Django member

Hi, it looks like you've sent a pull request without filing a Trac ticket. Please file a ticket to suggest changes.

See also our patch review checklist.

@wolever

Ah, I suppose this doesn't qualify as a "very minor change", then.

Ticket has been created: https://code.djangoproject.com/ticket/22804

@timgraham timgraham commented on the diff
django/core/signing.py
@@ -150,6 +155,10 @@ class Signer(object):
def __init__(self, key=None, sep=':', salt=None):
# Use of native strings in all versions of Python
self.sep = force_str(sep)
+ if self.sep in _SEP_UNSAFE:
+ warnings.warn(
+ "Unsafe Signer separator: %r (cannot be in %r)"
+ % (self.sep, "".join(_SEP_UNSAFE)))
@timgraham Django member

probably want something like: sorted(_SEP_UNSAFE) so the order isn't random. Might be clearer to output something like "0-9A-Za-z-_=" though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
django/core/signing.py
@@ -150,6 +155,10 @@ class Signer(object):
def __init__(self, key=None, sep=':', salt=None):
# Use of native strings in all versions of Python
self.sep = force_str(sep)
+ if self.sep in _SEP_UNSAFE:
@timgraham Django member

won't work if sep is more than a single character.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham
Django member

Please send a new PR if you can update it as described in the ticket, thanks.

@timgraham timgraham closed this
@timgraham timgraham changed the title from Warn on unsafe value of 'sep=' in Signer to Fixed #22804 -- Warned on unsafe value of 'sep=' in Signer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 9, 2014
  1. @wolever
This page is out of date. Refresh to see the latest.
Showing with 18 additions and 0 deletions.
  1. +9 −0 django/core/signing.py
  2. +9 −0 tests/signing/tests.py
View
9 django/core/signing.py
@@ -38,6 +38,7 @@
import base64
import json
import time
+import warnings
import zlib
from django.conf import settings
@@ -46,6 +47,10 @@
from django.utils.encoding import force_bytes, force_str, force_text
from django.utils.module_loading import import_string
+_SEP_UNSAFE = set(
+ "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+ "abcdefghijklmnopqrstuvwxyz-_="
+)
class BadSignature(Exception):
"""
@@ -150,6 +155,10 @@ class Signer(object):
def __init__(self, key=None, sep=':', salt=None):
# Use of native strings in all versions of Python
self.sep = force_str(sep)
+ if self.sep in _SEP_UNSAFE:
@timgraham Django member

won't work if sep is more than a single character.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ warnings.warn(
+ "Unsafe Signer separator: %r (cannot be in %r)"
+ % (self.sep, "".join(_SEP_UNSAFE)))
@timgraham Django member

probably want something like: sorted(_SEP_UNSAFE) so the order isn't random. Might be clearer to output something like "0-9A-Za-z-_=" though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
self.key = key or settings.SECRET_KEY
self.salt = force_str(salt or
'%s.%s' % (self.__class__.__module__, self.__class__.__name__))
View
9 tests/signing/tests.py
@@ -1,6 +1,7 @@
from __future__ import unicode_literals
import time
+import warnings
from django.core import signing
from django.test import TestCase
@@ -111,6 +112,14 @@ def test_works_with_non_ascii_keys(self):
s = signing.Signer(binary_key)
self.assertEqual('foo:6NB0fssLW5RQvZ3Y-MTerq2rX7w', s.sign('foo'))
+ def test_issues_warning_on_invalid_sep(self):
+ with warnings.catch_warnings(record=True) as recorded:
+ warnings.simplefilter('always')
+ signing.Signer(sep="-")
+ self.assertEqual(len(recorded), 1)
+ msg = str(recorded[0].message)
+ self.assertTrue(msg.startswith("Unsafe Signer separator"))
+
class TestTimestampSigner(TestCase):
Something went wrong with that request. Please try again.