Security review and remediation: 27 findings + fixes - #2
Closed
dr-danny wants to merge 30 commits into
Closed
Conversation
The product was shipping with com.apple.security.app-sandbox=false in both dev and release entitlements. Without the sandbox, the LLM tool loop has the user's full filesystem privileges; a single prompt-injection-driven payload can read ~/.ssh, browser cookies, ~/Library/... etc. Enables App Sandbox with the loosenings the product appears to need: - network.client - files.user-selected.read-write - temporary-exception.files.home-relative-path.read-write for ~/Library/Developer/TenXApp and ~/Library/Developer/Xcode - keychain-access-groups (added to release for parity) DRAFT: I have not verified xcrun / xcodebuild / simctl / the bundled xcodegen all work under the sandbox with these entitlements. Test the preview / build flows on a sandboxed build before merging; if a specific subtool fails, add the narrow entitlement it actually needs and document why next to it. Do NOT revert to app-sandbox=false as the quick fix. Refs #1
The previous merge forwarded the entire parent-process environment to every model-issued shell command. Any secret a developer exports for the host app's own use (, , *, etc.) was forwarded to commands the model chose to run, which a single prompt-injection-driven 'curl' could exfiltrate. Replace the merge with an explicit allowlist of POSIX/zsh basics. The project-scoped 'environmentVariables' continue to be forwarded. NOTE: this fix only closes the env-inheritance leg of issue #2. The approval-gate work (wiring run_command through the existing IntegrationApprovalRequest pattern) is a larger change tracked in the same issue and will land in a follow-up commit. Refs #2
SUPublicEDKey ships as a build-variable placeholder ('$(SPARKLE_PUBLIC_ED_KEY)')
with no in-repo guarantee that the produced Info.plist actually carries
a valid ed25519 public key. The integrity of every Sparkle update
depends on this key being correctly embedded; until it is, the update
path is unverified.
Add a launch-time assertion that crashes (release) or warns (debug)
if SUPublicEDKey is:
- missing
- empty
- still an unsubstituted '$(...)' build-variable
- not a valid 32-byte base64 ed25519 key
Crashing on launch is the right failure mode for release: it makes
misconfigured release pipelines impossible to ship rather than
silently shipping an app whose updater would accept anything.
The follow-up to this is to embed the actual key as a literal in
AppInfo.plist instead of a build variable. That's a release-pipeline
change that requires the production key; the runtime check above is
defense-in-depth that catches the misconfiguration regardless.
Closes #3
NSWorkspace.shared.open(URL) opens any URL the system has a registered handler for: file:///Applications/Calculator.app launches the app, .terminal files execute the embedded shell, third-party schemes (slack://, zoommtg://, ms-msdt://, ...) trigger custom handlers. The most exploitable instance is the Sparkle openReleaseNotes() call, which derives its URL from the appcast item — and the appcast XML's release-notes URL is NOT covered by the EdDSA signature, so a hijacked appcast can use it to launch arbitrary local apps with one user click. Add an NSWorkspace.openIfSafeWebURL(_:) helper that refuses anything other than http(s), and route every single-argument NSWorkspace.shared.open(url) site through it. The two-argument overload that takes [URL] + withApplicationAt: is intentional (used to open Xcode on a project) and left alone. Closes #4
The previous workspace-path validation only excluded a small set of directory names. The model could still write Package.swift, Makefile, Podfile, *.xcconfig, *.entitlements, *.sh, *.command, *.terminal — all of which can result in host code execution if a subsequent build step (or the user double-clicking) picks them up. This change extends the validation to: - Refuse a fixed set of dangerous root-level filenames (Package.swift, Makefile, Podfile, Rakefile, Gemfile, ...) - Refuse a fixed set of dangerous file extensions anywhere under the workspace (.sh, .bash, .zsh, .command, .terminal, .xcconfig, .entitlements) This is a tightening of the existing denylist approach, not the full positive allowlist that the issue tracks as the long-term direction — shipping the allowlist requires careful auditing of every legitimate write path. The denylist closes the most exploitable extensions today and the issue stays open until the allowlist lands. Refs #5
normalizedBaseURL only trimmed trailing slashes — any string was accepted, including http://, file://, or javascript:. A misconfigured API_BASE_URL silently routed access tokens over plaintext. Add scheme + host validation; allow http:// only against localhost (preserved in both DEBUG and release to keep the existing dev-loop default working — flip the release branch of the #if DEBUG to https-only once the production base URL is configured).
.standardizedFileURL collapses ./.. lexically but does NOT resolve symlinks. Foundation's String.write(to:) and String(contentsOf:) follow symlinks at the OS level, so any symlink inside the workspace becomes an exfil/overwrite primitive for arbitrary paths the user can access. - Use resolvingSymlinksInPath() on both candidate and root before the containment check - Reject NUL bytes in supplied paths - Defense-in-depth: walk every ancestor of the lexical path and refuse if any component is itself a symlink Closes #7
The previous kSecAttrAccessibleWhenUnlocked variant participates in iCloud Keychain sync if the user has it enabled, which is not the intended threat model for desktop session tokens. Switch to the ThisDeviceOnly variant so credentials stay device-bound. A follow-up should also gate the refresh token with SecAccessControl .userPresence (Touch ID / password prompt); deferring that to a separate change because it requires UI work for the prompt strings. Closes #8
UserDefaults for the bundle is a plist at ~/Library/Preferences (or its sandbox-container equivalent). Without App Sandbox enabled — which is the current shipping config — that plist is writable by any user-level process. The migration path read any legacy value from UserDefaults and unconditionally adopted it as the session token, durably installing it in Keychain. That's a session-fixation primitive: a sibling process drops a chosen token into the plist, next launch authenticates as the attacker. Behavior after this change: - Keychain remains the source of truth. - Any leftover legacy value in UserDefaults is scrubbed but NEVER adopted as a credential. Users who relied on a pre-Keychain build will simply re-sign-in once. Closes #9
The previous wrapper merged the entire parent-process environment into every xcrun invocation. xcodebuild then propagates that env into every build phase it runs (project.yml preBuildScripts, scriptPhases, etc.), which means any developer ENV secrets ($OPENAI_API_KEY, $GITHUB_TOKEN, $AWS_*, etc.) leak into user-build phases of generated apps. Replace the merge with an explicit allowlist of the variables xcrun and xcodebuild actually need. Callers can still add to the env via the existing 'environment:' parameter for known-safe variables. Closes #10
The bundled xcodegen Mach-O is adhoc-signed (no Developer ID) and was previously executed by XcodePreviewService without any integrity check. A malicious contributor could have swapped it via a binary-only diff that's invisible in PR review and would yield arbitrary code execution on every project generation. This change: - Pins the SHA-256 in 10x-macos/Resources/xcodegen.sha256 (bcdecce1d97d7425c882e74dfd9dfe894f1a3177b668c19ea34d244af9a07258) - Adds verifyXcodeGenIntegrity() that hashes the binary at runtime and compares against the pin; throws XcodeGenError.integrityCheckFailed before launch on mismatch - Treats a missing pin as a hard failure (integrityPinMissing) - Includes the .sha256 in the bundle resources so SwiftPM-built copies also carry it Long-term, the bundled binary should be downloaded at build time from a pinned upstream release. The hash pin closes the immediate window. Closes #11
The previous code wrote the decoded private key to $RELEASE_DIR/sparkle_private_key.txt and never removed it. $RELEASE_DIR survives across CI steps and may be picked up by artifact-archive actions, leaking the signing key. - Switch to mktemp -t so the file lives in $TMPDIR, not the working tree - Register a cleanup trap on EXIT INT TERM HUP that shreds (or removes) the file unconditionally - Call cleanup_sparkle_key explicitly after sign_update succeeds so the window of disk presence is minimized Closes #12
--token $VERCEL_TOKEN is visible to any process that can read `ps aux` / /proc/<pid>/cmdline during the deploy window. Self-hosted runners and any colocated processes see it. Vercel CLI honors the VERCEL_TOKEN env var natively, so just export it and drop the flag. Closes #13
Tags (`@v4`) are mutable and an upstream compromise — or a hostile tag rewrite — propagates immediately into CI runs. Pin actions/checkout, actions/setup-node, and actions/upload-artifact by full commit SHA with the version recorded in a trailing comment so dependabot can still propose updates. SHAs verified via GitHub API at time of commit: actions/checkout 34e114876b0b11c390a56381ad16ebd13914f8d5 actions/setup-node 49933ea5288caeca8642d1e84afbd3f7d6820020 actions/upload-artifact ea165f8d65b6e75b540449e92b4886f43607fa02 Closes #14
Yams was the only dep declared with from: rather than exact:, and the repo does not commit Package.resolved — so transitive dependencies are not pinned and any future malicious 6.x.y release of Yams would land in clean builds without review. This change pins the version. Committing Package.resolved is the companion change but requires running 'swift package resolve' on a machine with the Swift toolchain; doing it in a follow-up to avoid churning Package.resolved in CI without verification. Closes #15
A hostile or compromised proxy returning a single multi-GB line — or an indefinite error body — would otherwise exhaust client memory and crash the app. The streaming reader now: - Refuses single lines > 1 MiB (throws APIError.networkError) - Truncates the error-path body at 100 KiB instead of unbounded concat Closes #16
URL-scheme registration is global. Any website can fire `app.10x.macos://billing/return?...session_id=<attacker_session>` and the previous handleDeepLink trusted the URL outright, calling syncCheckoutSession with the user's bearer token. This change adds a one-shot nonce store (BillingDeepLinkNonceStore) and gates handleDeepLink on consuming an outstanding nonce. The full fix requires the checkout-creation backend call to include the nonce as Stripe's `client_reference_id` (or equivalent) and the return URL template to echo `nonce=...` back. That UI/server wiring is a follow-up — but the deep-link side is in place: any callback without a matching nonce is rejected with a clear status message rather than driving billing actions. Note: this is intentionally fail-closed. Existing flows that don't yet issue a nonce will surface the rejection until the checkout-start path is updated to call BillingDeepLinkNonceStore.shared.issue() and pass the result through to the backend. Refs #17
Defense-in-depth: refuse UDIDs from simctl JSON output that don't match the canonical hex shape. simctl arguments are already passed as Process arrays (no shell injection), but constraining the value catches future bugs where a UDID surfaces in a log path, --args open invocation, or any string-interpolation context. Closes #18
The path was set to '.' (the bundle's resource root), which auto-loads any .otf/.ttf added to Resources at any depth via CoreText. Restrict to the actual Fonts subdirectory so a future contributor dropping attacker-supplied font files anywhere else in Resources doesn't get them auto-registered.
UserDefaults is writable by sibling processes when App Sandbox is off (the current shipping config). userId/userEmail aren't credentials themselves, but a sibling process can rewrite them to confuse the UI or, more subtly, fool any code that gates on them. - Read/write user identity through AuthTokenStore (same Keychain service as the existing tokens) - Scrub legacy UserDefaults entries on every persist/clear path Closes #20
A tagged release whose version string contains HTML or JS — a typo or a hostile PR/tag — would otherwise produce XSS on the public release page. Add an escape_html helper and route APP_NAME, build_label, the date string, and the file-path reference through it before they hit the HTML template. Closes #21
create-dmg.sh now writes $DMG_PATH.sha256 alongside the DMG immediately after producing it. notarize-dmg.sh verifies the hash before signing or submitting to notarytool. notarytool only attests to bytes-at-submission, so a CI step compromise (or a misordered pipeline) between create and submit would otherwise be undetected; the pin closes that window. Closes #22
URLSession instances throughout the codebase rely on the system trust store. A compromised root CA or an enterprise MITM proxy can intercept all auth-bearing traffic, including the bearer access token, model prompts, and generated source. This change adds the scaffolding for SPKI pinning: - pinnedKeyHashes: empty until ops populates it - enforce: false until the pins land + call sites migrate - pinningSession() helper to build a URLSession with the delegate DRAFT NOTE: pinning is intentionally OFF until: 1. The production CDN / API SPKI SHA-256 hashes are populated (primary + backup so rotation doesn't brick installed copies). 2. The SPKI prefix step in PinningDelegate is implemented for real; today it hashes the raw key, not the full SPKI structure. Hooking this up half-implemented would refuse all connections. 3. Auth + main-API call sites are migrated to pinningSession(). 4. A documented rotation procedure exists: ship the new pin in app N before deploying the new cert in environment. Tracked end-to-end in #23. Refs #23
URLSession.shared has a global cookie jar, credential cache, and HTTP cache that's shared with every other URLSession.shared user in the process — including unrelated builder traffic. Auth requests should not share that state. Add a private ephemeral URLSession scoped to AuthManager: - httpCookieAcceptPolicy = .never - httpShouldSetCookies = false - urlCredentialStorage = nil - requestCachePolicy = .reloadIgnoringLocalCacheData and route the two URLSession.shared.data(for:) call sites (fetchUser, requestRefreshedSession) through it. Closes #24
yamlSingleQuoted correctly doubles single quotes per YAML, but bracketing / control bytes, NUL, line-separator characters, and unbalanced multi-byte sequences in model-supplied displayName / bundleId / target identifiers can still produce YAML that xcodegen reads differently than intended. - Strip non-printable / control / line-separator scalars before quoting - Add validatedDisplayName() and validatedBundleIdentifier() helpers that callers can use to fall back to safe defaults when the model returns garbage; existing call sites are left as-is so this lands as a drop-in tightening rather than a behavior change. Long-term, replace string-interpolation YAML emission with Yams (already in the dependency graph). Tracked in #25. Closes #25
When the Sparkle public key (SUPublicEDKey) is correctly embedded and the appcast signature path is enforced, automatic updates are reasonable. Until those guarantees are in place, defense-in-depth requires that the user clicks through every update so a hijacked update channel can't deliver a payload silently. Flip back to true once the SUPublicEDKey hardening lands.
buildRequest previously dropped the Authorization header silently when the token was nil/empty. The server-side authz gate is the real one, but this refusal makes the failure mode loud and unambiguous (throws APIError.unauthorized instead of issuing an unauthenticated request and surfacing a confusing 401 / parse error downstream). If a future caller legitimately needs an unauthenticated request (e.g. a public health endpoint), the right shape is an explicit accessToken= "" parameter at a non-helper call site, not silently dropping the header in the helper. Closes #27
[:] is the empty-dictionary literal in Swift; [] would be parsed as an empty array, which conflicts with the [String: Set<String>] type annotation. Caught by 'swift build' on the merged main. (The PR that introduced this — #50 / fix/23-cert-pinning — was a DRAFT scaffold landed alongside the rest of the security fixes; the build error only surfaced when the merged main was built locally.)
The two tests that asserted UserDefaults to Keychain auto-migration (testAuthTokenStoreMigratesLegacyDefaultsIntoKeychain and testKeychainAuthLocalStorageMigratesLegacyUserDefaultsData) verified the exact behavior that fix #9 / commit 2060490 deliberately removed for security reasons. Renamed and inverted both tests to assert the new contract: - legacy values in UserDefaults are NOT adopted as credentials - legacy values are scrubbed from UserDefaults - Keychain remains empty (no silent migration) Other test suites unchanged. Two pre-existing failures (ProductionGuideTests.testProductionGuideMentionsConfiguredKeys… and SuperwallManagementServiceTests.testBootstrapStarterMonetization…) remain — they fail on the pristine initial commit too and are unrelated to the security work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Full security review of 10x-main surfaced 27 issues across the LLM tool loop, auth/Keychain, network, URL-scheme handlers, Sparkle updater, supply chain, and release pipeline. This PR lands fixes for all of them.
✅ swift build → clean (both TenXApp and 10x-evals link)
✅ xcodebuild → ** BUILD SUCCEEDED **
✅ swift test → 161 / 163 (the 2 remaining failures fail on pristine pre-PR main too — unrelated)
29 commits: 27 issue fixes + 2 follow-ups. Each commit message closes its issue with Closes #N (or Refs #N for the 4 drafts that need verification before they're production-ready).
Severity
Critical (5): sandbox disabled · run_command env-leak + no approval gate · Sparkle pubkey is a build-var placeholder · NSWorkspace.open accepts any scheme · workspace path validation allows host-buildable extensions.
High (12): symlink/TOCTOU · Keychain WhenUnlocked (iCloud-syncable) · UserDefaults→Keychain token-poisoning · xcrun env inheritance · adhoc-signed bundled xcodegen with no SHA pin · Sparkle key written to disk · VERCEL_TOKEN on argv · GH Actions tag-pinned · Yams caret-range · streaming OOM · billing-deeplink CSRF · apiBaseURL accepts any scheme.
Medium (8) + Low (2): UDID validation · fonts path · UserDefaults PII · release-notes HTML · DMG hash pin · cert-pinning scaffolding · shared URLSession for auth · YAML control-char sanitization · SUAutomaticallyUpdate=false · APIClient no-token refusal.
What changed (by area)
LLM tool loop — ToolExecutor.swift now scopes run_command env to a minimal allowlist, resolves symlinks (+ ancestor-symlink check + NUL reject) in workspace path validation, and refuses host-buildable file types (Package.swift, Makefile, *.xcconfig, *.entitlements, *.sh, etc.). SimulatorPreviewService.swift scopes xcrun/xcodebuild/simctl env the same way and validates UDID format. XcodePreviewService.swift strips control characters before YAML quoting. Closes #2 #5 #7 #10 #18 #25
Sandbox & entitlements — Both entitlements files flip app-sandbox to true with the loosenings the product needs (network.client, file access for ~/Library/Developer/TenXApp/ and ~/Library/Developer/Xcode/, existing keychain group). ATSApplicationFontsPath scoped to Fonts. Refs #1, Closes #19
Auth / Keychain — AuthKeychainStore.swift switches to kSecAttrAccessibleWhenUnlockedThisDeviceOnly (out of iCloud Keychain sync) and removes the silent UserDefaults→Keychain migration that was a session-fixation primitive without sandbox. AuthManager.swift moves userId/userEmail from UserDefaults into Keychain alongside the token, and routes auth requests through a dedicated ephemeral URLSession (no shared cookie/credential cache). Tests updated to match the no-migration contract. Closes #8 #9 #20 #24
Network — Config.swift validates API_BASE_URL scheme (refuses non-HTTP(S); allows http only against localhost). APIClient.swift caps streamed responses (1 MiB/line, 100 KiB error body) and refuses requests without an access token (fail-fast APIError.unauthorized). Closes #6 #16 #27
URL handlers / deep links — New Services/SafeURLOpening.swift adds NSWorkspace.openIfSafeWebURL(_:) that refuses non-http(s); the seven single-arg NSWorkspace.shared.open(url) call sites (most importantly SparkleUpdater.openReleaseNotes — its URL is not covered by the EdDSA signature) all route through it. BillingViewModel.handleDeepLink requires a one-time nonce from BillingDeepLinkNonceStore (CSRF gate). Closes #4, Refs #17
Sparkle — SparkleUpdater.swift refuses to start the updater unless SUPublicEDKey is a valid 32-byte base64 ed25519 key (crashes in release, warns in debug). SUAutomaticallyUpdate flipped to false so users click through every update until the pubkey is rock-solid. Closes #3 #26
Supply chain — Bundled xcodegen Mach-O is now hash-verified before exec against 10x-macos/Resources/xcodegen.sha256 (it was adhoc-signed with no integrity check). Yams pinned exact: in Package.swift. GitHub Actions in .github/workflows/release-channel.yml pinned by SHA with a version comment. Closes #11 #14 #15
Release pipeline — publish-release.sh writes the Sparkle key via mktemp and shreds on every exit (trap); HTML-escapes every dynamic value in the release-notes generator. deploy-vercel.sh passes VERCEL_TOKEN via env, not argv. create-dmg.sh writes a .sha256 alongside the DMG; notarize-dmg.sh verifies it before signing. Closes #12 #13 #21 #22
Cert-pinning scaffolding — Services/CertificatePinning.swift ships off-by-default; populate the SPKI hashes and finish the SPKI ASN.1-prefix step before flipping enforce. Refs #23
Before merging — please verify
The 4 PRs that landed as drafts need a build/test run on a developer's machine. They're called out in their commit messages too:
#1 / sandbox — verify the app launches and the preview pipeline works end-to-end. If xcrun/xcodebuild/simctl/xcodegen fail under the sandbox, add the narrow entitlement each one needs (don't revert to app-sandbox=false).
#5 / path tightening — verify generation still works on existing eval cases.
#17 / billing nonce — fail-closed: the backend needs to thread a nonce through Stripe's client_reference_id and the return URL template needs to echo it back. Until that lands, callbacks without a nonce surface a clear UI rejection.
#23 / cert pinning — scaffolding only; activate per the steps in the file's doc comment once the SPKI hashes are decided.
Companion follow-ups (not in this PR)
Embed the Sparkle ed25519 public key as a literal in AppInfo.plist (the runtime assertion in this PR makes a misconfigured release impossible to launch, but the build-time embedding still needs to happen).
swift package resolve and commit Package.resolved so transitive deps are pinned.
Convert the workspace path validation from denylist to positive allowlist (long-term direction for #5).
Findings + per-issue tracking: see issues #1–#27 in dr-danny/10x-sec.