Skip to content

harden(save): sanitize --save-suffix into a path-safe token#736

Open
nate-avra wants to merge 1 commit into
mvanhorn:mainfrom
nate-avra:security/sanitize-save-suffix
Open

harden(save): sanitize --save-suffix into a path-safe token#736
nate-avra wants to merge 1 commit into
mvanhorn:mainfrom
nate-avra:security/sanitize-save-suffix

Conversation

@nate-avra

Copy link
Copy Markdown

What

--save-suffix is concatenated into the saved-report filename without sanitization (save_output, compute_save_path_display). This restricts it to the same [a-z0-9-] class already used for the topic slug.

Why

Not exploitable today — the topic goes through slugify(), and --save-suffix is fixed to v3 in the SKILL.md flow and only ever templated from a user's own shell variable in the CONFIGURATION.md wrapper examples, never from network/topic input. But the suffix is glued onto the fixed filename prefix ({slug}-{raw_label}) with no separator boundary, so a future refactor of how prefix/suffix are joined could turn it into a real path traversal. This closes that latent seam defensively rather than relying on the current prefix shape to neutralize it.

How

  • New sanitize_suffix() helper — mirrors slugify() but without the "last30days" fallback, so a suffix that sanitizes to nothing simply drops (no suffix part).
  • Applied at both the real-path (save_output) and display-path (compute_save_path_display) sites so the footer path and the written path stay consistent.
  • Ordinary values (v3, gemini, a client slug) are unchanged.

Tests

  • Extends test_save_output_uses_raw_html_extension_and_suffix with a traversal subcase (../../etc → flattened -etc, no .. in the path).
  • Adds test_save_suffix_cannot_escape_save_directory: save_output(..., suffix="../../ESCAPED") writes inside the target dir and the traversal fragment never lands in the filename.
  • Full suite green locally (2403 passed, 5 skipped).

@greptile-apps

greptile-apps Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a sanitize_suffix() helper that restricts --save-suffix to the [a-z0-9-] character class before it is concatenated into the saved-report filename, closing a latent path-traversal seam in both save_output and compute_save_path_display.

  • New sanitize_suffix() helper mirrors slugify() (same regex, same .strip("-")) but with no fallback token, so a suffix that reduces to nothing simply produces no suffix part rather than substituting a default.
  • Applied at both filename-construction sites (save_output and compute_save_path_display) so the footer-displayed path and the actually-written path stay in sync.
  • Tests added cover both the display path (traversal string ../../etc → flat token etc) and the real write path (../../ESCAPED stays inside the temp directory, out.parent equals the resolved tmpdir).

Confidence Score: 5/5

Safe to merge — tightly scoped defensive hardening with correct implementation and direct test coverage.

The new sanitize_suffix() function mirrors the already-trusted slugify() logic, applied consistently at both filename-construction sites. Ordinary suffix values pass through unchanged. Tests directly verify that a traversal input stays inside the target directory.

No files require special attention.

Important Files Changed

Filename Overview
skills/last30days/scripts/last30days.py Adds sanitize_suffix() helper restricting --save-suffix to [a-z0-9-] and applies it consistently in both save_output and compute_save_path_display; implementation is correct and mirrors existing slugify() logic.
tests/test_html_render.py Adds a traversal subcase to the existing suffix test and a new test_save_suffix_cannot_escape_save_directory that verifies the written file stays inside the target directory and contains no .. fragments.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["--save-suffix input"] --> B["sanitize_suffix()"]
    B --> C["re.sub + strip('-')"]
    C --> D{"safe_suffix empty?"}
    D -- yes --> E["no suffix part"]
    D -- no --> F["suffix_part = '-' + safe_suffix"]
    E --> G["filename written inside save_dir"]
    F --> G
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["--save-suffix input"] --> B["sanitize_suffix()"]
    B --> C["re.sub + strip('-')"]
    C --> D{"safe_suffix empty?"}
    D -- yes --> E["no suffix part"]
    D -- no --> F["suffix_part = '-' + safe_suffix"]
    E --> G["filename written inside save_dir"]
    F --> G
Loading

Reviews (2): Last reviewed commit: "harden(save): sanitize --save-suffix int..." | Re-trigger Greptile

The user-provided --save-suffix was glued directly into the saved-report
filename without sanitization. Not exploitable today (the topic is slugified,
and --save-suffix is fixed to 'v3' in the skill flow / only ever set from the
user's own shell var, never network/topic input), but a future refactor of the
filename prefix/suffix joining could reintroduce a real path traversal.

Restrict the suffix to the same [a-z0-9-] class as the topic slug via a new
sanitize_suffix() helper, applied at both the real-path (save_output) and
display-path (compute_save_path_display) sites so they stay consistent. A
suffix that sanitizes to nothing simply drops (no suffix part). Ordinary
values ('v3', 'gemini', a client slug) are unchanged.

Adds regression tests: a traversal suffix is flattened in the display path and
save_output writes stay inside the target directory.
@nate-avra nate-avra force-pushed the security/sanitize-save-suffix branch from 1996f6a to 892e7b8 Compare July 9, 2026 17:59
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.

2 participants