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

pre-commit hook scans working directory instead of staging area? #12

Closed
andreaswittig opened this issue Mar 23, 2016 · 3 comments · Fixed by #13
Closed

pre-commit hook scans working directory instead of staging area? #12

andreaswittig opened this issue Mar 23, 2016 · 3 comments · Fixed by #13

Comments

@andreaswittig
Copy link
Contributor

The pre_commit_hook() searches for staged files.

pre_commit_hook() {
  local file found_match=0 rev="4b825dc642cb6eb9a060e54bf8d69288fbee4904"
  # Diff against HEAD if this is not the first commit in the repo.
  git rev-parse --verify HEAD >/dev/null 2>&1 && rev="HEAD"
  # Filter out deleted files using --diff-filter
  scan_or_die "$(git diff-index --diff-filter 'ACMU' --name-only --cached $rev --)"
}

But scan() is operating on the working directory.

scan() {
  local files="$1" action='skip' patterns=$(load_patterns)
  local allowed=$(git config --get-all secrets.allowed)
  [ -z "${patterns}" ] && return 0
  if [ -z "${files}" ]; then
    output=$(GREP_OPTIONS= LC_ALL=C git grep -nwHE "${patterns}")
  else
    # -r only applies when file paths are provided.
    [ "${RECURSIVE}" -eq 1 ] && action="recurse"
    output=$(GREP_OPTIONS= LC_ALL=C grep -d $action -nwHE "${patterns}" $files)
  fi
  local status=$?
  case "$status" in
    0)
      [ -z "${allowed}" ] && echo "${output}" && return 1
      # Determine with a negative grep if the found matches are allowed
      echo "${output}" | GREP_OPTIONS= LC_ALL=C grep -Ev "${allowed}" \
        && return 1 || return 0
      ;;
    1) return 0 ;;
    *) exit $status
  esac
}

That means the pre-commit hook might not scan through the changes that will be committed.

Do I miss something? Do you agree, this is a problem? If so I would like to send you PR fixing that problem.

@andreaswittig andreaswittig changed the title Pre-commit hook scans working directory instead of staging area? pre-commit hook scans working directory instead of staging area? Mar 23, 2016
@mtdowling
Copy link
Contributor

That's true; because pre-commit passes in a list of files, the scan for pre-commit scans the working directory. If the staged files drift from the working directory, then the scan wouldn't catch violations in the staged files.

I've opened a pull request that addresses this issue. Could you give that a look and let me know what you think? #13

@andreaswittig
Copy link
Contributor Author

Hi @mtdowling, thanks for your response. Your solution #13 looks fine!

We solved the problem in a slightly different way by adding a new method --scan-cached (https://github.com/AutoScout24/git-secrets/blob/master/git-secrets). But I prefer your solution.

@mtdowling
Copy link
Contributor

Great. Thanks for taking a look.

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 a pull request may close this issue.

2 participants