From bfbdb61fb7aae898325240ddfeec0af6f44b9af6 Mon Sep 17 00:00:00 2001 From: Nicolas Mattia Date: Fri, 27 Oct 2023 14:43:14 +0200 Subject: [PATCH] Fix asset URL paths This updates the logic for serving assets from the canister. The logic previously did not cover all paths where an asset may be found. For instance, an asset `/foo/index.html` may have returned 200 on `/foo/` but 404 on `/foo`. Moreover the `/faq` endpoint is fixed to actually return the expected redirect to the FAQ. In practice the canister has extra logic for handling `/faq`, but this fixes the HTML-redirect fallback. This ensures that `/faq/` & `/faq/index.html` also redirect (which the canister does not currently check for). --- src/frontend/faq.html | 15 +++ src/frontend/src/index.ts | 8 -- src/internet_identity/src/assets.rs | 162 +++++++++++++++++++++++----- vite.config.ts | 1 + 4 files changed, 149 insertions(+), 37 deletions(-) create mode 100644 src/frontend/faq.html diff --git a/src/frontend/faq.html b/src/frontend/faq.html new file mode 100644 index 0000000000..aa1c1a550f --- /dev/null +++ b/src/frontend/faq.html @@ -0,0 +1,15 @@ + + + + + + + + Internet Identity + + + + diff --git a/src/frontend/src/index.ts b/src/frontend/src/index.ts index 03a89f30ac..1d381a086f 100644 --- a/src/frontend/src/index.ts +++ b/src/frontend/src/index.ts @@ -101,14 +101,6 @@ const init = async () => { // https://github.com/dfinity/internet-identity#build-features showWarningIfNecessary(); - // Redirect to the FAQ - // The canister should already be handling this with a 301 when serving "/faq", this is just a safety - // measure. - if (window.location.pathname === "/faq") { - const faqUrl = "https://identitysupport.dfinity.org/hc/en-us"; - window.location.replace(faqUrl); - } - const okOrReason = await checkRequiredFeatures(url); if (okOrReason !== true) { return compatibilityNotice(okOrReason); diff --git a/src/internet_identity/src/assets.rs b/src/internet_identity/src/assets.rs index 7ca4885079..775c6fa08b 100644 --- a/src/internet_identity/src/assets.rs +++ b/src/internet_identity/src/assets.rs @@ -261,7 +261,17 @@ fn collect_assets_from_dir(dir: &Dir) -> Vec<(String, Vec, ContentEncoding, ), }; - assets.push((file_to_asset_path(asset), content, encoding, content_type)); + let urlpaths = filepath_to_urlpaths(asset.path().to_str().unwrap().to_string()); + for urlpath in urlpaths { + // XXX: we clone the content for each asset instead of doing something smarter + // for simplicity & because the only assets that currently may be duplicated are + // small HTML files. + // + // XXX: the behavior is undefined for assets with overlapping URL paths (e.g. "foo.html" & + // "foo/index.html"). This assumes that the bundler creating the assets directory + // creates sensible assets. + assets.push((urlpath, content.clone(), encoding, content_type)); + } } assets } @@ -283,33 +293,127 @@ fn file_extension<'a>(asset: &'a File) -> &'a str { .1 } -/// Returns the asset path for a given file: -/// * make relative path absolute -/// * map **/index.html to **/ -/// * map **/.html to **/foo -/// * map **/.js.gz to **/.js -fn file_to_asset_path(asset: &File) -> String { - // make path absolute - let mut file_path = "/".to_string() + asset.path().to_str().unwrap(); - - if file_path.ends_with("index.html") { - // drop index.html filename (i.e. maps **/index.html to **/) - file_path = file_path - .chars() - .take(file_path.len() - "index.html".len()) - .collect() - } else if file_path.ends_with(".html") { - // drop .html file endings (i.e. maps **/.html to **/foo) - file_path = file_path - .chars() - .take(file_path.len() - ".html".len()) - .collect() - } else if file_path.ends_with(".gz") { - // drop .gz for .foo.gz files (i.e. maps **/.js.gz to **/.js) - file_path = file_path - .chars() - .take(file_path.len() - ".gz".len()) - .collect() +/// Returns the URL paths for a given asset filepath. For instance: +/// +/// * "index.html" -> "/", "/index.html" +/// * "foo/bar.html" -> "/foo/bar", "/foo/bar/", "foo/bar/index.html" +/// +/// NOTE: The behavior is undefined if the argument is NOT relative, i.e. if +/// the filepath has a leading slash. +/// +/// NOTE: The returned paths will always start with a slash. +fn filepath_to_urlpaths(file_path: String) -> Vec { + // Create paths, WITHOUT leading slash (leading lash is prepended later) + fn inner(elements: Vec<&str>, last: &str) -> Vec { + if elements.is_empty() && last == "index.html" { + // The special case of the root index.html, which we serve + // on both "/" and "/index.html" + vec!["".to_string(), "index.html".to_string()] + } else if last == "index.html" { + // An index.html in a subpath + let page = elements.join("/").to_string(); + vec![ + format!("{page}"), + format!("{page}/"), + format!("{page}/index.html"), + ] + } else if let Some(page) = last.strip_suffix(".html") { + // A (non-index) HTML page + let mut elements = elements.to_vec(); + elements.push(page); + let page = elements.join("/").to_string(); + vec![ + format!("{page}"), + format!("{page}/"), + format!("{page}/index.html"), + ] + } else if let Some(file) = last.strip_suffix(".gz") { + // A gzipped asset; remove suffix and retry + // XXX: this recursion is safe (i.e. not infinite) because + // we always reduce the argument (remove ".gz") + inner(elements, file) + } else { + // The default cases for any asset + // XXX: here we could create an iterator and `intersperse` + // the element but this feature is unstable at the time + // of writing: https://github.com/rust-lang/rust/issues/79524 + let mut elements = elements.clone(); + elements.push(last); + let asset = elements.join("/").to_string(); + vec![asset] + } + } + + let paths = match file_path.split('/').collect::>().split_last() { + None => { + // The argument was an empty string + // We can't really do much about this, so we fail explicitly + panic!("Expected non-empty filepath for asset"); + } + Some((last, elements)) => inner(elements.to_vec(), last), + }; + + // Prefix everything with "/" + paths.into_iter().map(|path| format!("/{path}")).collect() +} + +#[test] +fn test_filepath_urlpaths() { + fn assert_gen_paths(inp: String, exp: Vec) { + let mut exp = exp.clone(); + exp.sort(); + + let mut actual = filepath_to_urlpaths(inp); + actual.sort(); + assert_eq!(exp, actual); } - file_path + + assert_gen_paths( + "index.html".to_string(), + vec!["/".to_string(), "/index.html".to_string()], + ); + + assert_gen_paths( + "foo.html".to_string(), + vec![ + "/foo".to_string(), + "/foo/".to_string(), + "/foo/index.html".to_string(), + ], + ); + + assert_gen_paths( + "foo/index.html".to_string(), + vec![ + "/foo".to_string(), + "/foo/".to_string(), + "/foo/index.html".to_string(), + ], + ); + + assert_gen_paths("index.css".to_string(), vec!["/index.css".to_string()]); + assert_gen_paths("foo.bar.gz".to_string(), vec!["/foo.bar".to_string()]); + + assert_gen_paths( + "sub/foo.bar.gz".to_string(), + vec!["/sub/foo.bar".to_string()], + ); + + assert_gen_paths( + "foo.html.gz".to_string(), + vec![ + "/foo".to_string(), + "/foo/".to_string(), + "/foo/index.html".to_string(), + ], + ); + + assert_gen_paths( + "sub/foo.html.gz".to_string(), + vec![ + "/sub/foo".to_string(), + "/sub/foo/".to_string(), + "/sub/foo/index.html".to_string(), + ], + ); } diff --git a/vite.config.ts b/vite.config.ts index 8c1183dd60..de172b9ad7 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -43,6 +43,7 @@ export default defineConfig(({ mode }: UserConfig): UserConfig => { rollupOptions: { // Bundle only english words in bip39. external: /.*\/wordlists\/(?!english).*\.json/, + input: ["src/frontend/index.html", "src/frontend/faq.html"], output: { entryFileNames: `[name].js`, // II canister only supports resources that contains a single dot in their filenames. qr-creator.js.gz = ok. qr-creator.min.js.gz not ok. qr-creator.es6.min.js.gz no ok.