Skip to content

Conversation

@kaddujames501-ship-it
Copy link

@kaddujames501-ship-it kaddujames501-ship-it commented Oct 31, 2025

Bug #4330
Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.

Contribution style:

Our Contribution agreements:

Changes (if applicable):

Link to ticket: https://redmine.openinfosecfoundation.org/issues/4330#change-42436

Describe changes:

  • Previously, the force-hash configuration for file logging silently accepted
    unknown or invalid hash types (e.g., 'shanani'), leading to potential
    misconfiguration.

This patch updates FileForceHashParseCfg() in util-file.c to:

  • Allow only 'md5', 'sha1', or 'sha256' values.
  • Return an error for unknown values.
  • Propagate the error to the output module, causing Suricata to abort in
    test mode (-T) if invalid values are detected.

Tested by adding an invalid hash type ('shanani') in suricata.yaml and
confirming that Suricata fails with the correct error message.

Screenshot from 2025-11-05 17-37-44

Provide values to any of the below to override the defaults.

  • To use a Suricata-Verify or Suricata-Update pull request,
    link to the pull request in the respective _BRANCH variable.
  • Leave unused overrides blank or remove.

SV_BRANCH=OISF/suricata-verify#2735

James Kaddu: [email protected]

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

NOTE: This PR may contain new authors.

@codecov
Copy link

codecov bot commented Nov 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.17%. Comparing base (8542017) to head (aeb11e3).
⚠️ Report is 26 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14223      +/-   ##
==========================================
- Coverage   84.17%   84.17%   -0.01%     
==========================================
  Files        1013     1013              
  Lines      262327   262331       +4     
==========================================
+ Hits       220809   220812       +3     
- Misses      41518    41519       +1     
Flag Coverage Δ
fuzzcorpus 63.35% <0.00%> (+<0.01%) ⬆️
livemode 18.72% <0.00%> (-0.04%) ⬇️
pcap 44.59% <0.00%> (+<0.01%) ⬆️
suricata-verify 64.88% <100.00%> (-0.01%) ⬇️
unittests 59.18% <0.00%> (+<0.01%) ⬆️

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

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

Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

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

Code looks good.

⚠️ The commit title has an unneeded meading space
Also output/file instead of output-file
I think it is too long
The text could be something like

Previously, the force-hash configuration for file logging silently accepted
unknown or invalid hash types (e.g., 'shanani')
Now, invalid values trigger FatalError.

I put a question in the SV PR

@kaddujames501-ship-it
Copy link
Author

kaddujames501-ship-it commented Nov 5, 2025

Code looks good.

⚠️ The commit title has an unneeded meading space Also output/file instead of output-file I think it is too long The text could be something like

Previously, the force-hash configuration for file logging silently accepted
unknown or invalid hash types (e.g., 'shanani')
Now, invalid values trigger FatalError.

I put a question in the SV PR

thank for the review, all requested changes have been cleared. Even the SV PR

Previously, the force-hash configuration for file logging silently accepted
unknown or invalid hash types (e.g., 'shanani')
Now, invalid values trigger FatalError.

Bug OISF#4330
@inashivb
Copy link
Member

inashivb commented Nov 7, 2025

Hi @kaddujames501-ship-it ! Force pushes are discouraged here. Please read the contribution guidelines and create a new PR for any changes incorporated. Thank you!

@kaddujames501-ship-it
Copy link
Author

closing this for #14305

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

Labels

outreachy Contributions made by Outreachy applicants

Development

Successfully merging this pull request may close these issues.

4 participants