Skip to content

fix: fixing log forging vulnerability #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Farenheith wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: fixing log forging vulnerability #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 20 additions & 18 deletions cmd/openapi-mcp/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ package main
import (
"flag"
"fmt"
"log"
"os"
"path/filepath"
"strings"

"github.com/ckanthony/openapi-mcp/pkg/config"
"github.com/ckanthony/openapi-mcp/pkg/parser"
"github.com/ckanthony/openapi-mcp/pkg/server"
"github.com/ckanthony/openapi-mcp/pkg/utils"
"github.com/joho/godotenv"
)

Expand Down Expand Up @@ -56,33 +56,33 @@ func main() {
// --- Load .env after parsing flags ---
if *specPath != "" && !strings.HasPrefix(*specPath, "http://") && !strings.HasPrefix(*specPath, "https://") {
envPath := filepath.Join(filepath.Dir(*specPath), ".env")
log.Printf("Attempting to load .env file from spec directory: %s", envPath)
utils.SafeLogPrintf("Attempting to load .env file from spec directory: %s", envPath)
err := godotenv.Load(envPath)
if err != nil {
// It's okay if the file doesn't exist, log other errors.
if !os.IsNotExist(err) {
log.Printf("Warning: Error loading .env file from %s: %v", envPath, err)
utils.SafeLogPrintf("Warning: Error loading .env file from %s: %v", envPath, err)
} else {
log.Printf("Info: No .env file found at %s, proceeding without it.", envPath)
utils.SafeLogPrintf("Info: No .env file found at %s, proceeding without it.", envPath)
}
} else {
log.Printf("Successfully loaded .env file from %s", envPath)
utils.SafeLogPrintf("Successfully loaded .env file from %s", envPath)
}
} else if *specPath == "" {
log.Println("Skipping .env load because --spec is missing.")
utils.SafeLogPrintln("Skipping .env load because --spec is missing.")
} else {
log.Println("Skipping .env load because spec path appears to be a URL.")
utils.SafeLogPrintln("Skipping .env load because spec path appears to be a URL.")
}

// --- Read REQUEST_HEADERS env var ---
customHeadersEnv := os.Getenv("REQUEST_HEADERS")
if customHeadersEnv != "" {
log.Printf("Found REQUEST_HEADERS environment variable: %s", customHeadersEnv)
utils.SafeLogPrintf("Found REQUEST_HEADERS environment variable: %s", customHeadersEnv)
}

// --- Input Validation ---
if *specPath == "" {
log.Println("Error: --spec flag is required.")
utils.SafeLogPrintln("Error: --spec flag is required.")
flag.Usage()
os.Exit(1)
}
Expand All @@ -99,7 +99,8 @@ func main() {
case string(config.APIKeyLocationCookie):
apiKeyLocation = config.APIKeyLocationCookie
default:
log.Fatalf("Error: invalid --api-key-loc value: %s. Must be 'header', 'query', 'path', or 'cookie'.", *apiKeyLocStr)
utils.SafeLogPrintln("Error: invalid --api-key-loc value:", *apiKeyLocStr, "Must be 'header', 'query', 'path', or 'cookie'.")
os.Exit(1)
}
}

Expand All @@ -120,27 +121,28 @@ func main() {
CustomHeaders: customHeadersEnv,
}

log.Printf("Configuration loaded: %+v\n", cfg)
log.Println("API Key (resolved):", cfg.GetAPIKey())
utils.SafeLogPrintln("Configuration loaded.")
utils.SafeLogPrintln("API Key (resolved):", cfg.GetAPIKey())

// --- Call Parser ---
specDoc, version, err := parser.LoadSwagger(cfg.SpecPath)
if err != nil {
log.Fatalf("Failed to load OpenAPI/Swagger spec: %v", err)
utils.SafeLogFatalf("Failed to load OpenAPI/Swagger spec: %v", err)
}
log.Printf("Spec type %s loaded successfully from %s.\n", version, cfg.SpecPath)
utils.SafeLogPrintln("Spec type", version, "loaded successfully from", cfg.SpecPath)

toolSet, err := parser.GenerateToolSet(specDoc, version, cfg)
if err != nil {
log.Fatalf("Failed to generate MCP toolset: %v", err)
utils.SafeLogFatalf("Failed to generate MCP toolset: %v", err)
}
log.Printf("MCP toolset generated with %d tools.\n", len(toolSet.Tools))
utils.SafeLogPrintln("MCP toolset generated with", len(toolSet.Tools), "tools.")

// --- Start Server ---
addr := fmt.Sprintf(":%d", *port)
log.Printf("Starting MCP server on %s...", addr)
utils.SafeLogPrintf("Starting MCP server on %s...", addr)
err = server.ServeMCP(addr, toolSet, cfg) // Pass cfg to ServeMCP
if err != nil {
log.Fatalf("Failed to start server: %v", err)
utils.SafeLogPrintln("Failed to start server:", err)
os.Exit(1)
}
}
17 changes: 9 additions & 8 deletions pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package config

import (
"log"
"os"

"github.com/ckanthony/openapi-mcp/pkg/utils"
)

// APIKeyLocation specifies where the API key is located for requests.
Expand Down Expand Up @@ -43,28 +44,28 @@ type Config struct {

// GetAPIKey resolves the API key value, prioritizing the environment variable over the direct flag.
func (c *Config) GetAPIKey() string {
log.Println("GetAPIKey: Attempting to resolve API key...")
utils.SafeLogPrintf("GetAPIKey: Attempting to resolve API key...")

// 1. Check environment variable specified by --api-key-env
if c.APIKeyFromEnvVar != "" {
log.Printf("GetAPIKey: Checking environment variable specified by --api-key-env: %s", c.APIKeyFromEnvVar)
utils.SafeLogPrintf("GetAPIKey: Checking environment variable specified by --api-key-env: %s", c.APIKeyFromEnvVar)
val := os.Getenv(c.APIKeyFromEnvVar)
if val != "" {
log.Printf("GetAPIKey: Found key in environment variable %s.", c.APIKeyFromEnvVar)
utils.SafeLogPrintf("GetAPIKey: Found key in environment variable %s.", c.APIKeyFromEnvVar)
return val
}
log.Printf("GetAPIKey: Environment variable %s not found or empty.", c.APIKeyFromEnvVar)
utils.SafeLogPrintf("GetAPIKey: Environment variable %s not found or empty.", c.APIKeyFromEnvVar)
} else {
log.Println("GetAPIKey: No --api-key-env variable specified.")
utils.SafeLogPrintf("GetAPIKey: No --api-key-env variable specified.")
}

// 2. Check direct flag --api-key
if c.APIKey != "" {
log.Println("GetAPIKey: Found key provided directly via --api-key flag.")
utils.SafeLogPrintf("GetAPIKey: Found key provided directly via --api-key flag.")
return c.APIKey
}

// 3. No key found
log.Println("GetAPIKey: No API key found from config (env var or direct flag).")
utils.SafeLogPrintf("GetAPIKey: No API key found from config (env var or direct flag).")
return ""
}
38 changes: 19 additions & 19 deletions pkg/parser/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"encoding/json"
"fmt"
"io"
"log"
"net/http"
"net/url"
"os"
Expand All @@ -15,6 +14,7 @@ import (

"github.com/ckanthony/openapi-mcp/pkg/config"
"github.com/ckanthony/openapi-mcp/pkg/mcp"
"github.com/ckanthony/openapi-mcp/pkg/utils"
"github.com/getkin/kin-openapi/openapi3"
"github.com/go-openapi/loads"
"github.com/go-openapi/spec"
Expand All @@ -38,7 +38,7 @@ func LoadSwagger(location string) (interface{}, string, error) {
var absPath string // Store absolute path if it's a file

if !isURL {
log.Printf("Detected file path location: %s", location)
utils.SafeLogPrintf("Detected file path location: %s", location)
absPath, err = filepath.Abs(location)
if err != nil {
return nil, "", fmt.Errorf("failed to get absolute path for '%s': %w", location, err)
Expand All @@ -49,7 +49,7 @@ func LoadSwagger(location string) (interface{}, string, error) {
return nil, "", fmt.Errorf("failed reading file path '%s': %w", absPath, err)
}
} else {
log.Printf("Detected URL location: %s", location)
utils.SafeLogPrintf("Detected URL location: %s", location)
// Read data first for version detection
resp, err := http.Get(location)
if err != nil {
Expand Down Expand Up @@ -81,11 +81,11 @@ func LoadSwagger(location string) (interface{}, string, error) {

if !isURL {
// Use LoadFromFile for local files
log.Printf("Loading V3 spec using LoadFromFile: %s", absPath)
utils.SafeLogPrintf("Loading V3 spec using LoadFromFile: %s", absPath)
doc, loadErr = loader.LoadFromFile(absPath)
} else {
// Use LoadFromURI for URLs
log.Printf("Loading V3 spec using LoadFromURI: %s", location)
utils.SafeLogPrintf("Loading V3 spec using LoadFromURI: %s", location)
doc, loadErr = loader.LoadFromURI(locationURL)
}

Expand All @@ -99,7 +99,7 @@ func LoadSwagger(location string) (interface{}, string, error) {
return doc, VersionV3, nil
} else if _, ok := detector["swagger"]; ok {
// Swagger 2.0 - Still load from data as loads.Analyzed expects bytes
log.Printf("Loading V2 spec using loads.Analyzed from data (source: %s)", location)
utils.SafeLogPrintf("Loading V2 spec using loads.Analyzed from data (source: %s)", location)
doc, err := loads.Analyzed(data, "2.0")
if err != nil {
return nil, "", fmt.Errorf("failed to load or validate Swagger v2 spec from '%s': %w", location, err)
Expand Down Expand Up @@ -139,7 +139,7 @@ func generateToolSetV3(doc *openapi3.T, cfg *config.Config) (*mcp.ToolSet, error
// Determine Base URL once
baseURL, err := determineBaseURLV3(doc, cfg)
if err != nil {
log.Printf("Warning: Could not determine base URL for V3 spec: %v. Operations might fail if base URL override is not set.", err)
utils.SafeLogPrintf("Warning: Could not determine base URL for V3 spec: %v. Operations might fail if base URL override is not set.", err)
baseURL = "" // Allow proceeding if override is set
}

Expand Down Expand Up @@ -175,7 +175,7 @@ func generateToolSetV3(doc *openapi3.T, cfg *config.Config) (*mcp.ToolSet, error
// Handle request body
requestBody, err := requestBodyToMCPV3(op.RequestBody)
if err != nil {
log.Printf("Warning: skipping request body for %s %s due to error: %v", method, rawPath, err)
utils.SafeLogPrintf("Warning: skipping request body for %s %s due to error: %v", method, rawPath, err)
} else {
// Merge request body schema into the main parameter schema
if requestBody.Content != nil {
Expand All @@ -189,7 +189,7 @@ func generateToolSetV3(doc *openapi3.T, cfg *config.Config) (*mcp.ToolSet, error
}
} else {
// If body is not an object, represent as 'requestBody'
log.Printf("Warning: V3 request body for %s %s is not an object schema. Representing as 'requestBody' field.", method, rawPath)
utils.SafeLogPrintf("Warning: V3 request body for %s %s is not an object schema. Representing as 'requestBody' field.", method, rawPath)
parametersSchema.Properties["requestBody"] = mediaTypeSchema
}
break // Only process the first content type
Expand Down Expand Up @@ -219,7 +219,7 @@ func generateToolSetV3(doc *openapi3.T, cfg *config.Config) (*mcp.ToolSet, error
// Optionally, add a note if the requestBody itself was marked as required
if requestBody.Required { // Check the boolean field
// How to indicate this? Maybe add to description?
log.Printf("Note: Request body for %s %s is marked as required.", method, rawPath)
utils.SafeLogPrintf("Note: Request body for %s %s is marked as required.", method, rawPath)
// Or add all top-level body props to required? Needs decision.
}
}
Expand Down Expand Up @@ -309,18 +309,18 @@ func parametersToMCPSchemaAndDetailsV3(params openapi3.Parameters, cfg *config.C
opParams := []mcp.ParameterDetail{}
for _, paramRef := range params {
if paramRef.Value == nil {
log.Printf("Warning: Skipping parameter with nil value.")
utils.SafeLogPrintf("Warning: Skipping parameter with nil value.")
continue
}
param := paramRef.Value
if param.Schema == nil {
log.Printf("Warning: Skipping parameter '%s' with nil schema.", param.Name)
utils.SafeLogPrintf("Warning: Skipping parameter '%s' with nil schema.", param.Name)
continue
}

// Skip the API key parameter if configured
if cfg.APIKeyName != "" && param.Name == cfg.APIKeyName && param.In == string(cfg.APIKeyLocation) {
log.Printf("Parser V3: Skipping API key parameter '%s' ('%s') from input schema generation.", param.Name, param.In)
utils.SafeLogPrintf("Parser V3: Skipping API key parameter '%s' ('%s') from input schema generation.", param.Name, param.In)
continue
}

Expand Down Expand Up @@ -442,7 +442,7 @@ func generateToolSetV2(doc *spec.Swagger, cfg *config.Config) (*mcp.ToolSet, err
// Determine Base URL once
baseURL, err := determineBaseURLV2(doc, cfg)
if err != nil {
log.Printf("Warning: Could not determine base URL for V2 spec: %v. Operations might fail if base URL override is not set.", err)
utils.SafeLogPrintf("Warning: Could not determine base URL for V2 spec: %v. Operations might fail if base URL override is not set.", err)
baseURL = "" // Allow proceeding if override is set
}

Expand All @@ -455,7 +455,7 @@ func generateToolSetV2(doc *spec.Swagger, cfg *config.Config) (*mcp.ToolSet, err
if secDef.Type == "apiKey" {
apiKeyName = secDef.Name
apiKeyIn = secDef.In // "query" or "header"
log.Printf("Parser V2: Detected API key from security definition '%s': Name='%s', In='%s'", name, apiKeyName, apiKeyIn)
utils.SafeLogPrintf("Parser V2: Detected API key from security definition '%s': Name='%s', In='%s'", name, apiKeyName, apiKeyIn)
break // Assume only one apiKey definition for simplicity
}
}
Expand Down Expand Up @@ -519,7 +519,7 @@ func generateToolSetV2(doc *spec.Swagger, cfg *config.Config) (*mcp.ToolSet, err
}
} else {
// If body is not an object, represent as 'requestBody'
log.Printf("Warning: V2 request body for %s %s is not an object schema. Representing as 'requestBody' field.", method, rawPath)
utils.SafeLogPrintf("Warning: V2 request body for %s %s is not an object schema. Representing as 'requestBody' field.", method, rawPath)
if parametersSchema.Properties == nil {
parametersSchema.Properties = make(map[string]mcp.Schema)
}
Expand Down Expand Up @@ -629,7 +629,7 @@ func parametersToMCPSchemaAndDetailsV2(params []spec.Parameter, definitions spec
for _, param := range params {
// Skip the API key parameter if it's configured/detected
if apiKeyName != "" && param.Name == apiKeyName && (param.In == "query" || param.In == "header") {
log.Printf("Parser V2: Skipping API key parameter '%s' ('%s') from input schema generation.", param.Name, param.In)
utils.SafeLogPrintf("Parser V2: Skipping API key parameter '%s' ('%s') from input schema generation.", param.Name, param.In)
continue
}

Expand All @@ -643,7 +643,7 @@ func parametersToMCPSchemaAndDetailsV2(params []spec.Parameter, definitions spec
}

if param.In != "query" && param.In != "path" && param.In != "header" && param.In != "formData" {
log.Printf("Parser V2: Skipping unsupported parameter type '%s' for parameter '%s'", param.In, param.Name)
utils.SafeLogPrintf("Parser V2: Skipping unsupported parameter type '%s' for parameter '%s'", param.In, param.Name)
continue
}

Expand Down Expand Up @@ -702,7 +702,7 @@ func parametersToMCPSchemaAndDetailsV2(params []spec.Parameter, definitions spec

} else {
// Body param defined without a schema? Treat as simple string.
log.Printf("Warning: V2 body parameter '%s' defined without a schema. Treating as string.", bodyParam.Name)
utils.SafeLogPrintf("Warning: V2 body parameter '%s' defined without a schema. Treating as string.", bodyParam.Name)
bodySchema.Type = "string"
mcpSchema.Properties[bodyParam.Name] = bodySchema
if bodyParam.Required {
Expand Down
15 changes: 8 additions & 7 deletions pkg/server/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ package server

import (
"fmt"
"log"
"net/http"
"sync"

"github.com/ckanthony/openapi-mcp/pkg/utils"
)

// client holds information about a connected SSE client.
Expand Down Expand Up @@ -36,7 +37,7 @@ func (m *connectionManager) addClient(r *http.Request, w http.ResponseWriter, f
m.clients[r] = newClient
m.mu.Unlock()

log.Printf("Client connected: %s (Total: %d)", r.RemoteAddr, m.getClientCount())
utils.SafeLogPrintf("Client connected: %s (Total: %d)", r.RemoteAddr, m.getClientCount())

// Send initial toolset immediately
go m.sendToolset(newClient) // Send in a goroutine to avoid blocking registration?
Expand All @@ -48,9 +49,9 @@ func (m *connectionManager) removeClient(r *http.Request) {
_, ok := m.clients[r]
if ok {
delete(m.clients, r)
log.Printf("Client disconnected: %s (Total: %d)", r.RemoteAddr, len(m.clients))
utils.SafeLogPrintf("Client disconnected: %s (Total: %d)", r.RemoteAddr, len(m.clients))
} else {
log.Printf("Attempted to remove already disconnected client: %s", r.RemoteAddr)
utils.SafeLogPrintf("Attempted to remove already disconnected client: %s", r.RemoteAddr)
}
m.mu.Unlock()
}
Expand All @@ -68,15 +69,15 @@ func (m *connectionManager) sendToolset(c *client) {
if c == nil {
return
}
log.Printf("Attempting to send toolset to client...")
utils.SafeLogPrintf("Attempting to send toolset to client...")
_, err := fmt.Fprintf(c.writer, "event: tool_set\ndata: %s\n\n", string(m.toolSet))
if err != nil {
// This error often happens if the client disconnected before/during the write
log.Printf("Error sending toolset data to client: %v (client likely disconnected)", err)
utils.SafeLogPrintf("Error sending toolset data to client: %v (client likely disconnected)", err)
// Optionally trigger removal here if possible, though context done in handler is primary mechanism
return
}
// Flush the data
c.flusher.Flush()
log.Println("Sent tool_set event and flushed.")
utils.SafeLogPrintf("Sent tool_set event and flushed.")
}
Loading
Loading