Skip to content

Sanitize Telegram artifact filenames#553

Closed
ayskobtw-lil wants to merge 3 commits into
profullstack:masterfrom
ayskobtw-lil:codex/telegram-safe-artifact
Closed

Sanitize Telegram artifact filenames#553
ayskobtw-lil wants to merge 3 commits into
profullstack:masterfrom
ayskobtw-lil:codex/telegram-safe-artifact

Conversation

@ayskobtw-lil
Copy link
Copy Markdown
Contributor

Summary

  • sanitize Telegram bot usernames before using them in local manifest artifact filenames
  • keep the artifact path joined under ctx.outDir
  • add a regression test for usernames containing path separators

Fixes #552.

Verification

npx --yes pnpm@9.12.0 exec vitest run packages/targets/chat-telegram/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 / 31 tests passed

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

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 1, 2026

Greptile Summary

This PR fixes a path-traversal vulnerability in the Telegram target where a bot username containing / or .. could escape ctx.outDir when building the artifact filename. It also cleans up duplicate validator functions in the flatpak, snap, and winget targets and tightens their validation rules.

  • Telegram: introduces safeFileStem() to strip path-separator characters from bot usernames and switches to path.join() for the artifact path; a regression test covers the ../demo_bot case.
  • Flatpak: removes the duplicate validateAppId and updates the per-segment regex to require a letter as the first character, matching the Flatpak reverse-DNS spec.
  • Snap: adds an explicit consecutive-hyphen (--) guard and removes the older, less-strict single-regex validator; winget: removes the redundant second copy of validatePackageId.

Confidence Score: 4/5

Safe to merge; the core path-traversal fix is correct and well-tested. The only rough edge is cosmetic.

The safeFileStem logic is correct and the regression test confirms the ../demo_bot case. The duplicate-function cleanups in flatpak/snap/winget are straightforward. The one thing to tidy up before or after merge is that validateSnapName runs twice per build() call — once explicitly at the top of build() and again inside renderSnapcraftYaml — which is harmless but unnecessary noise.

packages/targets/pkg-snap/src/index.ts — the redundant second validateSnapName call inside renderSnapcraftYaml could be removed.

Important Files Changed

Filename Overview
packages/targets/chat-telegram/src/index.ts Adds safeFileStem to sanitize bot usernames before use in artifact filenames; switches from string concatenation to path.join — path traversal is correctly blocked.
packages/targets/chat-telegram/src/index.test.ts Adds regression test for ../demo_bot traversal username; verifies artifact stays inside outDir.
packages/targets/pkg-flatpak/src/index.ts Removes duplicate validateAppId and tightens segment regex to require letter-start; matches Flatpak reverse-DNS spec more precisely.
packages/targets/pkg-snap/src/index.ts Adds -- consecutive-hyphen check; moves validation into renderSnapcraftYaml, but also keeps explicit call in build(), causing validateSnapName to fire twice per build invocation.
packages/targets/pkg-winget/src/index.ts Removes the duplicate validatePackageId that lived below renderLocaleManifest; the rewritten function at the top (with proper dot-boundary and empty-segment checks) is retained.
packages/targets/pkg-flatpak/src/index.test.ts Adds tests verifying path-traversal and malformed app IDs are rejected before manifest generation and shipping.
packages/targets/pkg-snap/src/index.test.ts Adds snap validation tests for invalid names, including a filesystem-side-effect check that confirms no files are written before validation throws.
packages/targets/pkg-winget/src/index.test.ts Adds tests verifying NoDot, empty-segment, and slash-containing package IDs are rejected in both build and ship.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[build called with botUsername] --> B{safeFileStem}
    B --> C["replace non-[a-zA-Z0-9._-] with '-'"]
    C --> D["strip leading/trailing dots"]
    D --> E["strip leading/trailing hyphens"]
    E --> F{result empty?}
    F -- yes --> G["fallback: 'telegram-bot'"]
    F -- no --> H[safe stem]
    G --> I["join(ctx.outDir, 'telegram-' + stem + '.json')"]
    H --> I
    I --> J[artifact path stays inside outDir]
Loading

Reviews (1): Last reviewed commit: "Sanitize Telegram artifact filenames" | Re-trigger Greptile

}

function renderSnapcraftYaml(ctx: { projectDir: string; version: string; channel: string }, config: Config): string {
validateSnapName(config.snapName);
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 Double validation in build() path

validateSnapName is called inside renderSnapcraftYaml (line 63) AND again explicitly at the start of build() (line 114) before the same renderSnapcraftYaml is invoked. When build() runs, validation fires twice. The explicit guard at line 114 is the one that matters for the "no filesystem side-effects on bad input" guarantee — the call inside renderSnapcraftYaml is redundant and should be removed from there (keeping only the call sites in build and ship directly).

@ralyodio ralyodio closed this Jun 2, 2026
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: chat-telegram artifact path can escape outDir via botUsername

2 participants