Reject token and HMAC secrets shorter than 16 bytes

[mrrobertkent]

A token or HMAC secret shorter than 16 bytes now fails to build its check, the same way a missing secret does: the source is not ready (503, nothing forwarded, hookgate_source_ready 0) and hookgate validate exits 1. The error names the reference (env:NAME or file:PATH), never the value. Optional secrets that are set are held to the same minimum; set_headers values are upstream credentials and are not affected.

• Tests: 15-byte secrets rejected for token, HMAC and an optional rotation secret; a 16-byte secret accepted; the error never contains the value. Existing test secrets lengthened.
• docs/configuration.md documents the minimum; the README quick start uses a secret that meets it.

source "github": hmac X-Hub-Signature-256: env:GITHUB_WEBHOOK_SECRET is shorter than 16 bytes

Summary by CodeRabbit

• Bug Fixes

  • Token and HMAC secrets shorter than 16 bytes are now rejected. Affected sources remain unavailable and return 503, including when a short secret is marked optional.
  • Error messages identify the secret reference without exposing the secret value.

• Documentation

  • Updated the configuration guidance and Docker quick start with the minimum secret length and consistent signing-key examples.

[coderabbitai]

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 0ede3141-cd8e-4c03-a321-499f21700c3b

📥 Commits

Reviewing files that changed from the base of the PR and between 8d661bc and c32ba7d.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 8b022933-4e45-41e5-b575-e62db47a1b76

📥 Commits

Reviewing files that changed from the base of the PR and between 6e746b5 and 1745d16.

📒 Files selected for processing (7)

• README.md
• docs/configuration.md
• internal/gateway/admin_test.go
• internal/gateway/gateway_test.go
• internal/verify/fuzz_test.go
• internal/verify/verify.go
• internal/verify/verify_test.go

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

---

📝 Walkthrough

Walkthrough

Secret resolution now requires nonempty token and HMAC secrets to be at least 16 bytes. Tests, documentation, and the Quick start example use secrets that meet this minimum.

Changes

Secret Length Enforcement

Layer / File(s)	Summary
Enforce and document the secret minimum internal/verify/verify.go, internal/verify/verify_test.go, internal/verify/fuzz_test.go, internal/gateway/*_test.go, README.md, docs/configuration.md	MinSecretBytes is 16. Resolution rejects shorter nonempty secrets and reports the reference without exposing its value. Tests cover short and accepted secrets. Test fixtures and documentation reflect the minimum. The Quick start example uses demo-secret-change-me for the webhook secret and HMAC key.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 1745d

Short token and HMAC secrets fail validation, and affected sources do not forward requests. The implementation, tests, and documentation align; no merge-blocking risk was identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 1745d

The change strengthens authentication and preserves fail-closed behavior. The main rollout risk is that previously accepted short secrets, including populated optional rotation secrets, now disable their sources until corrected and restarted.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The compatibility impact covers every configured source using an undersized verifier secret; a shared reference can affect multiple sources. Unrelated sources remain independently eligible. External webhook callers do not control this eligibility through request headers or bodies.

Security Findings and Attack Paths

• inferred — No introduced bypass was identified in the inspected transition: a request cannot reach the upstream through a source whose short secret prevented construction, and retries are reached only after all configured request checks pass. This conclusion is bounded to the reviewed change, not a complete security assessment of the service.

Trust Boundaries and Controls

• observed — Configured environment or file references supply verifier credentials. Attacker-supplied headers and body bytes are checked against constructed token or HMAC verifiers before forwarding. The PR tightens credential eligibility without changing those request-to-upstream trust transitions or granting additional authority.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 5 files. (2 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The description explains the user-visible behavior, tests, documentation updates, and security constraint. It does not include the required "## What changes for users" and "## Checklist" sections, and…	Add the required template headings and checklist. Confirm the race tests, vet checks, and formatting checks. Mark the relevant checklist items as complete.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely states the main change: rejecting token and HMAC secrets shorter than 16 bytes.

Full details: Docstring Coverage

Explanation

Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 5 files. (2 skipped: 2 unsupported.)

Full details: Description check

Explanation

The description explains the user-visible behavior, tests, documentation updates, and security constraint. It does not include the required "## What changes for users" and "## Checklist" sections, and it does not confirm that go test -race ./..., go vet ./..., and gofmt -l . are clean.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[gitar-bot]

Code Review ✅ Approved

🔴 High risk · Rejecting token and HMAC secrets under 16 bytes changes request authentication; existing integrations using short secrets could become unavailable with 503 responses.

Adds validation to reject token and HMAC secrets shorter than 16 bytes, keeping affected sources unavailable (503) until corrected. Error messages identify the secret reference without exposing the value. Tests cover rejection of 15-byte secrets and acceptance of 16-byte secrets across token, HMAC, and optional rotation scenarios. Documentation and quick-start examples updated. No issues found.

Review coverage

📋 Rules No rules evaluated

🧪 Functional validation Not enabled · Set up

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}