[tor-commits] [stem/master] Extend circ_id validation even further

[atagar at torproject.org]

commit 1b9cfa7946ec607631d12b660e11e70f520c6263
Author: Damian Johnson <atagar at torproject.org>
Date:   Wed Jun 20 10:50:50 2018 -0700

    Extend circ_id validation even further
    
    At its root the prior commit contended with the fact we defaulted circ_id to
    zero. Here's the conundrum...
    
      * Zero is not a valid circuit identifier so using it as a default for
        CircuitCell subclasses is incorrect.
    
      * We chose zero because *non-curcuit cells* still need to fill something in,
        and zero is a good filler value.
    
    It took me a minute to realize this so clearly the code needs to be clarified.
    Hopefully this makes things clearer.
    
    As for the test it's definitely preferable to exercise public methods,  but if
    it adds much overhead I don't mind exercising a private method or two. This is
    our own tests after all - in java terms we can treat these as 'protected'
    rather than fully being off limits. As such, making our test a bit more
    targeted while also covering the new checks.
---
 stem/client/cell.py      | 14 +++++++++++---
 test/unit/client/cell.py | 27 ++++++++-------------------
 2 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/stem/client/cell.py b/stem/client/cell.py
index 809566a4..ea0cbcce 100644
--- a/stem/client/cell.py
+++ b/stem/client/cell.py
@@ -175,7 +175,7 @@ class Cell(object):
     return cls._unpack(payload, circ_id, link_protocol), content
 
   @classmethod
-  def _pack(cls, link_protocol, payload, circ_id = 0):
+  def _pack(cls, link_protocol, payload, circ_id = None):
     """
     Provides bytes that can be used on the wire for these cell attributes.
     Format of a properly packed cell depends on if it's fixed or variable
@@ -196,8 +196,16 @@ class Cell(object):
     :raise: **ValueError** if cell type invalid or payload makes cell too large
     """
 
-    if issubclass(cls, CircuitCell) and not circ_id:
-      raise ValueError('%s cells require a non-zero circ_id' % cls.NAME)
+    if issubclass(cls, CircuitCell):
+      if circ_id is None:
+        raise ValueError('%s cells require a circuit identifier' % cls.NAME)
+      elif circ_id < 1:
+        raise ValueError('Circuit identifiers must a positive integer, not %s' % circ_id)
+    else:
+      if circ_id is not None:
+        raise ValueError('%s cells should not specify a circuit identifier' % cls.NAME)
+
+      circ_id = 0  # cell doesn't concern a circuit, default field to zero
 
     link_protocol = LinkProtocol.for_version(link_protocol)
 
diff --git a/test/unit/client/cell.py b/test/unit/client/cell.py
index f7ab0885..905542a6 100644
--- a/test/unit/client/cell.py
+++ b/test/unit/client/cell.py
@@ -14,7 +14,6 @@ from test.unit.client import test_data
 from stem.client.cell import (
   FIXED_PAYLOAD_LEN,
   Cell,
-  CircuitCell,
   PaddingCell,
   RelayCell,
   DestroyCell,
@@ -123,28 +122,18 @@ class TestCell(unittest.TestCase):
     expected_message = 'Cell of type OVERSIZED is too large (%i bytes), must not be more than %i. Check payload size (was %i bytes)' % (FIXED_PAYLOAD_LEN + 4, FIXED_PAYLOAD_LEN + 3, FIXED_PAYLOAD_LEN + 1)
     self.assertRaisesRegexp(ValueError, re.escape(expected_message), instance.pack, 2)
 
-  def test_circuit_cell_requires_circ_id(self):
-    class SomeCircuitCell(CircuitCell):
-      NAME = 'SOME_CIRCUIT_CELL'
-      VALUE = 127  # currently nonsense, but potentially will be allocated in the distant future
-      IS_FIXED_SIZE = True
-
-      def __init__(self):
-        pass  # don't take in a circ_id, unlike super()
-
-      def pack(self, link_protocol):
-        return SomeCircuitCell._pack(link_protocol, b'')
+  def test_circuit_id_validation(self):
+    # only CircuitCell subclasses should provide a circ_id
 
-      def pack_with_circ_id(self, link_protocol, circ_id):
-        return SomeCircuitCell._pack(link_protocol, b'', circ_id)
+    self.assertRaisesRegexp(ValueError, 'PADDING cells should not specify a circuit identifier', PaddingCell._pack, 5, b'', 12)
 
-    instance = SomeCircuitCell()
+    # CircuitCell should validate its circ_id
 
-    expected_message_regex = '^%s$' % re.escape('SOME_CIRCUIT_CELL cells require a non-zero circ_id')
+    self.assertRaisesRegexp(ValueError, 'RELAY cells require a circuit identifier', RelayCell._pack, 5, b'', None)
 
-    self.assertRaisesRegexp(ValueError, expected_message_regex, instance.pack, 2)
-    self.assertRaisesRegexp(ValueError, expected_message_regex, instance.pack_with_circ_id, 2, None)
-    self.assertRaisesRegexp(ValueError, expected_message_regex, instance.pack_with_circ_id, 2, 0)
+    for circ_id in (0, -1, -50):
+      expected_msg = 'Circuit identifiers must a positive integer, not %s' % circ_id
+      self.assertRaisesRegexp(ValueError, expected_msg, RelayCell._pack, 5, b'', circ_id)
 
   def test_unpack_for_new_link(self):
     expected_certs = (