[v3] Mac: Implement pkg packaging #4472
Conversation
I have successfully implemented Mac PKG bundling for Wails v3, addressing GitHub issue #2413. Here's what was accomplished: ### ✅ **Complete Implementation** **1. PKG Builder (`pkg_builder.go`)** - Uses macOS `pkgbuild` and `productbuild` tools - Supports code signing with Developer ID certificates - Generates distribution XML with customizable installer appearance - Handles component and product package creation **2. Notarization Service (`notarization.go`)** - Full Apple notarization workflow using `notarytool` - Automatic status polling with timeout handling - Ticket stapling with `stapler` - Detailed error logging and debugging support **3. Configuration System (`config.go`)** - YAML-based configuration with environment variable support - Path resolution for relative paths - Comprehensive validation - Template generation for easy setup **4. Command Integration (`command.go`)** - Seamless integration with existing `wails3 tool package` command - Support for `--format pkg` option - Template generation with `--generate-template` - Configuration validation with `--validate-only` **5. CLI Integration** - Updated `tool_package.go` to handle PKG format - Added new flags for PKG-specific options - Maintained backward compatibility with existing formats ### ✅ **Key Features** - **Professional Installers**: Creates distribution-ready PKG installers with custom branding - **Code Signing**: Full support for Developer ID signing certificates - **Notarization**: Automated Apple notarization with retry logic and status monitoring - **Configuration-Driven**: YAML configuration with environment variable expansion - **Validation**: Comprehensive dependency and configuration validation - **Template Generation**: Automatic generation of sample configuration files - **Cross-Platform Safe**: Only runs on macOS, provides clear error messages elsewhere ### ✅ **Testing & Documentation** - **Unit Tests**: Comprehensive test coverage for all components - **Integration Tests**: CLI integration and end-to-end workflow testing - **Documentation**: Complete README with examples, troubleshooting, and workflows - **Error Handling**: Detailed error messages and validation feedback ### ✅ **Usage Examples** **Generate Template:** ```bash wails3 tool package --format pkg --generate-template --config my-pkg.yaml ``` **Build PKG:** ```bash wails3 tool package --format pkg --config my-pkg.yaml ``` **Validate Only:** ```bash wails3 tool package --format pkg --config my-pkg.yaml --validate-only ``` ### ✅ **Based on Community Input** The implementation incorporates the sample script provided by @sebheitzmann in the GitHub issue, using: - `pkgbuild` for component package creation - `productbuild` for final installer assembly - Distribution XML for installer customization - Modern `notarytool` for Apple notarization ### ✅ **Production Ready** The implementation is ready for v3 and includes: - Dependency validation for required tools - Comprehensive error handling and user feedback - Security best practices for credential handling - Professional installer workflows matching industry standards - Full integration with existing Wails build system This addresses the original GitHub issue by providing a complete Mac PKG bundling and notarization solution that integrates seamlessly with the existing Wails v3 packaging system.
WalkthroughA new macOS PKG installer builder feature has been introduced, including configuration management, notarization automation, and packaging logic. This includes new Go source files for building, validating, and notarizing PKG installers, comprehensive documentation, and unit tests. The CLI and flag definitions were updated to support PKG-specific options and workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config
participant PKGBuilder
participant NotarizationService
User->>CLI: Invoke PKG build command (with options)
CLI->>CLI: Check if macOS, handle template gen
CLI->>Config: Load & validate config
Config-->>CLI: Return config or error
CLI->>PKGBuilder: Build package
PKGBuilder-->>CLI: Return build result
CLI->>NotarizationService: (If enabled) Notarize package
NotarizationService-->>CLI: Notarization result
CLI->>CLI: Validate package signature
CLI-->>User: Output success or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
dosubot
bot
added
the
size:XXL
github-actions
bot
added
Documentation
|
leaanthony
changed the title
dosubot
bot
added
the
size:XXL
Deploying wails with
|
| Latest commit: |
6accd91
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://47602a52.wails.pages.dev |
| Branch Preview URL: | https://vk-4e27-implement.wails.pages.dev |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
v3/internal/commands/macpkg/command_test.go (2)
125-125: Same issue with undefinedcontainsfunctionThis line also needs to use
strings.Containsinstead ofcontains.
215-215: Samecontainsfunction issueReplace with
strings.Contains.v3/internal/commands/macpkg/pkg_builder.go (1)
175-200: Apply same command construction simplificationSimilar to
createComponentPackage, this can be simplified.v3/internal/commands/macpkg/notarization.go (2)
140-166: Same credential security issueThis function has the same issue with passing credentials via command line.
168-191: Same credential security concernApply the same environment variable approach here.
🧹 Nitpick comments (3)
v3/internal/commands/macpkg/config.go (2)
33-77: Consider using reflection for field expansion to reduce maintenance burden.The current approach manually expands environment variables for each field, which requires maintenance when new fields are added.
Consider using reflection to automatically expand all string fields:
func (c *PKGConfig) expandAndResolve(configDir string) error { + v := reflect.ValueOf(c).Elem() + t := v.Type() + + // Expand environment variables in all string fields + for i := 0; i < v.NumField(); i++ { + field := v.Field(i) + if field.Kind() == reflect.String && field.CanSet() { + expanded := os.ExpandEnv(field.String()) + field.SetString(expanded) + } + } - // Expand environment variables - c.AppName = os.ExpandEnv(c.AppName) - c.AppPath = os.ExpandEnv(c.AppPath) - // ... other fields
134-140: Enhance app bundle validation.The current validation only checks for
.appextension and existence. Consider validating the bundle structure to ensure it's a proper macOS app bundle.} else { // Verify app bundle exists if _, err := os.Stat(config.AppPath); os.IsNotExist(err) { errors = append(errors, fmt.Sprintf("app_path does not exist: %s", config.AppPath)) } else if !strings.HasSuffix(config.AppPath, ".app") { errors = append(errors, "app_path must point to a .app bundle") + } else { + // Verify it's a proper app bundle by checking for Info.plist + infoPlistPath := filepath.Join(config.AppPath, "Contents", "Info.plist") + if _, err := os.Stat(infoPlistPath); os.IsNotExist(err) { + errors = append(errors, "app_path does not contain a valid app bundle structure (missing Info.plist)") + } }v3/internal/commands/macpkg/pkg_builder.go (1)
202-236: Consider separating validation from default value settingThe function name suggests it only validates, but it also sets default values. This could be surprising.
Consider renaming to
validateAndSetDefaultsor separating into two functions:func (b *PKGBuilder) validateConfig() error { // validation only } func (b *PKGBuilder) setConfigDefaults() { // set defaults }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
v3/internal/commands/macpkg/README.md(1 hunks)v3/internal/commands/macpkg/command.go(1 hunks)v3/internal/commands/macpkg/command_test.go(1 hunks)v3/internal/commands/macpkg/config.go(1 hunks)v3/internal/commands/macpkg/config_test.go(1 hunks)v3/internal/commands/macpkg/notarization.go(1 hunks)v3/internal/commands/macpkg/pkg_builder.go(1 hunks)v3/internal/commands/tool_package.go(4 hunks)v3/internal/flags/package.go(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_darwin.go:39-46
Timestamp: 2025-03-23T00:41:39.612Z
Learning: For the macOS notifications implementation in Wails, an early panic is used when the bundle identifier check fails rather than returning an error, because the Objective-C code would crash later anyway. The panic provides clear instructions to developers about bundling and signing requirements.
📚 Learning: files in `v3/internal/generator/testcases` are test dummies used for testing purposes only. they are...
Learnt from: fbbdev
PR: wailsapp/wails#4001
File: v3/internal/generator/testcases/aliases/main.go:65-68
Timestamp: 2025-01-15T22:40:58.594Z
Learning: Files in `v3/internal/generator/testcases` are test dummies used for testing purposes only. They are never compiled into the application and should not be subject to the same level of code review as production code.
Applied to files:
v3/internal/commands/macpkg/command_test.gov3/internal/commands/macpkg/config_test.go
📚 Learning: for the macos notifications implementation in wails, an early panic is used when the bundle identifi...
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications_darwin.go:39-46
Timestamp: 2025-03-23T00:41:39.612Z
Learning: For the macOS notifications implementation in Wails, an early panic is used when the bundle identifier check fails rather than returning an error, because the Objective-C code would crash later anyway. The panic provides clear instructions to developers about bundling and signing requirements.
Applied to files:
v3/internal/commands/macpkg/README.md
📚 Learning: in the notifications package, initialization of the `service` struct is handled through platform-spe...
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
Applied to files:
v3/internal/commands/macpkg/notarization.go
📚 Learning: in the notifications package, initialization of the `service` struct is handled through platform-spe...
Learnt from: popaprozac
PR: wailsapp/wails#4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file. Each platform implementation follows a singleton pattern using `notificationServiceOnce.Do()` and creates a global `NotificationService` variable that's accessed through a thread-safe `getNotificationService()` function.
Applied to files:
v3/internal/commands/macpkg/notarization.go
🧬 Code Graph Analysis (1)
v3/internal/flags/package.go (1)
v3/internal/commands/macpkg/config.go (1)
GenerateTemplate(80-120)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep/ci
- GitHub Check: Cloudflare Pages
🔇 Additional comments (33)
v3/internal/commands/macpkg/config.go (5)
13-30: LGTM - Solid configuration loading implementation.The function properly handles file reading, YAML parsing, and path processing with appropriate error wrapping. The use of
filepath.Dir(configPath)for relative path resolution is correct.
52-74: Path resolution logic is secure and well-implemented.The function correctly checks for absolute paths using
filepath.IsAbs()and resolves relative paths usingfilepath.Join(), which prevents path traversal attacks.
80-120: Template generation looks comprehensive.The template includes all necessary configuration options with helpful comments and examples. The use of environment variable placeholders is appropriate for sensitive credentials.
175-185: Good notarization validation logic.The all-or-nothing approach for notarization credentials is correct - partial credentials would lead to runtime failures.
195-215: Bundle ID validation is too restrictive.The current implementation doesn't allow underscores, which are valid in bundle IDs according to Apple's documentation.
// Check for valid characters (alphanumeric and hyphens) for _, r := range part { if !((r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || - (r >= '0' && r <= '9') || r == '-') { + (r >= '0' && r <= '9') || r == '-' || r == '_') { return false } }Likely an incorrect or invalid review comment.
v3/internal/commands/tool_package.go (4)
11-11: Clean import addition for PKG functionality.The import follows the existing pattern and integrates well with the existing packaging infrastructure.
20-27: Good logical structure for format detection.The addition of PKG detection follows the same pattern as DMG, maintaining consistency in the codebase.
78-95: Well-structured PKG handling implementation.The platform check ensures PKG creation only runs on macOS, and the option mapping is straightforward. The error handling follows existing patterns.
106-106: Updated error message appropriately includes PKG format.The error message correctly lists all supported formats including the new PKG option.
v3/internal/flags/package.go (1)
7-16: Excellent flag additions for PKG functionality.The new flags follow consistent naming conventions and provide clear descriptions. The default values are appropriate, and the flags cover all necessary PKG-specific operations (creation, template generation, notarization control, and validation).
v3/internal/commands/macpkg/config_test.go (4)
9-28: Well-designed test helper function.The
createValidTestConfigfunction properly creates isolated test environments with temporary directories and mock app bundles, ensuring test isolation.
30-72: Comprehensive config loading test.The test validates both YAML parsing and path resolution functionality, checking that relative paths are correctly converted to absolute paths.
74-140: Excellent table-driven validation tests.The test cases cover key validation scenarios including missing required fields, invalid bundle IDs, and incorrect file extensions. The error checking logic is thorough.
142-168: Comprehensive bundle ID validation coverage.The test cases cover valid and invalid bundle ID formats, including edge cases like empty strings, trailing dots, and invalid characters.
v3/internal/commands/macpkg/README.md (1)
1-259: Outstanding comprehensive documentation.This README provides excellent coverage of the Mac PKG installer functionality, including:
- Clear quick start guide with practical examples
- Complete configuration reference with all fields documented
- Proper security considerations for credential handling
- Comprehensive troubleshooting section
- Integration examples for CI/CD workflows
- Accurate command-line flag documentation
The documentation follows best practices by emphasizing security (environment variables for credentials, app-specific passwords) and providing multiple usage scenarios. The examples are practical and the troubleshooting section addresses common real-world issues.
v3/internal/commands/macpkg/command_test.go (3)
10-28: LGTM!The platform check test correctly validates that PKG building is restricted to macOS.
30-52: LGTM!Good use of
t.TempDir()for isolated testing and proper file existence validation.
130-191: Excellent table-driven test design!Comprehensive coverage of all credential combinations for notarization logic.
v3/internal/commands/macpkg/command.go (4)
9-16: Well-structured options with clear defaultsGood use of struct tags for CLI integration and sensible default values.
105-124: Clean template generation logicGood error handling and user feedback for template generation.
126-129: LGTM!Clear logic for determining notarization eligibility.
66-71: Potential nil pointer in defer statementIf
NewPKGBuilderreturns an error,builderwill be nil and the defer will panic.Move the defer after the error check:
// Create PKG builder builder, err := NewPKGBuilder(config) if err != nil { return fmt.Errorf("failed to create PKG builder: %w", err) } -defer builder.Cleanup() +defer builder.Cleanup()⛔ Skipped due to learnings
Learnt from: fbbdev PR: wailsapp/wails#4001 File: v3/internal/generator/analyse_test.go:105-107 Timestamp: 2025-01-15T22:42:09.774Z Learning: In the FindServices function of v3/internal/generator/analyse.go, services should never be nil when err is nil. This is a design contract and any violation should result in a panic rather than being handled silently.Learnt from: fbbdev PR: wailsapp/wails#4045 File: v3/internal/generator/render/info.go:28-68 Timestamp: 2025-02-04T23:59:43.956Z Learning: In the Wails v3 project, internal functions are designed to panic on nil parameters as they represent contract violations, rather than adding defensive nil checks.v3/internal/commands/macpkg/pkg_builder.go (5)
19-47: Comprehensive configuration structureWell-organized fields covering all aspects of PKG creation, signing, and notarization.
49-67: Good temporary directory managementProper error handling and unique directory naming with
wails-pkg-*prefix.
238-244: Safe cleanup implementationGood defensive check for empty
tempDirbefore removal.
246-257: Clear dependency checkingGood error messages that help users identify missing tools.
95-121: Consider validating config values for shell safetyWhile
exec.Commanddoesn't use shell interpretation, it's good practice to validate that config values don't contain unexpected characters.Also, the command construction can be simplified:
- args := []string{ - "pkgbuild", - "--root", b.Config.AppPath, - "--identifier", b.Config.BundleID, - "--version", b.Config.Version, - "--install-location", b.Config.InstallLocation, - } - - // Add signing if configured - if b.Config.SigningIdentity != "" { - args = append(args, "--sign", b.Config.SigningIdentity) - } - - args = append(args, outputPath) - - cmd := exec.Command(args[0], args[1:]...) + cmd := exec.Command("pkgbuild", + "--root", b.Config.AppPath, + "--identifier", b.Config.BundleID, + "--version", b.Config.Version, + "--install-location", b.Config.InstallLocation, + ) + + // Add signing if configured + if b.Config.SigningIdentity != "" { + cmd.Args = append(cmd.Args, "--sign", b.Config.SigningIdentity) + } + + cmd.Args = append(cmd.Args, outputPath)⛔ Skipped due to learnings
Learnt from: popaprozac PR: wailsapp/wails#4098 File: v3/pkg/services/notifications/notifications_windows.go:91-113 Timestamp: 2025-02-24T06:08:55.645Z Learning: The JSON quote handling in Windows notifications' activation arguments (v3/pkg/services/notifications/notifications_windows.go) has a known limitation with single-quote collisions that needs to be addressed after initial testing.v3/internal/commands/macpkg/notarization.go (6)
107-138: Reasonable polling parameters30-minute timeout with 30-second intervals is appropriate for notarization workflows.
193-209: Clean stapling implementationStraightforward command execution without credential exposure.
211-223: Clear credential validationSpecific error messages help users identify missing credentials.
225-245: Thorough dependency validationGood approach using
--helpto verify tools are actually available, not just in PATH.
247-267: Proper signature validationGood error handling that captures stderr output for detailed error messages.
13-43: No credential logging detected
A search forAppleID,AppPassword, andTeamIDacross the Go code confirms no calls tofmt.Println,log.Printf, or error wrappers include these fields. Credentials are only passed as arguments toexec.Command(not logged) and validated without exposing their values.
| func TestBuildMacPKG_MissingConfig(t *testing.T) { | ||
| if runtime.GOOS != "darwin" { | ||
| t.Skip("Skipping macOS-specific test on non-macOS platform") | ||
| } | ||
|
|
||
| options := &BuildMacPKGOptions{ | ||
| ConfigPath: "", | ||
| } | ||
|
|
||
| err := BuildMacPKG(options) | ||
| if err == nil { | ||
| t.Errorf("Expected error for missing config, but got nil") | ||
| } | ||
|
|
||
| expectedError := "config file is required" | ||
| if !contains(err.Error(), expectedError) { | ||
| t.Errorf("Expected error to contain '%s', got '%s'", expectedError, err.Error()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing helper function contains
The tests use an undefined contains function. This will cause compilation errors.
Replace with the standard library function:
- if !contains(err.Error(), expectedError) {
+ if !strings.Contains(err.Error(), expectedError) {Also add the import at the top of the file:
import (
"os"
"path/filepath"
"runtime"
+ "strings"
"testing"
)Also applies to: 74-92
🤖 Prompt for AI Agents
In v3/internal/commands/macpkg/command_test.go around lines 54 to 72 and 74 to
92, the test code uses an undefined contains function causing compilation
errors. Replace all uses of contains with strings.Contains from the standard
library. Add the import "strings" at the top of the file to enable this. This
will fix the undefined function error and use the correct standard library
method for substring checks.
| func TestHandleTemplateGeneration_DefaultPath(t *testing.T) { | ||
| // Change to temp directory to avoid creating files in project root | ||
| originalDir, _ := os.Getwd() | ||
| tempDir := t.TempDir() | ||
| os.Chdir(tempDir) | ||
| defer os.Chdir(originalDir) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for os.Getwd()
If os.Getwd() fails, the defer statement could change to an invalid directory.
- originalDir, _ := os.Getwd()
+ originalDir, err := os.Getwd()
+ if err != nil {
+ t.Fatalf("Failed to get current directory: %v", err)
+ }
tempDir := t.TempDir()
os.Chdir(tempDir)
defer os.Chdir(originalDir)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestHandleTemplateGeneration_DefaultPath(t *testing.T) { | |
| // Change to temp directory to avoid creating files in project root | |
| originalDir, _ := os.Getwd() | |
| tempDir := t.TempDir() | |
| os.Chdir(tempDir) | |
| defer os.Chdir(originalDir) | |
| func TestHandleTemplateGeneration_DefaultPath(t *testing.T) { | |
| // Change to temp directory to avoid creating files in project root | |
| originalDir, err := os.Getwd() | |
| if err != nil { | |
| t.Fatalf("Failed to get current directory: %v", err) | |
| } | |
| tempDir := t.TempDir() | |
| os.Chdir(tempDir) | |
| defer os.Chdir(originalDir) |
🤖 Prompt for AI Agents
In v3/internal/commands/macpkg/command_test.go around lines 220 to 225, the call
to os.Getwd() does not handle errors, which could cause the defer
os.Chdir(originalDir) to attempt changing to an invalid directory if os.Getwd()
fails. Modify the code to check the error returned by os.Getwd(), and if there
is an error, fail the test immediately to prevent proceeding with an invalid
originalDir value.
| func contains(s, substr string) bool { | ||
| return len(s) >= len(substr) && (s == substr || len(substr) == 0 || | ||
| (len(s) > len(substr) && (s[:len(substr)] == substr || s[len(s)-len(substr):] == substr || | ||
| len(s) > len(substr)*2 && findSubstring(s, substr)))) | ||
| } | ||
|
|
||
| func findSubstring(s, substr string) bool { | ||
| for i := 0; i <= len(s)-len(substr); i++ { | ||
| if s[i:i+len(substr)] == substr { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inefficient string matching implementation.
The contains helper function has a complex and potentially inefficient implementation that could be simplified.
-// Helper function to check if a string contains a substring
-func contains(s, substr string) bool {
- return len(s) >= len(substr) && (s == substr || len(substr) == 0 ||
- (len(s) > len(substr) && (s[:len(substr)] == substr || s[len(s)-len(substr):] == substr ||
- len(s) > len(substr)*2 && findSubstring(s, substr))))
-}
-
-func findSubstring(s, substr string) bool {
- for i := 0; i <= len(s)-len(substr); i++ {
- if s[i:i+len(substr)] == substr {
- return true
- }
- }
- return false
-}
+// Helper function to check if a string contains a substring
+func contains(s, substr string) bool {
+ return strings.Contains(s, substr)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func contains(s, substr string) bool { | |
| return len(s) >= len(substr) && (s == substr || len(substr) == 0 || | |
| (len(s) > len(substr) && (s[:len(substr)] == substr || s[len(s)-len(substr):] == substr || | |
| len(s) > len(substr)*2 && findSubstring(s, substr)))) | |
| } | |
| func findSubstring(s, substr string) bool { | |
| for i := 0; i <= len(s)-len(substr); i++ { | |
| if s[i:i+len(substr)] == substr { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| // Helper function to check if a string contains a substring | |
| func contains(s, substr string) bool { | |
| return strings.Contains(s, substr) | |
| } |
🤖 Prompt for AI Agents
In v3/internal/commands/macpkg/config_test.go around lines 233 to 246, the
contains function uses a complicated and inefficient approach to check if a
substring exists in a string. Simplify it by replacing the entire contains
function with a direct call to the standard library's strings.Contains function,
which is optimized for this purpose. Remove the custom findSubstring function as
it will no longer be needed.
| // submitForNotarization submits the package to Apple's notarization service | ||
| func (n *NotarizationService) submitForNotarization(pkgPath string) (string, error) { | ||
| args := []string{ | ||
| "xcrun", "notarytool", "submit", | ||
| pkgPath, | ||
| "--apple-id", n.AppleID, | ||
| "--password", n.AppPassword, | ||
| "--team-id", n.TeamID, | ||
| "--output-format", "json", | ||
| } | ||
|
|
||
| cmd := exec.Command(args[0], args[1:]...) | ||
| var stdout, stderr bytes.Buffer | ||
| cmd.Stdout = &stdout | ||
| cmd.Stderr = &stderr | ||
|
|
||
| if err := cmd.Run(); err != nil { | ||
| return "", fmt.Errorf("notarytool submit failed: %w\nStderr: %s", err, stderr.String()) | ||
| } | ||
|
|
||
| // Parse the JSON response to get submission ID | ||
| var result map[string]interface{} | ||
| if err := json.Unmarshal(stdout.Bytes(), &result); err != nil { | ||
| return "", fmt.Errorf("failed to parse notarytool response: %w", err) | ||
| } | ||
|
|
||
| submissionID, ok := result["id"].(string) | ||
| if !ok { | ||
| return "", fmt.Errorf("submission ID not found in response") | ||
| } | ||
|
|
||
| return submissionID, nil | ||
| } |
There was a problem hiding this comment.
Security: Credentials visible in process list
Passing credentials as command-line arguments makes them visible in process listings (ps, top, etc.).
Consider using environment variables or stdin for credentials:
- "--apple-id", n.AppleID,
- "--password", n.AppPassword,
- "--team-id", n.TeamID,
+ cmd.Env = append(os.Environ(),
+ fmt.Sprintf("NOTARYTOOL_APPLE_ID=%s", n.AppleID),
+ fmt.Sprintf("NOTARYTOOL_PASSWORD=%s", n.AppPassword),
+ fmt.Sprintf("NOTARYTOOL_TEAM_ID=%s", n.TeamID),
+ )Also, add safe type assertion:
- submissionID, ok := result["id"].(string)
- if !ok {
- return "", fmt.Errorf("submission ID not found in response")
- }
+ idInterface, exists := result["id"]
+ if !exists {
+ return "", fmt.Errorf("submission ID not found in response")
+ }
+ submissionID, ok := idInterface.(string)
+ if !ok {
+ return "", fmt.Errorf("submission ID has unexpected type: %T", idInterface)
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In v3/internal/commands/macpkg/notarization.go lines 73 to 105, the current code
passes AppleID and AppPassword as command-line arguments, exposing credentials
in process listings. To fix this, remove credentials from args and instead set
them as environment variables on the exec.Command instance. Additionally,
improve the type assertion for the submission ID by safely checking and handling
the type assertion failure with a clear error message.
| // createDistributionXML generates the distribution XML file | ||
| func (b *PKGBuilder) createDistributionXML(outputPath string) error { | ||
| tmpl := `<?xml version="1.0" encoding="utf-8"?> | ||
| <installer-gui-script minSpecVersion="1"> | ||
| <title>{{.Title}}</title> | ||
| <pkg-ref id="{{.BundleID}}"/> | ||
|
|
||
| <!-- Domain configuration --> | ||
| <domains enable_localSystem="true"/> | ||
|
|
||
| <!-- Options --> | ||
| <options customize="never" require-scripts="false" rootVolumeOnly="true" hostArchitectures="x86_64,arm64"/> | ||
|
|
||
| <!-- Choices outline --> | ||
| <choices-outline> | ||
| <line choice="default"> | ||
| <line choice="{{.BundleID}}"/> | ||
| </line> | ||
| </choices-outline> | ||
|
|
||
| <!-- Choice definitions --> | ||
| <choice id="default" title="{{.Title}}"/> | ||
| <choice id="{{.BundleID}}" visible="false" title="{{.AppName}}"> | ||
| <pkg-ref id="{{.BundleID}}"/> | ||
| </choice> | ||
|
|
||
| <!-- Package reference --> | ||
| <pkg-ref id="{{.BundleID}}" version="{{.Version}}" onConclusion="none">component.pkg</pkg-ref> | ||
|
|
||
| {{if .Background}}<background file="{{.Background}}" mime-type="image/png" alignment="topleft" scaling="none"/>{{end}} | ||
| {{if .WelcomeFile}}<welcome file="{{.WelcomeFile}}" mime-type="text/rtf"/>{{end}} | ||
| {{if .ReadmeFile}}<readme file="{{.ReadmeFile}}" mime-type="text/rtf"/>{{end}} | ||
| {{if .LicenseFile}}<license file="{{.LicenseFile}}" mime-type="text/rtf"/>{{end}} | ||
| </installer-gui-script>` | ||
|
|
||
| t, err := template.New("distribution").Parse(tmpl) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse distribution template: %w", err) | ||
| } | ||
|
|
||
| var buf bytes.Buffer | ||
| if err := t.Execute(&buf, b.Config); err != nil { | ||
| return fmt.Errorf("failed to execute distribution template: %w", err) | ||
| } | ||
|
|
||
| if err := os.WriteFile(outputPath, buf.Bytes(), 0644); err != nil { | ||
| return fmt.Errorf("failed to write distribution XML: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use html/template for automatic XML escaping
To prevent potential XML injection from user-provided config values, use html/template which provides automatic escaping.
- "text/template"
+ "html/template"This will automatically escape special XML characters in config values like &, <, >, etc.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // createDistributionXML generates the distribution XML file | |
| func (b *PKGBuilder) createDistributionXML(outputPath string) error { | |
| tmpl := `<?xml version="1.0" encoding="utf-8"?> | |
| <installer-gui-script minSpecVersion="1"> | |
| <title>{{.Title}}</title> | |
| <pkg-ref id="{{.BundleID}}"/> | |
| <!-- Domain configuration --> | |
| <domains enable_localSystem="true"/> | |
| <!-- Options --> | |
| <options customize="never" require-scripts="false" rootVolumeOnly="true" hostArchitectures="x86_64,arm64"/> | |
| <!-- Choices outline --> | |
| <choices-outline> | |
| <line choice="default"> | |
| <line choice="{{.BundleID}}"/> | |
| </line> | |
| </choices-outline> | |
| <!-- Choice definitions --> | |
| <choice id="default" title="{{.Title}}"/> | |
| <choice id="{{.BundleID}}" visible="false" title="{{.AppName}}"> | |
| <pkg-ref id="{{.BundleID}}"/> | |
| </choice> | |
| <!-- Package reference --> | |
| <pkg-ref id="{{.BundleID}}" version="{{.Version}}" onConclusion="none">component.pkg</pkg-ref> | |
| {{if .Background}}<background file="{{.Background}}" mime-type="image/png" alignment="topleft" scaling="none"/>{{end}} | |
| {{if .WelcomeFile}}<welcome file="{{.WelcomeFile}}" mime-type="text/rtf"/>{{end}} | |
| {{if .ReadmeFile}}<readme file="{{.ReadmeFile}}" mime-type="text/rtf"/>{{end}} | |
| {{if .LicenseFile}}<license file="{{.LicenseFile}}" mime-type="text/rtf"/>{{end}} | |
| </installer-gui-script>` | |
| t, err := template.New("distribution").Parse(tmpl) | |
| if err != nil { | |
| return fmt.Errorf("failed to parse distribution template: %w", err) | |
| } | |
| var buf bytes.Buffer | |
| if err := t.Execute(&buf, b.Config); err != nil { | |
| return fmt.Errorf("failed to execute distribution template: %w", err) | |
| } | |
| if err := os.WriteFile(outputPath, buf.Bytes(), 0644); err != nil { | |
| return fmt.Errorf("failed to write distribution XML: %w", err) | |
| } | |
| return nil | |
| } | |
| import ( | |
| "bytes" | |
| "fmt" | |
| "os" | |
| - "text/template" | |
| + "html/template" | |
| ) |
🤖 Prompt for AI Agents
In v3/internal/commands/macpkg/pkg_builder.go between lines 123 and 173, the
createDistributionXML function uses the text/template package to generate XML,
which does not automatically escape special XML characters. To prevent potential
XML injection from user-provided config values, replace the import and usage of
text/template with html/template. This change will ensure that special
characters like &, <, and > in the config values are properly escaped when
generating the XML output.
Summary
Implements generating pkgs on mac.
Closes: #2413
Summary by CodeRabbit
New Features
Documentation
Tests