Skip to content

Fix invalid JSON escapes in GitHub remote MCP gateway config generation #41864

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

Merged
pelikhan merged 2 commits into from
Jun 27, 2026
Merged

Fix invalid JSON escapes in GitHub remote MCP gateway config generation #41864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08c4cbb
Initial plan
Copilot Jun 27, 2026
ba71ed7
Apply remaining changes
Copilot Jun 27, 2026
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
8 changes: 4 additions & 4 deletions .github/workflows/github-remote-mcp-auth-test.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

44 changes: 43 additions & 1 deletion actions/setup/js/start_mcp_gateway.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,38 @@ function sleep(ms) {
return new Promise(resolve => setTimeout(resolve, ms));
}

/**
* Builds targeted context for a JSON.parse error so logs can point at the likely key.
*
* @param {string} jsonText
* @param {string} parseErrorMessage
* @returns {{line: number, column: number, lineText: string, key: string | null} | null}
*/
function getJSONParseErrorContext(jsonText, parseErrorMessage) {
const posMatch = parseErrorMessage.match(/position\s+(\d+)/i);
if (!posMatch) {
return null;
}

const pos = Number(posMatch[1]);
if (!Number.isFinite(pos) || pos < 0) {
return null;
}

const safePos = Math.min(pos, Math.max(0, jsonText.length - 1));

@github-actions github-actions Bot Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] safePos clips pos to jsonText.length - 1 when the parse error position equals the string length (EOF). This shifts the reported column one character back, which could mislead debugging for truncated or unterminated-string errors. A brief comment explaining the intent would help future readers.

💡 Suggested clarification 
// Clip to last valid index; pos === length (EOF) is safe to round down by one.
const safePos = Math.min(pos, Math.max(0, jsonText.length - 1));

Alternatively, special-case pos >= length to report line: <last line>, column: <EOL+1> for a more accurate EOF diagnosis.

@copilot please address this.

const before = jsonText.slice(0, safePos);
const line = before.split("\n").length;
const lineStart = before.lastIndexOf("\n") + 1;
const lineEnd = jsonText.indexOf("\n", safePos);
const resolvedLineEnd = lineEnd === -1 ? jsonText.length : lineEnd;
const lineText = jsonText.slice(lineStart, resolvedLineEnd);
const column = safePos - lineStart + 1;
const keyMatch = lineText.match(/"([^"]+)"\s*:/);

@github-actions github-actions Bot Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] keyMatch scans all of lineText for the first "key": pattern, regardless of where in the line the error pos falls. For prettified JSON (one key per line) this works fine, but for minified or compacted JSON it could report a key that precedes the actual error position.

💡 Improvement suggestion

Consider searching only within the substring up to column so the match is anchored to the error site:

const keyMatch = lineText.slice(0, column).match(/"([^"]+)"\s*:\s*$/)
             || lineText.match(/"([^"]+)"\s*:/);

The fallback preserves the existing behaviour when no key appears before the error column (e.g., the error is inside a key name itself).

@copilot please address this.

const key = keyMatch ? keyMatch[1] : null;

return { line, column, lineText, key };
}

/**
* Normalizes GH_AW_OTLP_IF_MISSING to a supported mode.
* @param {string | undefined} value
Expand Down Expand Up @@ -367,7 +399,16 @@ async function main() {
core.error("ERROR: Configuration is not valid JSON");
core.error("");
core.error("JSON validation error:");
core.error(/** @type {Error} */ err.message);
const parseMessage = /** @type {Error} */ err.message;
core.error(parseMessage);
const parseContext = getJSONParseErrorContext(mcpConfig, parseMessage);
if (parseContext) {
core.error(`Likely offending location: line ${parseContext.line}, column ${parseContext.column}`);
if (parseContext.key) {
core.error(`Likely offending key: ${parseContext.key}`);
}
core.error(`Context line: ${parseContext.lineText}`);
}
core.error("");
core.error("Configuration content:");
const lines = mcpConfig.split("\n");
Expand Down Expand Up @@ -909,5 +950,6 @@ module.exports = {
getOTLPIfMissingMode,
hasNonEmptyOTLPHeaders,
isOTLPIfMissingIgnore,
getJSONParseErrorContext,
resolveCopilotConfigPaths,
};
26 changes: 25 additions & 1 deletion actions/setup/js/start_mcp_gateway.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { applyOTLPIgnoreIfMissing, detectEngineType, getOTLPIfMissingMode, hasNonEmptyOTLPHeaders, resolveCopilotConfigPaths } from "./start_mcp_gateway.cjs";
import { applyOTLPIgnoreIfMissing, detectEngineType, getJSONParseErrorContext, getOTLPIfMissingMode, hasNonEmptyOTLPHeaders, resolveCopilotConfigPaths } from "./start_mcp_gateway.cjs";

describe("start_mcp_gateway OTLP if-missing helpers", () => {
let originalWarning;
Expand Down Expand Up @@ -197,3 +197,27 @@ describe("start_mcp_gateway detectEngineType", () => {
expect(detectEngineType(configDir, env, existsSync)).toBe("copilot");
});
});

describe("start_mcp_gateway getJSONParseErrorContext", () => {
it("extracts line/column and key for invalid escape values", () => {
const invalidConfig = `{
"mcpServers": {
"github": {
"env": {
"GITHUB_HOST": "\\https://github.com"
}
}
}
}`;
let parseErrorMessage = "";
try {
JSON.parse(invalidConfig);
} catch (err) {
parseErrorMessage = /** @type {Error} */ err.message;
}
const context = getJSONParseErrorContext(invalidConfig, parseErrorMessage);
expect(context).toBeTruthy();
expect(context?.key).toBe("GITHUB_HOST");
expect(context?.lineText).toContain(`"GITHUB_HOST"`);

@github-actions github-actions Bot Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The assertions only verify key and lineText — line and column are part of the returned contract but are unchecked, so regressions in position arithmetic would silently pass.

💡 Suggested additions 
expect(context?.line).toBe(5);   // "GITHUB_HOST" is on line 5 of invalidConfig
expect(context?.column).toBeGreaterThan(0);

Pinning at least line turns this into a real specification and catches off-by-one regressions in getJSONParseErrorContext.

@copilot please address this.

});
});

@github-actions github-actions Bot Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] No test covers the null return path — when parseErrorMessage contains no "position N" substring the function silently returns null, but that branch is never exercised.

💡 Suggested test 
it("returns null when error message has no position info", () => {
  const context = getJSONParseErrorContext('{"x":1}', 'Unexpected token');
  expect(context).toBeNull();
});

This matters because different JS engines (or future Node.js versions) may not include "position N" in their error messages, and the calling code in main() already guards on if (parseContext) — so the null path is a real production path that should be tested.

@copilot please address this.

12 changes: 6 additions & 6 deletions pkg/workflow/github_remote_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ func TestRenderGitHubMCPRemoteConfig(t *testing.T) {
`"list_issues"`,
`"create_issue"`,
`"env": {`,
`"GITHUB_PERSONAL_ACCESS_TOKEN": "\\${GITHUB_MCP_SERVER_TOKEN}"`,
`"GITHUB_HOST": "\\${GITHUB_SERVER_URL}"`,
`"GITHUB_PERSONAL_ACCESS_TOKEN": "${GITHUB_MCP_SERVER_TOKEN}"`,
`"GITHUB_HOST": "${GITHUB_SERVER_URL}"`,
},
notExpected: []string{
`"X-MCP-Readonly"`,
Expand All @@ -104,8 +104,8 @@ func TestRenderGitHubMCPRemoteConfig(t *testing.T) {
`"Authorization": "Bearer \\${GITHUB_PERSONAL_ACCESS_TOKEN}"`,
`"X-MCP-Toolsets": "all"`,
`"env": {`,
`"GITHUB_PERSONAL_ACCESS_TOKEN": "\\${GITHUB_MCP_SERVER_TOKEN}"`,
`"GITHUB_HOST": "\\${GITHUB_SERVER_URL}"`,
`"GITHUB_PERSONAL_ACCESS_TOKEN": "${GITHUB_MCP_SERVER_TOKEN}"`,
`"GITHUB_HOST": "${GITHUB_SERVER_URL}"`,
},
notExpected: []string{
`"X-MCP-Readonly"`,
Expand All @@ -132,8 +132,8 @@ func TestRenderGitHubMCPRemoteConfig(t *testing.T) {
`"list_repositories"`,
`"get_repository"`,
`"env": {`,
`"GITHUB_PERSONAL_ACCESS_TOKEN": "\\${GITHUB_MCP_SERVER_TOKEN}"`,
`"GITHUB_HOST": "\\${GITHUB_SERVER_URL}"`,
`"GITHUB_PERSONAL_ACCESS_TOKEN": "${GITHUB_MCP_SERVER_TOKEN}"`,
`"GITHUB_HOST": "${GITHUB_SERVER_URL}"`,
},
notExpected: []string{},
},
Expand Down
4 changes: 2 additions & 2 deletions pkg/workflow/github_remote_mode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ This is a test workflow for GitHub remote mode configuration.
if !strings.Contains(lockContent, `"Authorization": "Bearer \\${GITHUB_PERSONAL_ACCESS_TOKEN}"`) {
t.Errorf("Expected Authorization header with ${GITHUB_PERSONAL_ACCESS_TOKEN} syntax but didn't find it in:\n%s", lockContent)
}
if !strings.Contains(lockContent, `"GITHUB_PERSONAL_ACCESS_TOKEN": "\\${GITHUB_MCP_SERVER_TOKEN}"`) {
if !strings.Contains(lockContent, `"GITHUB_PERSONAL_ACCESS_TOKEN": "${GITHUB_MCP_SERVER_TOKEN}"`) {
t.Errorf("Expected env section with GITHUB_PERSONAL_ACCESS_TOKEN passthrough but didn't find it in:\n%s", lockContent)
}
} else {
Expand Down Expand Up @@ -460,7 +460,7 @@ This tests that GITHUB_PERSONAL_ACCESS_TOKEN is exported and passed to Docker.
}

// Check that the env section still defines the variable
if !strings.Contains(lockContent, `"GITHUB_PERSONAL_ACCESS_TOKEN": "\\${GITHUB_MCP_SERVER_TOKEN}"`) {
if !strings.Contains(lockContent, `"GITHUB_PERSONAL_ACCESS_TOKEN": "${GITHUB_MCP_SERVER_TOKEN}"`) {
t.Errorf("Expected env section with GITHUB_PERSONAL_ACCESS_TOKEN but didn't find it in lock file")
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/workflow/mcp_renderer_github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ func RenderGitHubMCPRemoteConfig(yaml *strings.Builder, options GitHubMCPRemoteO
yaml,
" ",
"env",
buildGitHubMCPEnvVars("\\${GITHUB_MCP_SERVER_TOKEN}", "\\${GITHUB_SERVER_URL}", false, false, ""),
buildGitHubMCPEnvVars("${GITHUB_MCP_SERVER_TOKEN}", "${GITHUB_SERVER_URL}", false, false, ""),
hasGuardPolicies,
)
}
Expand Down
Loading