Skip to content

chore(hooks): install working pre-commit/pre-push git hooks - #82

Open
PenguinzTech wants to merge 1 commit into
v2.1.xfrom
chore/add-git-hooks
Open

chore(hooks): install working pre-commit/pre-push git hooks#82
PenguinzTech wants to merge 1 commit into
v2.1.xfrom
chore/add-git-hooks

Conversation

@PenguinzTech

Copy link
Copy Markdown
Contributor

Summary

  • .git/hooks/pre-commit was a pre-commit-framework shim with no .pre-commit-config.yaml on branches off main — every commit died with InvalidConfigError. v2.1.x already had a config, but it predated pre-commit's pre-commit/pre-push stage naming, hand-rolled several checks with silent || true tool-missing fallbacks, and linted Python with flake8/black/isort.
  • Modernized .pre-commit-config.yaml: proper pre-commit/pre-push staging, framework-managed gitleaks/shellcheck/golangci-lint (pinned v1.64.8 to match go-client-release.yml, fixing a v1-vs-v2 config-schema mismatch against system-installed golangci-lint) instead of PATH-dependent local hooks, actionlint (with its embedded shellcheck sub-check disabled — redundant with the dedicated shellcheck hook and otherwise surfaces dozens of pre-existing style findings), bandit + Dockerfile-rootless check at pre-push.
  • Migrated Python lint to ruff (backend-python.md): canonical [tool.ruff] in pyproject.toml; the blocking hook is scoped to --select=F,E9,B (the same bar flake8 already enforced) and all 50 findings in that scope are fixed, including a real bug (dpop_service.py called traceback.print_exc() without importing traceback, so any exception there raised NameError and masked the original error) and 4 missing raise ... from. The full canonical rule set (docstrings/naming/pyupgrade/security) surfaces ~1,700 pre-existing findings across five services — documented as advisory/follow-up in pyproject.toml, not fixed here (out of scope for a hooks-installation chore). Removed flake8/black/isort from requirements-dev.txt (all 5 services), .flake8, and CI.
  • Fixed the shellcheck (SC2145/SC2155/SC2034) and bandit (B104 server-bind false positives, annotated; B113 missing requests timeout) findings surfaced by wiring these checks in for the first time.
  • Copied scripts/install-pre-commit.sh + lib/detect-os.sh + hooks/check-dockerfile-rootless.sh from admin; fixed a worktree bug in --verify (it used "$root/.git/hooks", which is a file not a directory inside a worktree, so it always reported hooks as NOT INSTALLED even when correctly installed).
  • Added install-hooks/verify-hooks Makefile targets; setup now depends on install-hooks; updated lint/fix-lint/format for ruff.
  • Updated docs (CONTRIBUTING.md, DEVELOPMENT.md, PRE_COMMIT.md, STANDARDS.md, WORKFLOWS.md) to match.
  • ~80 unrelated files got trailing-whitespace/EOF fixes from running the new hygiene hooks --all-files for the first time (verified whitespace-only via git diff).

Not fully green — left open, not auto-merged

Per devops.md's Auto-Merge gate, pre-existing failures block merge regardless of relation to this change. make test-security and make test currently fail for reasons unrelated to this PR and predating it:

  • make test-securitygosec: 4 findings where the source uses //nolint:gosec (golangci-lint syntax), but the Makefile's test-security target runs bare gosec ./..., which only understands #nosec — a pre-existing tooling mismatch, not something introduced here.
  • make test-securitygovulncheck: local Go toolchain skew (govulncheck built with go1.26, go list on PATH is go1.25) — environment issue, not a repo bug.
  • make test: 6 pre-existing test failures reproduce identically against the unmodified files (test_dpop_service.py x4 — crypto verification bug, test_observability.py — OpenTelemetry package version mismatch, test_app.py — missing saml2 package), plus several services' test suites can't even collect in a from-scratch environment without their per-service venv installed (responses, saml2, pyOpenSSL missing).

None of the above are touched by this PR's diff. The hook infrastructure itself is fully verified green (see Test plan).

Test plan

  • pre-commit run --all-files — all hooks pass
  • pre-commit run --all-files --hook-stage pre-push — all hooks pass
  • make install-hooks / make verify-hooks — hooks installed, executable, non-empty (fixed the worktree false-negative)
  • make lint — exits 0 (openapi spectral warnings are pre-existing/advisory, 0 errors)
  • python3 -m py_compile on every edited .py file
  • Unit tests for touched files (dns-server, squawk-client) pass in full; manager/backend failures confirmed pre-existing by reproducing against the unmodified files
  • make test-security / make test — pre-existing failures unrelated to this change (see above); not fixed here

🤖 Generated with Claude Code

.git/hooks/pre-commit was a pre-commit-framework shim on repos branched
from main with no .pre-commit-config.yaml, so every commit died with
InvalidConfigError. v2.1.x already had a config but it predated the
framework's pre-commit/pre-push stage naming, hand-rolled several checks
with silent `|| true` tool-missing fallbacks, and linted Python with
flake8/black/isort.

- Modernize .pre-commit-config.yaml: pre-commit/pre-push stages, add
  missing hygiene hooks, framework-managed gitleaks/shellcheck/golangci-lint
  (pinned v1.64.8 to match CI, matching go-client-release.yml) instead of
  system-PATH-dependent local hooks, actionlint (shellcheck sub-check
  disabled -- redundant with the dedicated shellcheck hook), bandit +
  Dockerfile rootless check at pre-push.
- Migrate Python lint from flake8/black/isort to ruff (backend-python.md):
  canonical [tool.ruff] in pyproject.toml, blocking hook scoped to
  --select=F,E9,B (flake8's prior bar), full canonical rule set advisory
  (~1,700 pre-existing findings, mostly missing docstrings/pyupgrade
  across 5 services -- not in scope here, tracked via the pyproject.toml
  comment for incremental adoption). Fixed all 50 findings in the blocking
  scope, including a real bug (dpop_service.py referenced `traceback`
  without importing it) and 4 missing `raise ... from`. Removed
  flake8/black/isort from requirements-dev.txt, .flake8, and CI.
  Also fixed pre-existing bandit findings (B104 false-positives on server
  listen sockets/config defaults, annotated; B113 missing requests
  timeout) and shellcheck findings (SC2145/SC2155/SC2034) surfaced by
  wiring the tools in for the first time.
- Copy scripts/install-pre-commit.sh + lib/detect-os.sh +
  hooks/check-dockerfile-rootless.sh from admin; fix a worktree bug in
  --verify (used "$root/.git/hooks" instead of --git-common-dir, so it's
  a file not a dir inside a worktree and always reports NOT INSTALLED).
- Add install-hooks/verify-hooks Makefile targets; make setup depend on
  install-hooks; update lint/fix-lint/format for ruff.
- Update docs (CONTRIBUTING/DEVELOPMENT/PRE_COMMIT/STANDARDS/WORKFLOWS)
  to match.

Verified: pre-commit run --all-files and --hook-stage pre-push both pass
clean. make lint and make install-hooks/verify-hooks pass. Unit tests for
touched files pass where the local environment has their deps installed;
6 pre-existing failures (dpop_service crypto, observability, saml/scim
missing `saml2` package) reproduce identically against the unmodified
files and are out of scope here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PenguinzTech PenguinzTech self-assigned this Aug 10, 2026
@socket-security

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedpypi/​ruff@​0.16.2100100100100100

View full report

@socket-security

Copy link
Copy Markdown

Warning

Review the following alerts detected in dependencies.

According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.

Action Severity Alert  (click "▶" to expand/collapse)
Warn Low
Filesystem access: pypi ruff

Location: Package overview

From: dhcp-server/requirements-dev.txtpypi/ruff@0.16.2

ℹ Read more on: This package | This alert | What is filesystem access?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: If a package must read the file system, clarify what it will read and ensure it reads only what it claims to. If appropriate, packages can leave file system access to consumers and operate on data passed to it instead.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore pypi/ruff@0.16.2. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

@PenguinzTech

Copy link
Copy Markdown
Contributor Author

Filed the tenant-isolation finding as #83 (type:security, priority:critical) so it is tracked independently of this PR.

On closer inspection it is broader than noted in the PR body: the root cause is require_scim_token() hardcoding tenant = 'default' (scim.py:116-117), which makes g.tenant a constant for every token. All 9 user endpoints are unscoped, and 6 use bare integer PK lookups — including DELETE /Users/<id>, so it permits cross-tenant account deactivation, not only data exposure.

No change to this PR; hooks work is unaffected.

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.

1 participant