fix(tracing): redact tracer config in logs - #4247
Conversation
Since #4205 a scheme-bearing TRACER_URL is a real URL, so it can carry credentials in userinfo or its query string, and every tracer startup log wrote it verbatim. Endpoints are now logged through redactURL, the exporter name is logged as the matched constant, and the raw TRACER_INSECURE and unsupported TRACE_EXPORTER values are no longer echoed. This clears CodeQL go/clear-text-logging alerts 179-184 on pkg/gofr/otel.go, which failed the CodeQL check on the v1.61.0 release PR (#4246).
01543c7 to
ddb19c1
Compare
Umang01-hash
left a comment
There was a problem hiding this comment.
Verified independently at ddb19c1 — fuzzed redactURL and ran a live server, not just read the diff.
- All 5 URL log sites route through redactURL; the two value-echo lines now log static text. No breaking API change; gofmt/build/vet clean, golangci-lint --new-from-rev 0 issues.
- Live E2E: TRACER_URL=https://user:token@localhost:4317 logs https://REDACTED@localhost:4317 (secret absent). Mutation on the zipkin line fails Test_initTracer.../zipkin_query_key — tests are non-vacuous.
One LOW, non-blocking (inline). Also worth confirming in CI: otel.go:66 (unchanged) still echoes the raw TRACER_RATIO via the strconv error — not a credential, but check it isn't one of the 6 CodeQL alerts, else one survives.
Merge-ordering: this is on pre-#4206 otel.go; if the registry refactor lands first, redactURL + the log sites move into traces/exporters and this needs a rebase.
LGTM.
A schemeless TRACER_URL such as localhost:9411/api/v2/spans?api-key=... has no host after url.Parse, so it took the '@'-only branch and logged its query unredacted. Unparsed endpoints now go through redactUnparsedURL, which replaces userinfo first and then the query, and drops the fragment.
|
Thanks for checking it live. Replies to the two non-inline points:
Merge order with #4206: agreed. #4206 is still open against |
Umang01-hash
left a comment
There was a problem hiding this comment.
Verified locally: redaction wraps only log args (connection endpoint untouched), all endpoint-log sites route through redactURL, auth headers never logged, no exported-API change. Ran an adversarial bypass sweep (userinfo/query/encoded/IPv6/scheme-relative/uppercase/opaque) — no credential leak. Live E2E with a credential-bearing TRACER_URL + TRACER_AUTH_KEY: neither leaks, log shows https://REDACTED@…. gofmt/vet/lint clean, tests fail-on-revert. Just a few optional nits.
…ORTER Review follow-ups on the log redaction: - A newline or other control character in TRACER_URL fails url.Parse, so it reached the log verbatim through redactUnparsedURL and could forge a second line in the terminal's pretty output. That path now escapes control characters as \x.. / \u..... - Dropping the TRACE_EXPORTER value made a typo hard to spot. The value is echoed again when it looks like a typo of an exporter name -- at most 16 characters of letters, '-' and '_' -- and replaced with REDACTED otherwise, so a token pasted into the wrong variable still stays out of the log. - redactURL now states that secrets in path segments are out of scope.
gofr-dev#4247 redacted tracer config in pkg/gofr/otel.go. This branch had already moved those log lines into pkg/gofr/traces/exporters, so git reported the conflict only in otel.go: resolving it there alone would have kept the helpers and left every relocated line logging TRACER_URL verbatim, with nothing in the diff to show it. The helpers move with the lines they protect, into traces/exporters/redact.go. RedactURL and RedactMessage are exported because Builder is a public extension point and a third-party exporter logs endpoints for the same reasons the built-ins do; redactExporterName stays unexported. The OTLP builder is now registered as a closure over its matched constant, so the exporter name that reaches a log can never be the raw TRACE_EXPORTER. Two paths gofr-dev#4247 did not cover are redacted here as well, both found by running its own check against the whole process rather than against startup: - zipkin.New reports an unparsable endpoint as `invalid collector URL "<raw>"`, and spanExporter logs that error; - otelErrorHandler logs the SDK's export failures, which read `request to <TRACER_URL> failed: ...` on every batch a collector rejects. That one is present on development today. RedactMessage replaces the %q-escaped spelling as well as the raw one: url.Error renders with %q, and %q escapes exactly the characters that put a value on RedactURL's unparsed path to begin with. Measured in Docker against Jaeger, one run per exporter with a credential in TRACER_URL: a69a57f leaked it in 7 of 7 cases (1-3 occurrences each), this merge in 0 of 7. The shutdown fix this PR is about was re-verified on the merge result: shutdown-complete 10/10, spans-delivered 10/10.
* Merge pull request #4147 from gofr-dev/chore/gcp-exporter-pin-v1.60.1 chore(metrics): pin gcp exporter to gofr.dev v1.60.1 * fix(middleware): require a path separator in the well-known auth exemption (#4099) * fix(middleware): require a path separator in the well-known auth exemption * test(middleware): cover the rate limiter well-known exemption * chore(middleware): satisfy goconst and noctx in the well-known check * docs(auth): describe the separator requirement in the well-known exemption --------- Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> Co-authored-by: Umang Mundhra <mundhraumang.02@gmail.com> * perf(metrics): build the measurement option once per route, method and status (#3972) * perf(http): pool the response envelope and stop recanonicalising Content-Type (#3973) * perf(cors): build the fixed response headers once, not per request (#3971) * fix: prevent concurrent map panic during kafka client teardown (#3502) Co-authored-by: aryanmehrotra <aryanmehrotra2000@gmail.com> * fix(sql): correct query duration logging unit to microseconds (#3878) * chore(deps): consolidate minor/patch dependency updates (2026-08-21) (#4048) * fix(tools): require a path separator in the framework route check (#4102) Co-authored-by: Umang Mundhra <mundhraumang.02@gmail.com> Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> * perf(container): run health checks concurrently, and collapse concurrent probes (#3496) * feat(mqtt): add span links for pub/sub tracing (#3595) * fix(pubsub/google): guard shared subscription maps against concurrent writes (#4054) (#4055) --------- Co-authored-by: Umang Mundhra <mundhraumang.02@gmail.com> Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> * feat: implement EventHub Health check (#3649) * test(terminal): add unit tests for SetColor and ResetColor (#3648) * fix: add transaction support for dgraph migration (#3186) * fix(crud): keep digits intact in toSnakeCase for auto-generated SQL (#3676) * feat(metrics): add app_server_error/app_circuit_open_count counters + global metrics accessor (#3856) * test(service): fix flaky CB test intervals and add deterministic concurrent recovery test (#3755) * chore(deps): bump the go_modules group across 12 directories with 1 update (#4153) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> * feat(metrics): configurable per-instrument cardinality limit (#4160) * fix(websocket): don't panic on a plain HTTP request to a WebSocket route (#3932) * ci: run the full pipeline and a website build on docs-only PRs (#4195) * Fix CONTRIBUTING.md wording (#3647) Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> Co-authored-by: Umang Mundhra <mundhraumang.02@gmail.com> * fix: broken links in README and CONTRIBUTING (#3798) Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> * chore(deps): bump crate-ci/typos from 1.50.0 to 1.50.1 in the actions group (#4197) Closes: #4171 Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> * chore(deps): consolidate minor/patch dependency updates (2026-09-09) (#4196) Applied surgically per direct-dependency module (per-module go get + go mod tidy; no go work sync). All bumps are minor/patch/security — no majors, no go-redis mock regen needed (miniredis is a test double, not go-redis). Updates: - go.opentelemetry.io/otel/sdk 1.45.0 -> 1.46.0 (root, metrics/exporters/gcp, pubsub/nats, pubsub/sqs) - go.opentelemetry.io/otel/exporters/zipkin 1.44.0 -> 1.46.0 (root) - go.opentelemetry.io/contrib/detectors/gcp 1.44.0 -> 1.46.0 (metrics/exporters/gcp) - github.com/aws/aws-sdk-go-v2/config 1.32.37 -> 1.33.3 (file/s3, kv-store/dynamodb, pubsub/sqs, examples/using-s3-filestore) - github.com/aws/aws-sdk-go-v2/service/dynamodb 1.63.3 -> 1.67.0 (kv-store/dynamodb) - github.com/aws/aws-sdk-go-v2/service/sqs 1.46.6 -> 1.51.0 (pubsub/sqs) - github.com/nats-io/nats-server/v2 2.14.5 -> 2.14.6 (pubsub/nats) - golang.org/x/sync 0.22.0 -> 0.23.0 (root) - modernc.org/sqlite 1.56.0 -> 1.58.0 (root) - github.com/alicebob/miniredis/v2 2.38.0 -> 2.39.0 (root, test) - google.golang.org/grpc 1.83.1 -> 1.83.2 security (root + library modules + examples; pulls golang.org/x/net 0.57.0 -> 0.58.0). examples/using-gcp-metrics is excluded: it is baseline-untidy (excluded from the CI tidy gate) and tidying it to apply grpc would settle unrelated versions. Closes: #4172, #4173, #4174, #4175, #4176, #4177, #4178, #4179, #4180, #4181, #4182, #4183, #4184, #4185, #4186, #4187, #4188, #4189, #4190, #4191, #4192, #4194 Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> * docs: fix American English spelling of canceled (#3651) Co-authored-by: Umang Mundhra <mundhraumang.02@gmail.com> Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> * docs(cron): correct the minimum-interval claim and the "every 5 hours" example (#3876) (#3977) Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> * docs(readme): link Custom Middleware and update the Go version prerequisite (#3461) The broken Auth Middleware link this PR also carried has since landed via #3798, so what remains is: - "Custom Middleware" was plain text next to the linked Auth Middleware; it now points at docs/advanced-guide/middlewares. - The prerequisite said Go 1.24. Every go.mod in the repo declares go 1.26.0, and CI's 1.24/1.25 matrix entries only pass because a setup-go-toolchain step auto-upgrades them, so 1.26 is the real floor. Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> Co-authored-by: Umang Mundhra <mundhraumang.02@gmail.com> * fix(file): correct SFTP/S3 observability defects and stale comments (#3227) Co-authored-by: Umang Mundhra <mundhraumang.02@gmail.com> Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> * fix(sql): validate unsigned int not-null fields without panicking (#3677) * ci(website-prod): move prod build and deploy to asia-south1 zopdev-tech (#4198) Point the prod website image build/push at the asia-south1 zopdev Artifact Registry and deploy to the zopdev-tech cluster (asia-south1), replacing the us-central1 kops-dev registry and raramuri-tech cluster. Namespace, app name, and image tagging are unchanged. * feat(health): optional readiness check for /.well-known/health (#3857) (#3858) * ci(website-stage): move stage build and deploy to asia-south1 zopdev-tech (#4199) Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> * chore(deps): bump zopdev/static-server v0.0.9 -> v1.0.0 in (#4050) Major version bump (v0 -> v1) of the docs static-server base image. Consolidated from Dependabot #3990. Closes: #3990 Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> * Add HTTP QUERY Method Support (RFC 10008) (#3760) * fix(rbac): make wildcard-method rules reachable and resolve overlapping patterns deterministically (#3808) (#3934) Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> * fix(rbac): compile endpoint patterns once at load, not per rule per request (#3979) (#4047) * fix(ci): pull MinIO from quay.io — docker.io/minio/minio is no longer public (#4208) quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z carries the identical manifest list — sha256:14cea493… , byte-for-byte the digest already pinned — so this changes the registry and nothing else. The image CI runs is provably the one it ran on 2026-09-11, and the digest pin protecting the integration guard for #3804 stays intact. Bumping to a newer release instead would have changed two things at once. * fix(http,metrics,mcp): don't lose a shutdown that arrives before the server starts (#3801) (#4139) * fix(tracing): derive OTLP transport security from the TRACER_URL scheme (#4205) * ci: stop passing -short to submodule tests, which skipped 10 mongo tests entirely (#4201) Co-authored-by: Aryan Mehrotra <aryanmehrotra2000@gmail.com> * ci: measure every in-repo pkg/gofr package, not just the 29 top-level files (#4202) (#4203) * chore(deps): consolidate minor dependency updates (2026-09-16) (#4244) * update release version to v1.61.0 * fix(metrics): keep caller-controlled labels from crowding out real routes (#4163) * fix(tracing): redact tracer config in logs (#4247) Co-authored-by: Umang Mundhra <mundhraumang.02@gmail.com> * fix(tracing): stop tracer parameters shadowing the net/url import (#4249) --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Ronan Donnelly <ronan.donnelly@ymail.com> Co-authored-by: Umang Mundhra <mundhraumang.02@gmail.com> Co-authored-by: Aditya Kumar Mishra <154746713+adityakrmishra@users.noreply.github.com> Co-authored-by: Pritesh jena <priteshjena16@gmail.com> Co-authored-by: Yash Israni <118755067+yashisrani@users.noreply.github.com> Co-authored-by: Sujan Vulasala <154114096+SujanVulasala@users.noreply.github.com> Co-authored-by: Suhas-zs <110016885+Suhas-zs@users.noreply.github.com> Co-authored-by: Lokesh Malik <lokeshmalik2910@gmail.com> Co-authored-by: Hari Antara <hariantara.iputu@gmail.com> Co-authored-by: Krishna Potdar <potdarkrishna352@gmail.com> Co-authored-by: umar ahad uddin ahmed usmani <150610336+umarahad2005@users.noreply.github.com> Co-authored-by: Om Kulkarni <127368012+om7057@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Nivaas V <nivaas9293@gmail.com> Co-authored-by: Ryanisyyds <129717677+RyanisyydsTT@users.noreply.github.com> Co-authored-by: Salvatore Damon <rajarajanselvarajan02@gmail.com> Co-authored-by: Akshat Singhal <65562230+akshat-kumar-singhal@users.noreply.github.com> Co-authored-by: Napat Rungruangbangchan <Napat.joe@gmail.com> Co-authored-by: Rohit Nair P <rohitnairmuttathethu@gmail.com> Co-authored-by: Piyush Singh <piyush.singh@zop.dev> Co-authored-by: Gajendra Malviya <gajendra.malviya@zop.dev>
… and correct the docs akshat-kumar-singhal's blocking finding was right, and the measurement is unambiguous. #4206/#4207/#4247/#4249 moved the OTLP trace exporter to pkg/gofr/traces/exporters/otlp.go, where it self-registers and imports otlptracegrpc. This PR's gofr_nootlp split code out of an otel.go that no longer holds it, so the tag removed the metrics half and nothing else: otlptrace packages linked, before this commit default: 10 gofr_nootlp: 10 <- the tag removed none of them after default: 10 gofr_nootlp: 0 The constraint now sits on traces/exporters/otlp.go, with otlp_disabled.go registering "otlp" and "jaeger" to a builder that fails naming the tag. Registering rather than omitting the names is deliberate: Build's unknown-exporter path would report TRACE_EXPORTER=otlp as a name it does not recognize, which reads like a typo and sends an operator to check their spelling. The stub tells them the name is right and the binary does not carry it. Build's nil-exporter path then installs the NeverSample provider, so the service still starts with working correlation IDs. The obsolete pkg/gofr/otlp_trace*.go files are dropped. The metrics half (metrics/exporters/otlp_transport*.go) is unchanged and still correct. **CI now checks the three new tags.** gofr_nootlp and gofr_nodgraph get their own patterns; gofr_nogrpc is checked against the combined tag set because grpc is pinned by the OTLP exporters, the Dgraph protos and cloud.google.com/go as well as by the server -- alone it takes 4 of 86 packages, so a bare "still linked" assertion would fail for reasons unrelated to the tag working. All six checks pass locally. **Three doc corrections, all as reported:** - the all-six build command dropped gofr_nogrpc, because ./... cannot build with it in this repo; the exception is stated where the doc used to claim the tags never remove API - gofr_nootlp's settings are TRACE_EXPORTER=otlp|jaeger and METRICS_EXPORTER=otlp. GoFr never reads OTEL_EXPORTER_OTLP_ENDPOINT, and TRACER_URL still works for zipkin and gofr - gofr_nodgraph is broader than "Dgraph migrations": migration.go chains the Dgraph migrator whenever c.DGraph is set, so a service with a Dgraph datasource and only SQL migrations exits at app.Migrate Counts re-measured on darwin/arm64, Go 1.26.3: 828 default, 786 nootlp, 784 nosqldrivers, 808 nographql, 818 nogrpc, 826 nodgraph, 411 with all six. redact_test.go's four OTLP cases take an otlpTraceLinked skip, mirroring container.pubsubBackendsLinked. Verified: build and vet clean for default, each tag and all six; tests pass untagged and under all six; typos, gofmt and golangci-lint clean.
Description:
The v1.61.0 release PR (#4246) fails its CodeQL check with 6 high-severity
go/clear-text-loggingalerts (#179–#184), all inpkg/gofr/otel.go. This PR clears them. The CodeQL workflow that scans PRs from forks is split out into #4248, because it needs a repo settings change before it can merge.Tracer config in logs
CodeQL traces the taint back to
dbresolver'sGet("DB_PASSWORD"). That value comes back through the sharedconfig.Configinterface, so CodeQL treats everyConfig.Getresult as a password. Most alerts of this kind in the repo have been dismissed as false positives, and 3 of these 6 are the same log lines as alerts #114–#116, which were dismissed before #4205 moved them.One of the risks is real, though. Before #4205,
TRACER_URLcould only behost:port. Nowhttps://user:token@collector:4317is valid, andhttps://zipkin/api/v2/spans?api-key=…always was. Every tracer startup log printed the URL as-is.Exporting traces to <exporter> at <url>(otlp/jaeger)TRACE_EXPORTERredactURL(url)+ the matched constant (otlp/jaeger)Exporting traces to zipkin at <url>redactURL(url)Exporting traces to GoFr at <url>redactURL(url)TRACER_INSECURE is ignored for TRACER_URL=…redactURL(url)traces are exported to … over plaintext with headersredactURL(url)invalid TRACER_INSECURE …unsupported TRACE_EXPORTER …otlp, jaeger, zipkin or gofrredactURLswaps userinfo and the query string forREDACTEDinstead of dropping them, so the log still shows that something was set there. It also drops the fragment. For a schemelessuser:pass@host:port, it replaces everything before the last@. The value passed to the exporter itself is unchanged.What changes for users: the two error messages no longer echo the bad value. The redacted URLs show
REDACTEDwhere credentials were. Nothing changes for an endpoint without credentials, which is what every existing test uses.Breaking Changes (if applicable):
None to the API. Two log messages no longer include the configured value (see table).
Additional Information:
Test_redactURL: 10 cases (userinfo, token-only userinfo, query, fragment, schemeless userinfo, unparsable input, and cases that stay unchanged).Test_initTracer_doesNotLogCredentialsruns the realinitTracerpath for otlp, jaeger (with an ignoredTRACER_INSECURE), an invalidTRACER_INSECURE, zipkin and gofr. It asserts the redacted line is in stdout/stderr and the secret is not.go test ./pkg/gofr/: pass.golangci-lint --new-from-rev=origin/development: 0 issues.actionlintandtypos: clean.go/path-injection,datasource/file/local_fs.go) are already open onmainand predate this release.release/v1.61.0(cherry-pick, or re-cut the branch) for Release/v1.61.0 #4246's check to go green.Checklist:
goimportandgolangci-lint.