@joplin/onenote-converter: Path traversal in OneNote importer allows overwriting arbitrary files

@@ -66,6 +66,7 @@ exports[`InteropService_Importer_OneNote should be able to create notes from cor
     .l3 { padding: 10px 20px 10px 60px }
     .l4 { padding: 10px 20px 10px 80px }
     .l5 { padding: 10px 20px 10px 100px }
+    li.-error a { color: #C11; font-weight: bold; }
 </style>
 <script>
     document.addEventListener('click', function (event) {
@@ -115,7 +116,7 @@ exports[`InteropService_Importer_OneNote should be able to create notes from cor
 
 <div class="title" style="left: 48px; position: absolute; top: 24px;"><div class="container-outline" style="width: 624px;"><div class="outline-element" style="margin-left: 0px;"><span style="font-family: Calibri Light; font-size: 20pt;">title</span></div>
 </div></div><div class="container-outline" style="left: 48px; position: absolute; top: 120px; width: 624px;"><div class="outline-element" style="margin-left: 0px;"><p style="font-size: 11pt; line-height: 17px;"><a href=":/5">Untitled</a></p></div>
-<div class="outline-element" style="margin-left: 0px;"><p style="font-size: 11pt; line-height: 17px;"><a href=":/6">Untitled-0.</a></p></div>
+<div class="outline-element" style="margin-left: 0px;"><p style="font-size: 11pt; line-height: 17px;"><a href=":/6">Untitled_1</a></p></div>
 </div><div class="container-outline" style="left: 909px; position: absolute; top: 132px; width: 624px;"><div class="outline-element" style="margin-left: 0px;"><p style="font-family: Calibri; font-size: 11pt;">&nbsp;</p></div>
 </div>
 
@@ -391,6 +392,7 @@ exports[`InteropService_Importer_OneNote should expect notes to be rendered the
     .l3 { padding: 10px 20px 10px 60px }
     .l4 { padding: 10px 20px 10px 80px }
     .l5 { padding: 10px 20px 10px 100px }
+    li.-error a { color: #C11; font-weight: bold; }
 </style>
 <script>
     document.addEventListener('click', function (event) {
@@ -929,6 +931,7 @@ exports[`InteropService_Importer_OneNote should remove hyperlink from title: Qui
     .l3 { padding: 10px 20px 10px 60px }
     .l4 { padding: 10px 20px 10px 80px }
     .l5 { padding: 10px 20px 10px 100px }
+    li.-error a { color: #C11; font-weight: bold; }
 </style>
 <script>
     document.addEventListener('click', function (event) {
@@ -1185,6 +1188,7 @@ exports[`InteropService_Importer_OneNote should render audio as links to resourc
     .l3 { padding: 10px 20px 10px 60px }
     .l4 { padding: 10px 20px 10px 80px }
     .l5 { padding: 10px 20px 10px 100px }
+    li.-error a { color: #C11; font-weight: bold; }
 </style>
 <script>
     document.addEventListener('click', function (event) {
@@ -1315,6 +1319,7 @@ exports[`InteropService_Importer_OneNote should render links properly by ignorin
     .l3 { padding: 10px 20px 10px 60px }
     .l4 { padding: 10px 20px 10px 80px }
     .l5 { padding: 10px 20px 10px 100px }
+    li.-error a { color: #C11; font-weight: bold; }
 </style>
 <script>
     document.addEventListener('click', function (event) {

@@ -5,6 +5,7 @@ use crate::onenote::page::{Page, parse_page};
 use crate::onestore::OneStore;
 use crate::onestore::object_space::ObjectSpaceRef;
 use crate::shared::exguid::ExGuid;
+use itertools::{Either, Itertools};
 use parser_utils::errors::{ErrorKind, Result};
 
 /// A series of page.
@@ -16,13 +17,24 @@ use parser_utils::errors::{ErrorKind, Result};
 #[derive(Clone, Debug)]
 pub struct PageSeries {
     pages: Vec<Page>,
+    errors: Rc<Vec<String>>,
 }
 
-impl PageSeries {
+impl<'a> PageSeries {
     /// The pages contained in this page series.
     pub fn pages(&self) -> &[Page] {
         &self.pages
     }
+
+    /// Whether any pages failed to import
+    pub fn has_errors(&self) -> bool {
+        !self.errors.is_empty()
+    }
+
+    /// The errors associated with this page series.
+    pub fn errors(&self) -> &[String] {
+        &self.errors
+    }
 }
 
 pub(crate) fn parse_page_series(id: ExGuid, store: Rc<dyn OneStore>) -> Result<PageSeries> {
@@ -32,7 +44,7 @@ pub(crate) fn parse_page_series(id: ExGuid, store: Rc<dyn OneStore>) -> Result<P
         .ok_or_else(|| ErrorKind::MalformedOneNoteData("page series object is missing".into()))?;
     let data = page_series_node::parse(object.as_ref())?;
 
-    let pages = data
+    let pages_and_errors = data
         .page_spaces
         .into_iter()
         .map(|page_space_id| {
@@ -41,8 +53,15 @@ pub(crate) fn parse_page_series(id: ExGuid, store: Rc<dyn OneStore>) -> Result<P
                 .ok_or_else(|| ErrorKind::MalformedOneNoteData("page space is missing".into()))?;
             Ok(space)
         })
-        .map(|page_space: Result<ObjectSpaceRef>| parse_page(page_space?))
-        .collect::<Result<_>>()?;
+        .map(|page_space: Result<ObjectSpaceRef>| parse_page(page_space?));
+
+    let (pages, errors) = pages_and_errors.partition_map(|result| match result {
+        Ok(page) => Either::Left(page),
+        Err(error) => Either::Right(format!("Failed to parse page: {:?}", error)),
+    });
 
-    Ok(PageSeries { pages })
+    Ok(PageSeries {
+        pages,
+        errors: Rc::new(errors),
+    })
 }

@@ -19,6 +19,12 @@ pub trait FileApiDriver: Send + Sync {
     /// `path_2` is still appended to `path_1`.
     fn join(&self, path_1: &str, path_2: &str) -> String;
 
+    /// Splits filename into (base, extension).
+    fn split_file_name(&self, filename: &str) -> (String, String) {
+        let ext = self.get_file_extension(filename);
+        let base = filename.strip_suffix(&ext).unwrap_or(filename);
+        (base.into(), ext)
+    }
     fn remove_prefix<'a>(&self, full_path: &'a str, prefix: &str) -> &'a str {
         if let Some(without_prefix) = full_path.strip_prefix(prefix) {
             without_prefix

@@ -71,3 +71,28 @@ impl FileApiDriver for FileApiDriverImpl {
         Path::new(path_1).join(path_2).to_string_lossy().into()
     }
 }
+
+#[cfg(test)]
+mod test {
+    use crate::file_api::FileApiDriver;
+
+    use super::FileApiDriverImpl;
+
+    #[test]
+    fn should_split_file_name() {
+        let fs_driver = FileApiDriverImpl {};
+
+        assert_eq!(
+            fs_driver.split_file_name("a.txt"),
+            (String::from("a"), String::from(".txt"))
+        );
+        assert_eq!(
+            fs_driver.split_file_name("a"),
+            (String::from("a"), String::from(""))
+        );
+        assert_eq!(
+            fs_driver.split_file_name("a test.a.b"),
+            (String::from("a test.a"), String::from(".b"))
+        );
+    }
+}

@@ -0,0 +1,55 @@
+use color_eyre::eyre::Error as ColorError;
+use thiserror::Error;
+
+pub type Result<T> = std::result::Result<T, Error>;
+
+#[derive(Error, Debug)]
+#[error("{kind}")]
+pub struct Error {
+    pub kind: ErrorKind,
+}
+
+impl From<parser_utils::errors::Error> for Error {
+    fn from(value: parser_utils::errors::Error) -> Self {
+        Self {
+            kind: ErrorKind::ParseFailed(value),
+        }
+    }
+}
+
+impl From<ColorError> for Error {
+    fn from(value: ColorError) -> Self {
+        Self {
+            kind: ErrorKind::OtherError(value),
+        }
+    }
+}
+
+impl From<std::io::Error> for Error {
+    fn from(value: std::io::Error) -> Self {
+        Self {
+            kind: ErrorKind::IoError(value),
+        }
+    }
+}
+
+impl From<ErrorKind> for Error {
+    fn from(kind: ErrorKind) -> Self {
+        Self { kind }
+    }
+}
+
+#[derive(Error, Debug)]
+pub enum ErrorKind {
+    #[error("Parsing failed: {0}")]
+    ParseFailed(parser_utils::errors::Error),
+
+    #[error("Rendering failed: {0}")]
+    RenderFailed(String),
+
+    #[error("IO failure: {0}")]
+    IoError(std::io::Error),
+
+    #[error("Failure: {0}")]
+    OtherError(ColorError),
+}

@@ -5,6 +5,7 @@ use wasm_bindgen::{JsError, prelude::wasm_bindgen};
 
 use parser_utils::{fs_driver, log};
 
+mod errors;
 mod notebook;
 mod page;
 mod section;

@@ -77,10 +77,11 @@ impl Renderer {
         base_dir: String,
     ) -> Result<templates::notebook::Section> {
         let mut renderer = section::Renderer::new();
-        let section_path = renderer.render(section, notebook_dir)?;
+        let rendered_section = renderer.render(section, notebook_dir)?;
+        let section_path = &rendered_section.section_dir;
         log!("section_path: {:?}", section_path);
 
-        let path_from_base_dir = String::from(fs_driver().remove_prefix(&section_path, &base_dir));
+        let path_from_base_dir = String::from(fs_driver().remove_prefix(section_path, &base_dir));
         log!("path_from_base_dir: {:?}", path_from_base_dir);
         Ok(templates::notebook::Section {
             name: section.display_name().to_string(),

@@ -1,17 +1,17 @@
 use crate::page::Renderer;
 use color_eyre::Result;
-use color_eyre::eyre::ContextCompat;
 use parser::contents::EmbeddedFile;
 use parser::property::embedded_file::FileType;
 use parser_utils::{fs_driver, log};
-use std::path::PathBuf;
 
 impl<'a> Renderer<'a> {
     pub(crate) fn render_embedded_file(&mut self, file: &EmbeddedFile) -> Result<String> {
         let content;
 
-        let filename = self.determine_filename(file.filename())?;
-        let path = fs_driver().join(self.output.as_str(), filename.as_str());
+        let filename = self
+            .section
+            .to_unique_safe_filename(&self.output, file.filename())?;
+        let path = fs_driver().join(&self.output, &filename);
         log!("Rendering embedded file: {:?}", path);
         fs_driver().write_file(&path, file.data())?;
 
@@ -52,31 +52,4 @@ impl<'a> Renderer<'a> {
         }
         FileType::Unknown
     }
-
-    pub(crate) fn determine_filename(&mut self, filename: &str) -> Result<String> {
-        let mut i = 0;
-        let mut current_filename = filename.to_string();
-
-        loop {
-            if !self.section.files.contains(&current_filename) {
-                self.section.files.insert(current_filename.clone());
-
-                return Ok(current_filename);
-            }
-
-            let path = PathBuf::from(filename);
-            let ext = path.extension().unwrap_or_default();
-            let base = path
-                .as_os_str()
-                .to_str()
-                .wrap_err("Embedded file name is non utf-8")?
-                .strip_suffix(ext.to_string_lossy().as_ref())
-                .wrap_err("Failed to strip extension from file name")?
-                .trim_matches('.');
-
-            current_filename = format!("{}-{}.{}", base, i, ext.to_string_lossy());
-
-            i += 1;
-        }
-    }
 }

@@ -10,9 +10,9 @@ impl<'a> Renderer<'a> {
 
         if let Some(data) = image.data() {
             let filename = self.determine_image_filename(image)?;
-            let path = fs_driver().join(self.output.as_str(), filename.as_str());
+            let path = fs_driver().join(&self.output, &filename);
             log!("Rendering image: {:?}", path);
-            fs_driver().write_file(path.as_str(), &data[..])?;
+            fs_driver().write_file(&path, &data[..])?;
 
             let mut attrs = AttributeSet::new();
             let mut styles = StyleSet::new();
@@ -55,25 +55,17 @@ impl<'a> Renderer<'a> {
 
     fn determine_image_filename(&mut self, image: &Image) -> Result<String> {
         if let Some(name) = image.image_filename() {
-            return self.determine_filename(name);
+            let filename = self.section.to_unique_safe_filename(&self.output, name)?;
+            return Ok(filename);
         }
 
         let ext = image.extension().unwrap_or_else(|| {
             log_warn!("Image missing extension. Defaulting to .png.");
             ".png"
         });
-
-        let mut i = 0;
-        loop {
-            let filename = format!("image{}{}", i, ext);
-
-            if !self.section.files.contains(&filename) {
-                self.section.files.insert(filename.clone());
-
-                return Ok(filename);
-            }
-
-            i += 1;
-        }
+        let filename = self
+            .section
+            .to_unique_safe_filename(&self.output, &format!("image{}", ext))?;
+        Ok(filename)
     }
 }

@@ -1,24 +1,29 @@
+use crate::errors::{ErrorKind, Result};
+use crate::templates::section::TocEntry;
 use crate::{page, templates};
-use color_eyre::eyre::Result;
+use parser::page::Page;
 use parser::section::Section;
 use parser_utils::fs_driver;
 use parser_utils::log;
+use parser_utils::log_warn;
 use std::collections::HashSet;
 
 pub(crate) struct Renderer {
     pub(crate) files: HashSet<String>,
-    pub(crate) pages: HashSet<String>,
+}
+
+pub(crate) struct RenderedSection {
+    pub(crate) section_dir: String,
 }
 
 impl Renderer {
     pub fn new() -> Self {
         Renderer {
             files: Default::default(),
-            pages: Default::default(),
         }
     }
 
-    pub fn render(&mut self, section: &Section, output_dir: String) -> Result<String> {
+    pub fn render(&mut self, section: &Section, output_dir: String) -> Result<RenderedSection> {
         let section_dir = fs_driver().join(
             output_dir.as_str(),
             sanitize_filename::sanitize(section.display_name()).as_str(),
@@ -34,58 +39,147 @@ impl Renderer {
 
         let mut toc = Vec::new();
         let mut fallback_title_index = 0;
+        let mut errors: Vec<String> = Vec::new();
 
         for page_series in section.page_series() {
-            for page in page_series.pages() {
-                let title = page.title_text().map(|s| s.to_string()).unwrap_or_else(|| {
-                    fallback_title_index += 1;
+            let page_errors = page_series.errors();
+            for error in page_errors {
+                log_warn!("Page failed to parse: {:?}", error);
+                errors.push(format!("Parse error: {:?}", error));
+            }
 
-                    format!("Untitled Page {}", fallback_title_index)
-                });
+            for page in page_series.pages() {
+                let render_result =
+                    self.render_page_to_file(page, &section_dir, &output_dir, || {
+                        fallback_title_index += 1;
+                        fallback_title_index
+                    });
+                match render_result {
+                    Ok(toc_entry) => {
+                        toc.push(toc_entry);
+                    }
+                    Err(error) => {
+                        log_warn!("Error rendering page: {:?}", error);
+                        let title = page.title_text().unwrap_or_default();
+                        errors.push(format!("Render error for page {}: {:?}", title, error));
+                    }
+                }
+            }
+        }
 
-                let file_name = title.trim().replace("/", "_");
-                let file_name = self.determine_page_filename(&file_name)?;
-                let file_name = sanitize_filename::sanitize(file_name + ".html");
+        let errors_path = if !errors.is_empty() {
+            let error_toc_entry = self.render_errors_to_file(&errors, &output_dir)?;
+            let errors_path = fs_driver().join(&output_dir, &error_toc_entry.relative_path);
+            toc.push(error_toc_entry);
 
-                let page_path = fs_driver().join(section_dir.as_str(), file_name.as_str());
+            Some(errors_path)
+        } else {
+            None
+        };
 
-                let mut renderer = page::Renderer::new(section_dir.clone(), self);
-                let page_html = renderer.render_page(page)?;
+        let toc_html = templates::section::render(section.display_name(), toc)?;
+        let toc_path = self.write_html_file(&output_dir, section.display_name(), &toc_html)?;
+        log!("ToC: {}", toc_path);
+
+        if let Some(errors_path) = errors_path {
+            Err(ErrorKind::RenderFailed(format!(
+                "Some pages failed to render. First error: {:?}. Full error report written to {}",
+                errors.first(),
+                errors_path
+            ))
+            .into())
+        } else {
+            Ok(RenderedSection { section_dir })
+        }
+    }
 
-                log!("Creating page file: {:?}", page_path);
-                fs_driver().write_file(&page_path, page_html.as_bytes())?;
+    fn render_page_to_file<F>(
+        &mut self,
+        page: &Page,
+        section_dir: &str,
+        output_dir: &str,
+        fallback_title_idx: F,
+    ) -> Result<TocEntry>
+    where
+        F: FnOnce() -> u32,
+    {
+        let title = page
+            .title_text()
+            .map(|s| s.to_string())
+            .unwrap_or_else(|| format!("Untitled Page {}", fallback_title_idx()));
+
+        let mut renderer = page::Renderer::new(section_dir.into(), self);
+        let page_html = renderer.render_page(page)?;
+
+        let page_path = self.write_html_file(section_dir, &title, &page_html)?;
+        log!("Created page file: {:?}", page_path);
+
+        let page_path_without_basedir =
+            String::from(fs_driver().remove_prefix(&page_path, output_dir));
+        Ok(TocEntry {
+            name: title,
+            is_error: false,
+            relative_path: page_path_without_basedir,
+            level: page.level(),
+        })
+    }
 
-                let page_path_without_basedir =
-                    String::from(fs_driver().remove_prefix(&page_path, output_dir.as_str()));
-                toc.push((title, page_path_without_basedir, page.level()))
-            }
-        }
+    fn render_errors_to_file(
+        &mut self,
+        errors: &Vec<String>,
+        output_dir: &str,
+    ) -> Result<TocEntry> {
+        let error_html = templates::errors::render(&errors)?;
+        let errors_path = self.write_html_file(&output_dir, "Errors", &error_html)?;
+        log!("Errors: {}", errors_path);
+
+        Ok(TocEntry {
+            level: 1,
+            is_error: true,
+            name: "⚠️ Errors ⚠️".into(),
+            relative_path: fs_driver().remove_prefix(&errors_path, &output_dir).into(),
+        })
+    }
 
-        let toc_html = templates::section::render(section.display_name(), toc)?;
-        let toc_file = fs_driver().join(
-            output_dir.as_str(),
-            format!("{}.html", section.display_name()).as_str(),
-        );
-        log!("ToC: {:?}", toc_file);
-        fs_driver().write_file(toc_file.as_str(), toc_html.as_bytes())?;
+    fn write_html_file(&mut self, parent_dir: &str, title: &str, html: &str) -> Result<String> {
+        let filename = self.title_to_unique_safe_filename(parent_dir, title, ".html")?;
+        let path = fs_driver().join(&parent_dir, &filename);
+        fs_driver().write_file(&path, html.as_bytes())?;
+        Ok(path)
+    }
 
-        Ok(section_dir)
+    pub(crate) fn to_unique_safe_filename(
+        &mut self,
+        parent_dir: &str,
+        filename: &str,
+    ) -> Result<String> {
+        let (base, ext) = fs_driver().split_file_name(filename);
+        self.title_to_unique_safe_filename(parent_dir, &base, &ext)
     }
 
-    pub(crate) fn determine_page_filename(&mut self, filename: &str) -> Result<String> {
+    fn title_to_unique_safe_filename(
+        &mut self,
+        parent_dir: &str,
+        filename_base: &str,
+        extension: &str,
+    ) -> Result<String> {
+        let filename = filename_base.trim().replace("/", "_");
         let mut i = 0;
-        let mut current_filename = sanitize_filename::sanitize(filename);
+        let mut current_filename =
+            sanitize_filename::sanitize(format!("{}{}", filename, extension));
 
         loop {
-            if !self.pages.contains(&current_filename) {
-                self.pages.insert(current_filename.clone());
-
-                return Ok(current_filename);
+            let current_full_path = fs_driver().join(parent_dir, &current_filename);
+            if !self.files.contains(&current_full_path) {
+                self.files.insert(current_full_path);
+                break;
             }
 
             i += 1;
-
-            current_filename = format!("{}_{}", filename, i);
+            current_filename =
+                sanitize_filename::sanitize(format!("{}_{}{}", filename, i, extension));
         }
+
+        Ok(current_filename)
     }
 }

@@ -0,0 +1,31 @@
+<!DOCTYPE html>
+<html lang="en">
+<head>
+    <meta charset="UTF-8">
+    <title>Errors</title>
+    <style>
+        main {
+            font-family: sans-serif;
+        }
+    </style>
+</head>
+<body>
+
+    <main>
+        <h1>Errors</h1>
+        <p>The following errors occurred during the import process:</p>
+        <ul>
+            {% for entry in errors -%}
+                <li>{{ entry }}</li>
+            {% endfor %}
+        </ul>
+        <p>Some pages may be missing or not imported correctly.</p>
+    </main>
+
+<script>
+    if (window.parent !== null) {
+        window.parent.postMessage(window.location.href, '*');
+    }
+</script>
+</body>
+</html>
\ No newline at end of file

@@ -0,0 +1,15 @@
+use askama::Template;
+use color_eyre::Result;
+use color_eyre::eyre::WrapErr;
+
+#[derive(Template)]
+#[template(path = "errors.html")]
+struct ErrorPageTemplate<'a> {
+    errors: &'a Vec<String>,
+}
+
+pub(crate) fn render(errors: &Vec<String>) -> Result<String> {
+    ErrorPageTemplate { errors }
+        .render()
+        .wrap_err("Failed to render error list template")
+}

@@ -1,3 +1,4 @@
+pub(crate) mod errors;
 pub(crate) mod notebook;
 pub(crate) mod page;
 pub(crate) mod section;

@@ -6,7 +6,7 @@
 <nav>
     <ul>
         {% for page in pages %}
-        <li class="l{{page.level}}"><a href="{{ page.path|urlencode }}" target="content" title="{{ page.name }}">{{ page.name }}</a></li>
+        <li class="l{{page.level}}{% if page.is_error %} -error{% endif %}"><a href="{{ page.relative_path|urlencode }}" target="content" title="{{ page.name }}">{{ page.name }}</a></li>
         {% endfor %}
     </ul>
 </nav>
@@ -17,6 +17,7 @@
     .l3 { padding: 10px 20px 10px 60px }
     .l4 { padding: 10px 20px 10px 80px }
     .l5 { padding: 10px 20px 10px 100px }
+    li.-error a { color: #C11; font-weight: bold; }
 </style>
 <script>
     document.addEventListener('click', function (event) {

@@ -6,27 +6,18 @@ use color_eyre::eyre::WrapErr;
 #[template(path = "section.html")]
 struct NotebookTemplate<'a> {
     name: &'a str,
-    pages: Vec<Page<'a>>,
+    pages: Vec<TocEntry>,
 }
 
-struct Page<'a> {
-    name: &'a str,
-    path: &'a str,
-    level: i32,
+pub(crate) struct TocEntry {
+    pub(crate) name: String,
+    pub(crate) is_error: bool,
+    pub(crate) relative_path: String,
+    pub(crate) level: i32,
 }
 
-pub(crate) fn render(name: &str, pages: Vec<(String, String, i32)>) -> Result<String> {
-    let template = NotebookTemplate {
-        name,
-        pages: pages
-            .iter()
-            .map(|(name, path, level)| Page {
-                name,
-                path,
-                level: *level,
-            })
-            .collect(),
-    };
+pub(crate) fn render(name: &str, pages: Vec<TocEntry>) -> Result<String> {
+    let template = NotebookTemplate { name, pages };
 
     template
         .render()