Skip to content
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

Change pcap file upload logging to be more descriptive. #285

Closed
wants to merge 2 commits into from

Conversation

Hashfastr
Copy link

@Hashfastr Hashfastr commented Oct 24, 2023

πŸ—£ Description

Changing error log for file upload when there's an invalid file type uploaded. Previously just printed a single trashcan emoji (πŸ—‘οΈ), changed to be more descriptive.

πŸ’­ Motivation and context

When debugging why I couldn't upload a PCAP file to Malcolm I kept getting the below error.

WARNING: watch-pcap-uploads-folder.py:	πŸ—‘	/pcap/upload/AUTOSURICATA,AUTOZEEK,all,cloud,azure,USERTAG,cloud.pcapng (application/octet-stream/pcapng capture file - version 1.0)

This is minimally descriptive only providing a trashcan emoji along with the file name and mime type. The message can be assumed that your submitted file was immediately deleted. While the attached filename and mime type seems much more descriptive of what was uploaded, but not to why the error occurred.

This pull request changed this single line error to two much more descriptive errors:

ERROR: watch-pcap-uploads-folder.py: Invalid file type uploaded application/octet-stream/pcapng capture file - version 1.0
ERROR: watch-pcap-uploads-folder.py: Deleting /pcap/upload/AUTOSURICATA,AUTOZEEK,all,cloud,azure,USERTAG,cloud.pcapng

While I like the flair emojis give scripts, putting them into error logs is a misstep and either further obfuscates the error at hand, or makes it much more difficult to parse.

πŸ§ͺ Testing

Splits one print statement into two, removes an emoji, don't really think this applies.

βœ… Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.

βœ… Pre-merge checklist

  • Revert dependencies to default branches.
  • Finalize version.

βœ… Post-merge checklist

  • Create a release.

Changing error log for file upload when there's an invalid file type uploaded. Previously just printed a single trashcan emoji (πŸ—‘οΈ), changed to be more descriptive.
Fixed uploade -> uploaded typo
@Hashfastr
Copy link
Author

Unsure of labels or how to add them at the moment seeing it's greyed out for me.

@mmguero
Copy link
Collaborator

mmguero commented Oct 25, 2023

Thanks for the suggestion! There is a reason for the emojis: sometimes when I'm developing/debugging artifact processing I'm looking at hundreds or thousands of files, and they're zipping by in the logs quickly. Those icons (which I use across many of the internal malcolm scripts) make it easier to visually identify the operation at a glance.

But I do agree that in this case the message needs to be more helpful to the end user as well. I think I will probably retain the icon but also add a more descriptive error message as you suggested.

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.

2 participants