Skip to content

fix: strip control characters from sanitized filenames#9151

Open
karthiksuki wants to merge 1 commit into
makeplane:previewfrom
karthiksuki:fix/filename-control-char-sanitization
Open

fix: strip control characters from sanitized filenames#9151
karthiksuki wants to merge 1 commit into
makeplane:previewfrom
karthiksuki:fix/filename-control-char-sanitization

Conversation

@karthiksuki
Copy link
Copy Markdown

@karthiksuki karthiksuki commented May 27, 2026

Prevent tab, newline, and other ASCII control characters from appearing in S3 object keys generated from user-provided upload filenames.

Fixes #9127

Description

Prevent tab, newline and other ASCII Keys char in the s3 object pr regarding security standards

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced filename sanitization in file uploads to handle a broader range of control characters, improving compatibility with storage systems.

Review Change Stack

Prevent tab, newline, and other ASCII control characters from appearing
in S3 object keys generated from user-provided upload filenames.

Fixes makeplane#9127

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2e85ee27-7a6a-4d81-b510-8fc01f863e8a

📥 Commits

Reviewing files that changed from the base of the PR and between 0acb32e and 8274f60.

📒 Files selected for processing (1)
  • apps/api/plane/utils/path_validator.py

📝 Walkthrough

Walkthrough

The sanitize_filename function in the path validator utility now removes all ASCII control characters (code points 0–31 and 127) from filenames used in S3 object keys, broadening protection beyond null-byte filtering, with the docstring updated accordingly.

Changes

Filename Sanitization Control Characters

Layer / File(s) Summary
Sanitization logic and documentation
apps/api/plane/utils/path_validator.py
sanitize_filename switches from null-byte-only replacement to removing all ASCII control characters (0–31 and DEL/127), mitigating S3 key storage inconsistencies and display issues. Docstring updated to describe the expanded control-character filtering.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

  • #9127: This PR directly implements the suggested fix for the control-character sanitization vulnerability by updating sanitize_filename to strip all ASCII control characters (0–31 and 127) instead of only null bytes.

Poem

A rabbit hops through S3 keys so bright, ✨🐰
No control chars shall lurk out of sight,
From null bytes to DEL, they're all swept away,
Safe uploads for servers, hip-hip-hooray! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is incomplete; it lacks the required template structure with sections like Type of Change, Test Scenarios, and Screenshots. Fill out the provided template with all required sections: Type of Change (bug fix), Test Scenarios, and any applicable Screenshots/Media.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: removing control characters from sanitized filenames.
Linked Issues check ✅ Passed The PR correctly addresses issue #9127 by implementing the suggested fix to remove all ASCII control characters (0-31 and 127) from filenames.
Out of Scope Changes check ✅ Passed All changes are scoped to the sanitize_filename() function update in path_validator.py to strip control characters, directly addressing the linked security issue.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 27, 2026

CLA assistant check
All committers have signed the CLA.

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.

[Security/Medium] Filename Sanitization Does Not Strip Control Characters

2 participants