Harden input validation and clean up logging#264
Merged
Conversation
- FetchImage: validate path is rooted, within images dir, no traversal, image extensions only - Markdown: disable raw HTML passthrough in pipeline (use DisableHtml) - QR scanner: log only format and value length, not raw content - CopilotService: log session name and prompt length, not prompt content - Add InputValidationTests (38 tests) covering path validation and HTML sanitization - Add Markdig package reference to test project Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…write tests
- Extract FetchImage validation into static ValidateImagePath() helper
- Re-throw OperationCanceledException in catch block (was swallowed)
- Remove redundant Contains("..") pre-check (GetFullPath handles traversal)
- Remove dead fullPath.Equals(allowedDir) check
- Pin Markdig to 1.0.0 (was floating Version="*")
- Rewrite FetchImage tests to call production ValidateImagePath directly
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Resolve symlinks before containment check to prevent symlink-based bypass - Add missing .tiff -> image/tiff MIME mapping (was falling back to image/png) - Add symlink bypass test (creates real symlink, verifies rejection) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Extract MarkdownPipeline into MarkdownRenderer.cs (shared by ChatMessageList and tests) - Tests now call production MarkdownRenderer.ToHtml instead of a local pipeline copy - Remove Exists guard from symlink check (LinkTarget works on dangling symlinks too) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
force-pushed
the
security-review-march
branch
from
14772c6 to
f64a8fc
Compare
- Walk parent directories from file to allowedDir, reject if any is a symlink pointing outside the allowed boundary - Return error instead of falling back to unresolved path when resolution fails - Add directory symlink bypass test (symlinked subdir -> /etc) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add URL scheme sanitization to MarkdownRenderer to block javascript:, vbscript:, and data: protocols in Markdig-generated links (DisableHtml only strips raw HTML tags, not markdown-native link URLs) - Refactor ValidateImagePath to walk resolved symlink target parent dirs (prevents file→dir symlink chain bypasses) - Remove di.Exists gate on directory symlink checks (LinkTarget works without it, avoids edge cases with dangling dir symlinks) - Extract WalkParentDirs helper for reuse between original and resolved paths - Add 17 new tests: URL scheme blocking, case-insensitive extensions, double extensions, spaces in filenames Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ValidateImagePath now returns the validated resolved path via out parameter. The FetchImage handler uses this path directly for File.ReadAllBytesAsync, eliminating the window between validation and read where a symlink swap could occur. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- ChatMessageList.RenderMarkdown now calls MarkdownRenderer.ToHtml() which applies SanitizeUrls (blocks javascript:/vbscript:/data: in markdown-native links). Previously called Markdig.Markdown.ToHtml() directly, bypassing the URL sanitizer. - Extension allowlist now checks resolved symlink target path instead of original symlink filename (evil.png -> secrets.json was passing) - Remove unused MdPipeline field and Markdig using directive Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Testing
All 1698 tests pass (1 pre-existing PlatformHelper failure unrelated to this change).