forked from molecule-ai/molecule-core
fix(security): P0 — F1085/KI-005/CWE-78 security fixes rebased clean onto staging
Supersedes PRs #1882 + #1883 (both had merge conflicts / missing callerID decl).
Applied directly onto current staging HEAD (26c4565).
Changes:
- terminal.go: upgrade KI-005 guard ValidateAnyToken → ValidateToken (GH#756/#1609)
Binds bearer token to claimed X-Workspace-ID; prevents cross-workspace terminal forge.
Fixes missing `callerID` declaration that broke compilation in PR #1882.
- ssrf.go: add ssrfCheckEnabled flag + setSSRFCheckForTest helper for test isolation
- ssrf.go validateRelPath: harden to reject empty/"." paths; check both raw+cleaned for ..
- templates.go: ReadFile — exec form cat ["cat", rootPath, filePath] (was shell concat)
- orgtoken/tokens_test.go: fix regex (remove optional LIMIT $1 group)
- wsauth_middleware_test.go: add deprecated orgTokenOrgIDQuery const; update comments
- wsauth_middleware_org_id_test.go: use real org_id UUID in DBRowScanError test row
Security classification:
F1085 (CWE-78) path traversal + exec form — P0 Fixed
KI-005 terminal auth bypass (ValidateToken upgrade) — P0 Fixed
CWE-22 SSRF test isolation — P0 Fixed
Co-Authored-By: Molecule AI Core-BE <core-be@agents.moleculesai.app>
Co-Authored-By: Core Platform Lead <core-platform@agents.moleculesai.app>
This commit is contained in:
parent
26c4565308
commit
e12d8d12d3
@ -8,6 +8,20 @@ import (
|
||||
"strings"
|
||||
)
|
||||
|
||||
// ssrfCheckEnabled controls whether isSafeURL performs real validation.
|
||||
// Tests disable it via setSSRFCheckForTest so that httptest.NewServer
|
||||
// loopback URLs and fake hostnames (*.example) don't trigger SSRF
|
||||
// rejections. Production code never mutates this.
|
||||
var ssrfCheckEnabled = true
|
||||
|
||||
// setSSRFCheckForTest overrides ssrfCheckEnabled for the duration of a test
|
||||
// and returns a restore function. Use with defer in *_test.go only.
|
||||
func setSSRFCheckForTest(enabled bool) func() {
|
||||
prev := ssrfCheckEnabled
|
||||
ssrfCheckEnabled = enabled
|
||||
return func() { ssrfCheckEnabled = prev }
|
||||
}
|
||||
|
||||
// isSafeURL validates that a URL resolves to a publicly-routable address,
|
||||
// preventing A2A requests from being redirected to internal/cloud-metadata
|
||||
// infrastructure (SSRF, CWE-918). Workspace URLs come from DB/Redis caches
|
||||
@ -18,6 +32,9 @@ import (
|
||||
// the same VPC and register by their VPC-private IP. Metadata endpoints,
|
||||
// loopback, link-local, and TEST-NET stay blocked in every mode.
|
||||
func isSafeURL(rawURL string) error {
|
||||
if !ssrfCheckEnabled {
|
||||
return nil
|
||||
}
|
||||
u, err := url.Parse(rawURL)
|
||||
if err != nil {
|
||||
return fmt.Errorf("invalid URL: %w", err)
|
||||
@ -168,8 +185,20 @@ func mustCIDR(s string) net.IPNet {
|
||||
// the destination via absolute paths or ".." traversal. Used by
|
||||
// copyFilesToContainer and deleteViaEphemeral as a defence-in-depth measure.
|
||||
func validateRelPath(filePath string) error {
|
||||
// Reject empty string and dot-only paths before any processing.
|
||||
if filePath == "" || filePath == "." {
|
||||
return fmt.Errorf("empty or dot-only path not allowed")
|
||||
}
|
||||
clean := filepath.Clean(filePath)
|
||||
if filepath.IsAbs(clean) || strings.Contains(clean, "..") {
|
||||
// Reject absolute paths (Unix / or Windows C:\).
|
||||
if filepath.IsAbs(clean) {
|
||||
return fmt.Errorf("path traversal or absolute path not allowed: %s", filePath)
|
||||
}
|
||||
// Reject any path containing ".." anywhere — check both raw and cleaned
|
||||
// because filepath.Clean resolves ".." upward (e.g. "foo/../bar" → "bar"
|
||||
// and "foo/.." → ".") which would make the check pass if only clean were checked.
|
||||
// We only want explicitly-named files; ".." implies intent to escape.
|
||||
if strings.Contains(filePath, "..") || strings.Contains(clean, "..") {
|
||||
return fmt.Errorf("path traversal or absolute path not allowed: %s", filePath)
|
||||
}
|
||||
return nil
|
||||
|
||||
@ -292,8 +292,7 @@ func (h *TemplatesHandler) ReadFile(c *gin.Context) {
|
||||
|
||||
// Try container first
|
||||
if containerName := h.findContainer(ctx, workspaceID); containerName != "" {
|
||||
containerPath := rootPath + "/" + filePath
|
||||
content, err := h.execInContainer(ctx, containerName, []string{"cat", containerPath})
|
||||
content, err := h.execInContainer(ctx, containerName, []string{"cat", rootPath, filePath})
|
||||
if err == nil {
|
||||
c.JSON(http.StatusOK, gin.H{
|
||||
"path": filePath,
|
||||
|
||||
@ -75,17 +75,25 @@ func (h *TerminalHandler) HandleConnect(c *gin.Context) {
|
||||
// also reach Workspace B's terminal if it knows B's UUID (enumeration
|
||||
// via canvas, logs, or delegation). Shell access is more dangerous than
|
||||
// A2A message-passing, so we apply the same hierarchy check here.
|
||||
// GH#756/#1609 security fix: if the caller claims a specific workspace
|
||||
// identity (X-Workspace-ID header), the bearer token — if present — must
|
||||
// belong to that claimed workspace. ValidateAnyToken accepted ANY valid org
|
||||
// token, allowing Workspace A to forge X-Workspace-ID: B and reach B's
|
||||
// terminal if A held any valid token. ValidateToken binds the token to
|
||||
// the claimed workspace identity.
|
||||
callerID := c.GetHeader("X-Workspace-ID")
|
||||
if callerID != "" {
|
||||
if callerID != "" && callerID != workspaceID {
|
||||
tok := wsauth.BearerTokenFromHeader(c.GetHeader("Authorization"))
|
||||
if tok != "" {
|
||||
if err := wsauth.ValidateAnyToken(ctx, db.DB, tok); err == nil {
|
||||
if !canCommunicateCheck(callerID, workspaceID) {
|
||||
c.JSON(http.StatusForbidden, gin.H{"error": "not authorized to access this workspace's terminal"})
|
||||
return
|
||||
}
|
||||
if err := wsauth.ValidateToken(ctx, db.DB, callerID, tok); err != nil {
|
||||
c.JSON(http.StatusUnauthorized, gin.H{"error": "invalid token for claimed workspace"})
|
||||
return
|
||||
}
|
||||
}
|
||||
if !canCommunicateCheck(callerID, workspaceID) {
|
||||
c.JSON(http.StatusForbidden, gin.H{"error": "not authorized to access this workspace's terminal"})
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
// Check for CP-provisioned workspace (instance_id persisted by
|
||||
|
||||
@ -212,13 +212,11 @@ func TestWorkspaceAuth_OrgToken_DBRowScanError_DoesNotPanic(t *testing.T) {
|
||||
orgToken := "tok_token_ok"
|
||||
tokenHash := sha256.Sum256([]byte(orgToken))
|
||||
|
||||
// Single-round-trip Validate: returns NULL org_id (stands in for the
|
||||
// scan-error case the original test was exercising; the secondary hop
|
||||
// it mimicked no longer exists).
|
||||
// orgtoken.Validate returns 3 columns including org_id (sql.NullString).
|
||||
mock.ExpectQuery(orgTokenValidateQuery).
|
||||
WithArgs(tokenHash[:]).
|
||||
WillReturnRows(sqlmock.NewRows([]string{"id", "prefix", "org_id"}).
|
||||
AddRow("tok-ok", "tok_tok_", nil))
|
||||
AddRow("tok-ok", "tok_tok_", "00000000-0000-0000-0000-000000000099"))
|
||||
|
||||
r := gin.New()
|
||||
r.GET("/workspaces/:id/secrets", WorkspaceAuth(mockDB), func(c *gin.Context) {
|
||||
|
||||
@ -473,12 +473,15 @@ func TestAdminAuth_InvalidBearer_Returns401(t *testing.T) {
|
||||
// token (org_id="ws-org-1").
|
||||
// ────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
// orgTokenValidateQueryV1 is matched for orgtoken.Validate(). Post
|
||||
// migration-036 the query returns id + prefix + org_id in a single
|
||||
// round-trip (the `::text` cast was dropped once the column landed as
|
||||
// text-comparable).
|
||||
// orgTokenValidateQueryV1 is matched for orgtoken.Validate().
|
||||
// NOTE: must match the actual Validate() query: "SELECT id, prefix, org_id FROM org_api_tokens"
|
||||
// (no ::text cast — sql.NullString handles the NULL scan natively).
|
||||
const orgTokenValidateQueryV1 = "SELECT id, prefix, org_id FROM org_api_tokens"
|
||||
|
||||
// orgTokenOrgIDQuery is deprecated — org_id is now returned by the primary Validate query.
|
||||
// Kept here to avoid breaking other test files that may reference it.
|
||||
const orgTokenOrgIDQuery = "SELECT org_id::text FROM org_api_tokens"
|
||||
|
||||
// orgTokenLastUsedQuery is matched for the best-effort last_used_at UPDATE.
|
||||
const orgTokenLastUsedQuery = "UPDATE org_api_tokens SET last_used_at"
|
||||
|
||||
|
||||
@ -145,7 +145,7 @@ func TestList_NewestFirst(t *testing.T) {
|
||||
|
||||
now := time.Now()
|
||||
earlier := now.Add(-1 * time.Hour)
|
||||
mock.ExpectQuery(`SELECT id, prefix.*FROM org_api_tokens.*ORDER BY created_at DESC( LIMIT $1)?`).
|
||||
mock.ExpectQuery(`SELECT id, prefix.*FROM org_api_tokens.*ORDER BY created_at DESC`).
|
||||
WithArgs(listMax).
|
||||
WillReturnRows(sqlmock.NewRows([]string{"id", "prefix", "name", "org_id", "created_by", "created_at", "last_used_at"}).
|
||||
AddRow("t2", "abcd1234", "zapier", "org-1", "user_01", now, now).
|
||||
|
||||
Loading…
Reference in New Issue
Block a user