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

feat: configuration file support #116

Merged
merged 6 commits into from
Nov 5, 2024
Merged

feat: configuration file support #116

merged 6 commits into from
Nov 5, 2024

Conversation

woodruffw
Copy link
Owner

@woodruffw woodruffw commented Nov 3, 2024

WIP.

Fixes #12.

@woodruffw woodruffw added cli config Configuration functionality labels Nov 3, 2024
@woodruffw woodruffw self-assigned this Nov 3, 2024
@funnelfiasco
Copy link
Contributor

I haven't given it a thorough workout, but I had occasion to need it today and it worked exactly as I expected. (Excluding a rule on a specific line in a specific file)

I did do some quick "what interesting things can I do to break this?" checks.

  • It does not complain if I specify a line that doesn't exist (maybe it should, although not a blocker, IMO)
  • It fails if I use purple for the line number, which is good. I think the error message is sufficiently helpful, too. Error: rules.template-injection.ignore: invalid line number component (must be 1-based) at line 5 column 7
  • It also fails if I use -5 for the line number, with the same sufficient error message.
  • It fails if I specify a column but no line (e.g. pages.yml::9 with a sufficiently-helpful: Error: rules.template-injection.ignore: invalid line number component (must be 1-based) at line 5 column 7
  • It fails if I include a second colon, but no column number (e.g. pages.yml:42:) with a mostly-helpful Error: rules.template-injection.ignore[0]: invalid type: map, expected a string at line 5 column 9
  • It correctly does not complain if a listed file does not exist

So it mostly correctly handles the obvious user error cases in the config. The only thing that might be improved in that regard is erroring out when a specified line number is higher than the number of lines in the target file. This is probably a sign that the user did something that they did not mean to do (although the fact that they'd still get a message about an issue they thought they'd ignored would also be an indication)

The other suggestion I have is to include a count of ignored rules when no other issues are found. The following worked for me locally. If you find it useful, feel free to take it directly or if you want I can open a PR against this branch:

diff --git a/src/render.rs b/src/render.rs
index bd43a61..b5790b7 100644
--- a/src/render.rs
+++ b/src/render.rs
@@ -77,7 +77,9 @@ pub(crate) fn render_findings(
     }
 
     if findings.is_empty() {
-        println!("{}", "No findings to report. Good job!".green());
+        println!("{no_findings} ({nignored} ignored)",
+        no_findings = "No findings to report. Good job!".green(),
+        nignored = ignored.len().bright_yellow());
     } else {
         let mut findings_by_severity = HashMap::new();

@woodruffw
Copy link
Owner Author

Thanks for trial running @funnelfiasco! Yeah, a PR for that change would be great, thanks.

(I agree that it'd be good to have zizmor warn if a config line has no effect but I don't have a great idea for how to do that with the current flow, since the config's rules are currently "driven" by the findings themselves. I'm open to ideas on doing that efficiently.)

@funnelfiasco
Copy link
Contributor

PR #122 submitted (and I see you merged it in the time it took me to write the next paragraph. Talk about service!)

As for the config line issue, I'm not sure I have an elegant approach. My inelegant suggestion is to do something equivalent to wc -l before processing the rules and if there's a line greater than the count, abandon ship. Since the failure cases I can think of involve an audit rule not being ignored as expected, it's probably fine to leave it as-is until someone thinks of an elegant solution or a worse failure case.

@woodruffw
Copy link
Owner Author

SGTM! I'll round out the docs in a bit, and then merge this and include it in a 0.2.0 release.

@woodruffw woodruffw merged commit 801dfb3 into main Nov 5, 2024
4 checks passed
@woodruffw woodruffw deleted the ww/config branch November 5, 2024 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli config Configuration functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow users to ignore findings
2 participants