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

fix not being able to scan from subdirectories #774

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Ferada
Copy link

@Ferada Ferada commented Dec 22, 2023

  • Please check if the PR fulfills these requirements
  • Tests for the changes have been added
  • Docs have been added / updated (which one?)
  • All CI checks are green
  • What kind of change does this PR introduce?

bug fix

  • What is the current behavior?

When running detect-secrets scan from a subdirectory of the git root, no results will be returned (unless --all-files was specified). This doesn't seem to be intentional, as the path handling implies that the current directory may well be in some other place.

Compare detect-secrets scan docs in this repository with (cd docs && detect-secrets scan), the former returns hits for docs/design.md and docs/filters.md, the latter doesn't.

Why is it desirable to have these behave the same? Apart from it being weird if the program silently returns nothing with unexpected arguments, well, for example you might have a monorepo and just want to scan a subdirectory of it. When detect-secrets is being called from another tool, you might not necessarily be aware where the git root is and would have to first navigate there, then pass the path to the subdirectory explicitly, instead of just running detect-secrets scan from where you currently are.

  • What is the new behavior (if this is a feature change)?

Not a new feature, you'll just be able to run detect-secrets scan from a subdirectory and things will still work.

  • Does this PR introduce a breaking change?

Unless someone accidentally relied on no results being returned (I'd bet that'd always be a mistake), this is not a breaking change and might surface some secrets which were previously ignored if the program was called like described above.

lorenzodb1
lorenzodb1 previously approved these changes Dec 29, 2023
@lorenzodb1 lorenzodb1 dismissed their stale review December 29, 2023 21:52

I approved by mistake, apologies for the confusion.

@Ferada
Copy link
Author

Ferada commented Jan 4, 2024

@lorenzodb1 hi, since I've got you here, any thoughts on the problem statement and / or my attempt at fixing it? I've only ran the test cases on Linux, if the failed run there on Mac OS is real, I'll see if I can find a machine to replicate it.

@lorenzodb1
Copy link
Member

@Ferada so are you saying the test passes on Linux but fails on MacOS?

@Ferada
Copy link
Author

Ferada commented Jan 24, 2024

@Ferada so are you saying the test passes on Linux but fails on MacOS?

@lorenzodb1 I was saying that the GitHub actions output here has two failed runs and several cancelled ones and since I can't restart them I couldn't tell if they were real. However I found a machine to test it on and added one more change to fix up a path problem that, yes, only happens on Mac. I get zero test failures now.

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.

None yet

2 participants