fix: close Pi management final review findings

This commit is contained in:
2026-08-14 20:15:21 +02:00
parent 225d13cd75
commit 900faad983
13 changed files with 621 additions and 71 deletions
+116 -6
View File
@@ -2,15 +2,21 @@
package output
import (
"bytes"
"encoding/json"
"errors"
"io"
"regexp"
"sort"
"strings"
"github.com/aritmolab/thothii/tools/thothctl/internal/safeio"
"github.com/compose-spec/compose-go/v2/dotenv"
)
var credentialField = regexp.MustCompile(`(?im)(\b[\w.-]*(?:password|token|key)[\w.-]*\s*[:=]\s*)(?:"[^"\r\n]*"|'[^'\r\n]*'|[^\s,;]+)`)
var credentialField = regexp.MustCompile(`(?im)((?:"|')?[\w.-]*(?:password|token|key|secret|credential)[\w.-]*(?:"|')?\s*[:=]\s*)(?:"(?:\\.|[^"\\\r\n])*"|'[^'\r\n]*'|[^\s,;}]+)`)
var dotenvAssignment = regexp.MustCompile(`(?m)^\s*(?:export\s+)?[A-Za-z_][A-Za-z0-9_.-]*\s*=`)
const maxSecretFileBytes = 64 * 1024
@@ -18,6 +24,12 @@ const maxSecretSourceFiles = 32
const maxSecretSourceBytes = 256 * 1024
const maxSecretValuesPerFile = 1024
const maxSecretValues = 4096
const maxJSONSecretDepth = 32
const maxDiagnosticDetailBytes = 512
// Sanitize redacts common credential fields and every supplied secret value.
@@ -28,6 +40,9 @@ func Sanitize(text string, secretValues []string) string {
for _, value := range values {
if value != "" {
text = strings.ReplaceAll(text, value, "[REDACTED]")
if encoded, err := json.Marshal(value); err == nil {
text = strings.ReplaceAll(text, string(encoded), "[REDACTED]")
}
}
}
return text
@@ -60,7 +75,7 @@ func SecretValuesFromFiles(paths []string) ([]string, error) {
seen := make(map[string]struct{})
var totalBytes int64
for _, path := range paths {
value, size, err := readSecretFile(path)
contents, size, err := readSecretFile(path)
if err != nil {
return nil, err
}
@@ -68,21 +83,116 @@ func SecretValuesFromFiles(paths []string) ([]string, error) {
if totalBytes > maxSecretSourceBytes {
return nil, errors.New("declared secret file could not be read")
}
if value != "" {
extracted, err := extractSecretValues(contents)
if err != nil {
return nil, errors.New("declared secret file could not be read")
}
for _, value := range extracted {
if value == "" {
continue
}
if _, exists := seen[value]; exists {
continue
}
values = append(values, value)
seen[value] = struct{}{}
if len(values) > maxSecretValues {
return nil, errors.New("declared secret file could not be read")
}
}
}
return values, nil
}
func readSecretFile(path string) (string, int64, error) {
func readSecretFile(path string) ([]byte, int64, error) {
contents, err := safeio.ReadCanonicalRegular(path, maxSecretFileBytes)
if err != nil {
return "", 0, errors.New("declared secret file could not be read")
return nil, 0, errors.New("declared secret file could not be read")
}
return strings.TrimRight(string(contents), "\r\n"), int64(len(contents)), nil
return contents, int64(len(contents)), nil
}
func extractSecretValues(contents []byte) ([]string, error) {
whole := strings.TrimRight(string(contents), "\r\n")
trimmed := bytes.TrimSpace(contents)
if len(trimmed) == 0 {
return nil, nil
}
values := make([]string, 0, 8)
if whole != "" {
values = append(values, whole)
}
if trimmed[0] == '{' || trimmed[0] == '[' {
var document any
decoder := json.NewDecoder(bytes.NewReader(trimmed))
decoder.UseNumber()
if err := decoder.Decode(&document); err != nil {
return nil, err
}
var extra any
if err := decoder.Decode(&extra); !errors.Is(err, io.EOF) {
if err == nil {
return nil, errors.New("secret JSON contains multiple documents")
}
return nil, err
}
count := 0
if err := collectJSONSecretValues(document, 0, &count, &values); err != nil {
return nil, err
}
return values, nil
}
if dotenvAssignment.Match(trimmed) {
parsed, err := dotenv.Parse(bytes.NewReader(contents))
if err != nil || len(parsed) > maxSecretValuesPerFile {
return nil, errors.New("secret dotenv bundle is invalid")
}
keys := make([]string, 0, len(parsed))
for key := range parsed {
keys = append(keys, key)
}
sort.Strings(keys)
for _, key := range keys {
if parsed[key] != "" {
values = append(values, parsed[key])
}
}
}
return values, nil
}
func collectJSONSecretValues(value any, depth int, count *int, values *[]string) error {
if depth > maxJSONSecretDepth {
return errors.New("secret JSON nesting is too deep")
}
switch typed := value.(type) {
case map[string]any:
keys := make([]string, 0, len(typed))
for key := range typed {
keys = append(keys, key)
}
sort.Strings(keys)
for _, key := range keys {
if err := collectJSONSecretValues(typed[key], depth+1, count, values); err != nil {
return err
}
}
case []any:
for _, item := range typed {
if err := collectJSONSecretValues(item, depth+1, count, values); err != nil {
return err
}
}
default:
*count++
if *count > maxSecretValuesPerFile {
return errors.New("secret JSON contains too many scalar values")
}
if scalar, ok := typed.(string); ok && scalar != "" {
*values = append(*values, scalar)
}
}
return nil
}
@@ -3,6 +3,7 @@ package output
import (
"os"
"path/filepath"
"strings"
"testing"
)
@@ -34,6 +35,113 @@ func TestSanitizeRedactsSecretFileContents(t *testing.T) {
}
}
func TestSecretValuesFromFilesRedactsNestedJSONScalars(t *testing.T) {
t.Parallel()
secretFile := filepath.Join(physicalTempDir(t), "pi-auth.json")
contents := `{
"providers": {
"dummy-provider": {
"auth": {
"key": "dummy-canary-json-key",
"tokens": {
"access": "dummy-canary-json-access",
"refresh": "dummy-canary-json-refresh"
}
}
}
}
}`
if err := os.WriteFile(secretFile, []byte(contents), 0o600); err != nil {
t.Fatal(err)
}
secrets, err := SecretValuesFromFiles([]string{secretFile})
if err != nil {
t.Fatalf("SecretValuesFromFiles() error = %v", err)
}
got := Sanitize(
"unlabelled dummy-canary-json-key dummy-canary-json-access dummy-canary-json-refresh",
secrets,
)
for _, canary := range []string{
"dummy-canary-json-key",
"dummy-canary-json-access",
"dummy-canary-json-refresh",
} {
if strings.Contains(got, canary) {
t.Fatalf("Sanitize() exposed nested JSON scalar %q: %q", canary, got)
}
}
}
func TestSecretValuesFromFilesRedactsEveryDotenvBundleValue(t *testing.T) {
t.Parallel()
secretFile := filepath.Join(physicalTempDir(t), "thothii.secrets")
contents := "MODEL_API_KEY=dummy-canary-bundle-model\n" +
"DWH_PASSWORD='dummy-canary-bundle-dwh'\n" +
"SESSION_TOKEN=\"dummy-canary-bundle-session\"\n"
if err := os.WriteFile(secretFile, []byte(contents), 0o600); err != nil {
t.Fatal(err)
}
secrets, err := SecretValuesFromFiles([]string{secretFile})
if err != nil {
t.Fatalf("SecretValuesFromFiles() error = %v", err)
}
got := Sanitize(
"unlabelled dummy-canary-bundle-model dummy-canary-bundle-dwh dummy-canary-bundle-session",
secrets,
)
for _, canary := range []string{
"dummy-canary-bundle-model",
"dummy-canary-bundle-dwh",
"dummy-canary-bundle-session",
} {
if strings.Contains(got, canary) {
t.Fatalf("Sanitize() exposed dotenv bundle scalar %q: %q", canary, got)
}
}
}
func TestSanitizeRecognizesQuotedCredentialKeys(t *testing.T) {
t.Parallel()
got := Sanitize(`{"key":"dummy-canary-quoted-key","safe":"visible"}`, nil)
if strings.Contains(got, "dummy-canary-quoted-key") || !strings.Contains(got, `"safe":"visible"`) {
t.Fatalf("Sanitize() = %q, want only the quoted credential field redacted", got)
}
}
func TestSecretValuesFromFilesRejectsMalformedJSONAndExcessiveScalars(t *testing.T) {
t.Parallel()
t.Run("malformed", func(t *testing.T) {
secretFile := filepath.Join(physicalTempDir(t), "malformed-auth.json")
if err := os.WriteFile(secretFile, []byte(`{"auth":{"key":"dummy-canary-malformed"}`), 0o600); err != nil {
t.Fatal(err)
}
if _, err := SecretValuesFromFiles([]string{secretFile}); err == nil {
t.Fatal("SecretValuesFromFiles() error = nil, want malformed-JSON failure")
}
})
t.Run("scalar bound", func(t *testing.T) {
secretFile := filepath.Join(physicalTempDir(t), "many-auth-values.json")
values := make([]string, 1025)
for index := range values {
values[index] = `"dummy-canary-value"`
}
if err := os.WriteFile(secretFile, []byte("["+strings.Join(values, ",")+"]"), 0o600); err != nil {
t.Fatal(err)
}
if _, err := SecretValuesFromFiles([]string{secretFile}); err == nil {
t.Fatal("SecretValuesFromFiles() error = nil, want scalar-count failure")
}
})
}
func TestSecretValuesFromFilesRejectsOversizedFiles(t *testing.T) {
t.Parallel()