Skip to content

Flatten command field types for the jsonpacker#130

Closed
JSCU-CNI wants to merge 9 commits intofox-it:mainfrom
JSCU-CNI:fix/flatten-command-type
Closed

Flatten command field types for the jsonpacker#130
JSCU-CNI wants to merge 9 commits intofox-it:mainfrom
JSCU-CNI:fix/flatten-command-type

Conversation

@JSCU-CNI
Copy link
Contributor

@JSCU-CNI JSCU-CNI commented Aug 5, 2024

This PR flattens the field type command in the JSON packer and fixes #132.

Currently the dissect.target project is inconsistent in using the same field name command and the new field type command. This patch makes it possible to upload and aggregate on different records in Elasticsearch with the field name command and differing field types.

For example, see RunKeysPlugin.runkeys and PowerShellHistoryPlugin.powershell_history.

You could argue (and we agree) that this should be fixed in dissect.target as all RecordDescriptors currently using ("string", "command") should perhaps use the new command record type. That makes sense to do in the long run. Perhaps a field called full could be added to the standard output of the command fieldtype dict to still be able to index the full, original, command.

Historically the command field type introduced a backwards incompatible change into dissect. This PR fixes that inconsistency.

@codecov
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.00%. Comparing base (9775ddb) to head (06ea457).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
+ Coverage   82.25%   83.00%   +0.75%     
==========================================
  Files          34       34              
  Lines        3652     3602      -50     
==========================================
- Hits         3004     2990      -14     
+ Misses        648      612      -36     
Flag Coverage Δ
unittests 83.00% <100.00%> (+0.75%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Miauwkeru
Copy link
Contributor

Thanks for your contribution, we will look at the changes later. But for now, could you maybe create an issue and attach it to this PR? It would make keeping track of issues a lot easier for us.

@JSCU-CNI
Copy link
Contributor Author

JSCU-CNI commented Jan 9, 2025

Do you have an update for us regarding this pull request @Miauwkeru?

@EinatFox
Copy link

Hi @JSCU-CNI ,
This PR is on our radar but unfortunately due to low capacity I cannot yet tell when it will get priority.
However, we do agree about the need to fix the inconsistency that record fields named command should not use the command type. I have created a separate issue for that in dissect.target #984. If you can make a PR for it, it will get prioritized for review, and hopefully this will partially resolve your issue.

@JSCU-CNI
Copy link
Contributor Author

JSCU-CNI commented Aug 5, 2025

FYI we have been running this patch in production for a year now without problems.

def record_to_document(self, record: Record, index: str) -> dict:
"""Convert a record to a Elasticsearch compatible document dictionary"""
rdict = record._asdict()
rdict = self.json_packer.pack_obj(record)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, could the reviewer give a comment on this change? We are unsure what the implications of this are exactly.

@JSCU-CNI
Copy link
Contributor Author

Superseded by #183.

@JSCU-CNI JSCU-CNI closed this Jan 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flatten command field types for the jsonpacker

3 participants