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

Allow users to ignore findings #12

Closed
woodruffw opened this issue Sep 5, 2024 · 10 comments · Fixed by #116
Closed

Allow users to ignore findings #12

woodruffw opened this issue Sep 5, 2024 · 10 comments · Fixed by #116
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@woodruffw
Copy link
Owner

In theory we could do something like:

permissions: write-all # zizmor: ignore[excessive-permissions]

...but this is (1) annoying since it requires us to scan the YAML comments, and (2) might interfere with other tools that unfortunately read comments, like Dependabot.

@woodruffw
Copy link
Owner Author

yamlpath now has the ability to extract comments in a limited subset of cases, so this could be done.

@woodruffw woodruffw added the enhancement New feature or request label Oct 27, 2024
@woodruffw woodruffw added the help wanted Extra attention is needed label Oct 31, 2024
@ljharb
Copy link

ljharb commented Oct 31, 2024

It'd be nice to not have to put comments directly in the yml, too - iow, to optionally support a .github/workflows/zizmor config file of some kind.

@woodruffw
Copy link
Owner Author

woodruffw commented Oct 31, 2024

It'd be nice to not have to put comments directly in the yml, too - iow, to optionally support a .github/workflows/zizmor config file of some kind.

That's a good idea!

.github/zizmor.yml would be a good place to put that, and it could be something like:

version: 1
workflows:
  foobar.yml:
    ignore:
      - some-rule-name

or:

version: 1
rules:
  template-injection:
    ignore:
      - some-workflow.yml:line:col

(as a rough sketch)

@ljharb
Copy link

ljharb commented Oct 31, 2024

(That would also allow CODEOWNERS to control access to the overrides)

@michaelmior
Copy link

I personally would strongly prefer to put these as comments directly in the workflow YAML. That way whoever is responsible for that is also responsible for the overrides. Although I understand that might not be what everyone wants. If both approaches are possible, then great.

Another problem with the external config file is that you might want to ignore one rule only for part of a workflow. That gets messy to keep the two in sync.

@ljharb
Copy link

ljharb commented Oct 31, 2024

Both sounds good to me; it's how eslint does it, and in practice it's fine to keep the two in sync.

@woodruffw
Copy link
Owner Author

Yeah, I'm OK with having both. Comments will be little thorny in zizmor's case since there are other tools that use YAML comments for signaling (e.g. Dependabot supports version comments next to uses: clauses), but those can be documented edge cases.

(In terms of getting some form of this in, I'm personally going to prioritize the config approach, since it requires fewer design thoughts about extending yamlpath. But I happily welcome people to help me out on the comment approach as well!)

@woodruffw woodruffw changed the title Allow users to ignore findings? Allow users to ignore findings Oct 31, 2024
@funnelfiasco
Copy link
Contributor

I think my ideal situation would be to ignore on a line-by-line basis, because there might be, e.g., one template injection that I know is safe in practice but I wouldn't want to ignore others that come up and might be unsafe. I recognize that it's more difficult.

One approach could be to have optional lines in the config file. For example:

version: 1
workflows:
  foobar.yml:
    ignore:
      - template-injection
         line:
         - 8
         - 42

This would tell zizmor to ignore template injections it finds on lines 8 and 42, but if one pops up on like 37, don't ignore it. The down side, of course, is that every time the workflow yaml changes, you have to go change the zizmor config, too. But as a wise man once said: "Life is pain, highness. Anyone who says differently is selling something."

@woodruffw
Copy link
Owner Author

FYI: I've begun work on this in #116. There's still a bit more work to do, but it could definitely use early eyeballs/testers to make sure it accommodates your use cases!

@woodruffw
Copy link
Owner Author

The config file variant of this is done and released with v0.2.0! I'm going to make a separate issue for the inline-comment variant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants