fix: harden OAuth and configuration handling - #71
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the logging system to structured logging using log/slog, introduces a comprehensive configuration validation mechanism with a new -check-config CLI flag, and enhances the security and robustness of the OAuth authorization flow and token storage. Additionally, extensive unit tests have been added to verify these changes. The review feedback highlights two important issues: a potential race condition where the OAuth callback server might close before flushing the 'Authorization received' response to the browser, and a cross-platform compatibility issue on Windows where reserved device names (like CON or PRN) are not properly filtered out when generating safe token filenames.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| select { | ||
| case callbackChan <- params: | ||
| w.Header().Set("Content-Type", "text/html; charset=utf-8") | ||
| _, _ = w.Write([]byte(`<html><body><h1>Authorization received</h1><p>You can close this window and return to the terminal.</p></body></html>`)) | ||
| default: |
There was a problem hiding this comment.
There is a race condition where the local OAuth callback server is closed immediately after the authorization code is sent to callbackChan. Because authorizeInteractively exits and calls srv.Close() (which immediately aborts all active connections), the browser may receive a connection reset error instead of rendering the 'Authorization received' HTML page. Adding a brief sleep (e.g., 100ms) after writing the response ensures the TCP stack has enough time to transmit the HTML content to the browser before the server is closed.
\t\tselect {\n\t\tcase callbackChan <- params:\n\t\t\tw.Header().Set(\"Content-Type\", \"text/html; charset=utf-8\")\n\t\t\t_, _ = w.Write([]byte(\"<html><body><h1>Authorization received</h1><p>You can close this window and return to the terminal.</p></body></html>\"))\n\t\t\t// A brief sleep ensures the HTTP response is fully flushed and transmitted to the browser before srv.Close() is called.\n\t\t\ttime.Sleep(100 * time.Millisecond)\n\t\tdefault:| if serverName != "." && serverName != ".." && len(serverName) <= 128 { | ||
| safe := true | ||
| for _, r := range serverName { | ||
| if (r < 'a' || r > 'z') && (r < 'A' || r > 'Z') && (r < '0' || r > '9') && r != '.' && r != '_' && r != '-' { | ||
| safe = false | ||
| break | ||
| } | ||
| } | ||
| if safe { | ||
| return serverName, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Windows reserved device names (such as CON, PRN, AUX, NUL, COM1-9, LPT1-9) are restricted by the operating system and cannot be used as filenames, even with extensions (e.g., con.json or aux.json). If a user configures an MCP server with one of these names, writing the token file will fail or behave unexpectedly on Windows. Adding a check to treat these reserved names as unsafe (and thus hashing them) ensures full cross-platform compatibility.
\tif serverName != \".\" && serverName != \"..\" && len(serverName) <= 128 {\n\t\tisReserved := false\n\t\tif len(serverName) == 3 {\n\t\t\tc1, c2, c3 := serverName[0], serverName[1], serverName[2]\n\t\t\tisCON := (c1 == 'c' || c1 == 'C') && (c2 == 'o' || c2 == 'O') && (c3 == 'n' || c3 == 'N')\n\t\t\tisPRN := (c1 == 'p' || c1 == 'P') && (c2 == 'r' || c2 == 'R') && (c3 == 'n' || c3 == 'N')\n\t\t\tisAUX := (c1 == 'a' || c1 == 'A') && (c2 == 'u' || c2 == 'U') && (c3 == 'x' || c3 == 'X')\n\t\t\tisNUL := (c1 == 'n' || c1 == 'N') && (c2 == 'u' || c2 == 'U') && (c3 == 'l' || c3 == 'L')\n\t\t\tif isCON || isPRN || isAUX || isNUL {\n\t\t\t\tisReserved = true\n\t\t\t}\n\t\t} else if len(serverName) == 4 {\n\t\t\tc1, c2, c3, c4 := serverName[0], serverName[1], serverName[2], serverName[3]\n\t\t\tisCOM := (c1 == 'c' || c1 == 'C') && (c2 == 'o' || c2 == 'O') && (c3 == 'm' || c3 == 'M')\n\t\t\tisLPT := (c1 == 'l' || c1 == 'L') && (c2 == 'p' || c2 == 'P') && (c3 == 't' || c3 == 'T')\n\t\t\tif (isCOM || isLPT) && c4 >= '1' && c4 <= '9' {\n\t\t\t\tisReserved = true\n\t\t\t}\n\t\t}\n\t\tif !isReserved {\n\t\t\tsafe := true\n\t\t\tfor _, r := range serverName {\n\t\t\t\tif (r < 'a' || r > 'z') && (r < 'A' || r > 'Z') && (r < '0' || r > '9') && r != '.' && r != '_' && r != '-' {\n\t\t\t\t\tsafe = false\n\t\t\t\t\tbreak\n\t\t\t\t}\n\t\t\t}\n\t\t\tif safe {\n\t\t\t\treturn serverName, nil\n\t\t\t}\n\t\t}\n\t}
1 participant
Follow-up to #70 and #67. Integrates the useful structured logging work from #58, replaces #64's load-only check with full validation, hardens OAuth callback binding/state handling and atomic credential storage, makes server startup/shutdown return errors cleanly, and upgrades jsonparser to v1.1.2 to fix CVE-2026-32285. Verified with go test ./..., go test -race ./..., go vet ./..., build, and -check-config.