Opened at 2010-06-16T03:25:44Z
Last modified at 2020-01-17T14:30:29Z
#1085 closed defect
we shouldn't use "assert" to validate incoming data in introducer client — at Initial Version
Reported by: | zooko | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | soon |
Component: | code-network | Version: | 1.7β |
Keywords: | introducer | Cc: | writefaruq |
Launchpad Bug: |
Description
Says:
def _process_announcement(self, ann): self._debug_counts["inbound_announcement"] += 1 (furl, service_name, ri_name, nickname_utf8, ver, oldest) = ann if service_name not in self._subscribed_service_names: self.log("announcement for a service we don't care about [%s]" % (service_name,), level=log.UNUSUAL, umid="dIpGNA") self._debug_counts["wrong_service"] += 1 return self.log("announcement for [%s]: %s" % (service_name, ann), umid="BoKEag") assert type(furl) is str assert type(service_name) is str assert type(ri_name) is str assert type(nickname_utf8) is str nickname = nickname_utf8.decode("utf-8") assert type(nickname) is unicode assert type(ver) is str assert type(oldest) is str
This means that validation of this incoming data is turned off by the PYTHONOPTIMIZE setting, and it means that introducers have the power to cause AssertionFailure to be raised from this function. Now, in practice causing AssertionFailure to be raised from this function won't hurt anything in the current version, but this is still not the right way to do it. We would like for failures of any of these validations to result in an exception that will get logged as explicitly being "we received an ill-formed announcement" rather than "AssertionError", which is supposed to mean "there was an internal error in our source code".
(Note that the introduction server may well just be passing this announcement through as it was given to the introduction server by someone else, and may not be responsible for the ill-formedness itself...)
Relatedly, this code shouldn't catch all possible kinds of exceptions:
try: self._process_announcement(ann) except: log.err(format="unable to process announcement %(ann)s", ann=ann)
But should instead catch the specific kind of exception that means "we received an ill-formed announcement". Any other kind of exception, including AssertionFailure, should not be caught here.