Fixing Audit issues LOW-6 to LOW-10 #12
Conversation
| ): | ||
|
|
||
| raise_error = False | ||
| if (error and error.get('code') != 200): |
There was a problem hiding this comment.
I noticed in tests that we never will be equal to http.HTTPStatus.OK as it gets translated to the corresponding number
| if message.HasField('timestamp'): | ||
| # Sanity check, is date before year 2500 | ||
| # Technically up to 9999 is allowed | ||
| if message.timestamp.seconds > 16725225600: |
There was a problem hiding this comment.
why check for year 2500 and not now?
There was a problem hiding this comment.
We could do that, but that then implies that we assume that the client knows current time. There could also be some time skew, especially for subscriptions, so only accepting timestamps that are "older" than client time is maybe too strict.
But this can be discussed - shall we assume that client has a reasonably correct time and discard values that are in the future (maybe add some offset, like accept a few minutes time skew). I believe it does not matter most times you run on a virtual box, but could be more of a problem if you run on target devices without a functional real time clock
For now the test is a very rough sanity test that timestamp does not seem to malformed, and likely our code will no longer be in use year 2500
There was a problem hiding this comment.
@SebastianSchildt - any thoughts here? I.e. shall we have a stricter requirement on times received?
Or is that potentially part of a wider overhaul of how we handle time and timestamps? I am thinking - would it make sense to have a dynamic read-only datapoint like Kuksa.Databroker.Time which clients/providers could call to make sure that the client and Databroker has roughly the same understanding of time. Could be useful information for example if token validation fail, or if we would implement a stricter check here which fails as either broker or client does not know the correct time.
There was a problem hiding this comment.
@erikbosch It might makes sense to put some sanity check here, to make sure the ToDatetime can suceed, but if I look at the finding in the audit, hasn't the fault already happened earlier? Because I see a proto-related abcktrace
File ~/.../site-packages/google/protobuf/internal/well_known_types.py:232, in ,→ Timestamp.ToDatetime(self, tzinfo)
215 """Converts Timestamp to a datetime.
...
--> 232 delta = datetime.timedelta(
233 seconds=self.seconds,
234 microseconds=_RoundTowardZero(self.nanos, _NANOS_PER_MICROSECOND),
235 )
OverflowError: days=1628906115; must have magnitude <= 999999999}
and as this function already receives and types_pb2.Datapoint does it imply the error happened earlier?
There was a problem hiding this comment.
Ah ignore that, I do see that we call this directly with timestamp = message.timestamp.ToDatetime
I would just instead of doing an arbitrary limit, call that function in a try/except block, if it fails we can still return none
There was a problem hiding this comment.
Changed to try/except now, so now we supports dates up to 9999-12-31 🥇
In short: