Skip to content

Validate Safari bundle identifiers #609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rissrice2105-agent wants to merge 1 commit into from
Closed

Validate Safari bundle identifiers #609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions packages/targets/browser-safari/src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,29 @@ describe('Safari extension build planning', () => {
expect(plan.archive.args).toContain('-archivePath');
expect(plan.archive.args).toContain('generic/platform=macos');
});

it('rejects invalid bundle IDs before writing dry-run package plans', async () => {
const outDir = await mkdtemp(join(tmpdir(), 'sh1pt-safari-'));
tempDirs.push(outDir);

await expect(adapter.build(fakeBuildContext({
projectDir: '/tmp/source-project',
outDir,
version: '1.2.3',
dryRun: true,
}) as any, {
bundleId: 'com.acme/MyExtension',
})).rejects.toThrow(/reverse-DNS/);
});

it('rejects invalid bundle IDs before dry-run shipping', async () => {
await expect(adapter.ship(fakeBuildContext({
projectDir: '/tmp/source-project',
outDir: '/tmp/out',
version: '1.2.3',
dryRun: true,
}) as any, {
bundleId: 'MyExtension',
})).rejects.toThrow(/reverse-DNS/);
});
});
36 changes: 24 additions & 12 deletions packages/targets/browser-safari/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,16 @@ function safeFileStem(value: string): string {
return value.replace(/[^a-zA-Z0-9._-]+/g, '-').replace(/^-|-$/g, '') || 'safari-extension';
}

const BUNDLE_ID_PATTERN = /^[A-Za-z][A-Za-z0-9-]*(\.[A-Za-z][A-Za-z0-9-]*)+$/;

@greptile-apps greptile-apps Bot Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 BUNDLE_ID_PATTERN rejects digit-starting DNS components

The regex anchors every component to [A-Za-z] as its first character, so a bundle ID whose reverse-DNS origin starts with a digit — e.g. com.3m.MyApp or com.3shape.Plugin — is rejected even though Apple's bundle ID specification only requires alphanumeric characters, hyphens, and periods. Relaxing the per-component first character to [A-Za-z0-9] better matches the actual spec while still blocking obviously invalid identifiers like bare names without dots or slashes.

Suggested change
const BUNDLE_ID_PATTERN = /^[A-Za-z][A-Za-z0-9-]*(\.[A-Za-z][A-Za-z0-9-]*)+$/;
const BUNDLE_ID_PATTERN = /^[A-Za-z0-9][A-Za-z0-9-]*(\.[A-Za-z0-9][A-Za-z0-9-]*)+$/;


function requireBundleId(config: Config): string {
const bundleId = config.bundleId.trim();

@greptile-apps greptile-apps Bot Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unchecked .trim() on potentially absent bundleId

config.bundleId.trim() throws a TypeError: Cannot read properties of undefined (reading 'trim') when bundleId is absent or null — even though the TypeScript interface marks it as required, callers using as any or raw JSON configs can omit it at runtime. The resulting error gives no hint about the missing field, unlike the clear reverse-DNS message you get for a malformed value. A simple truthiness guard before trim() would surface a more actionable message in that case.

if (!BUNDLE_ID_PATTERN.test(bundleId)) {
throw new Error('Safari bundleId must be a reverse-DNS identifier such as com.example.MyExtension');
}
return bundleId;
}

function planPath(outDir: string, bundleId: string, version: string): string {
return joinLike(outDir, `${safeFileStem(bundleId)}-${safeFileStem(version)}.safari-plan.json`);
}
Expand All @@ -81,12 +91,13 @@ function buildPlan(
ctx: { projectDir: string; outDir: string; version: string },
config: Config,
): SafariPackagePlan {
const bundleId = requireBundleId(config);
const projectDir = resolveLike(ctx.projectDir, config.projectDir ?? '.');
const scheme = config.scheme ?? 'App';
const archivePath = joinLike(ctx.outDir, `${safeFileStem(config.bundleId)}-${safeFileStem(ctx.version)}.xcarchive`);
const archivePath = joinLike(ctx.outDir, `${safeFileStem(bundleId)}-${safeFileStem(ctx.version)}.xcarchive`);
const xcodeProj = joinLike(projectDir, `${scheme}.xcodeproj`);
const xcWorkspace = joinLike(projectDir, `${scheme}.xcworkspace`);
const appName = config.bundleId.split('.').pop() ?? 'Extension';
const appName = bundleId.split('.').pop() ?? 'Extension';
const archiveArgs = [
existsSync(xcWorkspace) ? '-workspace' : '-project',
existsSync(xcWorkspace) ? xcWorkspace : xcodeProj,
Expand All @@ -100,7 +111,7 @@ function buildPlan(
];

return {
bundleId: config.bundleId,
bundleId,
version: ctx.version,
projectDir,
scheme,
Expand All @@ -113,7 +124,7 @@ function buildPlan(
'--app-name',
appName,
'--bundle-identifier',
config.bundleId,
bundleId,
'--force',
'--no-open',
],
Expand All @@ -133,9 +144,9 @@ export default defineTarget<Config>({
label: 'App Store (Safari ext.)',
async build(ctx, config) {
const plan = buildPlan(ctx, config);
const artifact = planPath(ctx.outDir, config.bundleId, ctx.version);
const artifact = planPath(ctx.outDir, plan.bundleId, ctx.version);

ctx.log(`build Safari Web Extension for ${config.bundleId} v${ctx.version}`);
ctx.log(`build Safari Web Extension for ${plan.bundleId} v${ctx.version}`);
await mkdir(ctx.outDir, { recursive: true });

if (ctx.dryRun) {
Expand Down Expand Up @@ -172,9 +183,10 @@ export default defineTarget<Config>({
return { artifact: plan.archivePath };
},
async ship(ctx, config) {
ctx.log(`upload ${config.bundleId} to App Store Connect v${ctx.version}`);
const bundleId = requireBundleId(config);
ctx.log(`upload ${bundleId} to App Store Connect v${ctx.version}`);
if (ctx.dryRun) {
return { id: `${config.bundleId}@${ctx.version}`, url: `https://apps.apple.com/app/${config.bundleId}` };
return { id: `${bundleId}@${ctx.version}`, url: `https://apps.apple.com/app/${bundleId}` };
}

// Fetch App Store Connect API credentials from secrets
Expand All @@ -197,7 +209,7 @@ export default defineTarget<Config>({

// Step 1: Find the app in App Store Connect
ctx.log('looking up app in App Store Connect...');
const searchUrl = `${apiBase}/apps?filter[bundleId]=${encodeURIComponent(config.bundleId)}`;
const searchUrl = `${apiBase}/apps?filter[bundleId]=${encodeURIComponent(bundleId)}`;
const searchRes = await fetch(searchUrl, { headers: authHeaders });

if (!searchRes.ok) {
Expand All @@ -206,7 +218,7 @@ export default defineTarget<Config>({
const searchData = (await searchRes.json()) as { data?: Array<{ id: string; attributes: { name: string } }> };
const app = searchData.data?.[0];
if (!app) {
throw new Error(`No app found for bundle ID ${config.bundleId} — create the app record in App Store Connect first`);
throw new Error(`No app found for bundle ID ${bundleId} — create the app record in App Store Connect first`);
}
ctx.log(`✓ found app: ${app.attributes.name} (${app.id})`);

Expand Down Expand Up @@ -258,8 +270,8 @@ export default defineTarget<Config>({
ctx.log('✓ build uploaded to App Store Connect');

return {
id: `${config.bundleId}@${ctx.version}`,
url: `https://apps.apple.com/app/${config.bundleId}`,
id: `${bundleId}@${ctx.version}`,
url: `https://apps.apple.com/app/${bundleId}`,
};
},
async status(id) {
Expand Down
Loading