Skip to content

Commit a7bdf2c

Browse files
DumbrisDumbris
authored
feat(connect): classify config access + macOS TCC denial remediation (US2) (#707)
Classify each client-config access into accessible/absent/denied/malformed strictly from the OS error class (errors.Is on fs.ErrPermission / fs.ErrNotExist), never from string matching (Spec 075 FR-003/FR-011). - access.go: AccessOutcome, classifyAccess(err), typed *AccessError (Error()+Unwrap, errors.As-friendly) and canonical remediationText with the tccutil reset command + prod/dev bundle ids (FR-005). - GetStatus: a denied content read now resolves AccessState=denied and surfaces Remediation instead of a plain "not connected" (FR-004). - Connect/Disconnect: a permission denial anywhere in the read/backup/write chain returns a typed *AccessError with remediation; other error semantics (unknown client, already-exists, malformed) are preserved. - backup.go: failure paths keep wrapping the OS cause with %w so denials reach the Connect/Disconnect classifier. Tests (TDD, -race green): classifyAccess four-outcome table, AccessError unwrap/As, remediationText contents, GetStatus denied-surfaces-remediation, Connect denied-returns-AccessError. Related #696
1 parent 9194ba7 commit a7bdf2c

4 files changed

Lines changed: 300 additions & 16 deletions

File tree

‎internal/connect/access.go‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
package connect
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
"io/fs"
7+
)
8+
9+
// AccessOutcome classifies an attempt to read or write a client config file
10+
// (Spec 075 FR-003). It is an alias of string so it interoperates with the
11+
// existing untyped access* constants and the ClientStatus.AccessState wire
12+
// field, while giving classifier signatures a documented, enum-like type.
13+
//
14+
// Valid values are the access* constants in connect.go: accessAccessible,
15+
// accessAbsent, accessDenied, accessMalformed. (accessUnknown is the overall,
16+
// not-content-checked status default, not a classification outcome.)
17+
type AccessOutcome = string
18+
19+
// macOS bundle identifiers used in the tccutil reset remediation. Kept in sync
20+
// with native/macos/MCPProxy/MCPProxy/Info.plist (prod) and the .dev variant.
21+
const (
22+
bundleIDProd = "com.smartmcpproxy.mcpproxy"
23+
bundleIDDev = "com.smartmcpproxy.mcpproxy.dev"
24+
)
25+
26+
// classifyAccess maps a file-access error to an AccessOutcome strictly from the
27+
// error class (Spec 075 FR-011) — never from string-matching error text:
28+
//
29+
// - nil -> accessAccessible
30+
// - errors.Is(err, ErrNotExist) -> accessAbsent (client not installed)
31+
// - errors.Is(err, ErrPermission)-> accessDenied (macOS TCC App-Data block)
32+
// - any other error -> accessMalformed (read/parse failure)
33+
//
34+
// syscall.EPERM and syscall.EACCES both satisfy errors.Is(err, fs.ErrPermission)
35+
// via their Errno.Is implementation, so a real TCC denial classifies as denied.
36+
func classifyAccess(err error) AccessOutcome {
37+
switch {
38+
case err == nil:
39+
return accessAccessible
40+
case errors.Is(err, fs.ErrNotExist):
41+
return accessAbsent
42+
case errors.Is(err, fs.ErrPermission):
43+
return accessDenied
44+
default:
45+
return accessMalformed
46+
}
47+
}
48+
49+
// AccessError is returned by connect/disconnect (and surfaced by GetStatus via
50+
// the Remediation field) when a client-config access is permission-denied. It
51+
// is errors.As-discoverable so the REST layer can map it to the right field,
52+
// and unwraps to the underlying OS error so errors.Is(err, fs.ErrPermission)
53+
// still holds (data-model.md AccessError).
54+
type AccessError struct {
55+
Client string // client id/name
56+
Path string // config path attempted
57+
Outcome AccessOutcome // accessDenied (could also wrap malformed)
58+
Remediation string // actionable fix text
59+
Err error // underlying OS cause
60+
}
61+
62+
func (e *AccessError) Error() string {
63+
if e.Path != "" {
64+
return fmt.Sprintf("%s (config: %s)", e.Remediation, e.Path)
65+
}
66+
return e.Remediation
67+
}
68+
69+
// Unwrap exposes the underlying OS error for errors.Is/errors.As.
70+
func (e *AccessError) Unwrap() error { return e.Err }
71+
72+
// remediationText builds the canonical privacy-denied message (data-model.md):
73+
// it names the cause, the App Data settings path, and the exact tccutil reset
74+
// command with both the prod and dev bundle identifiers (FR-005).
75+
func remediationText(client string) string {
76+
return fmt.Sprintf(
77+
"macOS blocked mcpproxy from reading %s's configuration (Privacy & Security ▸ App Data).\n"+
78+
"Fix: System Settings ▸ Privacy & Security ▸ App Data ▸ enable mcpproxy,\n"+
79+
"or run: tccutil reset SystemPolicyAppData %s\n"+
80+
"(dev builds: %s)",
81+
client, bundleIDProd, bundleIDDev,
82+
)
83+
}
84+
85+
// newAccessError constructs an *AccessError for a denied access on the given
86+
// client/path, wrapping the OS cause.
87+
func (s *Service) newAccessError(client *ClientDef, path string, cause error) *AccessError {
88+
name := client.Name
89+
return &AccessError{
90+
Client: name,
91+
Path: path,
92+
Outcome: accessDenied,
93+
Remediation: remediationText(name),
94+
Err: cause,
95+
}
96+
}
97+
98+
// asAccessError wraps a connect/disconnect error as a typed *AccessError when it
99+
// classifies as a permission denial; otherwise it returns the error unchanged so
100+
// existing error semantics (unknown client, already-exists, parse failures) are
101+
// preserved (FR-004). A nil error stays nil.
102+
func (s *Service) asAccessError(client *ClientDef, path string, err error) error {
103+
if err == nil {
104+
return nil
105+
}
106+
if classifyAccess(err) == accessDenied {
107+
return s.newAccessError(client, path, err)
108+
}
109+
return err
110+
}

‎internal/connect/access_test.go‎

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
package connect
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
"io/fs"
7+
"os"
8+
"path/filepath"
9+
"strings"
10+
"syscall"
11+
"testing"
12+
)
13+
14+
// TestClassifyAccess covers the four outcomes derived from the OS error class
15+
// (Spec 075 FR-003/FR-011): classification comes from errors.Is, never from
16+
// string-matching arbitrary error text.
17+
func TestClassifyAccess(t *testing.T) {
18+
tests := []struct {
19+
name string
20+
err error
21+
want AccessOutcome
22+
}{
23+
{"nil is accessible", nil, accessAccessible},
24+
{"ErrNotExist is absent", fs.ErrNotExist, accessAbsent},
25+
{"PathError ENOENT is absent", &fs.PathError{Op: "open", Path: "/x", Err: syscall.ENOENT}, accessAbsent},
26+
{"ErrPermission is denied", fs.ErrPermission, accessDenied},
27+
{"PathError EPERM is denied", &fs.PathError{Op: "open", Path: "/x", Err: syscall.EPERM}, accessDenied},
28+
{"PathError EACCES is denied", &fs.PathError{Op: "open", Path: "/x", Err: syscall.EACCES}, accessDenied},
29+
{"wrapped EPERM is denied", fmt.Errorf("read /x: %w", &fs.PathError{Err: syscall.EPERM}), accessDenied},
30+
{"parse error is malformed", errors.New("invalid character 'x'"), accessMalformed},
31+
}
32+
for _, tt := range tests {
33+
t.Run(tt.name, func(t *testing.T) {
34+
if got := classifyAccess(tt.err); got != tt.want {
35+
t.Errorf("classifyAccess(%v) = %q, want %q", tt.err, got, tt.want)
36+
}
37+
})
38+
}
39+
}
40+
41+
// TestAccessError_ErrorAndUnwrap verifies the typed error carries remediation,
42+
// is errors.As-discoverable, and unwraps to the OS permission cause so the REST
43+
// layer can map it (data-model.md AccessError).
44+
func TestAccessError_ErrorAndUnwrap(t *testing.T) {
45+
cause := &fs.PathError{Op: "open", Path: "/cfg", Err: syscall.EPERM}
46+
ae := &AccessError{
47+
Client: "Claude Code",
48+
Path: "/cfg",
49+
Outcome: accessDenied,
50+
Remediation: remediationText("Claude Code"),
51+
Err: cause,
52+
}
53+
54+
if !strings.Contains(ae.Error(), "tccutil reset SystemPolicyAppData") {
55+
t.Errorf("Error() should include remediation, got: %q", ae.Error())
56+
}
57+
if !errors.Is(ae, fs.ErrPermission) {
58+
t.Error("AccessError should unwrap to fs.ErrPermission")
59+
}
60+
var target *AccessError
61+
if !errors.As(fmt.Errorf("connect: %w", ae), &target) {
62+
t.Error("AccessError should be discoverable via errors.As through a wrap")
63+
}
64+
}
65+
66+
// TestRemediationText asserts the canonical message names the cause, the App
67+
// Data settings path, the tccutil reset command, and both bundle ids (FR-005).
68+
func TestRemediationText(t *testing.T) {
69+
got := remediationText("Cursor")
70+
for _, want := range []string{
71+
"Cursor",
72+
"Privacy & Security",
73+
"App Data",
74+
"tccutil reset SystemPolicyAppData com.smartmcpproxy.mcpproxy",
75+
"com.smartmcpproxy.mcpproxy.dev",
76+
} {
77+
if !strings.Contains(got, want) {
78+
t.Errorf("remediationText missing %q; got:\n%s", want, got)
79+
}
80+
}
81+
}
82+
83+
// epermReader returns a content reader that always fails with a TCC-style
84+
// permission denial, simulating a macOS App-Data block without a real OS denial.
85+
func epermReader(path string) ([]byte, error) {
86+
return nil, &fs.PathError{Op: "open", Path: path, Err: syscall.EPERM}
87+
}
88+
89+
// TestGetStatus_DeniedSurfacesRemediation: a permission-denied content read on
90+
// an installed client resolves to access_state=denied with actionable
91+
// remediation, and must NOT be reported as plain "not connected" (FR-004).
92+
func TestGetStatus_DeniedSurfacesRemediation(t *testing.T) {
93+
svc, homeDir := testService(t)
94+
95+
// The config file must exist on disk so os.Stat (metadata) reports installed;
96+
// the denial happens on the content read, mirroring macOS TCC App-Data.
97+
cfgPath := ConfigPath("claude-code", homeDir)
98+
if err := os.MkdirAll(filepath.Dir(cfgPath), 0o755); err != nil {
99+
t.Fatal(err)
100+
}
101+
if err := os.WriteFile(cfgPath, []byte(`{"mcpServers":{}}`), 0o644); err != nil {
102+
t.Fatal(err)
103+
}
104+
svc.setReadFile(epermReader)
105+
106+
st, err := svc.GetStatus("claude-code")
107+
if err != nil {
108+
t.Fatalf("GetStatus should not hard-error on denial: %v", err)
109+
}
110+
if st.AccessState != accessDenied {
111+
t.Errorf("expected access_state=%q, got %q", accessDenied, st.AccessState)
112+
}
113+
if st.Connected {
114+
t.Error("denied access must not be reported as connected")
115+
}
116+
if !strings.Contains(st.Remediation, "tccutil reset SystemPolicyAppData") {
117+
t.Errorf("expected remediation with tccutil reset, got %q", st.Remediation)
118+
}
119+
if !strings.Contains(st.Remediation, "com.smartmcpproxy.mcpproxy") {
120+
t.Errorf("expected remediation to name the bundle id, got %q", st.Remediation)
121+
}
122+
}
123+
124+
// TestConnectDenied_ReturnsAccessError: a permission denial on the connect path
125+
// returns a typed *AccessError carrying remediation, distinct from the
126+
// unknown-client and not-supported errors (FR-004).
127+
func TestConnectDenied_ReturnsAccessError(t *testing.T) {
128+
svc, homeDir := testService(t)
129+
130+
cfgPath := ConfigPath("claude-code", homeDir)
131+
if err := os.MkdirAll(filepath.Dir(cfgPath), 0o755); err != nil {
132+
t.Fatal(err)
133+
}
134+
if err := os.WriteFile(cfgPath, []byte(`{"mcpServers":{}}`), 0o644); err != nil {
135+
t.Fatal(err)
136+
}
137+
svc.setReadFile(epermReader)
138+
139+
_, err := svc.Connect("claude-code", "", false)
140+
if err == nil {
141+
t.Fatal("expected an error when the connect read is permission-denied")
142+
}
143+
var ae *AccessError
144+
if !errors.As(err, &ae) {
145+
t.Fatalf("expected *AccessError, got %T: %v", err, err)
146+
}
147+
if ae.Outcome != accessDenied {
148+
t.Errorf("expected Outcome=%q, got %q", accessDenied, ae.Outcome)
149+
}
150+
if !strings.Contains(ae.Remediation, "tccutil reset SystemPolicyAppData") {
151+
t.Errorf("expected remediation, got %q", ae.Remediation)
152+
}
153+
154+
// A genuine unknown-client error must NOT be classified as a denial.
155+
if _, unknownErr := svc.Connect("does-not-exist", "", false); errors.As(unknownErr, &ae) {
156+
t.Error("unknown-client error must not be an *AccessError")
157+
}
158+
}

‎internal/connect/backup.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,11 @@ import (
1010

1111
// backupFile creates a timestamped backup of the given file.
1212
// Returns the backup path, or empty string if the source file does not exist.
13+
//
14+
// All failure paths wrap their OS cause with %w, so a permission denial here
15+
// (e.g. macOS TCC App-Data) preserves fs.ErrPermission up the call chain and is
16+
// classified into a typed *AccessError by the Connect/Disconnect boundary
17+
// (Spec 075 FR-004, see Service.asAccessError).
1318
func backupFile(path string) (string, error) {
1419
info, err := os.Stat(path)
1520
if os.IsNotExist(err) {

‎internal/connect/connect.go‎

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,7 @@ package connect
33
import (
44
"bytes"
55
"encoding/json"
6-
"errors"
76
"fmt"
8-
"io/fs"
97
"net/url"
108
"os"
119
"regexp"
@@ -22,7 +20,7 @@ const (
2220
accessAccessible = "accessible" // config read and parsed successfully
2321
accessAbsent = "absent" // config file does not exist (not installed)
2422
accessMalformed = "malformed" // config read but contents unparseable
25-
// accessDenied ("denied", macOS TCC App-Data) is added in US2.
23+
accessDenied = "denied" // blocked by OS permission (macOS TCC App-Data)
2624
)
2725

2826
// ConnectResult describes the outcome of a connect or disconnect operation.
@@ -237,25 +235,27 @@ func (s *Service) GetStatus(clientID string) (ClientStatus, error) {
237235

238236
name, found, outcome := s.entryAccess(*c, cfgPath)
239237
status.AccessState = outcome
240-
if outcome == accessAccessible && found {
238+
switch {
239+
case outcome == accessAccessible && found:
241240
status.Connected = true
242241
status.ServerName = name
242+
case outcome == accessDenied:
243+
// A macOS App-Data block must surface as actionable remediation, not as
244+
// a plain "not connected" (Spec 075 FR-004).
245+
status.Remediation = remediationText(c.Name)
243246
}
244247
return status, nil
245248
}
246249

247250
// entryAccess reads the client config exactly once via the seam, then reports
248251
// the registered server name (if any), whether mcpproxy is connected, and the
249-
// access outcome. US1 classifies accessible/absent/malformed; US2 refines a
250-
// permission denial (currently surfaced as accessUnknown) into accessDenied.
252+
// access outcome classified strictly from the error class (Spec 075 FR-011):
253+
// a read error maps to absent/denied/malformed via classifyAccess, and a parse
254+
// failure on otherwise-readable bytes maps to malformed.
251255
func (s *Service) entryAccess(client ClientDef, cfgPath string) (name string, found bool, outcome string) {
252256
raw, err := s.read(cfgPath)
253257
if err != nil {
254-
if errors.Is(err, fs.ErrNotExist) {
255-
return "", false, accessAbsent
256-
}
257-
// US1: unclassified read error. US2 maps fs.ErrPermission -> denied.
258-
return "", false, accessUnknown
258+
return "", false, classifyAccess(err)
259259
}
260260
name, found, parsedOK := s.findEntryFromBytes(client, raw)
261261
if !parsedOK {
@@ -292,10 +292,17 @@ func (s *Service) Connect(clientID, serverName string, force bool) (*ConnectResu
292292

293293
mcpURL := s.mcpURL()
294294

295+
var res *ConnectResult
296+
var err error
295297
if client.Format == "toml" {
296-
return s.connectTOML(client, cfgPath, serverName, mcpURL, force)
297-
}
298-
return s.connectJSON(client, cfgPath, serverName, mcpURL, force)
298+
res, err = s.connectTOML(client, cfgPath, serverName, mcpURL, force)
299+
} else {
300+
res, err = s.connectJSON(client, cfgPath, serverName, mcpURL, force)
301+
}
302+
// A permission denial anywhere in the read/backup/write chain (the errors
303+
// preserve their OS cause via %w) surfaces as a typed *AccessError with
304+
// remediation; other errors keep their existing semantics (Spec 075 FR-004).
305+
return res, s.asAccessError(client, cfgPath, err)
299306
}
300307

301308
// Disconnect removes the MCPProxy entry from the specified client's configuration.
@@ -317,10 +324,14 @@ func (s *Service) Disconnect(clientID, serverName string) (*ConnectResult, error
317324
return nil, fmt.Errorf("cannot determine config path for %s", clientID)
318325
}
319326

327+
var res *ConnectResult
328+
var err error
320329
if client.Format == "toml" {
321-
return s.disconnectTOML(client, cfgPath, serverName)
330+
res, err = s.disconnectTOML(client, cfgPath, serverName)
331+
} else {
332+
res, err = s.disconnectJSON(client, cfgPath, serverName)
322333
}
323-
return s.disconnectJSON(client, cfgPath, serverName)
334+
return res, s.asAccessError(client, cfgPath, err)
324335
}
325336

326337
// ---------- JSON helpers ----------

0 commit comments

Comments
 (0)