Reject control characters in decoded filenames

[nitrobass24]

Summary

• Add validation in _validate_filename() to reject ASCII control characters (0x00-0x1F, 0x7F) in filenames, preventing shell command injection and log injection via CR, LF, tab, and other control characters
• Add 7 integration tests covering control character rejection, normal filename acceptance, and path traversal rejection

Test plan

• ruff check and ruff format --check pass on controller.py
• pyright passes with 0 errors
• All 7 new tests pass: newline, carriage return, tab, SOH (0x01), DEL (0x7F) rejected; normal filenames accepted; path traversal still rejected
• 5 pre-existing test failures in TestControllerHandler (absolute path /value/with/slashes rejected by os.path.isabs) are unrelated to this change

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Enhanced filename validation to reject control characters and path traversal; fixed handling of URL-encoded filenames so filenames are interpreted without an unintended leading slash.

• Tests

  • Added comprehensive tests covering control-character rejects, path-traversal rejects, accepted normal filenames, and URL-encoded filename handling across queue/stop/extract/delete flows.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR tightens filename validation to reject control characters (Unicode 0x00–0x1F and 0x7F) and updates controller handler tests: removes leading / from some encoded payloads and adds seven tests covering control-character rejection, a normal filename acceptance, and a path-traversal rejection.

Changes

Cohort / File(s)	Summary
Validation Logic src/python/web/handler/controller.py	Extended _validate_filename to reject filenames containing control characters in ranges 0x00–0x1F and 0x7F in addition to existing checks.
Validation & Handler Tests src/python/tests/integration/test_web/test_handler/test_controller.py	Updated controller handler tests to remove leading / from certain URL-encoded payloads; added TestControllerHandlerValidation with seven tests: reject control chars (newline, CR, tab, SOH, DEL), accept normal_file.txt, and reject path traversal (../etc/passwd).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• Reject control characters in decoded filenames #300: Matches this change — adds rejection of control characters (0x00–0x1F, 0x7F) in _validate_filename.

Poem

🐰 I nibble bytes both day and night,

I banish sneaky control-char blight.
No tabs, no NULs, no slashy schemes —
Clean filenames hop through my dreams. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Reject control characters in decoded filenames' directly and clearly summarizes the main change: adding validation to reject ASCII control characters in filenames.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/reject-control-chars-filename

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/python/tests/integration/test_web/test_handler/test_controller.py`:
- Around line 195-218: Each control-char rejection test
(test_control_char_newline_rejected, test_control_char_carriage_return_rejected,
test_control_char_tab_rejected, test_control_char_soh_rejected,
test_control_char_del_rejected) should not only assert a 400 response but also
assert that the mocked dispatcher was not invoked; after each
self.test_app.get(...) add queue_command.assert_not_called() (using the existing
mock named queue_command in the test) to prove no command was queued for invalid
filenames—apply the same addition to the other similar rejection tests in this
file as well.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d1d75cbb-d3df-4cb2-9662-0bc420e30147

📥 Commits

Reviewing files that changed from the base of the PR and between e7b5577 and 5d3a352.

📒 Files selected for processing (2)

• src/python/tests/integration/test_web/test_handler/test_controller.py
• src/python/web/handler/controller.py

[nitrobass24]

@CodeRabbit review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

♻️ Duplicate comments (1)

src/python/tests/integration/test_web/test_handler/test_controller.py (1)

237-240: ⚠️ Potential issue | 🟡 Minor

Add fail-closed dispatch assertion for path traversal rejection.

test_path_traversal_rejected currently verifies only HTTP 400. It should also assert no command was queued, consistent with the other rejection tests in this class.

Suggested patch

     def test_path_traversal_rejected(self):
         uri = quote(quote("../etc/passwd", safe=""), safe="")
         resp = self.test_app.get("/server/command/queue/" + uri, expect_errors=True)
         self.assertEqual(400, resp.status_int)
+        self.controller.queue_command.assert_not_called()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/python/tests/integration/test_web/test_handler/test_controller.py` around
lines 237 - 240, test_path_traversal_rejected only checks for HTTP 400 but must
also assert the system fail-closes by not queuing any command; update the test
to, after the GET and status assertion, verify no command was queued by using
the same check used by the other rejection tests in this class (for example,
call the class helper that asserts no command was queued or
assertEqual(len(self.command_queue), 0) if the queue is exposed) so the test
both verifies the 400 response and that no command was enqueued.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/python/tests/integration/test_web/test_handler/test_controller.py`:
- Around line 237-240: test_path_traversal_rejected only checks for HTTP 400 but
must also assert the system fail-closes by not queuing any command; update the
test to, after the GET and status assertion, verify no command was queued by
using the same check used by the other rejection tests in this class (for
example, call the class helper that asserts no command was queued or
assertEqual(len(self.command_queue), 0) if the queue is exposed) so the test
both verifies the 400 response and that no command was enqueued.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6d8430bf-5433-4eb1-af7a-dc3b570ce1f8

📥 Commits

Reviewing files that changed from the base of the PR and between 5d3a352 and 3054779.

📒 Files selected for processing (1)

• src/python/tests/integration/test_web/test_handler/test_controller.py