Validate iOS bundle identifiers#593
Conversation
Greptile SummaryThis PR introduces a
Confidence Score: 3/5The validation logic contains a defect that lets malformed bundle IDs through, so the guard is not as complete as intended. The regex permits segments ending with a hyphen; a bundle ID like packages/targets/mobile-ios/src/index.ts — the BUNDLE_ID_PATTERN regex on line 11 needs tightening to disallow trailing hyphens in each label. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[build / ship called] --> B[requireBundleId]
B --> C{bundleId present?}
C -- No --> D[throw: mobile-ios requires bundleId]
C -- Yes --> E{BUNDLE_ID_PATTERN.test}
E -- Fail --> F[throw: must be valid reverse-DNS identifier]
E -- Pass --> G[return normalized bundleId]
G --> H[build: xcodebuild archive log]
G --> I[ship: upload to App Store / TestFlight]
I --> J{dryRun?}
J -- Yes --> K[return id: dry-run]
J -- No --> L[return id: bundleId@version]
Reviews (1): Last reviewed commit: "Validate iOS bundle identifiers" | Re-trigger Greptile |
| testflightGroups?: string[]; | ||
| } | ||
|
|
||
| const BUNDLE_ID_PATTERN = /^[A-Za-z][A-Za-z0-9-]*(\.[A-Za-z][A-Za-z0-9-]*)+$/; |
There was a problem hiding this comment.
The regex allows labels that end with a hyphen (e.g.,
com.example-.ios), which is invalid per RFC 1123 and Apple's bundle-ID rules. A segment like [A-Za-z0-9-]* can match a trailing hyphen because * imposes no constraint on the final character. This means requireBundleId would silently accept com.my-corp-.ios, com.acme-.app, etc., which Apple rejects at submission time.
| const BUNDLE_ID_PATTERN = /^[A-Za-z][A-Za-z0-9-]*(\.[A-Za-z][A-Za-z0-9-]*)+$/; | |
| const BUNDLE_ID_PATTERN = /^[A-Za-z]([A-Za-z0-9-]*[A-Za-z0-9])?(\.[A-Za-z]([A-Za-z0-9-]*[A-Za-z0-9])?)+$/; |
| describe('iOS target', () => { | ||
| it('rejects invalid bundle identifiers while building', async () => { | ||
| await expect(adapter.build(fakeBuildContext({ | ||
| outDir: '/repo/.sh1pt/out', | ||
| projectDir: '/repo', | ||
| }) as any, { | ||
| bundleId: '../Acme', | ||
| teamId: 'TEAM123456', | ||
| })).rejects.toThrow('mobile-ios bundleId must be a valid reverse-DNS identifier'); | ||
| }); | ||
|
|
||
| it('rejects invalid bundle identifiers while shipping', async () => { | ||
| await expect(adapter.ship(fakeShipContext({ | ||
| channel: 'stable', | ||
| dryRun: false, | ||
| version: '1.2.3', | ||
| }) as any, { | ||
| bundleId: 'com.acme/ios', | ||
| teamId: 'TEAM123456', | ||
| })).rejects.toThrow('mobile-ios bundleId must be a valid reverse-DNS identifier'); | ||
| }); |
There was a problem hiding this comment.
Both new tests cover rejection paths only. There is no positive test confirming that a well-formed bundle ID (e.g.,
com.example.app) actually lets build() and ship() proceed — the smokeTest skips both methods entirely. Without a passing case, a future regression that makes the regex overly strict (rejecting all IDs) would not be caught by this test suite.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Fixes #592.
Changes:
Validation: