fix: str_to_isoformat for unformatted datetime #1330
Conversation
|
@0xAlcidius thank you for your contribution! As this is your first code contribution, please read the following Contributor License Agreement (CLA). If you agree with the CLA, please reply with the following information:
Contributor License Agreement
Contribution License AgreementThis Contribution License Agreement ("Agreement") governs your Contribution(s) (as defined below) and conveys certain license rights to Fox-IT B.V. ("Fox-IT") for your Contribution(s) to Fox-IT"s open source Dissect project. This Agreement covers any and all Contributions that you ("You" or "Your"), now or in the future, Submit (as defined below) to this project. This Agreement is between Fox-IT B.V. and You and takes effect when you click an “I Accept” button, check box presented with these terms, otherwise accept these terms or, if earlier, when You Submit a Contribution.
|
|
@DissectBot agree [company="Hunt & Hackett"] |
|
Hi @0xAlcidius thanks for your contribution. Can you please add some unit-tests for this case? Maybe the fix can be much easier by parsing the datetime with datetime.strptime something like this: But I'm not sure if the format is always the same else some variants could be added to the parse function. Besides that you can simply return a datetime object, no need to format it as a string. |
|
Thank you @bobkarreman for your comment and suggestion! I’ve noticed that Windows Scheduled Tasks can be a bit inconsistent when it comes to the StartBoundary. I did try using Could you point me to the right place in the codebase to add the unit tests? |
https://github.com/fox-it/dissect.target/blob/main/tests/plugins/os/windows/test_tasks.py |
|
Added a few test cases with timestamps observed in scheduled tasks. |
|
Should we also update the records and change the return type to datetime? |
Please also include a test case for the case you described in the this PR ('2025-7-29T16:34:00') |
Parsing it with datetime.fromisoformat with some fallback logic is a cleaner way in my opinion (also fromisoformat has been improved in 3.11 so will work in almost all of the test cases). Could be something like this: def parse(s):
try:
d = datetime.fromisoformat(s)
except ValueError:
s = s.replace(" ", "T")
if 'Z' in s:
d = datetime.strptime(s, "%Y-%m-%dT%H:%M:%SZ")
return d.replace(tzinfo=timezone.utc)
d = datetime.strptime(s, "%Y-%m-%dT%H:%M:%S")
return d |
@B0TAxy The record already has the type datetime for start and end boundary. Or are you referring to something else? |
|
@bobkarreman Following your suggestion, I’ve refactored the function to make it cleaner while maintaining the same functionality. Additionally, I’ve added the missing test case you mentioned earlier. |
My mistake — you’re correct, the records are already in datetime format. Thanks for clarifying! |
| if "Z" in string_to_convert: | ||
| date = datetime.strptime(string_to_convert, "%Y-%m-%dT%H:%M:%S%z") | ||
| return date.replace(tzinfo=timezone.utc) | ||
| date = datetime.strptime(string_to_convert, "%Y-%m-%dT%H:%M:%S") # noqa: DTZ007 |
There was a problem hiding this comment.
This and the other noqa's should be resolved.
If the timezone in the source is local to the target self.target.datetime.tzinfo can be used if they are always utc they should be made timezone aware by setting it to utc.
There was a problem hiding this comment.
I think the changes you are made to the parsing function are not correct.
If the timestamp contains a Z it's timezone is UTC.
All other instances where there is a timezone it should also keep the timezone.
If there is no timezone it should be made timezone aware with the correct timezone, that depends on the logic of the log output. If its UTC, utc should be taken, if its local to the system then self.target.datetime.tzinfo should be used.
And the tests should also reflect that and check if the timezone is the expected one.
|
|
||
|
|
||
| def test_xml_task_time() -> None: | ||
| assert str_to_isoformat("2023-07-05T14:30:00") == datetime.strptime("2023-07-05 14:30:00", "%Y-%m-%d %H:%M:%S") # noqa: DTZ007 |
There was a problem hiding this comment.
Change to compare the output of the parse function against a datetime instance instead of string comparison, same for the other tests.
|
@bobkarreman Thank for you your review! I provided changes accordingly. |
|
@bobkarreman @Schamper Would any of you have had the time to look at the changes yet? |
I was on holiday and after that quite busy so didn't have time to look at it. I provided some feedback, let me know if its unclear else we can maybe have a chat on Discord or something if that makes it easier. |
| date = datetime.fromisoformat(value) | ||
| except ValueError: | ||
| value = value.replace(" ", "T") | ||
| date = datetime.strptime(value, "%Y-%m-%dT%H:%M:%S").replace(tzinfo=timezone.utc).replace(tzinfo=None) |
There was a problem hiding this comment.
The two replace(tzinfo=) calls cancel each other out here.
|
|
||
|
|
||
| def test_xml_task_time() -> None: | ||
| assert parse_datetime("2023-07-05T14:30:00") == datetime(2023, 7, 5, 14, 30, 0, tzinfo=timezone.utc).replace( |
6 participants
Summary
Introduce
str_to_isoformatto format datetime stringsencountered in Windows Task Scheduler XML. This ensures
non-padded months/days and space-separated datetimes are
converted into strict ISO 8601 strings.
Motivation
Parsing a velo package could result in the following error:
This PR solves that issue.