Skip to content

Sanitize Edge artifact filenames#549

Open
ayskobtw-lil wants to merge 4 commits into
profullstack:masterfrom
ayskobtw-lil:codex/find-next-paid-bug
Open

Sanitize Edge artifact filenames#549
ayskobtw-lil wants to merge 4 commits into
profullstack:masterfrom
ayskobtw-lil:codex/find-next-paid-bug

Conversation

@ayskobtw-lil
Copy link
Copy Markdown
Contributor

Fixes #548

Summary

  • sanitize �rowser-edge product IDs and versions before using them in local artifact filenames
  • keep the original productId unchanged in plan metadata/API usage
  • add a regression test for product IDs containing path separators so dry-run plans stay inside ctx.outDir

Verification

px --yes pnpm@9.12.0 exec vitest run packages/targets/browser-edge/src/index.test.ts -> 1 file / 6 tests passed

px --yes pnpm@9.12.0 --filter @profullstack/sh1pt-target-browser-edge typecheck -> passed

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 1, 2026

Greptile Summary

This PR sanitizes user-supplied identifiers before using them in local artifact filenames across four build targets, preventing path-traversal writes outside ctx.outDir.

  • browser-edge: Introduces safeFileStem() applied to both productId and version when constructing the .zip filename; the raw productId is kept unchanged in the plan JSON and API calls. The new regression test asserts the sanitized artifact path and that the original value survives in plan.productId.
  • pkg-snap / pkg-flatpak / pkg-winget: Removes duplicate validateXxx functions with weaker checks, replacing them with stricter versions. pkg-snap adds an early validateSnapName call before any filesystem side-effects. pkg-flatpak tightens the per-segment regex to require a letter-first character per D-Bus convention.

Confidence Score: 5/5

Safe to merge — the sanitisation logic is correct, tests cover the path-traversal regression and the no-filesystem-side-effects guarantee, and the removal of duplicate validator functions eliminates the only inconsistency that existed before.

The safeFileStem three-step pipeline correctly collapses repeated path separators, strips leading/trailing dots and dashes with + quantifiers, and falls back to a safe default. The original productId is preserved in plan metadata. Validation in the other three adapters fires before any filesystem operations, which the tests confirm explicitly. No incorrect data flows or broken contracts were found in the changed paths.

No files require special attention.

Important Files Changed

Filename Overview
packages/targets/browser-edge/src/index.ts Adds safeFileStem() to sanitize productId and version before use in artifact filenames; original productId is preserved in plan metadata.
packages/targets/browser-edge/src/index.test.ts Adds regression test with productId: '../edge-product'; asserts both the sanitized artifact path and that the original productId is preserved in the dry-run plan JSON.
packages/targets/pkg-snap/src/index.ts Moves validateSnapName to the top of the file, adds consecutive-hyphen check, and calls it inside renderSnapcraftYaml so validation fires before any filesystem work in build().
packages/targets/pkg-flatpak/src/index.ts Removes the duplicate lower validateAppId; updates segment regex to enforce letter-first per D-Bus convention and disallow trailing hyphens within segments.
packages/targets/pkg-winget/src/index.ts Removes the old duplicate validatePackageId from the bottom of the file; the new implementation adds explicit leading/trailing-dot checks and per-segment regex validation.
packages/targets/pkg-snap/src/index.test.ts Three new tests cover: invalid name rejection, no-filesystem-side-effects guarantee, and ship-path validation.
packages/targets/pkg-flatpak/src/index.test.ts Adds tests for build and ship path validation covering path traversal, too-few segments, digit-first segments, and trailing-hyphen segments.
packages/targets/pkg-winget/src/index.test.ts Adds tests covering NoDot, leading-dot, consecutive-dot, and slash-containing package IDs for both build and ship paths.

Reviews (2): Last reviewed commit: "Address Edge filename review" | Re-trigger Greptile

Comment thread packages/targets/browser-edge/src/index.ts Outdated
Comment thread packages/targets/browser-edge/src/index.test.ts
@ayskobtw-lil
Copy link
Copy Markdown
Contributor Author

CI follow-up: the failing full-suite test job was hitting the existing duplicate package-validator declarations in pkg-flatpak/pkg-snap/pkg-winget, not the browser-edge change. I added the same package-validator cleanup needed for the suite to run independently on this branch.

Fresh verification after the update:

px --yes pnpm@9.12.0 exec vitest run packages/targets/browser-edge/src/index.test.ts packages/targets/pkg-flatpak/src/index.test.ts packages/targets/pkg-snap/src/index.test.ts packages/targets/pkg-winget/src/index.test.ts -> 4 files / 28 tests passed

px --yes pnpm@9.12.0 --filter @profullstack/sh1pt-target-browser-edge --filter @profullstack/sh1pt-target-pkg-flatpak --filter @profullstack/sh1pt-target-pkg-snap --filter @profullstack/sh1pt-target-pkg-winget typecheck -> passed

@ayskobtw-lil
Copy link
Copy Markdown
Contributor Author

Addressed the Greptile follow-up:

  • tightened the leading/trailing dash stripping regex to remove repeated dashes
  • added an assertion that the dry-run plan preserves the original unsanitized productId

Fresh local verification after the update:

npx --yes pnpm@9.12.0 exec vitest run packages/targets/browser-edge/src/index.test.ts packages/targets/pkg-flatpak/src/index.test.ts packages/targets/pkg-snap/src/index.test.ts packages/targets/pkg-winget/src/index.test.ts
# 4 files / 28 tests passed

npx --yes pnpm@9.12.0 --filter @profullstack/sh1pt-target-browser-edge --filter @profullstack/sh1pt-target-pkg-flatpak --filter @profullstack/sh1pt-target-pkg-snap --filter @profullstack/sh1pt-target-pkg-winget typecheck
# passed

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.

Bug: browser-edge artifact path can escape outDir via productId

1 participant