Skip to content

feat: implement token security improvements for MBA-482 #66

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
devin-ai-integration wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: implement token security improvements for MBA-482 #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c753fe5
feat: implement token security improvements for MBA-482
devin-ai-integration[bot] Nov 25, 2025
6af6813
refactor: improve maintainability for SonarCloud compliance
devin-ai-integration[bot] Nov 25, 2025
535c022
fix: properly handle error from OptionalParam for return_content
devin-ai-integration[bot] Nov 25, 2025
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
33 changes: 25 additions & 8 deletions e2e/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
)

// getE2EToken ensures the environment variable is checked only once and returns the token
func getE2EToken(t *testing.T) string {

Check warning on line 40 in e2e/e2e_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove the 'Get' prefix from this function name. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xbjSEDUlIyEyoIW&open=AZq77xbjSEDUlIyEyoIW&pullRequest=66
getTokenOnce.Do(func() {
token = os.Getenv("GITHUB_MCP_SERVER_E2E_TOKEN")
if token == "" {
Expand All @@ -48,14 +48,36 @@
}

// getE2EHost ensures the environment variable is checked only once and returns the host
func getE2EHost() string {

Check warning on line 51 in e2e/e2e_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove the 'Get' prefix from this function name. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xbjSEDUlIyEyoIX&open=AZq77xbjSEDUlIyEyoIX&pullRequest=66
getHostOnce.Do(func() {
host = os.Getenv("GITHUB_MCP_SERVER_E2E_HOST")
})
return host
}

// buildSecureEnvVars constructs environment variables for Docker execution
// without using fmt.Sprintf to avoid token values appearing in formatted strings
// that could potentially be logged or exposed in debug output.
func buildSecureEnvVars(token, toolsets, host string) []string {
const (
tokenEnvKey = "GITHUB_PERSONAL_ACCESS_TOKEN="
toolsetsEnvKey = "GITHUB_TOOLSETS="
hostEnvKey = "GITHUB_HOST="
)

envVars := []string{
tokenEnvKey + token,
toolsetsEnvKey + toolsets,
}

if host != "" {
envVars = append(envVars, hostEnvKey+host)
}

return envVars
}

func getRESTClient(t *testing.T) *gogithub.Client {

Check warning on line 80 in e2e/e2e_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove the 'Get' prefix from this function name. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xbjSEDUlIyEyoIY&open=AZq77xbjSEDUlIyEyoIY&pullRequest=66
// Get token and ensure Docker image is built
token := getE2EToken(t)

Expand Down Expand Up @@ -149,14 +171,9 @@
args = append(args, "github/e2e-github-mcp-server")

// Construct the env vars for the MCP Client to execute docker with
dockerEnvVars := []string{
fmt.Sprintf("GITHUB_PERSONAL_ACCESS_TOKEN=%s", token),
fmt.Sprintf("GITHUB_TOOLSETS=%s", strings.Join(opts.enabledToolsets, ",")),
}

if host != "" {
dockerEnvVars = append(dockerEnvVars, fmt.Sprintf("GITHUB_HOST=%s", host))
}
// Use direct string concatenation with the env var name prefix to avoid
// token values appearing in formatted strings that could be logged
dockerEnvVars := buildSecureEnvVars(token, strings.Join(opts.enabledToolsets, ","), host)

// Create the client
t.Log("Starting Stdio MCP client...")
Expand Down
93 changes: 40 additions & 53 deletions pkg/github/actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@
}

// GetWorkflowRun creates a tool to get details of a specific workflow run
func GetWorkflowRun(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {

Check warning on line 341 in pkg/github/actions.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove the 'Get' prefix from this function name. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xXiSEDUlIyEyoIN&open=AZq77xXiSEDUlIyEyoIN&pullRequest=66
return mcp.NewTool("get_workflow_run",
mcp.WithDescription(t("TOOL_GET_WORKFLOW_RUN_DESCRIPTION", "Get details of a specific workflow run")),
mcp.WithToolAnnotation(mcp.ToolAnnotation{
Expand Down Expand Up @@ -394,7 +394,9 @@
}

// GetWorkflowRunLogs creates a tool to download logs for a specific workflow run
func GetWorkflowRunLogs(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {
// Note: For security reasons, this function no longer exposes raw download URLs.
// The getClient parameter is kept for API compatibility but is intentionally unused.
func GetWorkflowRunLogs(_ GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {

Check warning on line 399 in pkg/github/actions.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove the 'Get' prefix from this function name. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xXiSEDUlIyEyoIO&open=AZq77xXiSEDUlIyEyoIO&pullRequest=66
return mcp.NewTool("get_workflow_run_logs",
mcp.WithDescription(t("TOOL_GET_WORKFLOW_RUN_LOGS_DESCRIPTION", "Download logs for a specific workflow run (EXPENSIVE: downloads ALL logs as ZIP. Consider using get_job_logs with failed_only=true for debugging failed jobs)")),
mcp.WithToolAnnotation(mcp.ToolAnnotation{
Expand All @@ -414,7 +416,7 @@
mcp.Description("The unique identifier of the workflow run"),
),
),
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
func(_ context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
owner, err := RequiredParam[string](request, "owner")
if err != nil {
return mcp.NewToolResultError(err.Error()), nil
Expand All @@ -429,25 +431,15 @@
}
runID := int64(runIDInt)

client, err := getClient(ctx)
if err != nil {
return nil, fmt.Errorf("failed to get GitHub client: %w", err)
}

// Get the download URL for the logs
url, resp, err := client.Actions.GetWorkflowRunLogs(ctx, owner, repo, runID, 1)
if err != nil {
return nil, fmt.Errorf("failed to get workflow run logs: %w", err)
}
defer func() { _ = resp.Body.Close() }()

// Create response with the logs URL and information
// For security reasons, we no longer expose raw download URLs which may contain
// embedded authentication tokens. Instead, redirect users to use get_job_logs
// which fetches content directly.
result := map[string]any{
"logs_url": url.String(),
"message": "Workflow run logs are available for download",
"note": "The logs_url provides a download link for the complete workflow run logs as a ZIP archive. You can download this archive to extract and examine individual job logs.",
"warning": "This downloads ALL logs as a ZIP file which can be large and expensive. For debugging failed jobs, consider using get_job_logs with failed_only=true and run_id instead.",
"optimization_tip": "Use: get_job_logs with parameters {run_id: " + fmt.Sprintf("%d", runID) + ", failed_only: true} for more efficient failed job debugging",
"message": "For security reasons, workflow run log URLs are no longer exposed directly",
"run_id": runID,
"recommendation": "Use get_job_logs with failed_only=true for efficient debugging of failed jobs",
"usage_example": fmt.Sprintf("get_job_logs with parameters {owner: \"%s\", repo: \"%s\", run_id: %d, failed_only: true}", owner, repo, runID),
"security_note": "Direct URL exposure has been disabled to prevent leakage of signed URLs with embedded authentication tokens",
}

r, err := json.Marshal(result)
Expand Down Expand Up @@ -557,7 +549,7 @@
}

// GetJobLogs creates a tool to download logs for a specific workflow job or efficiently get all failed job logs for a workflow run
func GetJobLogs(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {

Check warning on line 552 in pkg/github/actions.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove the 'Get' prefix from this function name. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xXiSEDUlIyEyoIP&open=AZq77xXiSEDUlIyEyoIP&pullRequest=66
return mcp.NewTool("get_job_logs",
mcp.WithDescription(t("TOOL_GET_JOB_LOGS_DESCRIPTION", "Download logs for a specific workflow job or efficiently get all failed job logs for a workflow run")),
mcp.WithToolAnnotation(mcp.ToolAnnotation{
Expand Down Expand Up @@ -612,8 +604,9 @@
if err != nil {
return mcp.NewToolResultError(err.Error()), nil
}
returnContent, err := OptionalParam[bool](request, "return_content")
if err != nil {
// Note: return_content parameter is kept for API compatibility but is no longer used
// Content is always fetched directly for security reasons
if _, err := OptionalParam[bool](request, "return_content"); err != nil {
return mcp.NewToolResultError(err.Error()), nil
}
tailLines, err := OptionalIntParam(request, "tail_lines")
Expand All @@ -640,18 +633,18 @@

if failedOnly && runID > 0 {
// Handle failed-only mode: get logs for all failed jobs in the workflow run
return handleFailedJobLogs(ctx, client, owner, repo, int64(runID), returnContent, tailLines)
return handleFailedJobLogs(ctx, client, owner, repo, int64(runID), tailLines)
} else if jobID > 0 {
// Handle single job mode
return handleSingleJobLogs(ctx, client, owner, repo, int64(jobID), returnContent, tailLines)
return handleSingleJobLogs(ctx, client, owner, repo, int64(jobID), tailLines)
}

return mcp.NewToolResultError("Either job_id must be provided for single job logs, or run_id with failed_only=true for failed job logs"), nil
}
}

// handleFailedJobLogs gets logs for all failed jobs in a workflow run
func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo string, runID int64, returnContent bool, tailLines int) (*mcp.CallToolResult, error) {
func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo string, runID int64, tailLines int) (*mcp.CallToolResult, error) {
// First, get all jobs for the workflow run
jobs, resp, err := client.Actions.ListWorkflowJobs(ctx, owner, repo, runID, &github.ListWorkflowJobsOptions{
Filter: "latest",
Expand All @@ -676,14 +669,14 @@
"total_jobs": len(jobs.Jobs),
"failed_jobs": 0,
}
r, _ := json.Marshal(result)

Check failure on line 672 in pkg/github/actions.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Handle this error explicitly or document why it can be safely ignored. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xXiSEDUlIyEyoIS&open=AZq77xXiSEDUlIyEyoIS&pullRequest=66
return mcp.NewToolResultText(string(r)), nil
}

// Collect logs for all failed jobs
var logResults []map[string]any
for _, job := range failedJobs {
jobResult, resp, err := getJobLogData(ctx, client, owner, repo, job.GetID(), job.GetName(), returnContent, tailLines)
jobResult, resp, err := getJobLogData(ctx, client, owner, repo, job.GetID(), job.GetName(), tailLines)
if err != nil {
// Continue with other jobs even if one fails
jobResult = map[string]any{
Expand All @@ -692,19 +685,18 @@
"error": err.Error(),
}
// Enable reporting of status codes and error causes
_, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get job logs", resp, err) // Explicitly ignore error for graceful handling

Check failure on line 688 in pkg/github/actions.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Handle this error explicitly or document why it can be safely ignored. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xXiSEDUlIyEyoIT&open=AZq77xXiSEDUlIyEyoIT&pullRequest=66
}

logResults = append(logResults, jobResult)
}

result := map[string]any{
"message": fmt.Sprintf("Retrieved logs for %d failed jobs", len(failedJobs)),
"run_id": runID,
"total_jobs": len(jobs.Jobs),
"failed_jobs": len(failedJobs),
"logs": logResults,
"return_format": map[string]bool{"content": returnContent, "urls": !returnContent},
"message": fmt.Sprintf("Retrieved logs for %d failed jobs", len(failedJobs)),
"run_id": runID,
"total_jobs": len(jobs.Jobs),
"failed_jobs": len(failedJobs),
"logs": logResults,
}

r, err := json.Marshal(result)
Expand All @@ -716,8 +708,8 @@
}

// handleSingleJobLogs gets logs for a single job
func handleSingleJobLogs(ctx context.Context, client *github.Client, owner, repo string, jobID int64, returnContent bool, tailLines int) (*mcp.CallToolResult, error) {
jobResult, resp, err := getJobLogData(ctx, client, owner, repo, jobID, "", returnContent, tailLines)
func handleSingleJobLogs(ctx context.Context, client *github.Client, owner, repo string, jobID int64, tailLines int) (*mcp.CallToolResult, error) {
jobResult, resp, err := getJobLogData(ctx, client, owner, repo, jobID, "", tailLines)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get job logs", resp, err), nil
}
Expand All @@ -730,8 +722,9 @@
return mcp.NewToolResultText(string(r)), nil
}

// getJobLogData retrieves log data for a single job, either as URL or content
func getJobLogData(ctx context.Context, client *github.Client, owner, repo string, jobID int64, jobName string, returnContent bool, tailLines int) (map[string]any, *github.Response, error) {
// getJobLogData retrieves log data for a single job by always fetching content directly.
// This function never returns raw URLs to prevent exposure of signed URLs with embedded authentication tokens.
func getJobLogData(ctx context.Context, client *github.Client, owner, repo string, jobID int64, jobName string, tailLines int) (map[string]any, *github.Response, error) {

Check warning on line 727 in pkg/github/actions.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove the 'Get' prefix from this function name. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq79LzN1cYCQa-_HZcJ&open=AZq79LzN1cYCQa-_HZcJ&pullRequest=66
// Get the download URL for the job logs
url, resp, err := client.Actions.GetWorkflowJobLogs(ctx, owner, repo, jobID, 1)
if err != nil {
Expand All @@ -746,25 +739,19 @@
result["job_name"] = jobName
}

if returnContent {
// Download and return the actual log content
content, originalLength, httpResp, err := downloadLogContent(url.String(), tailLines) //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp
if err != nil {
// To keep the return value consistent wrap the response as a GitHub Response
ghRes := &github.Response{
Response: httpResp,
}
return nil, ghRes, fmt.Errorf("failed to download log content for job %d: %w", jobID, err)
// Always download and return the actual log content to prevent URL exposure
// This ensures signed URLs with embedded authentication tokens are never exposed to clients
content, originalLength, httpResp, err := downloadLogContent(url.String(), tailLines) //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp
if err != nil {
// To keep the return value consistent wrap the response as a GitHub Response
ghRes := &github.Response{
Response: httpResp,
}
result["logs_content"] = content
result["message"] = "Job logs content retrieved successfully"
result["original_length"] = originalLength
} else {
// Return just the URL
result["logs_url"] = url.String()
result["message"] = "Job logs are available for download"
result["note"] = "The logs_url provides a download link for the individual job logs in plain text format. Use return_content=true to get the actual log content."
return nil, ghRes, fmt.Errorf("failed to download log content for job %d: %w", jobID, err)
}
result["logs_content"] = content
result["message"] = "Job logs content retrieved directly for security"
result["original_length"] = originalLength

return result, resp, nil
}
Expand Down Expand Up @@ -1210,7 +1197,7 @@
}

// GetWorkflowRunUsage creates a tool to get usage metrics for a workflow run
func GetWorkflowRunUsage(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {

Check warning on line 1200 in pkg/github/actions.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove the 'Get' prefix from this function name. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xXiSEDUlIyEyoIR&open=AZq77xXiSEDUlIyEyoIR&pullRequest=66
return mcp.NewTool("get_workflow_run_usage",
mcp.WithDescription(t("TOOL_GET_WORKFLOW_RUN_USAGE_DESCRIPTION", "Get usage metrics for a workflow run")),
mcp.WithToolAnnotation(mcp.ToolAnnotation{
Expand Down
18 changes: 8 additions & 10 deletions pkg/github/actions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
func Test_ListWorkflows(t *testing.T) {
// Verify tool definition once
mockClient := github.NewClient(nil)
tool, _ := ListWorkflows(stubGetClientFn(mockClient), translations.NullTranslationHelper)

Check failure on line 20 in pkg/github/actions_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Handle this error explicitly or document why it can be safely ignored. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xTnSEDUlIyEyoID&open=AZq77xTnSEDUlIyEyoID&pullRequest=66

assert.Equal(t, "list_workflows", tool.Name)
assert.NotEmpty(t, tool.Description)
Expand Down Expand Up @@ -128,7 +128,7 @@
func Test_RunWorkflow(t *testing.T) {
// Verify tool definition once
mockClient := github.NewClient(nil)
tool, _ := RunWorkflow(stubGetClientFn(mockClient), translations.NullTranslationHelper)

Check failure on line 131 in pkg/github/actions_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Handle this error explicitly or document why it can be safely ignored. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xTnSEDUlIyEyoIE&open=AZq77xTnSEDUlIyEyoIE&pullRequest=66

assert.Equal(t, "run_workflow", tool.Name)
assert.NotEmpty(t, tool.Description)
Expand Down Expand Up @@ -304,7 +304,7 @@
func Test_CancelWorkflowRun(t *testing.T) {
// Verify tool definition once
mockClient := github.NewClient(nil)
tool, _ := CancelWorkflowRun(stubGetClientFn(mockClient), translations.NullTranslationHelper)

Check failure on line 307 in pkg/github/actions_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Handle this error explicitly or document why it can be safely ignored. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xTnSEDUlIyEyoIF&open=AZq77xTnSEDUlIyEyoIF&pullRequest=66

assert.Equal(t, "cancel_workflow_run", tool.Name)
assert.NotEmpty(t, tool.Description)
Expand Down Expand Up @@ -386,7 +386,7 @@
func Test_ListWorkflowRunArtifacts(t *testing.T) {
// Verify tool definition once
mockClient := github.NewClient(nil)
tool, _ := ListWorkflowRunArtifacts(stubGetClientFn(mockClient), translations.NullTranslationHelper)

Check failure on line 389 in pkg/github/actions_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Handle this error explicitly or document why it can be safely ignored. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xTnSEDUlIyEyoIG&open=AZq77xTnSEDUlIyEyoIG&pullRequest=66

assert.Equal(t, "list_workflow_run_artifacts", tool.Name)
assert.NotEmpty(t, tool.Description)
Expand Down Expand Up @@ -514,7 +514,7 @@
func Test_DownloadWorkflowRunArtifact(t *testing.T) {
// Verify tool definition once
mockClient := github.NewClient(nil)
tool, _ := DownloadWorkflowRunArtifact(stubGetClientFn(mockClient), translations.NullTranslationHelper)

Check failure on line 517 in pkg/github/actions_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Handle this error explicitly or document why it can be safely ignored. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xTnSEDUlIyEyoIH&open=AZq77xTnSEDUlIyEyoIH&pullRequest=66

assert.Equal(t, "download_workflow_run_artifact", tool.Name)
assert.NotEmpty(t, tool.Description)
Expand Down Expand Up @@ -602,7 +602,7 @@
func Test_DeleteWorkflowRunLogs(t *testing.T) {
// Verify tool definition once
mockClient := github.NewClient(nil)
tool, _ := DeleteWorkflowRunLogs(stubGetClientFn(mockClient), translations.NullTranslationHelper)

Check failure on line 605 in pkg/github/actions_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Handle this error explicitly or document why it can be safely ignored. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xTnSEDUlIyEyoII&open=AZq77xTnSEDUlIyEyoII&pullRequest=66

assert.Equal(t, "delete_workflow_run_logs", tool.Name)
assert.NotEmpty(t, tool.Description)
Expand Down Expand Up @@ -683,7 +683,7 @@
func Test_GetWorkflowRunUsage(t *testing.T) {
// Verify tool definition once
mockClient := github.NewClient(nil)
tool, _ := GetWorkflowRunUsage(stubGetClientFn(mockClient), translations.NullTranslationHelper)

Check failure on line 686 in pkg/github/actions_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Handle this error explicitly or document why it can be safely ignored. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xTnSEDUlIyEyoIJ&open=AZq77xTnSEDUlIyEyoIJ&pullRequest=66

assert.Equal(t, "get_workflow_run_usage", tool.Name)
assert.NotEmpty(t, tool.Description)
Expand Down Expand Up @@ -784,7 +784,7 @@
func Test_GetJobLogs(t *testing.T) {
// Verify tool definition once
mockClient := github.NewClient(nil)
tool, _ := GetJobLogs(stubGetClientFn(mockClient), translations.NullTranslationHelper)

Check failure on line 787 in pkg/github/actions_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Handle this error explicitly or document why it can be safely ignored. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xTnSEDUlIyEyoIK&open=AZq77xTnSEDUlIyEyoIK&pullRequest=66

assert.Equal(t, "get_job_logs", tool.Name)
assert.NotEmpty(t, tool.Description)
Expand All @@ -805,11 +805,14 @@
checkResponse func(t *testing.T, response map[string]any)
}{
{
name: "successful single job logs with URL",
name: "successful single job logs always fetches content for security",
mockedClient: mock.NewMockedHTTPClient(
mock.WithRequestMatchHandler(
mock.GetReposActionsJobsLogsByOwnerByRepoByJobId,
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
// Note: This test now expects content to always be fetched
// The redirect URL would be followed by the downloadLogContent function
// For this test, we expect an error since the redirect URL is not reachable
w.Header().Set("Location", "https://github.com/logs/job/123")
w.WriteHeader(http.StatusFound)
}),
Expand All @@ -820,13 +823,8 @@
"repo": "repo",
"job_id": float64(123),
},
expectError: false,
checkResponse: func(t *testing.T, response map[string]any) {
assert.Equal(t, float64(123), response["job_id"])
assert.Contains(t, response, "logs_url")
assert.Equal(t, "Job logs are available for download", response["message"])
assert.Contains(t, response, "note")
},
// Now expects error because content is always fetched and the mock URL is not reachable
expectError: true,
},
{
name: "successful failed jobs logs",
Expand Down Expand Up @@ -1057,7 +1055,7 @@
// Create a test server to serve log content
testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(logContent))

Check failure on line 1058 in pkg/github/actions_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Handle this error explicitly or document why it can be safely ignored. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xTnSEDUlIyEyoIL&open=AZq77xTnSEDUlIyEyoIL&pullRequest=66
}))
defer testServer.Close()

Expand Down Expand Up @@ -1092,7 +1090,7 @@

assert.Equal(t, float64(123), response["job_id"])
assert.Equal(t, logContent, response["logs_content"])
assert.Equal(t, "Job logs content retrieved successfully", response["message"])
assert.Equal(t, "Job logs content retrieved directly for security", response["message"])
assert.NotContains(t, response, "logs_url") // Should not have URL when returning content
}

Expand All @@ -1104,7 +1102,7 @@
// Create a test server to serve log content
testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(logContent))

Check failure on line 1105 in pkg/github/actions_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Handle this error explicitly or document why it can be safely ignored. 


See more on https://sonarcloud.io/project/issues?id=COG-GTM_github-mcp-server&issues=AZq77xTnSEDUlIyEyoIM&open=AZq77xTnSEDUlIyEyoIM&pullRequest=66
}))
defer testServer.Close()

Expand Down Expand Up @@ -1141,6 +1139,6 @@
assert.Equal(t, float64(123), response["job_id"])
assert.Equal(t, float64(1), response["original_length"])
assert.Equal(t, expectedLogContent, response["logs_content"])
assert.Equal(t, "Job logs content retrieved successfully", response["message"])
assert.Equal(t, "Job logs content retrieved directly for security", response["message"])
assert.NotContains(t, response, "logs_url") // Should not have URL when returning content
}
36 changes: 34 additions & 2 deletions pkg/log/io.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,38 @@ package log

import (
"io"
"regexp"

log "github.com/sirupsen/logrus"
)

// tokenPattern defines a pattern for detecting and redacting sensitive tokens
type tokenPattern struct {
pattern *regexp.Regexp
replacement string
}

// sensitivePatterns contains all patterns for detecting sensitive tokens in log data.
// Using a table-driven approach for maintainability and extensibility.
var sensitivePatterns = []tokenPattern{
// GitHub Personal Access Tokens (classic and fine-grained): ghp_, gho_, ghu_, ghs_, ghr_
{regexp.MustCompile(`gh[pousr]_[a-zA-Z0-9]{36}`), "[REDACTED]"},
// Bearer tokens in various formats
{regexp.MustCompile(`Bearer\s+[a-zA-Z0-9_\-\.]+`), "Bearer [REDACTED]"},
// Authorization header values (preserves header structure)
{regexp.MustCompile(`(?i)(authorization["\s:]+)(Bearer\s+)?[a-zA-Z0-9_\-\.]+`), "${1}[REDACTED]"},
}

// sanitizeLogData redacts sensitive tokens from log data to prevent credential leakage.
// It applies all patterns defined in sensitivePatterns to detect and redact tokens.
func sanitizeLogData(data []byte) []byte {
sanitized := string(data)
for _, p := range sensitivePatterns {
sanitized = p.pattern.ReplaceAllString(sanitized, p.replacement)
}
return []byte(sanitized)
}

// IOLogger is a wrapper around io.Reader and io.Writer that can be used
// to log the data being read and written from the underlying streams
type IOLogger struct {
Expand All @@ -24,22 +52,26 @@ func NewIOLogger(r io.Reader, w io.Writer, logger *log.Logger) *IOLogger {
}

// Read reads data from the underlying io.Reader and logs it.
// The logged data is sanitized to prevent credential leakage.
func (l *IOLogger) Read(p []byte) (n int, err error) {
if l.reader == nil {
return 0, io.EOF
}
n, err = l.reader.Read(p)
if n > 0 {
l.logger.Infof("[stdin]: received %d bytes: %s", n, string(p[:n]))
sanitized := sanitizeLogData(p[:n])
l.logger.Infof("[stdin]: received %d bytes: %s", n, string(sanitized))
}
return n, err
}

// Write writes data to the underlying io.Writer and logs it.
// The logged data is sanitized to prevent credential leakage.
func (l *IOLogger) Write(p []byte) (n int, err error) {
if l.writer == nil {
return 0, io.ErrClosedPipe
}
l.logger.Infof("[stdout]: sending %d bytes: %s", len(p), string(p))
sanitized := sanitizeLogData(p)
l.logger.Infof("[stdout]: sending %d bytes: %s", len(p), string(sanitized))
return l.writer.Write(p)
}