Adjust teamviewer plugin to account for daylight savings time in timestamps#1614
Adjust teamviewer plugin to account for daylight savings time in timestamps#1614
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1614 +/- ##
==========================================
+ Coverage 81.06% 81.09% +0.03%
==========================================
Files 402 402
Lines 35244 35264 +20
==========================================
+ Hits 28571 28599 +28
+ Misses 6673 6665 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| self.target.log.debug("", exc_info=e) | ||
| timestamp = 0 | ||
|
|
||
| if timestamp: |
There was a problem hiding this comment.
Assuming we have the target timezone self.target.datetime.tzinfo,
what if we renormalize every timestamp, instead of detecting deltas:
naive_dt = datetime.strptime(f"{date} {time}", "%Y/%m/%d %H:%M:%S.%f")
timestamp = naive_dt.replace(tzinfo=target_tz)
if prev_timestamp and prev_timestamp > timestamp:
timestamp = timestamp.replace(fold=1)
prev_timestamp = timestamp
Would that not also help with "spring forward" events?
In other words, why not rely on the target_tz calendar rules?
There was a problem hiding this comment.
I didn't know that the fold was used for this. I did notice when working on it that the fold doesn't get passed to flow.record. I made this PR for it fox-it/flow.record#217
|
I decided to ignore the |
Determine whether the timezone found by parsing `Start:` is the same timezone as that of the target. This automatically adjust the timestamp when dst time ends or starts. There is a grey area where it cannot determine whether a timestamp is in dst time or not, so when it is observed that the previous timestamp was less than the current one, it adjust the timezone for those entries (UTC+XXXX)
And add tests for it
59c3895 to
2d708e1
Compare
| except Exception as e: | ||
| self.target.log.warning("Unable to parse timestamp %r in file %s", line, logfile) | ||
| self.target.log.debug("", exc_info=e) | ||
| timestamp = 0 |
There was a problem hiding this comment.
Wouldn't this cause a timestamp of 0 to be yielded? Not sure if that behaviour is expected.
| except Exception as e: | |
| self.target.log.warning("Unable to parse timestamp %r in file %s", line, logfile) | |
| self.target.log.debug("", exc_info=e) | |
| timestamp = 0 | |
| except Exception as e: | |
| self.target.log.warning("Unable to parse timestamp %r in file %s", line, logfile) | |
| self.target.log.debug("", exc_info=e) | |
| continue |
There was a problem hiding this comment.
I was also confused about that, as it would return 1 January 1970, but didn't touch it as it is how it worked before.
This does mean we won't be able to get a record if we couldn't parse the timestamp. Tho, it is still available in the logging.
With this patch, other logic can also be updated
There was a problem hiding this comment.
I think having a log line without a timestamp is preferred over ignoring the log line altogether, as it could hold forensic value.
There was a problem hiding this comment.
The only times I have seen this code path occurring is specifically with the following entries after Start: which occurs when the teamviewer service gets started:
Start: 2026/03/05 13:33:56.539 (UTC+5:30)
Version: 15.74.6 (64-bit)
Version short hash: e34788d05e8
ID: 0
Loglevel: Critical
License: 0
IC: -2056635597
CPU: Intel64 Family 6 Model 141 Stepping 1, GenuineIntel
CPU extensions: k0
OS: Win_10.0.26100_W (64-bit)
IP: 127.0.0.1
MID: 0x525400fad56f_1da8405dc678cec_983722041
MIDv: 0
Proxy-Settings: Type=1 IP= User=
IE: 11.1.26100.0
AppPath: C:\Program Files\TeamViewer\TeamViewer_Service.exe
UserAccount: SYSTEM
Server-Environment: Global
All following entries have a timestamp until another Start: gets is found.
It could be a good compromise to use timestamp=start_time here instead, as those entries belong to start_time
In the worst case, the entries won't have any timestamp then
There was a problem hiding this comment.
Or we could store the last timestamp we encountered and use that one instead.
There was a problem hiding this comment.
as we already store the previous timestamp for overlap detection that should be fine
There was a problem hiding this comment.
@JSCU-CNI I changed it to use the previous timestamp. Note that this path is only reached when the timestamp contains a number that is invalid for a timestamp. For example:
2025/10/26 02:50:90.300
If the time regex itself doesn't match, for example if the timestamp contained a letter instead of a number, it already gets ignored
Co-authored-by: Stefan de Reuver <9864602+Horofic@users.noreply.github.com>
867cb2c to
451c426
Compare
…ion occurred during parsing The only case it gets in this exception is when `Start:` gets parsed and the next lines contain some system information. Technically those records have the same time as the start_date In the worst case scenario the `start_date` would be None if there is an error or the line couldn't get parsed.
53b5f78 to
3c2e678
Compare
Need to be invalid times that exceed normal datetime behaviour, because if they are not numbers they are already ignored due to the regex not matching
(UTC)inside the stringcloses #1488