New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved validator and clean up of definitions #306

Merged
merged 4 commits into from Feb 5, 2019

Conversation

Projects
None yet
2 participants
@joachimmetz
Copy link
Member

joachimmetz commented Feb 2, 2019

Improved validator and clean up of definitions:

  • Improved check for Windows path separator
  • Warn about unsupported users variables '%%users.variable%%'
  • Warn that %%users.homedir%% can be replaced by %%users.userprofile%%
  • Warn that '%%users.userprofile%%\AppData\Roaming' can be replaced by %%users.appdata%%
  • Warn that '%%users.userprofile%%\AppData\Local' can be replaced by %%users.localappdata%%

@joachimmetz joachimmetz force-pushed the joachimmetz:cleanup branch from 3446d48 to d4680d0 Feb 2, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 2, 2019

Codecov Report

Merging #306 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #306   +/-   ##
=======================================
  Coverage   91.12%   91.12%           
=======================================
  Files           7        7           
  Lines         417      417           
=======================================
  Hits          380      380           
  Misses         37       37

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4a8b0c...286e43e. Read the comment docs.

@joachimmetz joachimmetz force-pushed the joachimmetz:cleanup branch 4 times, most recently from fd6f520 to 8d15bea Feb 2, 2019

@joachimmetz joachimmetz changed the title Small changes to OperaHistory definition Improved validator and clean up of definitions Feb 2, 2019

@joachimmetz joachimmetz requested a review from grrrrrrrrr Feb 2, 2019

@joachimmetz joachimmetz self-assigned this Feb 2, 2019

@joachimmetz

This comment has been minimized.

Copy link
Member Author

joachimmetz commented Feb 2, 2019

@grrrrrrrrr can you have a look if this impacts GRR

@joachimmetz joachimmetz force-pushed the joachimmetz:cleanup branch from 7359b90 to 4e0f87f Feb 3, 2019

Show resolved Hide resolved tools/validator.py
source.separator, path, artifact_definition.name,
filename))
if not self._CheckWindowsPath(
filename, artifact_definition, source, path):
result = False

This comment has been minimized.

@grrrrrrrrr

grrrrrrrrr Feb 4, 2019

Contributor

A bit unrelated to this PR but all these could return straight away, wouldn't that be much easier to read?

This comment has been minimized.

@joachimmetz

joachimmetz Feb 4, 2019

Author Member

The idea is to keep checking, but return false at the end

filename))
result = False

if source.separator == '\\':

This comment has been minimized.

@grrrrrrrrr

grrrrrrrrr Feb 4, 2019

Contributor

I'd go
if source.separator != '\':
# Can't do any further validations
return True

and move all the remaining code one level out.

This comment has been minimized.

@joachimmetz

joachimmetz Feb 4, 2019

Author Member

ack, good point

path_lower = path.lower()
path_segments = path_lower.split(source.separator)

if path_segments[0].startswith('%%users.') and path_segments[0] not in (

This comment has been minimized.

@grrrrrrrrr

grrrrrrrrr Feb 4, 2019

Contributor

Will this check work for this path I found in the same pr?
'%%users.localappdata%%\ConnectedDevicesPlatform\L.%%users.username%%\ActivitiesCache.db'

In GRR we use a regex to get the interpolations our, have a look at https://github.com/google/grr/blob/8123918157e0c53a2ba36d225cec542c6ee4524d/grr/core/grr_response_core/lib/artifact_utils.py#L75 Maybe you can reuse that code?

This comment has been minimized.

@joachimmetz

joachimmetz Feb 4, 2019

Author Member

Good point tracking this in #310 for a follow up CL

@joachimmetz joachimmetz merged commit 65a407f into ForensicArtifacts:master Feb 5, 2019

5 checks passed

CodeFactor No issues found.
Details
codecov/patch Coverage not affected when comparing b4a8b0c...286e43e
Details
codecov/project 91.12% remains the same compared to b4a8b0c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joachimmetz joachimmetz deleted the joachimmetz:cleanup branch Feb 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment