forked from molecule-ai/molecule-core
Issue #120 (HIGH — immediately exploitable): PATCH /workspaces/:id was registered on the root router with no auth middleware. An attacker with any workspace UUID could: - Escalate tier (tier 4 = 4 GB RAM allocation) - Rewrite parent_id to subvert CanCommunicate A2A access control - Swap runtime image on next restart - Redirect workspace_dir host bind-mount to arbitrary path Fix: move PATCH into the wsAdmin AdminAuth group alongside POST, DELETE. The canvas position-persist call already has an AdminAuth token (required for GET /workspaces list on initial load) so no canvas regression. Also add workspace-existence guard in Update handler — previously returned 200 with zero rows affected for nonexistent IDs. Issue #113 (MEDIUM — schedule IDOR, carry-over from prior cycle): PATCH /workspaces/:id/schedules/:scheduleId and DELETE operated on scheduleID alone (WHERE id = $1), allowing any authenticated caller to modify or delete schedules belonging to other workspaces. Fix: bind workspace_id = c.Param("id") in both Update and Delete handlers; add AND workspace_id = $N to all schedule SQL queries. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
69ba583508
commit
590eefb5ae
@ -151,6 +151,7 @@ type updateScheduleRequest struct {
|
||||
// provided fields are changed — no dynamic SQL construction.
|
||||
func (h *ScheduleHandler) Update(c *gin.Context) {
|
||||
scheduleID := c.Param("scheduleId")
|
||||
workspaceID := c.Param("id") // #113: bind to owning workspace to prevent IDOR
|
||||
ctx := c.Request.Context()
|
||||
|
||||
var body updateScheduleRequest
|
||||
@ -164,7 +165,8 @@ func (h *ScheduleHandler) Update(c *gin.Context) {
|
||||
if body.CronExpr != nil || body.Timezone != nil {
|
||||
var currentCron, currentTZ string
|
||||
err := db.DB.QueryRowContext(ctx,
|
||||
`SELECT cron_expr, timezone FROM workspace_schedules WHERE id = $1`, scheduleID,
|
||||
`SELECT cron_expr, timezone FROM workspace_schedules WHERE id = $1 AND workspace_id = $2`,
|
||||
scheduleID, workspaceID,
|
||||
).Scan(¤tCron, ¤tTZ)
|
||||
if err != nil {
|
||||
c.JSON(http.StatusNotFound, gin.H{"error": "schedule not found"})
|
||||
@ -199,8 +201,8 @@ func (h *ScheduleHandler) Update(c *gin.Context) {
|
||||
enabled = COALESCE($6, enabled),
|
||||
next_run_at = COALESCE($7, next_run_at),
|
||||
updated_at = now()
|
||||
WHERE id = $1
|
||||
`, scheduleID, body.Name, body.CronExpr, body.Timezone, body.Prompt, body.Enabled, nextRunAt)
|
||||
WHERE id = $1 AND workspace_id = $8
|
||||
`, scheduleID, body.Name, body.CronExpr, body.Timezone, body.Prompt, body.Enabled, nextRunAt, workspaceID)
|
||||
if err != nil {
|
||||
log.Printf("Schedules.Update: error: %v", err)
|
||||
c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to update schedule"})
|
||||
@ -218,10 +220,12 @@ func (h *ScheduleHandler) Update(c *gin.Context) {
|
||||
// Delete removes a schedule.
|
||||
func (h *ScheduleHandler) Delete(c *gin.Context) {
|
||||
scheduleID := c.Param("scheduleId")
|
||||
workspaceID := c.Param("id") // #113: bind to owning workspace to prevent IDOR
|
||||
ctx := c.Request.Context()
|
||||
|
||||
result, err := db.DB.ExecContext(ctx,
|
||||
`DELETE FROM workspace_schedules WHERE id = $1`, scheduleID)
|
||||
`DELETE FROM workspace_schedules WHERE id = $1 AND workspace_id = $2`,
|
||||
scheduleID, workspaceID)
|
||||
if err != nil {
|
||||
c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to delete schedule"})
|
||||
return
|
||||
|
||||
@ -402,6 +402,16 @@ func (h *WorkspaceHandler) Update(c *gin.Context) {
|
||||
|
||||
ctx := c.Request.Context()
|
||||
|
||||
// #120: guard — return 404 for nonexistent workspace IDs instead of
|
||||
// silently applying zero-row UPDATEs and returning 200.
|
||||
var exists bool
|
||||
if err := db.DB.QueryRowContext(ctx,
|
||||
`SELECT EXISTS(SELECT 1 FROM workspaces WHERE id = $1)`, id,
|
||||
).Scan(&exists); err != nil || !exists {
|
||||
c.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"})
|
||||
return
|
||||
}
|
||||
|
||||
if name, ok := body["name"]; ok {
|
||||
if _, err := db.DB.ExecContext(ctx, `UPDATE workspaces SET name = $2, updated_at = now() WHERE id = $1`, id, name); err != nil {
|
||||
log.Printf("Update name error for %s: %v", id, err)
|
||||
|
||||
@ -87,23 +87,26 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi
|
||||
// Scrape with: curl http://localhost:8080/metrics
|
||||
r.GET("/metrics", metrics.Handler())
|
||||
|
||||
// Workspace read-only endpoints accessible without an explicit workspace ID.
|
||||
// /workspaces/:id and PATCH (position-persist) remain open for the canvas
|
||||
// browser frontend which does not carry a bearer token in those calls.
|
||||
// Single-workspace read — open so canvas nodes can fetch their own state
|
||||
// without a token (used by WorkspaceNode polling and health checks).
|
||||
r.GET("/workspaces/:id", wh.Get)
|
||||
r.PATCH("/workspaces/:id", wh.Update)
|
||||
|
||||
// C1 + C20 + C18-adjacent: workspace list and mutating operations all gated
|
||||
// behind AdminAuth — any valid workspace bearer token grants access.
|
||||
// C1 + C20 + C18-adjacent + #120: workspace list and ALL mutating operations
|
||||
// gated behind AdminAuth — any valid workspace bearer token grants access.
|
||||
// Fail-open when no tokens exist anywhere (fresh install / pre-Phase-30).
|
||||
// This blocks:
|
||||
// C1 — unauthenticated GET /workspaces (workspace topology exposure)
|
||||
// C20 — unauthenticated DELETE /workspaces/:id (mass-deletion attack)
|
||||
// unauthenticated POST /workspaces (workspace creation)
|
||||
// C1 — unauthenticated GET /workspaces (workspace topology exposure)
|
||||
// C20 — unauthenticated DELETE /workspaces/:id (mass-deletion attack)
|
||||
// unauthenticated POST /workspaces (workspace creation)
|
||||
// #120 — unauthenticated PATCH /workspaces/:id (tier escalation, parent_id
|
||||
// hierarchy manipulation, runtime swap, workspace_dir path hijack)
|
||||
// NOTE: canvas position-persist (PATCH with {x,y}) uses the same AdminAuth
|
||||
// token already required for GET /workspaces list on initial load.
|
||||
{
|
||||
wsAdmin := r.Group("", middleware.AdminAuth(db.DB))
|
||||
wsAdmin.GET("/workspaces", wh.List)
|
||||
wsAdmin.POST("/workspaces", wh.Create)
|
||||
wsAdmin.PATCH("/workspaces/:id", wh.Update)
|
||||
wsAdmin.DELETE("/workspaces/:id", wh.Delete)
|
||||
}
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user