feat: update libs#106
Conversation
WalkthroughMarks the CLI Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as OIDC Router Hook
participant OT as OpenTelemetry
participant H as Handler
C->>R: HTTP request (e.g. /orgs/123)
R->>R: chi.RouteContext(ctx).RoutePatterns = [r.URL.Path]
R->>OT: trace.SpanFromContext(ctx).SetName(r.URL.Path)
R->>H: forward request to handler
H-->>C: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cmd/serve.go (4)
48-76: Do not ship with a hardcoded private key as the defaultEmbedding a default RSA private key is a severe security risk. Anyone with this source can mint valid tokens if this key is ever used in non‑debug environments.
Apply these changes to only use the built‑in key in debug mode and never as the CLI default:
- Keep the constant only for debug fallback.
- Remove it as the default flag value (see Line 104 comment).
- When no key is provided and debug is enabled, explicitly fall back to this key and log a prominent warning (see Lines 125‑136 comment).
104-104: Remove insecure default for --signing-keyDon’t default the flag to the embedded key. Require an explicit key unless debug mode is on.
- cmd.Flags().String(SigningKeyFlag, defaultSigningKey, "Signing key") + cmd.Flags().String(SigningKeyFlag, "", "Signing key (PEM-encoded RSA private key). Required unless --debug")
125-136: Require signing key; allow explicit debug fallback with warningAdd a safe fallback only in debug and warn loudly. Otherwise, fail fast.
- signingKey, _ := cmd.Flags().GetString(SigningKeyFlag) - if signingKey == "" { - return errors.New("signing key must be defined") - } + signingKey, _ := cmd.Flags().GetString(SigningKeyFlag) + if signingKey == "" { + if service.IsDebug(cmd) { + logging.FromContext(cmd.Context()).Warnf("Using built-in development signing key; DO NOT USE IN PRODUCTION") + signingKey = defaultSigningKey + } else { + return errors.New("signing key must be defined") + } + }
147-153: Close the config file to avoid a file descriptor leakThe opened file is never closed.
configFile, err := os.Open(config) if err != nil { return err } + defer configFile.Close() if err := yaml.NewDecoder(configFile).Decode(&o); err != nil { return err }
🧹 Nitpick comments (4)
cmd/serve.go (4)
135-138: Support PKCS#8 keys in addition to PKCS#1Many deployments use PKCS#8 (“BEGIN PRIVATE KEY”). Fallback to PKCS#8 and ensure the key is RSA.
[Suggest_recommended_refactor]- key, err := x509.ParsePKCS1PrivateKey(block.Bytes) - if err != nil { - return err - } + var key *rsa.PrivateKey + if block.Type == "RSA PRIVATE KEY" { + k, err := x509.ParsePKCS1PrivateKey(block.Bytes) + if err != nil { + return err + } + key = k + } else { + k, err := x509.ParsePKCS8PrivateKey(block.Bytes) + if err != nil { + return errors.Wrap(err, "invalid signing key") + } + rsaKey, ok := k.(*rsa.PrivateKey) + if !ok { + return errors.New("signing key must be an RSA private key") + } + key = rsaKey + }Add the missing import:
// at top of file import ( "crypto/rsa" )
95-99: Enforce no positional args with cobra.NoArgsPrevents accidental args and improves UX.
cmd := &cobra.Command{ Use: "serve", + Args: cobra.NoArgs, RunE: runServe, }
106-107: Fix misleading help text for --configFlag expects a path, not a “name without extension.”
- cmd.Flags().String(ConfigFlag, "", "Config file name without extension") + cmd.Flags().String(ConfigFlag, "", "Config file path (YAML)")
195-203: Build RedirectURL robustly to avoid accidental double slashesUsing simple string formatting can produce “//” if baseUrl ends with “/”.
- RedirectURL: fmt.Sprintf("%s/authorize/callback", baseUrl), + RedirectURL: func() string { + u, err := url.JoinPath(baseUrl, "authorize", "callback") + if err != nil { + // Fallback to naive concat if join fails + return fmt.Sprintf("%s/authorize/callback", baseUrl) + } + return u + }(),Add import:
import "net/url"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
cmd/serve.go(1 hunks)
🔇 Additional comments (1)
cmd/serve.go (1)
119-119: Blank identifier for unused args — LGTMSignature change is correct for Cobra and silences the unused param warning.
gfyrag
force-pushed
the
feat/bun-tracing
branch
3 times, most recently
from
4cd32d2 to
a259427
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/oidc/router.go (1)
39-53: Harden against nil RouteContext and make span name more consistent; consider setting http.routeTwo tweaks:
- Guard chi.RouteContext to avoid a rare nil deref.
- Include the HTTP method in the span name for consistency with common conventions and your client formatter.
Optionally, set http.route to the path to retain useful grouping in backends (be mindful of cardinality if any dynamic segments exist).
Apply this diff inside the handler:
- chi.RouteContext(r.Context()).RoutePatterns = []string{"fake"} - trace.SpanFromContext(r.Context()).SetName(r.URL.Path) + if ctx := chi.RouteContext(r.Context()); ctx != nil { + ctx.RoutePatterns = []string{"fake"} + } + span := trace.SpanFromContext(r.Context()) + span.SetName(fmt.Sprintf("%s %s", r.Method, r.URL.Path)) + // Optional: keep http.route aligned with the chosen naming + // span.SetAttributes(attribute.String("http.route", r.URL.Path))And add the necessary imports:
// add import ( "fmt" "go.opentelemetry.io/otel/attribute" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (2)
cmd/serve.go(1 hunks)pkg/oidc/router.go(1 hunks)
🔇 Additional comments (2)
cmd/serve.go (1)
119-119: Idiomatic unused-parameter marker retained RunE compatibilityUsing
_ []stringkeeps the Cobra signature while silencing linters. Looks good.pkg/oidc/router.go (1)
34-36: Span-name override for subrouter is a sensible workaroundThe rationale to avoid otelchi’s default "/*" is sound in this context.
gfyrag
force-pushed
the
feat/bun-tracing
branch
from
a259427 to
4b2f1dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (2)
cmd/serve.go(1 hunks)pkg/oidc/router.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/serve.go
🔇 Additional comments (2)
pkg/oidc/router.go (2)
37-38: Good placement of middleware around the mounted handlerWrapping the mounted provider with r.With(...) is the right place to influence how upstream middleware (otelchi) names spans for this subtree.
34-36: Instrument subrouter with otelmux, not manual chi.RouteContext edits
- Wrap the gorilla/mux router returned by provider.HttpHandler() with otelmux.Middleware("oidc") (import go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux) instead of mutating chi.RouteContext/SpanName manually.
- Confirm otelchi.Middleware is applied to the parent chi.Router—without it, these overrides have no effect.
| chi.RouteContext(r.Context()).RoutePatterns = []string{r.URL.Path} | ||
| trace.SpanFromContext(r.Context()).SetName(r.URL.Path) |
There was a problem hiding this comment.
Avoid high-cardinality span names/attributes; remove redundant SetName and normalize the route
Using r.URL.Path for both RoutePatterns and span name can explode span/attribute cardinality and costs. Also, otelchi will likely override your SetName in its deferred block, making it redundant. Normalize to a stable pattern and guard against a missing chi.RouteContext.
Apply this diff:
- chi.RouteContext(r.Context()).RoutePatterns = []string{r.URL.Path}
- trace.SpanFromContext(r.Context()).SetName(r.URL.Path)
+ if rc := chi.RouteContext(r.Context()); rc != nil {
+ // Use a low‑cardinality pattern so metrics/traces aggregate well
+ rp := normalizeOIDCRoute(r.URL.Path)
+ rc.RoutePatterns = []string{rp}
+ }And add this helper in the same package (adjust/extend as needed):
// normalizeOIDCRoute returns a low‑cardinality route pattern for known OIDC endpoints.
func normalizeOIDCRoute(path string) string {
switch path {
case "/.well-known/openid-configuration",
"/.well-known/jwks.json",
"/authorize",
"/token",
"/userinfo",
"/introspect",
"/revoke",
"/logout",
AuthorizeCallbackPath:
return path
default:
// Fallback keeps cardinality bounded instead of using the raw path
return "/*"
}
}Operational note: If you truly need full-path span names, prefer setting only the span name (not HTTP route attribute) and ensure paths don’t contain IDs/PII. Otherwise, prefer instrumenting the subrouter directly (see next comment) to get proper patterns.
🤖 Prompt for AI Agents
In pkg/oidc/router.go around lines 51-52, the code sets high-cardinality values
using r.URL.Path for RoutePatterns and span name and doesn't guard against a
missing chi.RouteContext; replace these with a normalized, low-cardinality route
pattern and avoid calling SetName redundantly (otelchi will override it). Update
the logic to first check chi.RouteContext(r.Context()) != nil before setting
RoutePatterns, use a new normalizeOIDCRoute(path string) helper (placed in the
same package) to map known OIDC endpoints to stable patterns and return a
bounded fallback like "/*" for everything else, and remove the explicit
trace.SpanFromContext(...).SetName(r.URL.Path) call so only normalized patterns
are recorded.
There was a problem hiding this comment.
No worries, oauth2 path does not contains dynamic part.
There was a problem hiding this comment.
@gfyrag Thanks for the clarification! You're absolutely right that standard OAuth2/OIDC endpoints are static paths without dynamic segments, so the cardinality concern is much less relevant in this context.
Since you're confident about the path structure, the current approach should work fine for your use case. The only minor consideration that remains is adding a nil check for chi.RouteContext(r.Context()) to be defensive, but if your middleware setup guarantees it's always present, that's fine too.
I appreciate you sharing the domain context - it helps understand why the direct path usage is appropriate here.
✏ Learnings added
Learnt from: gfyrag
PR: formancehq/auth#106
File: pkg/oidc/router.go:51-52
Timestamp: 2025-09-24T17:39:43.652Z
Learning: In the formancehq/auth codebase, OAuth2/OIDC paths are static and do not contain dynamic segments, making direct use of r.URL.Path for span names acceptable without cardinality concerns.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
gfyrag
force-pushed
the
feat/bun-tracing
branch
from
4b2f1dc to
554fdcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/oidc/router.go (1)
34-36: Prefer instrumenting the mux subrouter directly (optional)If feasible, wrap provider.HttpHandler() with otelmux (gorilla/mux) or otelhttp instrumentation rather than faking chi route patterns; this yields correct patterns and cleaner code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (2)
cmd/serve.go(1 hunks)pkg/oidc/router.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/serve.go
🔇 Additional comments (1)
pkg/oidc/router.go (1)
34-52: Prevent panic and avoid high-cardinality spans; normalize route and drop SetName
- chi.RouteContext(r.Context()) can be nil here → potential panic on Line 51.
- Using r.URL.Path for RoutePatterns and span name creates high-cardinality telemetry and costs.
- otelchi will set the span name from HTTP route in its deferred block; explicit SetName is redundant.
Apply:
- // set a fake route pattern in the route pattern to prevent otelchi middleware - // from overriding the span name + // Set a low-cardinality route pattern so otelchi derives a stable span name. // The code from otelchi : // // set span name & http route attribute if route pattern cannot be determined // // during span creation // if len(routePattern) == 0 { // routePattern = chi.RouteContext(r.Context()).RoutePattern() // span.SetAttributes(semconv.HTTPRoute(routePattern)) // // spanName = addPrefixToSpanName(tw.requestMethodInSpanName, r.Method, routePattern) // span.SetName(spanName) // } - chi.RouteContext(r.Context()).RoutePatterns = []string{r.URL.Path} - trace.SpanFromContext(r.Context()).SetName(r.URL.Path) + if rc := chi.RouteContext(r.Context()); rc != nil { + rc.RoutePatterns = []string{normalizeOIDCRoute(r.URL.Path)} + }Note: After removing SetName, the import go.opentelemetry.io/otel/trace will likely be unused; remove it.
Add this helper in the same package:
// normalizeOIDCRoute returns a low‑cardinality route pattern for known OIDC endpoints. func normalizeOIDCRoute(path string) string { switch path { case "/.well-known/openid-configuration", "/.well-known/jwks.json", "/authorize", AuthorizeCallbackPath, "/token", "/userinfo", "/introspect", "/revoke", "/logout": return path default: // Fallback keeps cardinality bounded instead of using the raw path return "/*" } }
gfyrag
force-pushed
the
feat/bun-tracing
branch
3 times, most recently
from
9f30c77 to
da1cdf6
Compare
gfyrag
force-pushed
the
feat/bun-tracing
branch
from
da1cdf6 to
764ba59
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/oidc/router.go (1)
39-52: Guard against nil chi.RouteContext; avoid redundant SetName
- Potential NPE: chi.RouteContext(r.Context()) can be nil; add a nil guard.
- Redundancy: If otelchi is present, setting RoutePatterns should be enough; explicit SetName is unnecessary and may be overridden or duplicate work.
- Re cardinality: Using r.URL.Path is acceptable here per your note that OAuth2/OIDC paths are static (ack’d).
Apply this diff:
- // set a fake route pattern in the route pattern to prevent otelchi middleware + // set a fake route pattern in the route context to prevent otelchi middleware // from overriding the span name // The code from otelchi : // // set span name & http route attribute if route pattern cannot be determined // // during span creation // if len(routePattern) == 0 { // routePattern = chi.RouteContext(r.Context()).RoutePattern() // span.SetAttributes(semconv.HTTPRoute(routePattern)) // // spanName = addPrefixToSpanName(tw.requestMethodInSpanName, r.Method, routePattern) // span.SetName(spanName) // } - chi.RouteContext(r.Context()).RoutePatterns = []string{r.URL.Path} - trace.SpanFromContext(r.Context()).SetName(r.URL.Path) + if rc := chi.RouteContext(r.Context()); rc != nil { + rc.RoutePatterns = []string{r.URL.Path} + } + // Let otelchi derive the span name from the route patternpkg/oidc/storage.go (1)
326-341: Repeat check for WithDescription calls; consider not leaking internal errors
- Same API concern applies to these three calls.
- Consider avoiding raw internal error messages in OIDC error descriptions to prevent leaking internals. Log server-side; return a generic description to clients.
Apply the API fix consistently:
- return oidc.ErrServerError().WithDescription("%s", err) + return oidc.ErrServerError().WithDescriptionf("%s", err)If you opt to avoid leaking internals:
- return oidc.ErrServerError().WithDescriptionf("%s", err) + // log the error server-side instead; return a generic description to the client + return oidc.ErrServerError().WithDescription("internal server error")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (3)
cmd/serve.go(1 hunks)pkg/oidc/router.go(1 hunks)pkg/oidc/storage.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/serve.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T17:39:43.652Z
Learnt from: gfyrag
PR: formancehq/auth#106
File: pkg/oidc/router.go:51-52
Timestamp: 2025-09-24T17:39:43.652Z
Learning: In the formancehq/auth codebase, OAuth2/OIDC paths are static and do not contain dynamic segments, making direct use of r.URL.Path for span names acceptable without cardinality concerns.
Applied to files:
pkg/oidc/router.go
🧬 Code graph analysis (1)
pkg/oidc/storage.go (1)
pkg/storage/errors.go (1)
IgnoreNotFoundError(11-16)
🔇 Additional comments (2)
pkg/oidc/router.go (1)
34-35: Ack: good context on span naming overrideComment clearly explains the gorilla/mux subrouter case and why a custom span naming approach is needed here.
pkg/oidc/storage.go (1)
312-324: No change needed for WithDescription usage
WithDescription(desc string, args ...any) supports printf-style formatting, so.WithDescription("%s", err)is valid.
2 participants
No description provided.