fix: harden audit logger and add search input validation

[hybridindie]

Summary

• Audit logger hardening (feat: add workflow composition tools (create, modify, validate) #9): Write failures now raise OSError instead of being silently swallowed — if audit logging breaks, tool calls fail rather than operating unaudited. Symlinked audit file paths are rejected to prevent log redirection attacks. Directory creation is cached to avoid repeated mkdir calls.
• Search input validation (feat: add workflow composition tools (create, modify, validate) #8): Empty/whitespace queries rejected, max length enforced on query (200 chars) and model_type (50 chars) to prevent abuse of external API requests to HuggingFace/CivitAI.

Test plan

• All 357 tests pass (6 new tests)
• ruff check clean
• ruff format --check clean
• mypy clean
• All pre-commit hooks pass
• Verify CI passes on GitHub

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Hardens AuditLogger behavior (fail-closed on write errors, mitigate log redirection) and adds input validation to search_models to reduce abuse of external model search APIs.

Changes:

• Enforce non-empty and max-length limits for search_models inputs (query, model_type).
• Update audit logging to raise on write failures and add a symlink-path rejection check with cached directory creation.
• Add tests covering the new search validation and audit hardening behaviors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
src/comfyui_mcp/tools/models.py	Adds search input validation limits before issuing external API calls.
src/comfyui_mcp/audit.py	Changes audit logger to fail closed on write errors and attempts to block symlinked audit log paths.
tests/test_tools_models.py	Adds tests for empty/whitespace/oversized query and oversized model_type.
tests/test_audit.py	Adds tests for write failures raising and for rejecting a symlinked audit log path.

---

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

The symlink check is currently gated by self._audit_file.exists(). A dangling symlink returns exists() == False but is_symlink() == True, so this would not be rejected and open(..., 'a') would follow the symlink target. Consider checking is_symlink() unconditionally (no exists() guard), ideally immediately before each open to reduce TOCTOU risk.

[Copilot]

_ensure_directory() returns early once _dir_created is set, which also skips the symlink safety check on subsequent writes. That allows the audit file to be swapped to a symlink after the first successful (or failed) write without being detected. Consider caching only the directory creation work, but re-checking symlink/hardlink safety on every log() call (or using an os.open-based approach with O_NOFOLLOW on supported platforms).

Suggested change

	if self._dir_created:
	return
	parent = self._audit_file.parent
	parent.mkdir(parents=True, exist_ok=True)
	# Reject symlinked audit file — could redirect entries to attacker-controlled path
	if self._audit_file.exists() and self._audit_file.is_symlink():
	raise OSError(f"Audit log path is a symlink — refusing to write: {self._audit_file}")
	self._dir_created = True
	parent = self._audit_file.parent
	if not self._dir_created:
	parent.mkdir(parents=True, exist_ok=True)
	self._dir_created = True
	# Reject symlinked audit file — could redirect entries to attacker-controlled path
	if self._audit_file.exists() and self._audit_file.is_symlink():
	raise OSError(f"Audit log path is a symlink — refusing to write: {self._audit_file}")

[Copilot]

The permission restoration for the readonly directory only runs if the expected exception is raised. To avoid leaving tmp_path/readonly non-traversable (which can break pytest cleanup) when this test fails unexpectedly on some environments, wrap the chmod change in a try/finally to always restore permissions.

Suggested change

	(tmp_path / "readonly").mkdir()
	(tmp_path / "readonly").chmod(0o444)
	logger = AuditLogger(audit_file=log_file)
	with pytest.raises(OSError):
	logger.log(tool="test", action="called")
	# Restore permissions for cleanup
	(tmp_path / "readonly").chmod(0o755)
	readonly_dir = tmp_path / "readonly"
	readonly_dir.mkdir()
	try:
	readonly_dir.chmod(0o444)
	logger = AuditLogger(audit_file=log_file)
	with pytest.raises(OSError):
	logger.log(tool="test", action="called")
	finally:
	# Restore permissions for cleanup
	readonly_dir.chmod(0o755)

[Copilot]

The new symlink protection is only tested for a symlink pointing to an existing target. Consider adding coverage for a dangling symlink (target missing) and for replacing the audit log with a symlink after an initial successful write, to prevent regressions in the symlink-hardening logic.

[hybridindie]

Closing — superseded by #30 which includes all these changes plus the full P1 security/performance batch.