Skip to content

fix: block traversal segments in standard page export paths - #787

Merged
surajshetty3416 merged 2 commits into
frappe:developfrom
surajshetty3416:fix/export-path-segments
Aug 28, 2026
Merged

fix: block traversal segments in standard page export paths#787
surajshetty3416 merged 2 commits into
frappe:developfrom
surajshetty3416:fix/export-path-segments

Conversation

@surajshetty3416

Copy link
Copy Markdown
Member

The semgrep frappe-security-file-traversal rule fires on the standard page exporters (export_page_as_standard, export_client_scripts), which write fixtures into an installed app's builder_files/ directory.

Both are gated behind frappe.conf.developer_mode and the path segments come from export_dir_name(), which scrubs the record name and replaces slashes. It did not, however, reject . or .. as a whole segment, so a record named .. would write one directory above its export root.

template_sync.py already had safe_segment() for exactly this. Moved it into builder/utils.py (which template_sync already imports from), routed export_dir_name() through it, and marked the audited writes # nosemgrep, matching how the template exporter annotates its own.

Tested with test_standard_page_sync (28) and test_utils (21), all passing.

export_dir_name scrubbed the name and replaced slashes, but let "." and
".." through as a whole segment. Route it through safe_segment, which
already guards the template fixture paths, and move that helper into
utils so both exporters share one implementation.

Co-Authored-By: Claude <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Reviews (2): Last reviewed commit: "test: cover the export path segment guar..." | Re-trigger Greptile

Comment thread builder/utils.py
@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.06%. Comparing base (81174f2) to head (7d8211d).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #787      +/-   ##
===========================================
+ Coverage    65.98%   66.06%   +0.08%     
===========================================
  Files           93       93              
  Lines        10373    10386      +13     
===========================================
+ Hits          6845     6862      +17     
+ Misses        3528     3524       -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Co-Authored-By: Claude <noreply@anthropic.com>
@surajshetty3416

Copy link
Copy Markdown
Member Author

Added regression coverage for the guard in test_utils (7d8211d): normal names, separator neutralisation (../../etc/passwd -> .._.._etc_passwd), and ValidationError on the empty/./.. segments.

@surajshetty3416
surajshetty3416 merged commit 12c787a into frappe:develop Aug 28, 2026
11 checks passed
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 1.33.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant