fix(logs): sanitize per-server log filename for namespaced registry names (MCP-1111)#604
Merged
Merged
Conversation
Official modelcontextprotocol/registry server names are namespace/name (e.g. io.github.evidai/polymarket-guard). The per-server log filename is derived from the server name, so an unsanitized "/" turned server-<ns>/<name>.log into a nested directory instead of a single flat log file. Add serverLogFilename/sanitizeServerLogName and route all three derivation sites (CreateUpstreamServerLogger, CreateCLIUpstreamServerLogger, ReadUpstreamServerLogTail) through it so writes and the tail reader agree on one flat path. Characters outside [A-Za-z0-9._-] become "_". The companion registry-add percent-decode is already on main (decodePathParam in internal/httpapi/server.go, PR #591 / MCP-1056, covered by TestAddFromRegistry_SlashServerIDUnescaped), so this change completes the remaining log-path half. Related #598
Deploying mcpproxy-docs with
|
| Latest commit: |
41f94ea
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2d5ab194.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-mcp-1111-registry-log-sa.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The lumberjack writer keeps the per-server log file open for the logger's lifetime, and Windows cannot remove an open file. t.TempDir's cleanup asserts RemoveAll succeeds, failing the Cross-Platform Logging Tests job on windows-latest. Switch to os.MkdirTemp with a best-effort cleanup (mirroring TestE2E_LogRotation) and stop asserting on Sync(). Related #598
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 27012727518 --repo smart-mcp-proxy/mcpproxy-go
|
…lper Addresses Codex review on PR #604: the writer sanitized per-server log filenames but two READ sites still derived the raw server-%s.log path, so GET /api/v1/servers/{id}/logs and no-daemon CLI reads still 404'd for namespace/name registry servers. Export ServerLogFilename and route both read sites through it; tie the read-path filename to the writer's flat file in the regression test. Related #598
Third read site for the namespaced-name log fix (Codex review round 2 on PR #604): the daemon-backed CLI logs client built /servers/%s/logs with the raw name, so 'mcpproxy upstream logs io.github.x/name' missed the chi route. PathEscape the name (matching the registry client) + regression test. Related #598
Closes the daemon server-side half of the namespaced-name log fix (Codex review round 3 on PR #604): the client now encodes the name, but the handler read chi.URLParam("id") raw, so %2F reached the lookup un-decoded. Route it through decodePathParam (matching handleAddFromRegistry) + router regression test proving /servers/io.github.evidai%2Fpolymarket-guard/logs decodes. Related #598
CodeQL go/path-injection flagged the daemon log reader (internal/server/server.go) where a user-controlled server name flows into the log file path. logs.ServerLogFilename already sanitizes the name to a single path element, so the flow is safe, but CodeQL does not recognize the strings.Map char-whitelist as a barrier. Add an explicit filepath.Clean + prefix containment guard at both log-read sites (daemon reader and the no-daemon CLI reader, which feeds tail) so the resolved path can never escape the log directory. Defense-in-depth + clears the alert without weakening any check. Valid namespaced names (io.github.owner/repo) are unaffected — their sanitized filename stays a single element under logDir. Related #604
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Backend half of #598 (child of MCP-1090). Official
modelcontextprotocol/registryserver names arenamespace/name(e.g.io.github.evidai/polymarket-guard). The per-server log filename is derived from the server name, so the unsanitized/turnedserver-io.github.evidai/polymarket-guard.loginto a nested directory instead of a single flat log file.What changed
internal/logs/logger.go: newserverLogFilename/sanitizeServerLogNamehelpers. All three derivation sites now route through them so the writers and the tail reader agree on one flat path:CreateUpstreamServerLoggerCreateCLIUpstreamServerLoggerReadUpstreamServerLogTail[A-Za-z0-9._-]→_. Result is always a single path element, so it can't escape the log dir or create nested dirs.docs/configuration.md,docs/cli/management-commands.md: note the sanitization next to theserver-{name}.logdescription (ENG-9).Registry-add percent-decode (already on main)
The issue's first bullet — percent-decode the registry Add route server name — is already on
origin/mainviadecodePathParamininternal/httpapi/server.go(PR #591 / MCP-1056), covered byTestAddFromRegistry_SlashServerIDUnescaped. No change needed there; this PR completes the remaining log-path half.Tests (TDD, written first, watched fail)
internal/logs/logger_test.go:TestServerLogFilename_SanitizesPathSeparators— table: plain, namespaced/, Windows\,:, spaces; asserts result is a single path element.TestCreateUpstreamServerLogger_NamespacedNameFlatFile— end-to-end regression withio.github.evidai/polymarket-guard: a flatserver-io.github.evidai_polymarket-guard.logis created, no nestedserver-io.github.evidai/dir, andReadUpstreamServerLogTailround-trips the same raw name back to it.Verification
go test ./internal/logs/... -race✅go build ./cmd/mcpproxy✅./scripts/run-linter.sh✅ (0 issues)gofmtcleanOpen PR — not merging (Gate 3). QA self-dispatch to follow.
Related #598