Skip to content

Validate Edge extension product config #631

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

Merged
ralyodio merged 1 commit into from
Jun 6, 2026
Merged

Validate Edge extension product config #631

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
47 changes: 40 additions & 7 deletions packages/targets/browser-edge/src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,24 +86,57 @@ describe('browser-edge target adapter', () => {
expect(result).toEqual({ artifact });
});

it('keeps product IDs with path separators inside the output directory', async () => {
it('rejects Edge product IDs that are blank or not URL path segments', async () => {
const outDir = await mkdtemp(join(tmpdir(), 'sh1pt-edge-out-'));
const projectDir = await mkdtemp(join(tmpdir(), 'sh1pt-edge-project-'));
tempDirs.push(outDir, projectDir);

const result = await adapter.build(fakeBuildContext({
await expect(adapter.build(fakeBuildContext({
outDir,
projectDir,
version: '1.2.3',
dryRun: true,
}) as any, {
productId: '../edge-product',
sourceDir: 'extension-dist',
});
})).rejects.toThrow('productId must be a single URL path segment');

const plan = JSON.parse(await readFile(result.artifact, 'utf-8'));
expect(plan.productId).toBe('../edge-product');
expect(plan.artifact).toBe(join(outDir, 'edge-product-1.2.3.zip'));
expect(plan.command).toEqual(['zip', '-r', join(outDir, 'edge-product-1.2.3.zip'), '.']);
await expect(adapter.build(fakeBuildContext({
outDir,
projectDir,
version: '1.2.3',
dryRun: true,
}) as any, {
productId: ' ',
sourceDir: 'extension-dist',
})).rejects.toThrow('browser-edge requires productId');

expect(execMock).not.toHaveBeenCalled();
});

it('rejects blank optional text before packaging or publishing', async () => {
const outDir = await mkdtemp(join(tmpdir(), 'sh1pt-edge-out-'));
const projectDir = await mkdtemp(join(tmpdir(), 'sh1pt-edge-project-'));
tempDirs.push(outDir, projectDir);

await expect(adapter.build(fakeBuildContext({
outDir,
projectDir,
version: '1.2.3',
dryRun: true,
}) as any, {
productId: 'edge-product',
sourceDir: ' ',
})).rejects.toThrow('browser-edge requires sourceDir');

await expect(adapter.ship(fakeBuildContext({
outDir,
projectDir,
version: '1.2.3',
dryRun: true,
}) as any, {
productId: 'edge-product',
notes: ' ',
})).rejects.toThrow('browser-edge requires notes');
});
});
47 changes: 36 additions & 11 deletions packages/targets/browser-edge/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,29 @@ interface Config {
notes?: string; // release notes for reviewer
}

function requireText(value: string | undefined, name: string): string {
if (typeof value !== 'string' || value.trim() === '') {
throw new Error(`browser-edge requires ${name}`);
}
return value.trim();
}

function optionalText(value: string | undefined, name: string): string | undefined {
if (value === undefined) return undefined;
const trimmed = requireText(value, name);
return trimmed;
}
Comment on lines +18 to +22

@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 The optionalText helper stores the requireText return value in a local trimmed variable only to immediately return it. Returning the result directly is equivalent and removes the extra variable.

Suggested change
function optionalText(value: string | undefined, name: string): string | undefined {
if (value === undefined) return undefined;
const trimmed = requireText(value, name);
return trimmed;
}
function optionalText(value: string | undefined, name: string): string | undefined {
if (value === undefined) return undefined;
return requireText(value, name);
}

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!


function requireProductId(value: string | undefined): string {
const productId = requireText(value, 'productId');
if (/[\\/?#\x00-\x1F\x7F]/.test(productId)) {
throw new Error('browser-edge productId must be a single URL path segment');
}
return productId;
}
Comment on lines +24 to +30

@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.

P1 .. bypasses the path-segment guard and traverses the API URL

The regex /[\\/?#\x00-\x1F\x7F]/ does not block . or ... Because fetch parses its URL argument via the WHATWG URL algorithm, a productId of .. causes the upload and submit URLs to be silently normalised: …/v1/products/../submissions/… → …/v1/submissions/…. Both API calls then hit the wrong endpoint and the access token is included in those misdirected requests. Add a dot-segment check after requireText returns, e.g. if (productId === '.' || productId === '..') throw new Error('browser-edge productId must be a single URL path segment');, or switch to an allowlist regex.


function sourceDir(ctx: { projectDir: string }, config: Config): string {
const dir = config.sourceDir ?? 'dist';
const dir = optionalText(config.sourceDir, 'sourceDir') ?? 'dist';
return isAbsolute(dir) ? dir : join(ctx.projectDir, dir);
}

Expand All @@ -21,15 +42,16 @@ function safeFileStem(value: string): string {
}

function packageArtifact(ctx: { outDir: string; version: string }, config: Config): string {
return join(ctx.outDir, `${safeFileStem(config.productId)}-${safeFileStem(ctx.version)}.zip`);
return join(ctx.outDir, `${safeFileStem(requireProductId(config.productId))}-${safeFileStem(ctx.version)}.zip`);
}

function packagePlan(ctx: { projectDir: string; outDir: string; version: string }, config: Config) {
const productId = requireProductId(config.productId);
const src = sourceDir(ctx, config);
const artifact = packageArtifact(ctx, config);
return {
provider: 'microsoft-edge-addons',
productId: config.productId,
productId,
version: ctx.version,
sourceDir: src,
artifact,
Expand All @@ -43,10 +65,11 @@ export default defineTarget<Config>({
kind: 'browser-ext',
label: 'Microsoft Edge Add-ons',
async build(ctx, config) {
const productId = requireProductId(config.productId);
const src = sourceDir(ctx, config);
const zipPath = packageArtifact(ctx, config);
Comment on lines 67 to 70

@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 requireProductId called three times per build

build() validates productId on line 68, then packageArtifact validates it again (line 45), and packagePlan validates it a third time (line 49). Threading the already-validated productId into those helpers as a parameter would remove the redundancy.

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!


ctx.log(`pack Edge extension from ${src} for v${ctx.version}`);
ctx.log(`pack Edge extension ${productId} from ${src} for v${ctx.version}`);

if (ctx.dryRun) {
const planPath = join(ctx.outDir, 'edge-package.json');
Expand Down Expand Up @@ -79,9 +102,12 @@ export default defineTarget<Config>({
return { artifact: zipPath };
},
async ship(ctx, config) {
ctx.log(`upload ${config.productId} to Edge Partner Center (v${ctx.version})`);
const productId = requireProductId(config.productId);
const notes = optionalText(config.notes, 'notes');

ctx.log(`upload ${productId} to Edge Partner Center (v${ctx.version})`);
if (ctx.dryRun) {
return { id: `${config.productId}@${ctx.version}`, url: `https://microsoftedge.microsoft.com/addons/detail/${config.productId}` };
return { id: `${productId}@${ctx.version}`, url: `https://microsoftedge.microsoft.com/addons/detail/${productId}` };
}

// Fetch secrets for Edge Publish API OAuth
Expand Down Expand Up @@ -119,7 +145,7 @@ export default defineTarget<Config>({

// Step 2: Upload the package (zip) as a draft submission
ctx.log('uploading package...');
const uploadUrl = `https://api.addons.microsoftedge.microsoft.com/v1/products/${config.productId}/submissions/draft/package`;
const uploadUrl = `https://api.addons.microsoftedge.microsoft.com/v1/products/${productId}/submissions/draft/package`;
const zipBuf = await readFile(ctx.artifact);

const uploadRes = await fetch(uploadUrl, {
Expand All @@ -141,8 +167,7 @@ export default defineTarget<Config>({

// Step 3: Submit the draft for review
ctx.log('submitting for review...');
const submitUrl = `https://api.addons.microsoftedge.microsoft.com/v1/products/${config.productId}/submissions`;
const notes = config.notes;
const submitUrl = `https://api.addons.microsoftedge.microsoft.com/v1/products/${productId}/submissions`;
const submitBody: Record<string, unknown> = {};
if (notes) { submitBody.notes = notes; }

Expand All @@ -164,8 +189,8 @@ export default defineTarget<Config>({
ctx.log('✓ submitted to Edge Partner Center');

return {
id: `${config.productId}@${ctx.version}`,
url: `https://microsoftedge.microsoft.com/addons/detail/${config.productId}`,
id: `${productId}@${ctx.version}`,
url: `https://microsoftedge.microsoft.com/addons/detail/${productId}`,
meta: { submissionId: submitData.id },
};
},
Expand Down
Loading