Skip to content

Fix remaining CodeQL security alerts (round 2) - #69

Merged
liususan091219 merged 1 commit into
mainfrom
fix/codeql-remaining-alerts
Apr 1, 2026
Merged

liususan091219 merged 1 commit into
mainfrom
fix/codeql-remaining-alerts

Conversation

@liususan091219

Copy link
Copy Markdown
Collaborator

Summary

Addresses the 10 CodeQL alerts that remained after PR #68.

Path traversal (agent-api.py) — alerts #2-7, #14

  • Add _safe_path() helper that combines _safe_id() sanitization with resolve() + is_relative_to() validation
  • All file access now goes through _safe_path() instead of constructing paths directly from user input
  • Media endpoint: replace mimetypes.guess_type() with a fixed SAFE_TYPES allowlist (eliminates HTTP response splitting vector)
  • Media path: add strict character allowlist ([a-zA-Z0-9_./-]) before resolving

HTTP response splitting (agent-api.py) — alert #1

  • Eliminated by replacing dynamic mime type guessing with fixed allowlist

Clear-text logging (conversation-server.ts) — alert #15

  • Replace regex phone masking with simple *** + last 4 digits (CodeQL-friendly)

Reflected XSS (conversation-server.ts) — alert #13

  • This is a false positive: twimlResponse() sends XML to Twilio's server, not a browser. All interpolated values already use esc() for XML escaping. May need CodeQL suppression comment if alert persists.

Test plan

  • python3 -c "import py_compile; py_compile.compile('src/agent-api.py', doraise=True)" — syntax check passes
  • Verify CodeQL alerts resolve after merge: gh api repos/sonichi/sutando/code-scanning/alerts --jq '[.[] | select(.state == "open")] | length'

🤖 Generated with Claude Code

agent-api.py:
- Add _safe_path() with resolve() + is_relative_to() for all file access
- Replace mimetypes.guess_type with fixed SAFE_TYPES allowlist
- Sanitize media path with strict character allowlist
- Add resolve + is_relative_to guard on answer task file

conversation-server.ts:
- Replace regex phone masking with simple slice (CodeQL-friendly)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — solid security hardening.

Reviewed each change:

  • _safe_path(): correct defense-in-depth (sanitize + resolve + is_relative_to). Minor: docstring says returns None but type hint is Path (cosmetic).
  • SAFE_TYPES mime allowlist: reasonable coverage, eliminates response splitting. Note: .html/.js/.svg could be XSS vectors if repo contains attacker-influenced files — low risk for local agent.
  • Media endpoint: null byte rejection + strict character allowlist + no silent mutation. Solid.
  • Answer task file write: now validates resolved path stays within tasks/. Good.
  • conversation-server.ts: handles undefined TWILIO_PHONE_NUMBER gracefully.

No regressions. Ship it.

@liususan091219
liususan091219 merged commit a037809 into main Apr 1, 2026
@liususan091219
liususan091219 deleted the fix/codeql-remaining-alerts branch April 1, 2026 23:21
liususan091219 added a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
agent-api.py:
- Add _safe_path() with resolve() + is_relative_to() for all file access
- Replace mimetypes.guess_type with fixed SAFE_TYPES allowlist
- Sanitize media path with strict character allowlist
- Add resolve + is_relative_to guard on answer task file

conversation-server.ts:
- Replace regex phone masking with simple slice (CodeQL-friendly)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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