Skip to content

feat: enhance release workflow with notarization and code signing var…#28

Merged
krishnakishorep merged 1 commit intomainfrom
addNotarization
Jan 9, 2026
Merged

feat: enhance release workflow with notarization and code signing var…#28
krishnakishorep merged 1 commit intomainfrom
addNotarization

Conversation

@krishnakishorep
Copy link
Collaborator

@krishnakishorep krishnakishorep commented Jan 9, 2026

  • Add environment variables for code signing and notarization in release workflow
  • Include new permission for Bash find command in settings.local.json

Pull Request

Description

Please provide a brief description of the changes in this PR. Include the motivation for these changes and any relevant context.

Type of Change

Please check the type of change your PR introduces:

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📝 Documentation update
  • 🎨 Code style update (formatting, renaming)
  • ♻️ Code refactoring (no functional changes, no API changes)
  • ⚡ Performance improvement
  • ✅ Test update
  • 🔧 Configuration change
  • 🏗️ Infrastructure/build change
  • 🔐 Security fix

Testing Performed

Please describe the tests you've run to verify your changes. Provide instructions so reviewers can reproduce.

  • Unit tests pass locally (composer test)
  • Frontend builds successfully (npm run build)
  • Code follows project style guidelines (php artisan pint and npm run format)
  • Manual testing completed

Test Configuration

  • PHP Version:
  • Node Version:
  • Browser (if applicable):
  • Operating System:

Manual Testing Steps

Checklist

Please review the following items before submitting your PR:

Code Quality

  • My code follows the project's code style and conventions
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings or errors
  • TypeScript types are properly defined (no any types unless absolutely necessary)

Database Changes (if applicable)

  • I have created necessary migrations
  • Migrations have been tested on both databases (database.sqlite and nativephp.sqlite)
  • Database schema changes are backward compatible

Frontend Changes (if applicable)

  • Component changes follow Vue 3 Composition API patterns
  • UI components maintain consistency with existing design system
  • Changes are responsive and work on different screen sizes
  • Dark mode compatibility has been verified

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally
  • Any dependent changes have been merged and published

Related Issues

Please link any related issues here using the format Fixes #issue_number or Relates to #issue_number

  • Fixes #
  • Relates to #

Screenshots (if applicable)

If your changes include UI updates, please add screenshots showing:

Before

After

Dark Mode (if UI changes)

Additional Notes

Add any additional notes, concerns, or discussion points for reviewers.

Reviewer Checklist

For reviewers to complete:

  • Code follows project conventions and style guide
  • Changes are well-documented and easy to understand
  • Tests adequately cover the changes
  • No security vulnerabilities introduced
  • Performance impact is acceptable
  • UI changes are consistent with design system
  • Database migrations are safe and reversible

…iables

- Add environment variables for code signing and notarization in release workflow
- Include new permission for Bash find command in settings.local.json
@claude
Copy link

claude bot commented Jan 9, 2026

Claude finished @krishnakishorep's task —— View job


PR Review: Notarization and Code Signing Variables

Reviewing this pull request for code quality, security, and best practices.

Todo List:

  • Read and analyze the modified workflow file
  • Review for security concerns and best practices
  • Check for potential issues or improvements
  • Provide comprehensive feedback

Review Feedback

I've analyzed the changes in .github/workflows/release.yml and have several observations about the notarization and code signing implementation:

Positive Aspects

  1. Comprehensive Environment Variables: The PR adds proper environment variables for both code signing and notarization, covering multiple compatibility scenarios:

    • NativePHP-specific variables (NATIVEPHP_APPLE_ID, etc.)
    • Electron-builder compatibility (CSC_LINK, CSC_KEY_PASSWORD)
    • Electron-notarize compatibility (APPLE_ID, APPLE_TEAM_ID, etc.)
  2. Conditional Execution: Good use of if: env.NATIVEPHP_CERTIFICATE_BASE64 != '' to only run code signing when certificates are available.

  3. Secure Secret Handling: Proper use of GitHub Secrets for sensitive data.

⚠️ Security Concerns

  1. Keychain Security: Lines 93-109 show good keychain management practices:

    • Creates temporary keychain with random password
    • Sets appropriate timeout (21600 seconds = 6 hours)
    • Cleans up certificate file after import
    • However, consider adding cleanup of the keychain itself in a cleanup step
  2. Secret Exposure Risk: The current implementation could potentially expose secrets in logs if any of the commands fail. Consider adding error handling to prevent secret leakage.

🔧 Recommended Improvements

  1. Add Keychain Cleanup: Add a cleanup step to remove the temporary keychain:

    - name: Cleanup keychain
      if: always() && env.NATIVEPHP_CERTIFICATE_BASE64 != ''
      run: |
        KEYCHAIN_PATH=$RUNNER_TEMP/app-signing.keychain-db
        security delete-keychain "$KEYCHAIN_PATH" || true
  2. Error Handling: Consider adding set -e or better error handling in shell scripts to fail fast and avoid potential secret exposure.

  3. Validation: Consider adding validation that required secrets are present before proceeding with code signing operations.

📋 Code Quality Assessment

  • Best Practices: ✅ Follows GitHub Actions security best practices
  • Readability: ✅ Well-organized and commented
  • Maintainability: ✅ Environment variables are clearly named and documented

🔍 No Issues Found

  • Performance: No performance concerns
  • Bugs: No apparent bugs in the implementation
  • Test Coverage: Not applicable for workflow files

📝 Overall Assessment

This is a solid implementation of code signing and notarization for macOS builds. The approach is comprehensive and handles multiple compatibility scenarios well. The main recommendation is to add proper keychain cleanup for better security hygiene.

Recommendation: ✅ Approve with minor improvements suggested above.


@krishnakishorep krishnakishorep merged commit 28eaae9 into main Jan 9, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant