Skip to content

Commit 8d3ec6b

Browse files
idoubiidoubi
authored
Merge pull request #51 from Bin-Huang/codex/fix-global-skill-install-auth
[codex] fix global skill install authorization
2 parents 2231659 + d6ab971 commit 8d3ec6b

2 files changed

Lines changed: 221 additions & 19 deletions

File tree

‎internal/setup/skill_install.go‎

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"path/filepath"
1414
"strings"
1515

16+
"github.com/fastclaw-ai/fastclaw/internal/auth"
1617
"github.com/fastclaw-ai/fastclaw/internal/config"
1718
"github.com/fastclaw-ai/fastclaw/internal/skills"
1819
)
@@ -52,13 +53,8 @@ func (s *Server) handleInstallSkill(w http.ResponseWriter, r *http.Request) {
5253
return
5354
}
5455

55-
if req.Agent != "" {
56-
// Owner-only — Identity.CanAccessAgent is a deferred-true for
57-
// session callers, so without an explicit owner check anyone
58-
// could push a skill into anyone else's agent home dir.
59-
if s.requireAgentOwner(w, r, req.Agent) == nil {
60-
return
61-
}
56+
if !s.authorizeSkillInstallTarget(w, r, req.Agent) {
57+
return
6258
}
6359
targetDir, err := resolveInstallTarget(r, req.Agent)
6460
if err != nil {
@@ -106,8 +102,33 @@ func (s *Server) handleInstallSkill(w http.ResponseWriter, r *http.Request) {
106102
})
107103
}
108104

109-
// resolveInstallTarget picks the target directory for an install and enforces
110-
// the admin-only rule for global installs.
105+
// authorizeSkillInstallTarget enforces the mutation and target-scope rules
106+
// shared by registry installs and zip uploads.
107+
func (s *Server) authorizeSkillInstallTarget(w http.ResponseWriter, r *http.Request, agentID string) bool {
108+
if !s.requireWritable(w, r) {
109+
return false
110+
}
111+
if agentID != "" {
112+
// Owner-only — Identity.CanAccessAgent is a deferred-true for
113+
// session callers, so without an explicit owner check anyone
114+
// could push a skill into anyone else's agent home dir.
115+
return s.requireAgentOwner(w, r, agentID) != nil
116+
}
117+
ident, ok := auth.FromContext(r.Context())
118+
if !ok {
119+
jsonResponse(w, http.StatusUnauthorized, map[string]any{"ok": false, "error": "unauthorized"})
120+
return false
121+
}
122+
if !ident.CanAdminPlatform() {
123+
jsonResponse(w, http.StatusForbidden, map[string]any{"ok": false, "error": "platform admin required"})
124+
return false
125+
}
126+
return true
127+
}
128+
129+
// resolveInstallTarget picks the target directory for an install. Authorization
130+
// happens before this helper is called: agent installs are owner-only; global
131+
// installs are platform-admin-only.
111132
func resolveInstallTarget(r *http.Request, agentID string) (string, error) {
112133
if agentID != "" {
113134
// agents.id is globally unique, so the home dir doesn't need a
@@ -122,9 +143,6 @@ func resolveInstallTarget(r *http.Request, agentID string) (string, error) {
122143
}
123144
return dir, nil
124145
}
125-
// Global install — super_admin only. Caller has already been
126-
// validated by the route's RequireSuperAdmin middleware when this
127-
// path is reached for global installs.
128146
home, err := config.HomeDir()
129147
if err != nil {
130148
return "", err
@@ -188,6 +206,11 @@ func runInstall(source, name, repo, targetDir string) (*skills.Result, error) {
188206
// doesn't auto-follow them but we also refuse to recreate them on disk.
189207
func (s *Server) handleUploadSkill(w http.ResponseWriter, r *http.Request) {
190208
const maxUploadSize = 64 << 20 // 64 MiB
209+
agentID := r.URL.Query().Get("agent")
210+
if !s.authorizeSkillInstallTarget(w, r, agentID) {
211+
return
212+
}
213+
191214
if err := r.ParseMultipartForm(maxUploadSize); err != nil {
192215
jsonResponse(w, http.StatusBadRequest, map[string]any{"ok": false, "error": err.Error()})
193216
return
@@ -204,13 +227,6 @@ func (s *Server) handleUploadSkill(w http.ResponseWriter, r *http.Request) {
204227
return
205228
}
206229

207-
agentID := r.URL.Query().Get("agent")
208-
if agentID != "" {
209-
// Owner-only — see comment on the JSON-install path above.
210-
if s.requireAgentOwner(w, r, agentID) == nil {
211-
return
212-
}
213-
}
214230
targetDir, err := resolveInstallTarget(r, agentID)
215231
if err != nil {
216232
jsonResponse(w, http.StatusForbidden, map[string]any{"ok": false, "error": err.Error()})

‎internal/setup/skill_install_test.go‎

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
package setup
2+
3+
import (
4+
"context"
5+
"net/http"
6+
"net/http/httptest"
7+
"path/filepath"
8+
"testing"
9+
10+
"github.com/fastclaw-ai/fastclaw/internal/auth"
11+
"github.com/fastclaw-ai/fastclaw/internal/store"
12+
"github.com/fastclaw-ai/fastclaw/internal/users"
13+
)
14+
15+
func TestAuthorizeSkillInstallTargetRequiresAdminForGlobalInstalls(t *testing.T) {
16+
s := NewServer(0)
17+
18+
tests := []struct {
19+
name string
20+
ident auth.Identity
21+
wantOK bool
22+
wantStatus int
23+
}{
24+
{
25+
name: "regular user session rejected",
26+
ident: auth.Identity{
27+
UserID: "u_user",
28+
Role: users.RoleUser,
29+
AuthMethod: "session",
30+
},
31+
wantOK: false,
32+
wantStatus: http.StatusForbidden,
33+
},
34+
{
35+
name: "super admin session allowed",
36+
ident: auth.Identity{
37+
UserID: "u_admin",
38+
Role: users.RoleSuperAdmin,
39+
AuthMethod: "session",
40+
},
41+
wantOK: true,
42+
},
43+
{
44+
name: "admin api key allowed",
45+
ident: auth.Identity{
46+
UserID: "u_user",
47+
Role: users.RoleUser,
48+
AuthMethod: "apikey",
49+
APIKeyType: users.APIKeyTypeAdmin,
50+
},
51+
wantOK: true,
52+
},
53+
{
54+
name: "actAs super admin rejected as read only",
55+
ident: auth.Identity{
56+
UserID: "u_admin",
57+
Role: users.RoleSuperAdmin,
58+
AuthMethod: "session",
59+
ActAsUserID: "u_other",
60+
},
61+
wantOK: false,
62+
wantStatus: http.StatusForbidden,
63+
},
64+
}
65+
66+
for _, tt := range tests {
67+
t.Run(tt.name, func(t *testing.T) {
68+
rr := httptest.NewRecorder()
69+
ok := s.authorizeSkillInstallTarget(rr, skillInstallRequest(tt.ident), "")
70+
if ok != tt.wantOK {
71+
t.Fatalf("ok = %v, want %v", ok, tt.wantOK)
72+
}
73+
if !tt.wantOK && rr.Code != tt.wantStatus {
74+
t.Fatalf("status = %d, want %d", rr.Code, tt.wantStatus)
75+
}
76+
})
77+
}
78+
}
79+
80+
func TestAuthorizeSkillInstallTargetKeepsAgentInstallsOwnerScoped(t *testing.T) {
81+
ctx := context.Background()
82+
s, st, accts := newSkillInstallAuthServer(t, ctx)
83+
owner := createSkillInstallTestUser(t, ctx, accts, "owner", users.RoleUser)
84+
other := createSkillInstallTestUser(t, ctx, accts, "other", users.RoleUser)
85+
if err := st.SaveAgent(ctx, &store.AgentRecord{
86+
ID: "agt_owner",
87+
UserID: owner.ID,
88+
Name: "Owner Agent",
89+
}); err != nil {
90+
t.Fatalf("SaveAgent: %v", err)
91+
}
92+
93+
tests := []struct {
94+
name string
95+
ident auth.Identity
96+
wantOK bool
97+
wantStatus int
98+
}{
99+
{
100+
name: "owner allowed",
101+
ident: auth.Identity{
102+
UserID: owner.ID,
103+
Role: users.RoleUser,
104+
AuthMethod: "session",
105+
},
106+
wantOK: true,
107+
},
108+
{
109+
name: "non owner rejected",
110+
ident: auth.Identity{
111+
UserID: other.ID,
112+
Role: users.RoleUser,
113+
AuthMethod: "session",
114+
},
115+
wantOK: false,
116+
wantStatus: http.StatusForbidden,
117+
},
118+
{
119+
name: "read only owner rejected",
120+
ident: auth.Identity{
121+
UserID: "u_admin",
122+
Role: users.RoleSuperAdmin,
123+
AuthMethod: "session",
124+
ActAsUserID: owner.ID,
125+
},
126+
wantOK: false,
127+
wantStatus: http.StatusForbidden,
128+
},
129+
}
130+
131+
for _, tt := range tests {
132+
t.Run(tt.name, func(t *testing.T) {
133+
rr := httptest.NewRecorder()
134+
ok := s.authorizeSkillInstallTarget(rr, skillInstallRequest(tt.ident), "agt_owner")
135+
if ok != tt.wantOK {
136+
t.Fatalf("ok = %v, want %v", ok, tt.wantOK)
137+
}
138+
if !tt.wantOK && rr.Code != tt.wantStatus {
139+
t.Fatalf("status = %d, want %d", rr.Code, tt.wantStatus)
140+
}
141+
})
142+
}
143+
}
144+
145+
func skillInstallRequest(ident auth.Identity) *http.Request {
146+
req := httptest.NewRequest(http.MethodPost, "/api/skills/install", nil)
147+
return req.WithContext(auth.WithIdentity(req.Context(), ident))
148+
}
149+
150+
func newSkillInstallAuthServer(t *testing.T, ctx context.Context) (*Server, store.Store, *users.Accounts) {
151+
t.Helper()
152+
153+
dbPath := filepath.Join(t.TempDir(), "fastclaw.db")
154+
st, err := store.NewDBStore("sqlite", "file:"+dbPath+"?cache=shared")
155+
if err != nil {
156+
t.Fatalf("NewDBStore: %v", err)
157+
}
158+
t.Cleanup(func() {
159+
_ = st.Close()
160+
})
161+
if err := st.Migrate(ctx); err != nil {
162+
t.Fatalf("Migrate: %v", err)
163+
}
164+
accts, err := users.NewAccounts(st)
165+
if err != nil {
166+
t.Fatalf("NewAccounts: %v", err)
167+
}
168+
s := NewServer(0)
169+
s.SetStore(st)
170+
return s, st, accts
171+
}
172+
173+
func createSkillInstallTestUser(t *testing.T, ctx context.Context, accts *users.Accounts, username, role string) *users.Account {
174+
t.Helper()
175+
176+
acct, err := accts.Create(ctx, users.CreateInput{
177+
Username: username,
178+
Email: username + "@example.test",
179+
Password: "password",
180+
Role: role,
181+
})
182+
if err != nil {
183+
t.Fatalf("Create(%s): %v", username, err)
184+
}
185+
return acct
186+
}

0 commit comments

Comments
 (0)