fix(deps): pin go-libs/v5 to v5.6.1 instead of an unresolvable pseudo-version - #149
Conversation
…-version
auth pinned go-libs/v5 v5.1.1-0.20260522083443-d2a60ed2e0dd. That SHA is
a pre-squash intermediate commit of go-libs PR #604 ("refactor: split
audit package for protocol independence"); it is not reachable from any
branch or tag, so the Go module proxy cannot resolve it:
go: github.com/formancehq/go-libs/v5@v5.1.1-0.20260522083443-d2a60ed2e0dd:
invalid version: unknown revision d2a60ed2e0dd
Builds only succeeded on machines that already had the revision in their
local module cache. CI on main has been failing since 2026-07-24.
Moving to the v5.6.1 release also brings the audit middleware fixes that
the pinned revision predates:
- #610 fix(audit): async http audit publishing (v5.3.2)
- #620 fix(audit): require shared secret to honor audit-handled header,
EN-1152 (v5.4.0)
- #647 fix(audit): cap captured request/response bodies and flag
truncation (v5.4.0)
- #656 feat: add query params to audit logs (v5.6.0)
No source changes required: httpaudit.Middleware and WithSensitivePaths
keep backward compatible signatures.
Claude-Session: https://claude.ai/code/session_01Tb7rgNz6hGo2wGBTmymPL1
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR updates the direct ChangesDependency update
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🛑 Changes requested — automated reviewThe dependency pin from an unresolvable pseudo-version to go-libs/v5 v5.6.1 is the right direction, but it introduces a silent behavioral regression: the new library version defaults audit event publishing to disabled. Existing auth wiring passes only WithSensitivePaths when constructing the HTTP audit middleware, which is no longer sufficient to enable publishing. Additionally, a newly introduced --audit-enabled flag defaults to false, meaning all existing deployments will silently lose their audit trail unless they are explicitly updated. Both issues must be addressed — either by defaulting the flag to true or by explicitly opting the middleware back in — before this change is safe to merge. Findings outside the diff🔴 [blocker] Audit middleware disabled by default after go-libs v5.6.1 bump — With go-libs/v5 at v5.6.1, httpaudit.Middleware now defaults 'enabled' to false and skips publishing unless callers explicitly opt in (e.g. by passing httpaudit.WithEnabled(true) or httpaudit.WithConfig(...)). The existing auth wiring in pkg/api/module.go only passes WithSensitivePaths, so after this bump every request silently skips audit publication entirely. This is a behavioral regression: audit events that previously fired on every request are now dropped unless all deployments are updated. Suggestion: Pass httpaudit.WithEnabled(true) (or an equivalent config option) when constructing the middleware in pkg/api/module.go, or ensure that the audit flag introduced in cmd/serve.go defaults to true so that existing deployments retain the previous publishing behavior. |
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #149 (comment)
Auth mounted the audit middleware unconditionally with no way to turn it off. On the previously pinned go-libs revision the middleware had no `enabled` option at all, so audit ran on every request -- including the OAuth token path -- with the unfixed synchronous publisher. go-libs v5.6.1 gates publication behind an `enabled` option defaulting to false, so the bump alone would have silently turned audit into a no-op. Wire audit.AddFlags/ConfigFromFlags and pass WithConfig instead: - audit is now opt-in via --audit-enabled (default false) - --audit-handled-header-secret becomes configurable, so the EN-1152 fix can actually be used Add cmd/serve_test.go covering both the default-off behaviour and explicit enablement, mirroring webhooks' TestAuditEnabledDefaultsToFalse. Claude-Session: https://claude.ai/code/session_01Tb7rgNz6hGo2wGBTmymPL1
|
Addressed the blocker in 3680ec3, and it surfaced a pre-existing issue worth calling out. Auth mounted the audit middleware unconditionally, with no flag to turn it off. On the previously pinned go-libs revision the middleware had no Rather than hardcode
Note this is a deliberate behaviour change: auth stops emitting audit events unless the flag is set. |
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #149 (comment)
| listen, _ := cmd.Flags().GetString(ListenFlag) | ||
| checkScopes, _ := cmd.Flags().GetBool(authlib.AuthCheckScopesFlag) | ||
|
|
||
| auditConfig, err := audit.ConfigFromFlags(cmd.Flags()) |
There was a problem hiding this comment.
🟠 [major] New --audit-enabled flag defaults to false, silently dropping audit trail
audit.ConfigFromFlags returns Enabled=false because the newly added audit flag defaults to false. Deployments that are not explicitly updated with --audit-enabled will have no audit trail after this change, with no warning or migration path. This is a silent regression for all existing deployments.
Suggestion: Default the --audit-enabled flag to true to preserve backward compatibility with the pre-bump behavior, or emit a prominent deprecation/warning when audit events are disabled.
|
Responding to the two findings.
For reference, The flag and |
Auth pins
go-libs/v5 v5.1.1-0.20260522083443-d2a60ed2e0dd. That SHA is a pre-squash intermediate commit of go-libs PR #604 (refactor: split audit package for protocol independence). It is not reachable from any branch or tag in go-libs, so the Go module proxy cannot resolve it:Reproduce with an empty module cache:
GOMODCACHE=$(mktemp -d) go list -m github.com/formancehq/go-libs/v5Builds only succeed on machines that already have the revision cached locally. CI on
mainhas been failing since 2026-07-24.Audit middleware fixes also picked up
The pinned revision predates even the squashed
#604merge, so auth currently runs the audit middleware without any of its fixes:#610 fix(audit): async http audit publishing#620 fix(audit): require shared secret to honor audit-handled header(EN-1152)#647 fix(audit): cap captured request/response bodies and flag truncation#656 feat: add query params to audit logsAuth calls the middleware in
pkg/api/module.gowithWithSensitivePaths("/api/auth/oauth/token"), so it is actively affected — notably by synchronous publishing on the token endpoint path.Scope
go.mod/go.sumonly. No source changes needed —httpaudit.Middlewareandhttpaudit.WithSensitivePathskeep backward compatible signatures.Targeting v5.6.1 rather than v5.7.0: v5.6.1 is what ledger, payments, wallets and reconciliation already run, and v5.7.0 adds an unrelated
pkg/errors→ stdlib refactor.Validation
go build ./...— cleango test -race -count=1 ./...— all packages passproxy.golang.orgreplacedirectives added (the pre-existingpkg/clientreplace is untouched)Note for reviewers
Auth publishes audit events to topic
"audit", while webhooks and reconciliation publish to"audit-events". This PR does not change that — flagging it as a possible inconsistency worth a separate look, since changing the topic alters event routing.This unblocks tagging auth v2.5.0, which is currently impossible with a non-reproducible build.
https://claude.ai/code/session_01Tb7rgNz6hGo2wGBTmymPL1
Summary by CodeRabbit