Skip to content

Fix plugins where _target is not provided and add tests to check that all TargetRecordDescriptor records are instanciated with the _target field - #1902

Open
william-billaud wants to merge 7 commits into
fox-it:mainfrom
william-billaud:fix_missing_target
Open

Fix plugins where _target is not provided and add tests to check that all TargetRecordDescriptor records are instanciated with the _target field#1902
william-billaud wants to merge 7 commits into
fox-it:mainfrom
william-billaud:fix_missing_target

Conversation

@william-billaud

@william-billaud william-billaud commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Fix plugins where _target is not provided and add tests to check that all TargetRecordDescriptor records are instanciated with the _target field

In some plugins, the _target field is not provided when creating records. Thus the resulting records does not have some important field : hostname, domain and _source.
Enforcing of this rule is up to the reviewer, which is not perfecte

Proposed Changes

Create a new warning DissectWarning and a subclass MissingTargetInRecordWarning. When a record is initiated without the _target, a warning is created using the warnings.warn function.

When test are executed, these warning are considerered as error and will make the test fails.

On normal execution, this is up to of how the warning filter are configured. By default "print the first occurrence of matching warnings for each location (module + line number) where the warning is issued".

The warning will be printed on stderr using the logger configuration when (logging.captureWarnings(True))[https://github.com/fox-it/dissect.target/blob/d1c70c59217792237a5ff31a378c892bc5aa800b/dissect/target/tools/utils/logging.py#L93] is defined.

This is the case for dissect.target tools. but it will ignore the --quiet flag as the log level is only defined for the dissect logger, and not py.warnings, but this is something that can be easily changed.

Main change

  • For the Windows scheduled/at task plugin I changed some object constructor to provide the target object where they are instanced. Value can be None so it should not break anything.
  • For the target-diif tool : use the src target as _target.
  • For some specific test, ensure that the warning is raised/ignore them.
  • For others plugin, just add the missing _target field.
  • This does not add wasted cycle, as the if record_kwargs.get("_target") check is already present, we just define another branch.

Other possibilities

I have also testes the following solution, without success (mainly with the help of an LLM)

  • CodeQL : Query language is very complex, the codeql CLi (not the engine) has a specific [license] (https://github.com/github/codeql-cli-binaries/blob/main/LICENSE.md) which allows usage only on open source project (manually) or in CI hosted on github.com. Furthermore the Call expression, that represent a function call has limitation when arguments came from an unpacked dict/structure (E.g TestRecord(**a_value)) (which is understandable for a static code analysis tool).
  • SemGrep : Seems more limited that CodeQL (not an expert on both, but learning curve for these tools is not easy). It would also have required to add a new dependency in CI/tooling
  • Custom Ruff rule : Not possible at the time : Meta issue: plugin system astral-sh/ruff#283
  • Custom script parsing the AST : LLM generated a working script, with false negative, but this does not sound really maintainable over time

IMO the warning option is the less complex regarding the current code base, and does not add a lot of noise to end users, unless they encounter a case no detected in tests.

It's also possible to raise an error/depreciation warning and enforce presence of this field in a later release.

Checklist

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.

Fix plugins where _target is not provided and add tests to check that all TargetRecordDescriptor records are instanciated with the _target field

1 participant