Skip to content

Commit ccf4e5f

Browse files
patrickhenerclaude
patrickhener
and
claude
authored
Fix ConPtyShell.ps1 auth bypass (GHSA-6m5c-fv2q-jrj2) (#224)
* Fixes GHSA-6m5c-fv2q-jrj2 The unauthenticated ConPtyShell.ps1 exemption in BasicAuthMiddleware and InvisibleBasicAuthMiddleware only matched on the `conpty` query key and the request path. It did not restrict the HTTP method and did not require `conpty` to be the sole query parameter, so an attacker could append `?conpty` to reach the full authenticated API surface anonymously (?bulk webroot read, ?goshs-info, ?ws, ?catcher-api, ?cbDown, ?embedded) and issue unauthenticated PUT/DELETE writes — a critical auth bypass on servers started with -b/--basic-auth or -ca/--cert-auth. Scope the exemption to its intended use via a shared conPtyExempt() helper: a GET/HEAD of exactly /ConPtyShell.ps1 whose only query parameter is `conpty`. The generated upgrade URL (/ConPtyShell.ps1?conpty) still serves without credentials; every other method or extra feature key falls through to auth. Adds regression tests in middleware_test.go pinning the exemption boundaries for both middlewares. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * README --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent e1ef349 commit ccf4e5f

3 files changed

Lines changed: 160 additions & 2 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,7 @@ These are the awesome contributors that made `goshs` even more secure :heart:
157157
<td align="center"><a href="https://github.com/tonghuaroot"><img src="https://github.com/tonghuaroot.png?size=50" width="50" height="50"></a></td>
158158
<td align="center"><a href="https://github.com/arpitjain099"><img src="https://github.com/arpitjain099.png?size=50" width="50" height="50"></a></td>
159159
<td align="center"><a href="https://github.com/manus-use"><img src="https://github.com/manus-use.png?size=50" width="50" height="50"></a></td>
160+
<td align="center"><a href="https://github.com/shotintoeternity"><img src="https://github.com/shotintoeternity.png?size=50" width="50" height="50"></a></td>
160161
<td align="center"><a href="https://github.com/wooseokdotkim">wooseokdotkim</a></td>
161162
<td align="center"><a href="https://github.com/Guilhem7">Guilhem7</a></td>
162163
</tr></table>

‎httpserver/middleware.go‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,11 +109,30 @@ func (fs *FileServer) verifyCredentials(r *http.Request) (authVal string, ok boo
109109
return authVal, true
110110
}
111111

112+
// conPtyExempt reports whether a request qualifies for the unauthenticated
113+
// ConPtyShell.ps1 exemption. A caught Windows host has no credentials, so the
114+
// generated upgrade URL (`/ConPtyShell.ps1?conpty`) must be reachable without
115+
// auth. The exemption is deliberately narrow: only a GET/HEAD of that exact
116+
// path carrying `conpty` as its *sole* query parameter qualifies. Any other
117+
// method (PUT/DELETE/POST) or any additional feature key (`bulk`, `ws`,
118+
// `goshs-info`, `catcher-api`, `cbDown`, `embedded`, …) would otherwise ride
119+
// the exemption and reach the authenticated API surface anonymously.
120+
func conPtyExempt(r *http.Request) bool {
121+
if r.Method != http.MethodGet && r.Method != http.MethodHead {
122+
return false
123+
}
124+
if r.URL.Path != "/ConPtyShell.ps1" {
125+
return false
126+
}
127+
q := r.URL.Query()
128+
return len(q) == 1 && q.Has("conpty")
129+
}
130+
112131
// BasicAuthMiddleware is a middleware to handle the basic auth
113132
func (fs *FileServer) BasicAuthMiddleware(next http.Handler) http.Handler {
114133
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
115134
// Allow unauthenticated access to ConPtyShell.ps1 for catcher upgrades
116-
if r.URL.Query().Has("conpty") && r.URL.Path == "/ConPtyShell.ps1" {
135+
if conPtyExempt(r) {
117136
next.ServeHTTP(w, r)
118137
return
119138
}
@@ -142,7 +161,7 @@ func (fs *FileServer) BasicAuthMiddleware(next http.Handler) http.Handler {
142161
func (fs *FileServer) InvisibleBasicAuthMiddleware(next http.Handler) http.Handler {
143162
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
144163
// Allow unauthenticated access to ConPtyShell.ps1 for catcher upgrades
145-
if r.URL.Query().Has("conpty") && r.URL.Path == "/ConPtyShell.ps1" {
164+
if conPtyExempt(r) {
146165
next.ServeHTTP(w, r)
147166
return
148167
}

‎httpserver/middleware_test.go‎

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,3 +334,141 @@ func TestInvisibleBasicAuth_WrongBcryptUsername(t *testing.T) {
334334

335335
require.False(t, called)
336336
}
337+
338+
// ─── ConPtyShell.ps1 auth exemption scope (GHSA-6m5c-fv2q-jrj2) ───────────────
339+
//
340+
// The unauthenticated exemption for catcher upgrades must cover ONLY a GET/HEAD
341+
// of /ConPtyShell.ps1 whose sole query parameter is `conpty`. Any other method
342+
// or any extra feature key must fall through to the auth check, or an
343+
// unauthenticated attacker reaches the full authenticated API surface (webroot
344+
// read via ?bulk, ?goshs-info, ?ws, ?catcher-api, PUT/DELETE writes, …).
345+
346+
// The genuine upgrade URL generated by the Web UI / TUI must still be served
347+
// without credentials.
348+
func TestBasicAuthMiddleware_ConPtyExemptGET(t *testing.T) {
349+
fs := newFS("user", "pass")
350+
called := false
351+
handler := fs.BasicAuthMiddleware(nextHandler(&called))
352+
353+
r := httptest.NewRequest(http.MethodGet, "/ConPtyShell.ps1?conpty", nil)
354+
w := httptest.NewRecorder()
355+
handler.ServeHTTP(w, r)
356+
357+
require.True(t, called, "genuine GET /ConPtyShell.ps1?conpty must be exempt")
358+
require.Equal(t, http.StatusOK, w.Code)
359+
}
360+
361+
// HEAD of the upgrade URL is equally harmless and stays exempt.
362+
func TestBasicAuthMiddleware_ConPtyExemptHEAD(t *testing.T) {
363+
fs := newFS("user", "pass")
364+
called := false
365+
handler := fs.BasicAuthMiddleware(nextHandler(&called))
366+
367+
r := httptest.NewRequest(http.MethodHead, "/ConPtyShell.ps1?conpty", nil)
368+
w := httptest.NewRecorder()
369+
handler.ServeHTTP(w, r)
370+
371+
require.True(t, called)
372+
require.Equal(t, http.StatusOK, w.Code)
373+
}
374+
375+
// A non-GET/HEAD method must NOT ride the exemption: PUT/DELETE/POST on the
376+
// exempt path would otherwise write or delete files unauthenticated.
377+
func TestBasicAuthMiddleware_ConPtyRejectsWriteMethods(t *testing.T) {
378+
for _, method := range []string{http.MethodPut, http.MethodDelete, http.MethodPost} {
379+
t.Run(method, func(t *testing.T) {
380+
fs := newFS("user", "pass")
381+
called := false
382+
handler := fs.BasicAuthMiddleware(nextHandler(&called))
383+
384+
r := httptest.NewRequest(method, "/ConPtyShell.ps1?conpty", nil)
385+
w := httptest.NewRecorder()
386+
handler.ServeHTTP(w, r)
387+
388+
require.False(t, called, method+" must not be exempt")
389+
require.Equal(t, http.StatusUnauthorized, w.Code)
390+
})
391+
}
392+
}
393+
394+
// Smuggling any additional feature parameter alongside `conpty` must break the
395+
// exemption, so the request is authenticated like any other.
396+
func TestBasicAuthMiddleware_ConPtyRejectsExtraFeatureKeys(t *testing.T) {
397+
extras := []string{
398+
"/ConPtyShell.ps1?conpty&bulk&file=/",
399+
"/ConPtyShell.ps1?conpty&goshs-info",
400+
"/ConPtyShell.ps1?conpty&ws",
401+
"/ConPtyShell.ps1?conpty&catcher-api=list",
402+
"/ConPtyShell.ps1?conpty&cbDown",
403+
"/ConPtyShell.ps1?conpty&embedded",
404+
}
405+
for _, target := range extras {
406+
t.Run(target, func(t *testing.T) {
407+
fs := newFS("user", "pass")
408+
called := false
409+
handler := fs.BasicAuthMiddleware(nextHandler(&called))
410+
411+
r := httptest.NewRequest(http.MethodGet, target, nil)
412+
w := httptest.NewRecorder()
413+
handler.ServeHTTP(w, r)
414+
415+
require.False(t, called, "extra feature key must void the exemption: "+target)
416+
require.Equal(t, http.StatusUnauthorized, w.Code)
417+
})
418+
}
419+
}
420+
421+
// `conpty` on a different path must not be exempt.
422+
func TestBasicAuthMiddleware_ConPtyRejectsOtherPaths(t *testing.T) {
423+
fs := newFS("user", "pass")
424+
called := false
425+
handler := fs.BasicAuthMiddleware(nextHandler(&called))
426+
427+
r := httptest.NewRequest(http.MethodGet, "/secret.txt?conpty", nil)
428+
w := httptest.NewRecorder()
429+
handler.ServeHTTP(w, r)
430+
431+
require.False(t, called)
432+
require.Equal(t, http.StatusUnauthorized, w.Code)
433+
}
434+
435+
// The same scoping must hold for invisible mode, whose exemption block is
436+
// identical. A blocked request calls handleInvisible (a no-op on the recorder),
437+
// so `next` is never reached.
438+
func TestInvisibleBasicAuth_ConPtyExemptGET(t *testing.T) {
439+
fs := newFS("user", "pass")
440+
called := false
441+
handler := fs.InvisibleBasicAuthMiddleware(nextHandler(&called))
442+
443+
r := httptest.NewRequest(http.MethodGet, "/ConPtyShell.ps1?conpty", nil)
444+
w := httptest.NewRecorder()
445+
handler.ServeHTTP(w, r)
446+
447+
require.True(t, called, "genuine GET /ConPtyShell.ps1?conpty must be exempt in invisible mode")
448+
}
449+
450+
func TestInvisibleBasicAuth_ConPtyRejectsWriteAndExtras(t *testing.T) {
451+
cases := []struct {
452+
name string
453+
method string
454+
target string
455+
}{
456+
{"PUT", http.MethodPut, "/ConPtyShell.ps1?conpty"},
457+
{"DELETE", http.MethodDelete, "/ConPtyShell.ps1?conpty"},
458+
{"bulk", http.MethodGet, "/ConPtyShell.ps1?conpty&bulk&file=/"},
459+
{"ws", http.MethodGet, "/ConPtyShell.ps1?conpty&ws"},
460+
}
461+
for _, tc := range cases {
462+
t.Run(tc.name, func(t *testing.T) {
463+
fs := newFS("user", "pass")
464+
called := false
465+
handler := fs.InvisibleBasicAuthMiddleware(nextHandler(&called))
466+
467+
r := httptest.NewRequest(tc.method, tc.target, nil)
468+
w := httptest.NewRecorder()
469+
handler.ServeHTTP(w, r)
470+
471+
require.False(t, called, "must not be exempt in invisible mode: "+tc.target)
472+
})
473+
}
474+
}

0 commit comments

Comments
 (0)