Conversation
ファイル変更時に全ファイルを再ビルドするのではなく、変更があったファイルのみを 再ビルドするように改善。 - Markdown ファイル変更時はそのファイルのみ再生成 - 静的ファイル変更時はそのファイルのみコピー - テンプレート変更時はそのテンプレートを使用する Markdown ファイルのみ再生成 - 設定ファイル (vss.toml) 変更時のみフルビルドを実行 - ファイル削除時は対応する出力ファイルを削除 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @zztkm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! このプルリクエストは、 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
このプルリクエストは、serve コマンドに増分ビルド機能を追加し、開発体験を向上させる素晴らしい改善です。しかし、不適切なパス処理と入力検証の欠如に関連する複数のセキュリティ問題が特定されました。特に、run_incremental_build におけるファイルウォッチャーからの絶対パスの使用は、Path::join が予期せぬ動作をするため、ソースディレクトリでの不正なファイル書き込みや削除につながります。さらに、Markdown のフロントマターからの未検証のタグ名がタグページ生成ロジックで出力ファイルパスの構築に使用されており、パス・トラバーサルの脆弱性が存在します。これらのセキュリティ問題への対処に加え、テンプレートファイル削除時のバグ修正、テンプレート読み込みやタグページ再生成におけるパフォーマンス改善、およびモジュール間のコード重複削減のためのリファクタリングも検討してください。
| if path_str.ends_with(".md") { | ||
| // Markdown ファイルの削除 | ||
| delete_output_file(path, "", &config.dist, Some("html"))?; | ||
| need_regenerate_tags = true; | ||
| } else if path.starts_with(&config.r#static) { | ||
| // 静的ファイルの削除 | ||
| delete_output_file(path, &config.r#static, &config.dist, None)?; | ||
| } |
There was a problem hiding this comment.
The delete_output_file function is vulnerable to arbitrary file deletion. When called at line 798 in run_incremental_build with an absolute path from the file watcher and an empty src_base_dir, Path::join behaves unexpectedly, leading to the deletion of files in the source directory instead of the dist directory. Additionally, the current implementation at this location does not correctly handle the deletion of template files, which can leave old HTML files behind. It is safer to fall back to a full rebuild if a template is deleted.
if path_str.ends_with(".md") {
// Markdown ファイルの削除
delete_output_file(path, "", &config.dist, Some("html"))?;
need_regenerate_tags = true;
} else if path.starts_with(&config.r#static) {
// 静的ファイルの削除
delete_output_file(path, &config.r#static, &config.dist, None)?;
} else if path.starts_with(&config.layouts) {
// テンプレートが削除された場合はフルビルドを要求
return Err(anyhow::anyhow!(
"Template file deleted, full rebuild required"
));
}
src/subcommand_build.rs
Outdated
| if let Ok((frontmatter, _)) = parse_frontmatter(&content) { | ||
| if let Some(tags_vec) = &frontmatter.tags { | ||
| if !tags_vec.is_empty() { | ||
| let html_path = md_path.with_extension("html"); | ||
| let url = format!("/{}", html_path.to_string_lossy()); | ||
| tag_posts.push(PostMetadata { | ||
| title: frontmatter.title, | ||
| description: frontmatter.description, | ||
| author: frontmatter.author, | ||
| pub_datetime: frontmatter.pub_datetime, | ||
| url, | ||
| tags: Some(tags_vec.clone()), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if !tag_posts.is_empty() { | ||
| generate_tag_pages(tag_posts, &config, &templates)?; |
There was a problem hiding this comment.
The run_incremental_build function parses frontmatter from Markdown files and collects tags without validation. These unvalidated tags are then used to construct file paths in generate_tag_pages. If a tag name contains path traversal sequences like ../, it can lead to arbitrary file writes outside the intended dist directory. This is a critical security vulnerability. Additionally, the metadata collection logic for tag page regeneration duplicates logic found in process_markdown_file. Consider refactoring this into a helper function to improve code maintainability and reduce duplication, while also ensuring proper validation of tag names.
src/subcommand_build.rs
Outdated
| } | ||
|
|
||
| // 単一の Markdown ファイルを処理 | ||
| if let Ok(Some(metadata)) = process_markdown_file(path, &config, &templates) { |
There was a problem hiding this comment.
In run_incremental_build, the path of the changed Markdown file is an absolute path provided by the file watcher. When this path is passed to process_markdown_file, it is used to construct output_path using Path::join (at line 450, although not shown in the diff, it is reachable via this call). In Rust, joining an absolute path to another path replaces the base path. Consequently, the rendered HTML is written to the absolute path of the source file (with a .html extension) instead of being confined to the dist directory. This leads to unauthorized file writes in the source tree.
| // テンプレート変更時は、そのテンプレートを使用する全 Markdown を再ビルド | ||
| let affected_md_files = find_markdown_files_using_template( | ||
| path, | ||
| &config.layouts, | ||
| &config.build.ignore_files, | ||
| )?; | ||
|
|
||
| // テンプレートを再読み込み | ||
| let fresh_templates = load_templates(&config.layouts)?; | ||
|
|
||
| for md_path in affected_md_files { | ||
| if let Ok(Some(metadata)) = | ||
| process_markdown_file(&md_path, &config, &fresh_templates) | ||
| { | ||
| all_posts.push(metadata); | ||
| } | ||
| } | ||
| need_regenerate_tags = true; | ||
| } |
There was a problem hiding this comment.
テンプレートが変更された際の処理で、load_templates が複数回呼び出されており非効率です。find_markdown_files_using_template の内部と、その直後の let fresh_templates = ... の両方で load_templates を呼び出しています。
find_markdown_files_using_template が読み込んだテンプレートを返すようにシグネチャを変更し、呼び出し側でそれを再利用することで、冗長なファイル読み込みをなくし、パフォーマンスを改善できます。
例えば、find_markdown_files_using_template のシグネチャを -> Result<(Vec<PathBuf>, HashMap<String, ramhorns::Template<'static>>)> に変更し、この部分を以下のように修正します。
ChangedFile::Template(path) => {
// テンプレート変更時は、そのテンプレートを使用する全 Markdown を再ビルド
let (affected_md_files, fresh_templates) = find_markdown_files_using_template(
path,
&config.layouts,
&config.build.ignore_files,
)?;
for md_path in affected_md_files {
if let Ok(Some(metadata)) =
process_markdown_file(&md_path, &config, &fresh_templates)
{
all_posts.push(metadata);
}
}
need_regenerate_tags = true;
}
src/subcommand_serve.rs
Outdated
| // 設定を取得 | ||
| let serve_config = get_serve_config(config_path)?; | ||
| let dist_dir = serve_config.dist.clone(); | ||
| let static_dir = serve_config.r#static.clone(); | ||
| let layouts_dir = serve_config.layouts.clone(); |
There was a problem hiding this comment.
get_serve_config を使って設定の一部を読み込む代わりに、subcommand_build::load_config を直接呼び出すことで、コードの重複を減らし、実装を簡潔にできます。subcommand_build::Config は pub(crate) であり、serve モジュールはすでに build モジュールに依存しているため、この変更は自然です。これにより、get_serve_config 関数と ServeConfig 構造体を削除できます。
| // 設定を取得 | |
| let serve_config = get_serve_config(config_path)?; | |
| let dist_dir = serve_config.dist.clone(); | |
| let static_dir = serve_config.r#static.clone(); | |
| let layouts_dir = serve_config.layouts.clone(); | |
| // 設定を取得 | |
| let config = subcommand_build::load_config(config_path)?; | |
| let dist_dir = config.dist.clone(); | |
| let static_dir = config.r#static.clone(); | |
| let layouts_dir = config.layouts.clone(); |
- 絶対パスを相対パスに変換する処理を追加（セキュリティ修正） - テンプレート削除時にフルビルドへフォールバック - Markdown ファイル削除時の出力ファイル削除を安全に修正 - load_templates の重複呼び出しを削除（パフォーマンス改善） - subcommand_serve.rs で subcommand_build::load_config を直接使用 - ServeConfig と get_serve_config を削除（コード重複削除） - collapsible if と useless format の clippy 警告を修正 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
/gemini review 再レビューしてください |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the development experience by adding incremental build functionality to the serve command, speeding up rebuilds on file changes. However, it introduces a critical path traversal vulnerability in the tag page generation logic, where unsanitized tag names from Markdown frontmatter can be exploited to write arbitrary files to the filesystem. It is highly recommended to add validation or sanitization for tag names to prevent this. Furthermore, the code could benefit from refactoring to address some duplication, unused variables, and scattered file search logic, which would improve overall maintainability and performance.
| for md_path in &md_files { | ||
| // frontmatter を読み取ってメタデータを収集 | ||
| if let Ok(content) = fs::read_to_string(md_path) | ||
| && let Ok((frontmatter, _)) = parse_frontmatter(&content) | ||
| && let Some(tags_vec) = &frontmatter.tags | ||
| && !tags_vec.is_empty() | ||
| { | ||
| let html_path = md_path.with_extension("html"); | ||
| let url = format!("/{}", html_path.to_string_lossy()); | ||
| tag_posts.push(PostMetadata { | ||
| title: frontmatter.title, | ||
| description: frontmatter.description, | ||
| author: frontmatter.author, | ||
| pub_datetime: frontmatter.pub_datetime, | ||
| url, | ||
| tags: Some(tags_vec.clone()), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| if !tag_posts.is_empty() { | ||
| generate_tag_pages(tag_posts, &config, &templates)?; |
There was a problem hiding this comment.
The incremental build logic collects tags from all Markdown files and passes them to generate_tag_pages. However, the tag names are used to construct output file paths without proper sanitization. A malicious Markdown file with a tag name containing path traversal sequences (e.g., ../../evil) can cause the application to write files outside the intended dist directory. This is a Path Traversal vulnerability.
Specifically, in generate_tag_pages (called here), the tag_name is substituted into the url_pattern and then joined with the dist directory path. If the resulting relative path contains .., it can escape the dist directory.
| let templates = load_templates(&config.layouts)?; | ||
|
|
||
| // 投稿メタデータを収集（タグページ再生成用） | ||
| let mut all_posts: Vec<PostMetadata> = Vec::new(); |
There was a problem hiding this comment.
all_posts変数はここで初期化され、changed_filesのループ内（773行目、794行目）で値が設定されていますが、実際には一度も読み取られていません。後のタグ再生成ロジック（841-880行目）では、すべてのMarkdownファイルを再スキャンして新しいtag_postsリストを構築しています。これにより、all_postsへのメタデータの収集が冗長で非効率になっています。
効率とコードの明確さを向上させるために、all_posts変数とそれに関連する値設定のロジックを削除することを推奨します。
| // Markdown ファイルの削除 | ||
| // 絶対パスを相対パスに変換してから処理 | ||
| let rel_path = to_relative_path(path); | ||
| let html_path = rel_path.with_extension("html"); | ||
| let output_path = Path::new(&config.dist).join(&html_path); | ||
| if output_path.exists() { | ||
| fs::remove_file(&output_path).with_context(|| { | ||
| format!("Failed to delete output file: {}", output_path.display()) | ||
| })?; | ||
| println!("Deleted: {}", output_path.display()); | ||
| } |
There was a problem hiding this comment.
削除されたMarkdownファイルに対応する出力ファイルを削除するこのコードブロックは、delete_output_file関数内にすでに存在するロジックと重複しています。保守性を向上させ、コードの重複を減らすために、このブロックをdelete_output_fileの呼び出しに置き換えることを検討してください。
プロジェクトルートにあるMarkdownファイルの場合、src_base_dirはカレントディレクトリになります。
// Markdown ファイルの削除
delete_output_file(
path,
¤t_dir.to_string_lossy(),
&config.dist,
Some("html"),
)?;| let md_files = find_files_with_glob("md").context("Failed to find markdown files")?; | ||
| let md_files: Vec<PathBuf> = md_files | ||
| .into_iter() | ||
| .filter(|path| { | ||
| let path_str = path.to_string_lossy(); | ||
| !config | ||
| .build | ||
| .ignore_files | ||
| .iter() | ||
| .any(|ignore| path_str.contains(ignore)) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
すべてのMarkdownファイルを検索し、ignore_filesに基づいてフィルタリングするロジックがここで重複しています。同様のコードがrun_build関数（350-363行目）やfind_markdown_files_using_template関数（671-681行目）にも存在します。
保守性を向上させるために、このロジックを例えばfind_markdown_files(ignore_files: &[String]) -> Result<Vec<PathBuf>>のような専用のヘルパー関数に抽出することを検討してください。これにより、ファイルの探索とフィルタリングのロジックが一元化され、コードがよりクリーンで保守しやすくなります。
There was a problem hiding this comment.
Code Review
This pull request introduces incremental build functionality to the serve command, which is a significant improvement for development experience by speeding up rebuilds on file changes. However, it's critical to address a path traversal vulnerability in run_incremental_build's path handling. The use of to_relative_path can allow absolute paths from outside the project directory to be used in file write and delete operations, which is exploitable via symlinks. Beyond this, there are also opportunities to fix a file path comparison bug, enhance error handling robustness, and consider performance improvements to make the code more stable and maintainable.
| if path_str.ends_with(".md") { | ||
| // Markdown ファイルの削除 | ||
| // 絶対パスを相対パスに変換してから処理 | ||
| let rel_path = to_relative_path(path); | ||
| let html_path = rel_path.with_extension("html"); | ||
| let output_path = Path::new(&config.dist).join(&html_path); | ||
| if output_path.exists() { | ||
| fs::remove_file(&output_path).with_context(|| { | ||
| format!("Failed to delete output file: {}", output_path.display()) | ||
| })?; | ||
| println!("Deleted: {}", output_path.display()); | ||
| } | ||
| need_regenerate_tags = true; |
There was a problem hiding this comment.
This incremental build deletion logic is vulnerable to path traversal. If a deleted markdown file's path is absolute and outside the current directory, to_relative_path returns the absolute path. Joining this with config.dist can lead to the deletion of arbitrary .html files across the filesystem via fs::remove_file. Additionally, the current implementation duplicates logic that could be refactored to use an existing delete_output_file function for better reusability and maintainability. To fix the path traversal, validate that the path is under the current directory using strip_prefix.
| if path_str.ends_with(".md") { | |
| // Markdown ファイルの削除 | |
| // 絶対パスを相対パスに変換してから処理 | |
| let rel_path = to_relative_path(path); | |
| let html_path = rel_path.with_extension("html"); | |
| let output_path = Path::new(&config.dist).join(&html_path); | |
| if output_path.exists() { | |
| fs::remove_file(&output_path).with_context(|| { | |
| format!("Failed to delete output file: {}", output_path.display()) | |
| })?; | |
| println!("Deleted: {}", output_path.display()); | |
| } | |
| need_regenerate_tags = true; | |
| if path_str.ends_with(".md") { | |
| // Markdown ファイルの削除 | |
| // 絶対パスを相対パスに変換してから処理 | |
| if let Ok(rel_path) = path.strip_prefix(¤t_dir) { | |
| let html_path = rel_path.with_extension("html"); | |
| let output_path = Path::new(&config.dist).join(&html_path); | |
| if output_path.exists() { | |
| fs::remove_file(&output_path).with_context(|| { | |
| format!("Failed to delete output file: {}", output_path.display()) | |
| })?; | |
| println!("Deleted: {}", output_path.display()); | |
| } | |
| } | |
| need_regenerate_tags = true; |
| // 絶対パスを相対パスに変換 | ||
| let rel_path = to_relative_path(path); | ||
|
|
||
| // Markdown ファイルが ignore_files に含まれているかチェック | ||
| let path_str = rel_path.to_string_lossy(); | ||
| if config | ||
| .build | ||
| .ignore_files | ||
| .iter() | ||
| .any(|ignore| path_str.contains(ignore)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| // 単一の Markdown ファイルを処理（相対パスを使用） | ||
| if let Ok(Some(metadata)) = process_markdown_file(&rel_path, &config, &templates) { |
There was a problem hiding this comment.
The run_incremental_build function is vulnerable to path traversal when processing markdown files. It uses to_relative_path which can return an absolute path if the input path is not under the current directory (e.g., when following symlinks). When this absolute path is passed to process_markdown_file, it is joined with config.dist using Path::join. In Rust, joining an absolute path replaces the base path entirely, allowing rendered HTML to be written to arbitrary locations on the filesystem.
To fix this, ensure the path is always relative to the current directory before processing.
// 絶対パスを相対パスに変換
let Ok(rel_path) = path.strip_prefix(¤t_dir) else {
continue;
};
// Markdown ファイルが ignore_files に含まれているかチェック
let path_str = rel_path.to_string_lossy();
if config
.build
.ignore_files
.iter()
.any(|ignore| path_str.contains(ignore))
{
continue;
}
// 単一の Markdown ファイルを処理（相対パスを使用）
if let Ok(Some(metadata)) = process_markdown_file(rel_path, &config, &templates) {| fn classify_changed_file( | ||
| path: &Path, | ||
| config_path: &Path, | ||
| static_dir: &str, | ||
| layouts_dir: &str, | ||
| ) -> Option<ChangedFile> { | ||
| let path_str = path.to_string_lossy(); | ||
|
|
||
| // 設定ファイルの変更 | ||
| if path == config_path { | ||
| return Some(ChangedFile::Config); | ||
| } | ||
|
|
||
| fn default_dist() -> String { | ||
| "dist".to_string() | ||
| // 静的ファイルの変更 | ||
| if path.starts_with(static_dir) || path_str.starts_with(&format!("./{}", static_dir)) { | ||
| return Some(ChangedFile::Static(path.to_path_buf())); | ||
| } | ||
|
|
||
| let content = std::fs::read_to_string(config_path) | ||
| .with_context(|| format!("Failed to read config file: {}", config_path.display()))?; | ||
| let config: Config = toml::from_str(&content) | ||
| .with_context(|| format!("Failed to parse config file: {}", config_path.display()))?; | ||
| // テンプレートファイルの変更 | ||
| if path.starts_with(layouts_dir) || path_str.starts_with(&format!("./{}", layouts_dir)) { | ||
| return Some(ChangedFile::Template(path.to_path_buf())); | ||
| } | ||
|
|
||
| Ok(config.dist) | ||
| // Markdown ファイルの変更 | ||
| if path.extension().is_some_and(|ext| ext == "md") { | ||
| return Some(ChangedFile::Markdown(path.to_path_buf())); | ||
| } | ||
|
|
||
| None | ||
| } |
There was a problem hiding this comment.
classify_changed_file 関数は、notify からの絶対パスと設定からの相対パスを比較しているため、静的ファイルとテンプレートファイルの変更を正しく検知できません。このままでは、serve 中にこれらのファイルを変更しても再ビルドがトリガーされません。
この問題を修正するには、呼び出し元の watch_files 関数で static_dir と layouts_dir を絶対パスに変換し、classify_changed_file は &Path を受け取るように変更する必要があります。
watch_files 内の変更例:
// src/subcommand_serve.rs
// ...
let config = subcommand_build::load_config(config_path)?;
let dist_dir = config.dist.clone();
let static_dir = config.r#static.clone();
let layouts_dir = config.layouts.clone();
let current_dir = std::env::current_dir().context("Failed to get current directory")?;
let dist_path = current_dir.join(&dist_dir);
let static_path = current_dir.join(&static_dir); // 追加
let layouts_path = current_dir.join(&layouts_dir); // 追加
// ...
// debouncer のクロージャ内
// ...
} else if let Some(classified) = classify_changed_file(
path,
&config_path_clone,
&static_path, // 変更
&layouts_path, // 変更
) {
// ...fn classify_changed_file(
path: &Path,
config_path: &Path,
static_dir: &Path,
layouts_dir: &Path,
) -> Option<ChangedFile> {
// 設定ファイルの変更
if path == config_path {
return Some(ChangedFile::Config);
}
// 静的ファイルの変更
if path.starts_with(static_dir) {
return Some(ChangedFile::Static(path.to_path_buf()));
}
// テンプレートファイルの変更
if path.starts_with(layouts_dir) {
return Some(ChangedFile::Template(path.to_path_buf()));
}
// Markdown ファイルの変更
if path.extension().is_some_and(|ext| ext == "md") {
return Some(ChangedFile::Markdown(path.to_path_buf()));
}
None
}| if need_regenerate_tags && config.build.tags.enable { | ||
| // 全 Markdown ファイルからメタデータを再収集してタグページを生成 | ||
| let md_files = find_files_with_glob("md").context("Failed to find markdown files")?; | ||
| let md_files: Vec<PathBuf> = md_files | ||
| .into_iter() | ||
| .filter(|path| { | ||
| let path_str = path.to_string_lossy(); | ||
| !config | ||
| .build | ||
| .ignore_files | ||
| .iter() | ||
| .any(|ignore| path_str.contains(ignore)) | ||
| }) | ||
| .collect(); | ||
|
|
||
| let mut tag_posts: Vec<PostMetadata> = Vec::new(); | ||
| for md_path in &md_files { | ||
| // frontmatter を読み取ってメタデータを収集 | ||
| if let Ok(content) = fs::read_to_string(md_path) | ||
| && let Ok((frontmatter, _)) = parse_frontmatter(&content) | ||
| && let Some(tags_vec) = &frontmatter.tags | ||
| && !tags_vec.is_empty() | ||
| { | ||
| let html_path = md_path.with_extension("html"); | ||
| let url = format!("/{}", html_path.to_string_lossy()); | ||
| tag_posts.push(PostMetadata { | ||
| title: frontmatter.title, | ||
| description: frontmatter.description, | ||
| author: frontmatter.author, | ||
| pub_datetime: frontmatter.pub_datetime, | ||
| url, | ||
| tags: Some(tags_vec.clone()), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| if !tag_posts.is_empty() { | ||
| generate_tag_pages(tag_posts, &config, &templates)?; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Markdown ファイルが変更されるたびに、すべての Markdown ファイルを再度ディスクから読み込んで解析し、タグページを再生成しています。サイトの投稿数が増えると、この処理がパフォーマンスのボトルネックになる可能性があります。
改善案として、起動時にすべての投稿のメタデータをキャッシュし、ファイル変更時にはそのファイルのメタデータのみを更新、ファイル削除時にはキャッシュから削除するようにし、そのキャッシュからタグページを生成する方法が考えられます。これにより、ディスク I/O を大幅に削減できます。
| let error_msg = format!("{:#}", e); | ||
| if error_msg.contains("full rebuild required") || has_deletion { |
There was a problem hiding this comment.
エラーメッセージの文字列 "full rebuild required" に依存してフォールバック処理を行うのは、将来の変更で壊れやすく、堅牢性に欠けます。
run_incremental_build がフルビルドの必要性を明確に伝えるための専用のエラー型を返すようにリファクタリングすることをお勧めします。例えば、anyhow::Error::downcast_ref やカスタムエラー enum を使用する方法が考えられます。
thiserror を使った実装例:
// In src/subcommand_build.rs
#[derive(Debug, thiserror::Error)]
#[error("{0}")]
pub struct FullRebuildRequired(pub String);
// In run_incremental_build()
// ...
return Err(FullRebuildRequired("Config changed, full rebuild required".to_string()).into());
// In src/subcommand_serve.rs
// ...
Err(e) => {
if e.is::<subcommand_build::FullRebuildRequired>() || has_deletion {
println!("[INFO] Falling back to full rebuild...");
// ...
} else {
eprintln!("[ERROR] Incremental rebuild failed: {:#}", e);
}
}タグ名に危険な文字列（.., /, \, null バイト）が含まれている場合、 タグページの生成をスキップして警告を表示するように修正。 これにより、悪意のあるタグ名による dist ディレクトリ外への ファイル書き込みを防止。 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
/gemini review 再レビューしてください、日本語でコメントしてください |
There was a problem hiding this comment.
Code Review
This pull request introduces an incremental build feature to the serve command, significantly enhancing build efficiency by implementing specific rebuild strategies for various file types and deletions. However, a critical high-severity path traversal vulnerability has been identified in the incremental build logic for Markdown files. This flaw allows for arbitrary file write and deletion because absolute paths from file system events are not properly validated or relativized before being used in file operations. Addressing this vulnerability is crucial.
| fn to_relative_path(path: &Path) -> PathBuf { | ||
| if path.is_absolute() | ||
| && let Ok(current_dir) = std::env::current_dir() | ||
| && let Ok(rel) = path.strip_prefix(¤t_dir) | ||
| { | ||
| return rel.to_path_buf(); | ||
| } | ||
| path.to_path_buf() | ||
| } |
There was a problem hiding this comment.
The incremental build logic is vulnerable to path traversal because it improperly handles absolute paths. When a Markdown file change or deletion is detected, the code uses to_relative_path to process the file path. If the path is absolute and outside the current working directory, to_relative_path returns the absolute path unchanged. This path is then joined with the output directory using Path::join. In Rust, joining a path with an absolute path replaces the base path entirely. Consequently, the application may write rendered HTML or delete files at arbitrary locations on the file system, limited only by the process's permissions.
For example, if a file change event is triggered for /etc/passwd.md, to_relative_path returns /etc/passwd.md, and Path::new("dist").join("/etc/passwd.html") results in /etc/passwd.html. The application will then attempt to write to or delete this file.
| fn to_relative_path(path: &Path) -> PathBuf { | |
| if path.is_absolute() | |
| && let Ok(current_dir) = std::env::current_dir() | |
| && let Ok(rel) = path.strip_prefix(¤t_dir) | |
| { | |
| return rel.to_path_buf(); | |
| } | |
| path.to_path_buf() | |
| } | |
| fn to_relative_path(path: &Path) -> Option<PathBuf> { | |
| if let Ok(current_dir) = std::env::current_dir() { | |
| if let Ok(rel) = path.strip_prefix(¤t_dir) { | |
| return Some(rel.to_path_buf()); | |
| } | |
| } | |
| if path.is_relative() { | |
| return Some(path.to_path_buf()); | |
| } | |
| None | |
| } |
| let rel_path = to_relative_path(path); | ||
| let html_path = rel_path.with_extension("html"); | ||
| let output_path = Path::new(&config.dist).join(&html_path); | ||
| if output_path.exists() { | ||
| fs::remove_file(&output_path).with_context(|| { | ||
| format!("Failed to delete output file: {}", output_path.display()) | ||
| })?; | ||
| println!("Deleted: {}", output_path.display()); | ||
| } |
There was a problem hiding this comment.
This manual deletion logic for Markdown files is vulnerable to path traversal because it uses to_relative_path, which may return an absolute path if the input is outside the project root. If html_path is absolute, Path::join will replace the config.dist base, leading to arbitrary file deletion. It is recommended to use the delete_output_file function instead, as it correctly uses strip_prefix to ensure the path is relative to a trusted base directory.
// Markdown ファイルの削除
delete_output_file(
path,
¤t_dir.to_string_lossy(),
&config.dist,
Some("html"),
)?;1 participant
Summary
serveコマンドでファイル変更時に全ファイルを再ビルドするのではなく、変更があったファイルのみを再ビルドするように改善Test plan
vss serveを起動vss.tomlを編集 → フルビルドが実行されることを確認🤖 Generated with Claude Code