fix: address validated pre-release review findings
- commit
- eae8025ec18dfb67777f47b523ddbb3111f9f7e9
- author
- brice · · 2026-04-08 16:17 UTC
- parent
- 0986ec3
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
10 files · +149 -11
M internal/adapters/in/cli/backup.go +2 -1
| @@ -7,9 +7,10 @@ import ( | |||
| 7 | 7 | "text/tabwriter" | |
| 8 | 8 | "time" | |
| 9 | 9 | ||
| 10 | + | "github.com/spf13/cobra" | |
| 11 | + | ||
| 10 | 12 | "github.com/bnema/gordon/internal/adapters/dto" | |
| 11 | 13 | "github.com/bnema/gordon/pkg/bytesize" | |
| 12 | - | "github.com/spf13/cobra" | |
| 13 | 14 | ) | |
| 14 | 15 | ||
| 15 | 16 | // newBackupCmd creates the backup command group. | |
| @@ -7,9 +7,10 @@ import ( | |||||
| 7 | "text/tabwriter" | 7 | "text/tabwriter" | ||
| 8 | "time" | 8 | "time" | ||
| 9 | 9 | ||||
| 10 | + | "github.com/spf13/cobra" | |||
| 11 | + | ||||
| 10 | "github.com/bnema/gordon/internal/adapters/dto" | 12 | "github.com/bnema/gordon/internal/adapters/dto" | ||
| 11 | "github.com/bnema/gordon/pkg/bytesize" | 13 | "github.com/bnema/gordon/pkg/bytesize" | ||
| 12 | - | "github.com/spf13/cobra" | |||
| 13 | ) | 14 | ) | ||
| 14 | 15 | ||||
| 15 | // newBackupCmd creates the backup command group. | 16 | // newBackupCmd creates the backup command group. | ||
M internal/adapters/in/cli/remote/config.go +3 -1
| @@ -110,7 +110,9 @@ func migrateClientConfig(config *ClientConfig) { | |||
| 110 | 110 | config.Active = "default" | |
| 111 | 111 | } | |
| 112 | 112 | ||
| 113 | - | _ = SaveRemotes("", config) | |
| 113 | + | if err := SaveRemotes("", config); err != nil { | |
| 114 | + | fmt.Fprintf(os.Stderr, "Warning: failed to save migrated remotes: %v\n", err) | |
| 115 | + | } | |
| 114 | 116 | } | |
| 115 | 117 | ||
| 116 | 118 | func defaultGordonTomlPath() string { | |
| @@ -110,7 +110,9 @@ func migrateClientConfig(config *ClientConfig) { | |||||
| 110 | config.Active = "default" | 110 | config.Active = "default" | ||
| 111 | } | 111 | } | ||
| 112 | 112 | ||||
| 113 | - | _ = SaveRemotes("", config) | 113 | + | if err := SaveRemotes("", config); err != nil { |
| 114 | + | fmt.Fprintf(os.Stderr, "Warning: failed to save migrated remotes: %v\n", err) | |||
| 115 | + | } | |||
| 114 | } | 116 | } | ||
| 115 | 117 | ||||
| 116 | func defaultGordonTomlPath() string { | 118 | func defaultGordonTomlPath() string { | ||
M internal/adapters/in/http/middleware/accesslog.go +42 -1
| @@ -3,6 +3,7 @@ package middleware | |||
| 3 | 3 | import ( | |
| 4 | 4 | "net" | |
| 5 | 5 | "net/http" | |
| 6 | + | "net/url" | |
| 6 | 7 | "strings" | |
| 7 | 8 | "time" | |
| 8 | 9 | ||
| @@ -60,7 +61,7 @@ func AccessLogger(writer out.AccessLogWriter, excludeHealthChecks bool, log zero | |||
| 60 | 61 | Method: r.Method, | |
| 61 | 62 | Host: r.Host, | |
| 62 | 63 | Path: r.URL.Path, | |
| 63 | - | Query: r.URL.RawQuery, | |
| 64 | + | Query: sanitizeLoggedQuery(r.URL.RawQuery), | |
| 64 | 65 | Status: rw.StatusCode(), | |
| 65 | 66 | BytesSent: rw.BytesWritten(), | |
| 66 | 67 | DurationMS: durationMS, | |
| @@ -85,3 +86,43 @@ func AccessLogger(writer out.AccessLogWriter, excludeHealthChecks bool, log zero | |||
| 85 | 86 | }) | |
| 86 | 87 | } | |
| 87 | 88 | } | |
| 89 | + | ||
| 90 | + | func sanitizeLoggedQuery(rawQuery string) string { | |
| 91 | + | if rawQuery == "" { | |
| 92 | + | return "" | |
| 93 | + | } | |
| 94 | + | ||
| 95 | + | parts := strings.Split(rawQuery, "&") | |
| 96 | + | for i, part := range parts { | |
| 97 | + | if part == "" { | |
| 98 | + | continue | |
| 99 | + | } | |
| 100 | + | ||
| 101 | + | key, _, found := strings.Cut(part, "=") | |
| 102 | + | decodedKey, err := url.QueryUnescape(key) | |
| 103 | + | if err != nil { | |
| 104 | + | decodedKey = key | |
| 105 | + | } | |
| 106 | + | ||
| 107 | + | if !isSensitiveQueryKey(decodedKey) { | |
| 108 | + | continue | |
| 109 | + | } | |
| 110 | + | ||
| 111 | + | if found { | |
| 112 | + | parts[i] = key + "=[REDACTED]" | |
| 113 | + | } else { | |
| 114 | + | parts[i] = key | |
| 115 | + | } | |
| 116 | + | } | |
| 117 | + | ||
| 118 | + | return strings.Join(parts, "&") | |
| 119 | + | } | |
| 120 | + | ||
| 121 | + | func isSensitiveQueryKey(key string) bool { | |
| 122 | + | switch strings.ToLower(key) { | |
| 123 | + | case "access_token", "auth", "code", "key", "password", "refresh_token", "reset", "secret", "sig", "signature", "token": | |
| 124 | + | return true | |
| 125 | + | default: | |
| 126 | + | return false | |
| 127 | + | } | |
| 128 | + | } | |
| @@ -3,6 +3,7 @@ package middleware | |||||
| 3 | import ( | 3 | import ( | ||
| 4 | "net" | 4 | "net" | ||
| 5 | "net/http" | 5 | "net/http" | ||
| 6 | + | "net/url" | |||
| 6 | "strings" | 7 | "strings" | ||
| 7 | "time" | 8 | "time" | ||
| 8 | 9 | ||||
| @@ -60,7 +61,7 @@ func AccessLogger(writer out.AccessLogWriter, excludeHealthChecks bool, log zero | |||||
| 60 | Method: r.Method, | 61 | Method: r.Method, | ||
| 61 | Host: r.Host, | 62 | Host: r.Host, | ||
| 62 | Path: r.URL.Path, | 63 | Path: r.URL.Path, | ||
| 63 | - | Query: r.URL.RawQuery, | 64 | + | Query: sanitizeLoggedQuery(r.URL.RawQuery), |
| 64 | Status: rw.StatusCode(), | 65 | Status: rw.StatusCode(), | ||
| 65 | BytesSent: rw.BytesWritten(), | 66 | BytesSent: rw.BytesWritten(), | ||
| 66 | DurationMS: durationMS, | 67 | DurationMS: durationMS, | ||
| @@ -85,3 +86,43 @@ func AccessLogger(writer out.AccessLogWriter, excludeHealthChecks bool, log zero | |||||
| 85 | }) | 86 | }) | ||
| 86 | } | 87 | } | ||
| 87 | } | 88 | } | ||
| 89 | + | ||||
| 90 | + | func sanitizeLoggedQuery(rawQuery string) string { | |||
| 91 | + | if rawQuery == "" { | |||
| 92 | + | return "" | |||
| 93 | + | } | |||
| 94 | + | ||||
| 95 | + | parts := strings.Split(rawQuery, "&") | |||
| 96 | + | for i, part := range parts { | |||
| 97 | + | if part == "" { | |||
| 98 | + | continue | |||
| 99 | + | } | |||
| 100 | + | ||||
| 101 | + | key, _, found := strings.Cut(part, "=") | |||
| 102 | + | decodedKey, err := url.QueryUnescape(key) | |||
| 103 | + | if err != nil { | |||
| 104 | + | decodedKey = key | |||
| 105 | + | } | |||
| 106 | + | ||||
| 107 | + | if !isSensitiveQueryKey(decodedKey) { | |||
| 108 | + | continue | |||
| 109 | + | } | |||
| 110 | + | ||||
| 111 | + | if found { | |||
| 112 | + | parts[i] = key + "=[REDACTED]" | |||
| 113 | + | } else { | |||
| 114 | + | parts[i] = key | |||
| 115 | + | } | |||
| 116 | + | } | |||
| 117 | + | ||||
| 118 | + | return strings.Join(parts, "&") | |||
| 119 | + | } | |||
| 120 | + | ||||
| 121 | + | func isSensitiveQueryKey(key string) bool { | |||
| 122 | + | switch strings.ToLower(key) { | |||
| 123 | + | case "access_token", "auth", "code", "key", "password", "refresh_token", "reset", "secret", "sig", "signature", "token": | |||
| 124 | + | return true | |||
| 125 | + | default: | |||
| 126 | + | return false | |||
| 127 | + | } | |||
| 128 | + | } | |||
M internal/adapters/in/http/middleware/accesslog_test.go +17 -0
| @@ -58,6 +58,23 @@ func TestAccessLogger_EmitsEntryPerRequest(t *testing.T) { | |||
| 58 | 58 | assert.Equal(t, "192.168.1.100", e.ClientIP) | |
| 59 | 59 | } | |
| 60 | 60 | ||
| 61 | + | func TestAccessLogger_RedactsSensitiveQueryParams(t *testing.T) { | |
| 62 | + | mock := &mockAccessLogWriter{} | |
| 63 | + | log := testLogger() | |
| 64 | + | ||
| 65 | + | handler := AccessLogger(mock, false, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 66 | + | w.WriteHeader(http.StatusOK) | |
| 67 | + | })) | |
| 68 | + | ||
| 69 | + | req := httptest.NewRequest(http.MethodGet, "/oauth/callback?code=secret-code&state=ok&token=secret-token", nil) | |
| 70 | + | rw := httptest.NewRecorder() | |
| 71 | + | ||
| 72 | + | handler.ServeHTTP(rw, req) | |
| 73 | + | ||
| 74 | + | require.Len(t, mock.entries, 1) | |
| 75 | + | assert.Equal(t, "code=[REDACTED]&state=ok&token=[REDACTED]", mock.entries[0].Query) | |
| 76 | + | } | |
| 77 | + | ||
| 61 | 78 | func TestAccessLogger_RequestIDReused(t *testing.T) { | |
| 62 | 79 | mock := &mockAccessLogWriter{} | |
| 63 | 80 | log := testLogger() | |
| @@ -58,6 +58,23 @@ func TestAccessLogger_EmitsEntryPerRequest(t *testing.T) { | |||||
| 58 | assert.Equal(t, "192.168.1.100", e.ClientIP) | 58 | assert.Equal(t, "192.168.1.100", e.ClientIP) | ||
| 59 | } | 59 | } | ||
| 60 | 60 | ||||
| 61 | + | func TestAccessLogger_RedactsSensitiveQueryParams(t *testing.T) { | |||
| 62 | + | mock := &mockAccessLogWriter{} | |||
| 63 | + | log := testLogger() | |||
| 64 | + | ||||
| 65 | + | handler := AccessLogger(mock, false, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 66 | + | w.WriteHeader(http.StatusOK) | |||
| 67 | + | })) | |||
| 68 | + | ||||
| 69 | + | req := httptest.NewRequest(http.MethodGet, "/oauth/callback?code=secret-code&state=ok&token=secret-token", nil) | |||
| 70 | + | rw := httptest.NewRecorder() | |||
| 71 | + | ||||
| 72 | + | handler.ServeHTTP(rw, req) | |||
| 73 | + | ||||
| 74 | + | require.Len(t, mock.entries, 1) | |||
| 75 | + | assert.Equal(t, "code=[REDACTED]&state=ok&token=[REDACTED]", mock.entries[0].Query) | |||
| 76 | + | } | |||
| 77 | + | ||||
| 61 | func TestAccessLogger_RequestIDReused(t *testing.T) { | 78 | func TestAccessLogger_RequestIDReused(t *testing.T) { | ||
| 62 | mock := &mockAccessLogWriter{} | 79 | mock := &mockAccessLogWriter{} | ||
| 63 | log := testLogger() | 80 | log := testLogger() | ||
M internal/adapters/in/http/middleware/logging.go +1 -1
| @@ -130,7 +130,7 @@ func RequestLogger(log zerowrap.Logger, trustedNets ...[]*net.IPNet) func(http.H | |||
| 130 | 130 | Str("request_id", requestID). | |
| 131 | 131 | Str(zerowrap.FieldMethod, r.Method). | |
| 132 | 132 | Str(zerowrap.FieldPath, r.URL.Path). | |
| 133 | - | Str("query", r.URL.RawQuery). | |
| 133 | + | Str("query", sanitizeLoggedQuery(r.URL.RawQuery)). | |
| 134 | 134 | Str(zerowrap.FieldHost, r.Host). | |
| 135 | 135 | Str("user_agent", r.UserAgent()). | |
| 136 | 136 | Str("referer", r.Referer()). | |
| @@ -130,7 +130,7 @@ func RequestLogger(log zerowrap.Logger, trustedNets ...[]*net.IPNet) func(http.H | |||||
| 130 | Str("request_id", requestID). | 130 | Str("request_id", requestID). | ||
| 131 | Str(zerowrap.FieldMethod, r.Method). | 131 | Str(zerowrap.FieldMethod, r.Method). | ||
| 132 | Str(zerowrap.FieldPath, r.URL.Path). | 132 | Str(zerowrap.FieldPath, r.URL.Path). | ||
| 133 | - | Str("query", r.URL.RawQuery). | 133 | + | Str("query", sanitizeLoggedQuery(r.URL.RawQuery)). |
| 134 | Str(zerowrap.FieldHost, r.Host). | 134 | Str(zerowrap.FieldHost, r.Host). | ||
| 135 | Str("user_agent", r.UserAgent()). | 135 | Str("user_agent", r.UserAgent()). | ||
| 136 | Str("referer", r.Referer()). | 136 | Str("referer", r.Referer()). | ||
M internal/adapters/out/accesslog/writer_test.go +6 -3
| @@ -72,7 +72,8 @@ func TestNew_FileOutput_CreatesDirectory(t *testing.T) { | |||
| 72 | 72 | func TestWriter_Stdout_JSON(t *testing.T) { | |
| 73 | 73 | // Replace stdout to capture output. | |
| 74 | 74 | orig := os.Stdout | |
| 75 | - | r, w, _ := os.Pipe() | |
| 75 | + | r, w, err := os.Pipe() | |
| 76 | + | require.NoError(t, err) | |
| 76 | 77 | os.Stdout = w | |
| 77 | 78 | ||
| 78 | 79 | writer, err := New(Config{Format: "json", Output: "stdout"}) | |
| @@ -97,7 +98,8 @@ func TestWriter_Stdout_JSON(t *testing.T) { | |||
| 97 | 98 | ||
| 98 | 99 | func TestWriter_Stdout_CLF(t *testing.T) { | |
| 99 | 100 | orig := os.Stdout | |
| 100 | - | r, w, _ := os.Pipe() | |
| 101 | + | r, w, err := os.Pipe() | |
| 102 | + | require.NoError(t, err) | |
| 101 | 103 | os.Stdout = w | |
| 102 | 104 | ||
| 103 | 105 | writer, err := New(Config{Format: "clf", Output: "stdout"}) | |
| @@ -119,7 +121,8 @@ func TestWriter_Stdout_CLF(t *testing.T) { | |||
| 119 | 121 | ||
| 120 | 122 | func TestWriter_Stdout_Combined(t *testing.T) { | |
| 121 | 123 | orig := os.Stdout | |
| 122 | - | r, w, _ := os.Pipe() | |
| 124 | + | r, w, err := os.Pipe() | |
| 125 | + | require.NoError(t, err) | |
| 123 | 126 | os.Stdout = w | |
| 124 | 127 | ||
| 125 | 128 | writer, err := New(Config{Format: "combined", Output: "stdout"}) | |
| @@ -72,7 +72,8 @@ func TestNew_FileOutput_CreatesDirectory(t *testing.T) { | |||||
| 72 | func TestWriter_Stdout_JSON(t *testing.T) { | 72 | func TestWriter_Stdout_JSON(t *testing.T) { | ||
| 73 | // Replace stdout to capture output. | 73 | // Replace stdout to capture output. | ||
| 74 | orig := os.Stdout | 74 | orig := os.Stdout | ||
| 75 | - | r, w, _ := os.Pipe() | 75 | + | r, w, err := os.Pipe() |
| 76 | + | require.NoError(t, err) | |||
| 76 | os.Stdout = w | 77 | os.Stdout = w | ||
| 77 | 78 | ||||
| 78 | writer, err := New(Config{Format: "json", Output: "stdout"}) | 79 | writer, err := New(Config{Format: "json", Output: "stdout"}) | ||
| @@ -97,7 +98,8 @@ func TestWriter_Stdout_JSON(t *testing.T) { | |||||
| 97 | 98 | ||||
| 98 | func TestWriter_Stdout_CLF(t *testing.T) { | 99 | func TestWriter_Stdout_CLF(t *testing.T) { | ||
| 99 | orig := os.Stdout | 100 | orig := os.Stdout | ||
| 100 | - | r, w, _ := os.Pipe() | 101 | + | r, w, err := os.Pipe() |
| 102 | + | require.NoError(t, err) | |||
| 101 | os.Stdout = w | 103 | os.Stdout = w | ||
| 102 | 104 | ||||
| 103 | writer, err := New(Config{Format: "clf", Output: "stdout"}) | 105 | writer, err := New(Config{Format: "clf", Output: "stdout"}) | ||
| @@ -119,7 +121,8 @@ func TestWriter_Stdout_CLF(t *testing.T) { | |||||
| 119 | 121 | ||||
| 120 | func TestWriter_Stdout_Combined(t *testing.T) { | 122 | func TestWriter_Stdout_Combined(t *testing.T) { | ||
| 121 | orig := os.Stdout | 123 | orig := os.Stdout | ||
| 122 | - | r, w, _ := os.Pipe() | 124 | + | r, w, err := os.Pipe() |
| 125 | + | require.NoError(t, err) | |||
| 123 | os.Stdout = w | 126 | os.Stdout = w | ||
| 124 | 127 | ||||
| 125 | writer, err := New(Config{Format: "combined", Output: "stdout"}) | 128 | writer, err := New(Config{Format: "combined", Output: "stdout"}) | ||
M internal/adapters/out/pki/mobileconfig.go +36 -2
| @@ -4,6 +4,8 @@ import ( | |||
| 4 | 4 | "encoding/base64" | |
| 5 | 5 | "fmt" | |
| 6 | 6 | "html" | |
| 7 | + | "strings" | |
| 8 | + | "unicode" | |
| 7 | 9 | ||
| 8 | 10 | "github.com/google/uuid" | |
| 9 | 11 | ) | |
| @@ -16,6 +18,7 @@ func GenerateMobileconfig(rootDER []byte, rootCN string) []byte { | |||
| 16 | 18 | certUUID := uuid.New().String() | |
| 17 | 19 | ||
| 18 | 20 | safeCN := html.EscapeString(rootCN) | |
| 21 | + | identifierCN := payloadIdentifierComponent(rootCN) | |
| 19 | 22 | profile := fmt.Sprintf(`<?xml version="1.0" encoding="UTF-8"?> | |
| 20 | 23 | <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | |
| 21 | 24 | <plist version="1.0"> | |
| @@ -53,8 +56,39 @@ func GenerateMobileconfig(rootDER []byte, rootCN string) []byte { | |||
| 53 | 56 | <string>%s</string> | |
| 54 | 57 | <key>PayloadVersion</key> | |
| 55 | 58 | <integer>1</integer> | |
| 56 | - | </dict> | |
| 57 | - | </plist>`, b64Cert, safeCN, certUUID, certUUID, profileUUID) | |
| 59 | + | </dict> | |
| 60 | + | </plist>`, b64Cert, safeCN, identifierCN, certUUID, profileUUID) | |
| 58 | 61 | ||
| 59 | 62 | return []byte(profile) | |
| 60 | 63 | } | |
| 64 | + | ||
| 65 | + | func payloadIdentifierComponent(rootCN string) string { | |
| 66 | + | var b strings.Builder | |
| 67 | + | lastHyphen := false | |
| 68 | + | ||
| 69 | + | for _, r := range rootCN { | |
| 70 | + | r = unicode.ToLower(r) | |
| 71 | + | switch { | |
| 72 | + | case (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '.': | |
| 73 | + | b.WriteRune(r) | |
| 74 | + | lastHyphen = false | |
| 75 | + | case r == '-': | |
| 76 | + | if !lastHyphen { | |
| 77 | + | b.WriteRune(r) | |
| 78 | + | lastHyphen = true | |
| 79 | + | } | |
| 80 | + | default: | |
| 81 | + | if !lastHyphen { | |
| 82 | + | b.WriteRune('-') | |
| 83 | + | lastHyphen = true | |
| 84 | + | } | |
| 85 | + | } | |
| 86 | + | } | |
| 87 | + | ||
| 88 | + | identifier := strings.Trim(b.String(), "-.") | |
| 89 | + | if identifier == "" { | |
| 90 | + | return "gordon-ca" | |
| 91 | + | } | |
| 92 | + | ||
| 93 | + | return identifier | |
| 94 | + | } | |
| @@ -4,6 +4,8 @@ import ( | |||||
| 4 | "encoding/base64" | 4 | "encoding/base64" | ||
| 5 | "fmt" | 5 | "fmt" | ||
| 6 | "html" | 6 | "html" | ||
| 7 | + | "strings" | |||
| 8 | + | "unicode" | |||
| 7 | 9 | ||||
| 8 | "github.com/google/uuid" | 10 | "github.com/google/uuid" | ||
| 9 | ) | 11 | ) | ||
| @@ -16,6 +18,7 @@ func GenerateMobileconfig(rootDER []byte, rootCN string) []byte { | |||||
| 16 | certUUID := uuid.New().String() | 18 | certUUID := uuid.New().String() | ||
| 17 | 19 | ||||
| 18 | safeCN := html.EscapeString(rootCN) | 20 | safeCN := html.EscapeString(rootCN) | ||
| 21 | + | identifierCN := payloadIdentifierComponent(rootCN) | |||
| 19 | profile := fmt.Sprintf(`<?xml version="1.0" encoding="UTF-8"?> | 22 | profile := fmt.Sprintf(`<?xml version="1.0" encoding="UTF-8"?> | ||
| 20 | <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | 23 | <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | ||
| 21 | <plist version="1.0"> | 24 | <plist version="1.0"> | ||
| @@ -53,8 +56,39 @@ func GenerateMobileconfig(rootDER []byte, rootCN string) []byte { | |||||
| 53 | <string>%s</string> | 56 | <string>%s</string> | ||
| 54 | <key>PayloadVersion</key> | 57 | <key>PayloadVersion</key> | ||
| 55 | <integer>1</integer> | 58 | <integer>1</integer> | ||
| 56 | - | </dict> | 59 | + | </dict> |
| 57 | - | </plist>`, b64Cert, safeCN, certUUID, certUUID, profileUUID) | 60 | + | </plist>`, b64Cert, safeCN, identifierCN, certUUID, profileUUID) |
| 58 | 61 | ||||
| 59 | return []byte(profile) | 62 | return []byte(profile) | ||
| 60 | } | 63 | } | ||
| 64 | + | ||||
| 65 | + | func payloadIdentifierComponent(rootCN string) string { | |||
| 66 | + | var b strings.Builder | |||
| 67 | + | lastHyphen := false | |||
| 68 | + | ||||
| 69 | + | for _, r := range rootCN { | |||
| 70 | + | r = unicode.ToLower(r) | |||
| 71 | + | switch { | |||
| 72 | + | case (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '.': | |||
| 73 | + | b.WriteRune(r) | |||
| 74 | + | lastHyphen = false | |||
| 75 | + | case r == '-': | |||
| 76 | + | if !lastHyphen { | |||
| 77 | + | b.WriteRune(r) | |||
| 78 | + | lastHyphen = true | |||
| 79 | + | } | |||
| 80 | + | default: | |||
| 81 | + | if !lastHyphen { | |||
| 82 | + | b.WriteRune('-') | |||
| 83 | + | lastHyphen = true | |||
| 84 | + | } | |||
| 85 | + | } | |||
| 86 | + | } | |||
| 87 | + | ||||
| 88 | + | identifier := strings.Trim(b.String(), "-.") | |||
| 89 | + | if identifier == "" { | |||
| 90 | + | return "gordon-ca" | |||
| 91 | + | } | |||
| 92 | + | ||||
| 93 | + | return identifier | |||
| 94 | + | } | |||
M internal/adapters/out/pki/mobileconfig_test.go +9 -0
| @@ -21,3 +21,12 @@ func TestGenerateMobileconfig(t *testing.T) { | |||
| 21 | 21 | assert.Contains(t, string(mc), "<key>PayloadType</key>") | |
| 22 | 22 | assert.Contains(t, string(mc), "com.apple.security.root") | |
| 23 | 23 | } | |
| 24 | + | ||
| 25 | + | func TestGenerateMobileconfig_UsesIdentifierSafePayloadIdentifier(t *testing.T) { | |
| 26 | + | mc := pki.GenerateMobileconfig([]byte("cert"), `Gordon Internal CA <prod>`) | |
| 27 | + | ||
| 28 | + | assert.Contains(t, string(mc), "<string>Gordon Internal CA <prod></string>") | |
| 29 | + | assert.Contains(t, string(mc), "<string>dev.gordon.ca.gordon-internal-ca-prod</string>") | |
| 30 | + | assert.NotContains(t, string(mc), "dev.gordon.ca.Gordon Internal CA") | |
| 31 | + | assert.NotContains(t, string(mc), "dev.gordon.ca.gordon-internal-ca-<prod>") | |
| 32 | + | } | |
| @@ -21,3 +21,12 @@ func TestGenerateMobileconfig(t *testing.T) { | |||||
| 21 | assert.Contains(t, string(mc), "<key>PayloadType</key>") | 21 | assert.Contains(t, string(mc), "<key>PayloadType</key>") | ||
| 22 | assert.Contains(t, string(mc), "com.apple.security.root") | 22 | assert.Contains(t, string(mc), "com.apple.security.root") | ||
| 23 | } | 23 | } | ||
| 24 | + | ||||
| 25 | + | func TestGenerateMobileconfig_UsesIdentifierSafePayloadIdentifier(t *testing.T) { | |||
| 26 | + | mc := pki.GenerateMobileconfig([]byte("cert"), `Gordon Internal CA <prod>`) | |||
| 27 | + | ||||
| 28 | + | assert.Contains(t, string(mc), "<string>Gordon Internal CA <prod></string>") | |||
| 29 | + | assert.Contains(t, string(mc), "<string>dev.gordon.ca.gordon-internal-ca-prod</string>") | |||
| 30 | + | assert.NotContains(t, string(mc), "dev.gordon.ca.Gordon Internal CA") | |||
| 31 | + | assert.NotContains(t, string(mc), "dev.gordon.ca.gordon-internal-ca-<prod>") | |||
| 32 | + | } | |||
M internal/usecase/container/service.go +5 -2
| @@ -835,10 +835,13 @@ func isPullFailure(err error) bool { | |||
| 835 | 835 | return false | |
| 836 | 836 | } | |
| 837 | 837 | ||
| 838 | + | if errors.Is(err, domain.ErrImagePullFailed) { | |
| 839 | + | return true | |
| 840 | + | } | |
| 841 | + | ||
| 838 | 842 | msg := strings.ToLower(err.Error()) | |
| 839 | 843 | return strings.Contains(msg, "failed to pull image") || | |
| 840 | - | strings.Contains(msg, "pull access denied") || | |
| 841 | - | strings.Contains(msg, "unauthorized") | |
| 844 | + | strings.Contains(msg, "pull access denied") | |
| 842 | 845 | } | |
| 843 | 846 | ||
| 844 | 847 | func (s *Service) activateDeployedContainer(ctx context.Context, domainName string, container *domain.Container) bool { | |
| @@ -835,10 +835,13 @@ func isPullFailure(err error) bool { | |||||
| 835 | return false | 835 | return false | ||
| 836 | } | 836 | } | ||
| 837 | 837 | ||||
| 838 | + | if errors.Is(err, domain.ErrImagePullFailed) { | |||
| 839 | + | return true | |||
| 840 | + | } | |||
| 841 | + | ||||
| 838 | msg := strings.ToLower(err.Error()) | 842 | msg := strings.ToLower(err.Error()) | ||
| 839 | return strings.Contains(msg, "failed to pull image") || | 843 | return strings.Contains(msg, "failed to pull image") || | ||
| 840 | - | strings.Contains(msg, "pull access denied") || | 844 | + | strings.Contains(msg, "pull access denied") |
| 841 | - | strings.Contains(msg, "unauthorized") | |||
| 842 | } | 845 | } | ||
| 843 | 846 | ||||
| 844 | func (s *Service) activateDeployedContainer(ctx context.Context, domainName string, container *domain.Container) bool { | 847 | func (s *Service) activateDeployedContainer(ctx context.Context, domainName string, container *domain.Container) bool { | ||
M internal/usecase/container/service_test.go +28 -0
| @@ -502,6 +502,34 @@ func TestService_Deploy_ImagePullFailure(t *testing.T) { | |||
| 502 | 502 | assert.ErrorContains(t, deployErr.Err, "failed to pull image") | |
| 503 | 503 | } | |
| 504 | 504 | ||
| 505 | + | func TestService_Deploy_DoesNotMisclassifyUnauthorizedEnvLoadFailure(t *testing.T) { | |
| 506 | + | runtime := mocks.NewMockContainerRuntime(t) | |
| 507 | + | envLoader := mocks.NewMockEnvLoader(t) | |
| 508 | + | eventBus := mocks.NewMockEventPublisher(t) | |
| 509 | + | ||
| 510 | + | svc := NewService(runtime, envLoader, eventBus, nil, Config{}, nil) | |
| 511 | + | ctx := testContext() | |
| 512 | + | ||
| 513 | + | route := domain.Route{ | |
| 514 | + | Domain: "test.example.com", | |
| 515 | + | Image: "myapp:latest", | |
| 516 | + | } | |
| 517 | + | ||
| 518 | + | runtime.EXPECT().ListContainers(mock.Anything, false).Return([]*domain.Container{}, nil) | |
| 519 | + | runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{}, nil) | |
| 520 | + | runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) | |
| 521 | + | runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) | |
| 522 | + | runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) | |
| 523 | + | envLoader.EXPECT().LoadEnv(mock.Anything, "test.example.com").Return(nil, errors.New("unauthorized")) | |
| 524 | + | ||
| 525 | + | result, err := svc.Deploy(ctx, route) | |
| 526 | + | ||
| 527 | + | assert.Nil(t, result) | |
| 528 | + | assert.ErrorContains(t, err, "failed to load environment variables: unauthorized") | |
| 529 | + | var deployErr *domain.DeployFailureError | |
| 530 | + | assert.False(t, errors.As(err, &deployErr)) | |
| 531 | + | } | |
| 532 | + | ||
| 505 | 533 | func TestService_CaptureRecentContainerLogs_SkipsCanceledContext(t *testing.T) { | |
| 506 | 534 | runtime := mocks.NewMockContainerRuntime(t) | |
| 507 | 535 | svc := NewService(runtime, nil, nil, nil, Config{}, nil) | |
| @@ -502,6 +502,34 @@ func TestService_Deploy_ImagePullFailure(t *testing.T) { | |||||
| 502 | assert.ErrorContains(t, deployErr.Err, "failed to pull image") | 502 | assert.ErrorContains(t, deployErr.Err, "failed to pull image") | ||
| 503 | } | 503 | } | ||
| 504 | 504 | ||||
| 505 | + | func TestService_Deploy_DoesNotMisclassifyUnauthorizedEnvLoadFailure(t *testing.T) { | |||
| 506 | + | runtime := mocks.NewMockContainerRuntime(t) | |||
| 507 | + | envLoader := mocks.NewMockEnvLoader(t) | |||
| 508 | + | eventBus := mocks.NewMockEventPublisher(t) | |||
| 509 | + | ||||
| 510 | + | svc := NewService(runtime, envLoader, eventBus, nil, Config{}, nil) | |||
| 511 | + | ctx := testContext() | |||
| 512 | + | ||||
| 513 | + | route := domain.Route{ | |||
| 514 | + | Domain: "test.example.com", | |||
| 515 | + | Image: "myapp:latest", | |||
| 516 | + | } | |||
| 517 | + | ||||
| 518 | + | runtime.EXPECT().ListContainers(mock.Anything, false).Return([]*domain.Container{}, nil) | |||
| 519 | + | runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{}, nil) | |||
| 520 | + | runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil) | |||
| 521 | + | runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil) | |||
| 522 | + | runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:latest").Return(nil, nil) | |||
| 523 | + | envLoader.EXPECT().LoadEnv(mock.Anything, "test.example.com").Return(nil, errors.New("unauthorized")) | |||
| 524 | + | ||||
| 525 | + | result, err := svc.Deploy(ctx, route) | |||
| 526 | + | ||||
| 527 | + | assert.Nil(t, result) | |||
| 528 | + | assert.ErrorContains(t, err, "failed to load environment variables: unauthorized") | |||
| 529 | + | var deployErr *domain.DeployFailureError | |||
| 530 | + | assert.False(t, errors.As(err, &deployErr)) | |||
| 531 | + | } | |||
| 532 | + | ||||
| 505 | func TestService_CaptureRecentContainerLogs_SkipsCanceledContext(t *testing.T) { | 533 | func TestService_CaptureRecentContainerLogs_SkipsCanceledContext(t *testing.T) { | ||
| 506 | runtime := mocks.NewMockContainerRuntime(t) | 534 | runtime := mocks.NewMockContainerRuntime(t) | ||
| 507 | svc := NewService(runtime, nil, nil, nil, Config{}, nil) | 535 | svc := NewService(runtime, nil, nil, nil, Config{}, nil) | ||