Skip to content

Commit 701d202

Browse files
anshulsaoclaudenachiket0310anujhydrabadi
authored
fix(mcp): preserve POST across gateway redirect + unwrap MCP envelope (#19) (#20)
* fix(mcp): preserve POST across gateway redirect + unwrap MCP envelope QA #18 surfaced a P0: the default host https://askpraxis.ai 301-redirects to www, and Go's default http.Client downgrades the invoke POST->GET and drops the body on a 301, so every MCP invoke hits a body-less GET that 404s and is misreported as "unknown mcp/fn" — a fresh install can't call any MCP function. - callMCP: custom CheckRedirect preserves method, body, and Authorization across redirects (fixes invokes against any redirecting host, including already-stored stale URLs). - credentials.DefaultURL: default to the canonical https://www.askpraxis.ai so fresh logins don't redirect at all. - mcp pretty output: new prettyMCPOutput unwraps single-text {content:[{type:text,text}]} envelopes for human output (E3); --json output is unchanged. Tests (TDD, watched each fail first): TestCallMCP_PreservesPOSTAcrossRedirect, TestDefaultURL_IsCanonicalHost, TestPrettyMCPOutput_*. go test -race + vet + gofmt clean. Refs #19. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(login): persist the canonical post-redirect host (#19) fetchAuthMe already follows the apex → www 301; capture the URL it landed on and store that, so neither fresh logins nor stale stored profiles force every later MCP invoke through the redirect. Reuse logins self-heal an apex URL already in the store. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(mcp): forward Authorization only to same-domain redirect targets (#19) The CheckRedirect that preserves POST + body across the gateway's 301 cloned all headers unconditionally, so a redirect to a foreign host would carry the bearer token with it. Mirror net/http's own sensitive-header rule: forward Authorization only when the target is the original host or a subdomain of it (apex → www stays covered). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(readme): raptor surface is read-write, RBAC enforced server-side (#19) Corrects the read-only framing from the #18 QA brief per the decision recorded in #19-B. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: nachiket0310 <nachiket.warhade@facets.cloud> Co-authored-by: Anuj Hydrabadi <129152617+anujhydrabadi@users.noreply.github.com>
1 parent 3051655 commit 701d202

7 files changed

Lines changed: 398 additions & 7 deletions

File tree

README.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,10 @@ Once installed and logged in, your local AI host can:
1818
- **Query cloud infra** — run read-only `aws`, `gcloud`, and `az`
1919
commands against your org's integrations through the `cloud_cli`
2020
MCP. Mutating verbs blocked at the validator.
21-
- **Drive Facets via Raptor** — every read-only `raptor` verb
22-
(projects, releases, environments, schemas, logs) through the
23-
`raptor_cli` MCP, under your org's Facets PAT.
21+
- **Drive Facets via Raptor** — the full `raptor` verb surface, read
22+
and write (projects, releases, environments, schemas, logs), through
23+
the `raptor_cli` MCP, under your org's Facets PAT. RBAC and audit
24+
are enforced server-side.
2425
- **Read & search the infrastructure catalog** — list registered
2526
repos, search GitHub, register newly discovered repos via the
2627
`catalog_ops` MCP.

cmd/login.go

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,11 @@ func tryReuseStoredToken(out io.Writer, asJSON bool, profileName, baseURL string
181181
}
182182
return false, nil // graceful fallback to the browser
183183
}
184+
// Persist the canonical (post-redirect) host so a stale stored URL
185+
// self-heals on the next login (issue #19-A).
186+
if user.canonicalBaseURL != "" {
187+
baseURL = user.canonicalBaseURL
188+
}
184189
return true, persistAndSetup(out, asJSON, profileName, baseURL, prof.Token, user.Email)
185190
}
186191

@@ -353,6 +358,12 @@ func saveAndVerifyToken(out io.Writer, asJSON bool, profileName, baseURL, token
353358
exitcode.Auth)
354359
os.Exit(exitcode.Auth)
355360
}
361+
// Store the canonical (post-redirect) host, not what the user typed:
362+
// a stored apex URL would force every later MCP invoke through the
363+
// apex → www 301 (issue #19-A).
364+
if user.canonicalBaseURL != "" {
365+
baseURL = user.canonicalBaseURL
366+
}
356367
return persistAndSetup(out, asJSON, profileName, baseURL, token, user.Email)
357368
}
358369

@@ -402,6 +413,14 @@ type authMeResponse struct {
402413
UserID string `json:"user_id"`
403414
Email string `json:"email"`
404415
Username string `json:"username"`
416+
417+
// canonicalBaseURL is the deployment base URL the /auth/me call
418+
// actually landed on after following redirects (e.g. the apex
419+
// askpraxis.ai 301s to www). Login persists this instead of the URL
420+
// the user typed, so later MCP invokes never pay that redirect.
421+
// Empty when a test stub doesn't set it — callers fall back to the
422+
// URL they already have.
423+
canonicalBaseURL string
405424
}
406425

407426
// fetchAuthMe is the seam: tests swap it to avoid hitting a real server.
@@ -424,6 +443,17 @@ var fetchAuthMe = func(baseURL, token string) (*authMeResponse, error) {
424443
if err := json.NewDecoder(resp.Body).Decode(&me); err != nil {
425444
return nil, err
426445
}
446+
// resp.Request is the FINAL request after the client followed any
447+
// redirects; strip the known endpoint path to recover the canonical
448+
// deployment base (preserving any path prefix the deployment lives
449+
// under). Fall back to what the caller passed if the shape is ever
450+
// unexpected.
451+
me.canonicalBaseURL = baseURL
452+
if final := resp.Request.URL.String(); strings.HasSuffix(final, "/ai-api/auth/me") {
453+
if base := strings.TrimRight(strings.TrimSuffix(final, "/ai-api/auth/me"), "/"); base != "" {
454+
me.canonicalBaseURL = base
455+
}
456+
}
427457
return &me, nil
428458
}
429459

cmd/login_canonical_test.go

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
package cmd
2+
3+
import (
4+
"io"
5+
"net/http"
6+
"net/http/httptest"
7+
"testing"
8+
9+
"github.com/Facets-cloud/praxis-cli/internal/credentials"
10+
)
11+
12+
// ─── issue #19-A: canonicalize the host at login ─────────────────────────
13+
//
14+
// The apex https://askpraxis.ai 301-redirects to www. fetchAuthMe's GET
15+
// follows the redirect, so login *works* against the apex — but if the
16+
// CLI then stores the apex URL, every later MCP invoke pays (and used to
17+
// fail on) that redirect. Login must store the scheme://host the
18+
// /auth/me call actually landed on.
19+
20+
// canonicalPair spins up a "final" server serving /ai-api/auth/me and a
21+
// "stale" server that 301-redirects everything to it, mimicking the
22+
// apex → www hop.
23+
func canonicalPair(t *testing.T) (stale, final *httptest.Server) {
24+
t.Helper()
25+
final = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
26+
if r.URL.Path != "/ai-api/auth/me" {
27+
http.NotFound(w, r)
28+
return
29+
}
30+
w.Header().Set("Content-Type", "application/json")
31+
_, _ = w.Write([]byte(`{"user_id":"u1","email":"qa@facets.cloud"}`))
32+
}))
33+
t.Cleanup(final.Close)
34+
stale = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
35+
http.Redirect(w, r, final.URL+r.URL.Path, http.StatusMovedPermanently)
36+
}))
37+
t.Cleanup(stale.Close)
38+
return stale, final
39+
}
40+
41+
func TestFetchAuthMe_ReportsCanonicalHostAfterRedirect(t *testing.T) {
42+
stale, final := canonicalPair(t)
43+
44+
me, err := fetchAuthMe(stale.URL, "sk_test_T")
45+
if err != nil {
46+
t.Fatalf("fetchAuthMe: %v", err)
47+
}
48+
if me.Email != "qa@facets.cloud" {
49+
t.Errorf("email = %q, want qa@facets.cloud", me.Email)
50+
}
51+
if me.canonicalBaseURL != final.URL {
52+
t.Errorf("canonicalBaseURL = %q, want %q (the post-redirect host)", me.canonicalBaseURL, final.URL)
53+
}
54+
}
55+
56+
func TestFetchAuthMe_NoRedirectKeepsBaseURL(t *testing.T) {
57+
_, final := canonicalPair(t)
58+
59+
me, err := fetchAuthMe(final.URL, "sk_test_T")
60+
if err != nil {
61+
t.Fatalf("fetchAuthMe: %v", err)
62+
}
63+
if me.canonicalBaseURL != final.URL {
64+
t.Errorf("canonicalBaseURL = %q, want %q (unchanged when no redirect)", me.canonicalBaseURL, final.URL)
65+
}
66+
}
67+
68+
// End to end through the --token login path: the profile must be stored
69+
// with the canonical (post-redirect) URL, not the stale one the user
70+
// passed.
71+
func TestSaveAndVerifyToken_StoresCanonicalURL(t *testing.T) {
72+
isolateHome(t)
73+
stubPostAuth(t)
74+
stale, final := canonicalPair(t)
75+
76+
if err := saveAndVerifyToken(io.Discard, true, "default", stale.URL, "sk_test_T"); err != nil {
77+
t.Fatalf("saveAndVerifyToken: %v", err)
78+
}
79+
80+
store, err := credentials.Load()
81+
if err != nil {
82+
t.Fatalf("load credentials: %v", err)
83+
}
84+
got := store["default"].URL
85+
if got != final.URL {
86+
t.Errorf("stored URL = %q, want canonical %q", got, final.URL)
87+
}
88+
}
89+
90+
// Token reuse against a stored stale URL must self-heal the stored URL
91+
// to the canonical host on the next login.
92+
func TestTryReuseStoredToken_SelfHealsStaleURL(t *testing.T) {
93+
isolateHome(t)
94+
stubPostAuth(t)
95+
stale, final := canonicalPair(t)
96+
seedProfile(t, "default", stale.URL, "sk_test_T")
97+
98+
reused, err := tryReuseStoredToken(io.Discard, true, "default", stale.URL)
99+
if err != nil {
100+
t.Fatalf("tryReuseStoredToken: %v", err)
101+
}
102+
if !reused {
103+
t.Fatal("expected the stored token to be reused")
104+
}
105+
106+
store, err := credentials.Load()
107+
if err != nil {
108+
t.Fatalf("load credentials: %v", err)
109+
}
110+
if got := store["default"].URL; got != final.URL {
111+
t.Errorf("stored URL after reuse = %q, want self-healed canonical %q", got, final.URL)
112+
}
113+
}

cmd/mcp.go

Lines changed: 76 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,8 +136,7 @@ Examples:
136136
if asJSON {
137137
_, _ = out.Write(append(bytes.TrimRight(resp, "\n"), '\n'))
138138
} else {
139-
pretty := prettyJSON(resp)
140-
fmt.Fprintln(out, pretty)
139+
fmt.Fprintln(out, prettyMCPOutput(resp))
141140
}
142141

143142
if exitWithToolError {
@@ -314,7 +313,39 @@ var callMCP = func(baseURL, token, mcp, fn string, body []byte, timeout time.Dur
314313
if baseURL == "" {
315314
return nil, 0, errors.New("profile has no URL set")
316315
}
317-
client := &http.Client{Timeout: timeout}
316+
client := &http.Client{
317+
Timeout: timeout,
318+
// Go's default redirect policy downgrades POST→GET and drops the
319+
// body on 301/302/303. A gateway that 301-redirects (e.g. the
320+
// askpraxis.ai apex → www) would therefore turn every invoke into
321+
// a body-less GET that 404s — misreported downstream as "unknown
322+
// mcp/fn". Preserve the method, body, and Authorization header so
323+
// the invoke survives the redirect intact.
324+
CheckRedirect: func(req *http.Request, via []*http.Request) error {
325+
if len(via) >= 10 {
326+
return errors.New("stopped after 10 redirects")
327+
}
328+
orig := via[0]
329+
req.Method = orig.Method
330+
req.Header = orig.Header.Clone()
331+
// Never leak the bearer token to a foreign domain: mirror
332+
// Go's own sensitive-header rule and forward Authorization
333+
// only when the redirect target is the original host or a
334+
// subdomain of it (apex → www stays covered).
335+
if !isDomainOrSubdomain(req.URL.Hostname(), orig.URL.Hostname()) {
336+
req.Header.Del("Authorization")
337+
}
338+
if orig.GetBody != nil {
339+
b, err := orig.GetBody()
340+
if err != nil {
341+
return err
342+
}
343+
req.Body = b
344+
req.ContentLength = orig.ContentLength
345+
}
346+
return nil
347+
},
348+
}
318349
url := strings.TrimRight(baseURL, "/") + "/ai-api/v1/mcp/" + mcp + "/" + fn
319350
req, err := http.NewRequest("POST", url, bytes.NewReader(body))
320351
if err != nil {
@@ -334,6 +365,18 @@ var callMCP = func(baseURL, token, mcp, fn string, body []byte, timeout time.Dur
334365
return raw, resp.StatusCode, nil
335366
}
336367

368+
// isDomainOrSubdomain reports whether child is the same host as parent
369+
// or a label-aligned subdomain of it (www.askpraxis.ai ⊂ askpraxis.ai,
370+
// but evilaskpraxis.ai ⊄ askpraxis.ai). This is the same rule net/http
371+
// uses to decide whether sensitive headers may follow a redirect.
372+
func isDomainOrSubdomain(child, parent string) bool {
373+
child, parent = strings.ToLower(child), strings.ToLower(parent)
374+
if child == parent {
375+
return true
376+
}
377+
return strings.HasSuffix(child, "."+parent)
378+
}
379+
337380
// extractDetail tries to pull `detail` out of a FastAPI-style error body
338381
// for friendlier error printing; falls back to fallback on any failure.
339382
func extractDetail(raw []byte, fallback string) string {
@@ -373,3 +416,33 @@ func prettyJSON(raw []byte) string {
373416
}
374417
return string(out)
375418
}
419+
420+
// prettyMCPOutput renders an MCP response for human (non-JSON) output.
421+
// The gateway wraps tool output as
422+
//
423+
// {"content":[{"type":"text","text":"<payload>"}]}
424+
//
425+
// where the payload is itself usually escaped JSON. When the envelope
426+
// carries exactly one text item, unwrap it: pretty-print the inner JSON
427+
// if it parses, otherwise surface the text verbatim. Anything that
428+
// isn't this single-text shape (plain responses, multi-item content,
429+
// non-text items) falls through to ordinary pretty-printing so we never
430+
// hide structure by guessing. JSON output (`--json`) is unaffected — it
431+
// still passes the canonical envelope through untouched.
432+
func prettyMCPOutput(raw []byte) string {
433+
var env struct {
434+
Content []struct {
435+
Type string `json:"type"`
436+
Text string `json:"text"`
437+
} `json:"content"`
438+
}
439+
if err := json.Unmarshal(raw, &env); err == nil &&
440+
len(env.Content) == 1 && env.Content[0].Type == "text" && env.Content[0].Text != "" {
441+
inner := env.Content[0].Text
442+
if json.Valid([]byte(inner)) {
443+
return prettyJSON([]byte(inner))
444+
}
445+
return inner
446+
}
447+
return prettyJSON(raw)
448+
}

0 commit comments

Comments
 (0)