Skip to content

Remove temporary-exception entitlement rejected by App Store#578

Open
PureWeen wants to merge 2 commits intomainfrom
fix/remove-temporary-exception-entitlement
Open

Remove temporary-exception entitlement rejected by App Store#578
PureWeen wants to merge 2 commits intomainfrom
fix/remove-temporary-exception-entitlement

Conversation

@PureWeen
Copy link
Copy Markdown
Owner

@PureWeen PureWeen commented Apr 9, 2026

Problem

Apple rejected the Mac App Store submission under Guideline 2.4.5(i) - Performance:

Entitlement com.apple.security.temporary-exception.files.home-relative-path.read-only value "/" is not permitted.

Temporary exception entitlements are not approved for new Mac App Store submissions.

Fix

Removed the com.apple.security.temporary-exception.files.home-relative-path.read-write entitlement from Entitlements.AppStore.plist.

This entitlement was unnecessary because:

  • The macOS sandbox remaps HOME to ~/Library/Containers/<bundle-id>/Data/
  • All UserProfile-based paths (~/.polypilot/, ~/.copilot/) resolve inside the container automatically
  • The copilot helper process inherits the sandbox (via Entitlements.Helper.plist) and sees the same remapped HOME

Testing

  • All 3331 unit tests pass
  • No code changes needed — only the entitlements plist was modified

Apple rejected Guideline 2.4.5(i): the temporary-exception entitlement
com.apple.security.temporary-exception.files.home-relative-path is not
permitted for Mac App Store apps.

The exception is unnecessary because the macOS sandbox remaps HOME to
~/Library/Containers/<bundle-id>/Data/, so all UserProfile-based paths
(~/.polypilot/, ~/.copilot/) resolve inside the container automatically.
The copilot helper process inherits the sandbox via Entitlements.Helper.plist
and sees the same remapped HOME.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@PureWeen
Copy link
Copy Markdown
Owner Author

PureWeen commented Apr 9, 2026

🔍 Multi-Model Code Review — PR #578 (Re-review after fixes)

Summary

Removes the temporary-exception.files.home-relative-path entitlement rejected by Apple App Store. Adds a one-time sandbox data migration in Program.cs for users upgrading from sideloaded builds. Fixes stale comments in ConnectionSettings.cs.

CI Status

⚠️ No CI checks reported on this branch (platform-specific change)


Previous Review Findings — Status

Finding Status
🟡 No migration path from non-sandboxed to sandboxed builds ✅ FIXED — MigrateLegacyDataIfNeeded() added
🟢 Stale comment in ConnectionSettings.cs ✅ FIXED — comments updated
🟢 System CLI fallback unreachable in sandbox ➖ N/A — pre-existing, out of scope

New Findings (from migration code)

🟡 MODERATE — Migration marker written unconditionally, permanently sealing failed/blocked migrations (3/3 reviewers)

File: PolyPilot/Platforms/MacCatalyst/Program.cs:167-171
What: The .sandbox-migrated marker file is written at the end of MigrateLegacyDataIfNeeded() regardless of whether any files were actually copied or whether the sandbox blocked access entirely. Two failure modes:

  1. Sandbox blocks access: Directory.Exists(legacyPolypilot) returns false for both "doesn't exist" and "access denied (EPERM)" — the code can't distinguish. migratedAny stays false, but the marker is written as "No legacy data found." Migration is permanently sealed.

  2. Partial copy failure: CopyDirectoryRecursive swallows per-file errors via catch {}. If some files copy and others fail (transient I/O, locked file), migratedAny is true (based on directory existence, not copy success) and the marker is written. Permanently incomplete data, no retry.

Fix: Only write the marker when migratedAny == true. Also track copy errors — if any file copy fails, don't write the marker. The "don't clobber" semantics make retries safe and idempotent. This way, if the sandbox blocks everything, the migration harmlessly retries each launch (O(1) stat calls) until it either succeeds or the user has no legacy data.

🟡 MODERATE — Migration is effectively dead code on sandboxed App Store builds (3/3 reviewers, severity 2× 🔴 1× 🟢 → median 🟡)

File: PolyPilot/Platforms/MacCatalyst/Program.cs:126-178 and Entitlements.AppStore.plist
What: This PR removes the temporary-exception.files.home-relative-path.read-write entitlement — which was the only mechanism granting the sandboxed process read access to the real ~/.polypilot/ and ~/.copilot/. Without it, Directory.Exists() on the real home paths returns false and the migration copies nothing. The code's xmldoc explicitly says "Best-effort: if the sandbox blocks access to the real home directory, we skip gracefully" — so this is partially by design.

Impact: Users upgrading from sideloaded/dev builds to App Store won't get their data migrated. Their files still exist at the real home (no data loss), but they're invisible to the sandboxed app.

Suggestion: If this matters for the user base, investigate com.apple.security.files.home-relative-path.read-only (non-temporary, may be accepted by App Review for one-time migration). Otherwise, document this limitation in the xmldoc and PR description so future developers understand the migration only helps non-sandboxed→non-sandboxed scenarios (e.g., dev builds with different bundle IDs sharing the same home directory).

🟢 MINOR — Symlink following could cause StackOverflow or copy unintended data (2/3 reviewers)

File: PolyPilot/Platforms/MacCatalyst/Program.cs:185-207
What: Directory.GetDirectories() returns symlinked directories indistinguishably from real ones on macOS. A circular symlink (e.g., .polypilot/link → .polypilot/) causes infinite recursion → StackOverflowException (uncatchable, terminates process). Non-circular symlinks pointing outside .polypilot/ could copy unintended data.

Fix: Check new DirectoryInfo(dir).LinkTarget != null and skip symlinked entries, or add a depth limit.

🟢 MINOR — Missing newline at end of file (1/3 reviewers)

File: PolyPilot/Platforms/MacCatalyst/Program.cs:208
What: Diff shows \ No newline at end of file. Trivial but should be fixed for consistency.


Discarded Findings

Finding Flagged by Why discarded
Startup delay from synchronous copy of 750+ session dirs 1/3 2/3 reviewers noted that in practice, the sandbox blocks Directory.Exists() immediately so no real I/O occurs. Would only matter if the migration ever works, at which point it should be revisited.

Test Coverage

  • ✅ All 3331 unit tests pass
  • Migration code is Mac Catalyst platform-specific (Program.cs) and cannot be tested from the net10.0 test project — follows the same pattern as RecoverSecretsFromSecureStorage in ConnectionSettings.cs
  • No new testable logic in the net10.0 test project

Recommendation

⚠️ Request changes — Fix the marker file issue before merging:

  1. Only write .sandbox-migrated when migratedAny == true (prevents permanently sealing blocked migrations)
  2. Track copy errors in CopyDirectoryRecursive and suppress the marker if any files failed (prevents sealing partial migrations)

The entitlement removal itself is correct and safe. The migration is a nice-to-have but needs the marker fix to avoid being harmful (permanently lying about migration state).

When a user upgrades from a sideloaded (non-sandboxed) build to the
App Store (sandboxed) build, their data at the real ~/.polypilot/ and
~/.copilot/ becomes invisible because the sandbox remaps HOME to the
container directory.

This adds a one-time migration in Program.cs that:
- Detects the sandbox by checking for /Library/Containers/ in HOME
- Derives the real home path from the container path
- Copies .polypilot/ and .copilot/ data into the container
- Uses a .sandbox-migrated marker to prevent repeated attempts
- Skips gracefully if the sandbox blocks access (best-effort)
- Never overwrites existing container data (don't-clobber invariant)

Also fixes stale comments in ConnectionSettings.cs that incorrectly
stated Mac Catalyst runs without app sandbox.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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