From e71a0cc0c2daefa6d95beda5154b9815a3e5ae89 Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Fri, 3 Nov 2023 15:20:26 +0100 Subject: [PATCH 1/7] unit test --- .../frontend/ic-asset/src/asset/config.rs | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/src/canisters/frontend/ic-asset/src/asset/config.rs b/src/canisters/frontend/ic-asset/src/asset/config.rs index 68a883dde3..4d000cfa94 100644 --- a/src/canisters/frontend/ic-asset/src/asset/config.rs +++ b/src/canisters/frontend/ic-asset/src/asset/config.rs @@ -936,4 +936,82 @@ mod with_tempdir { }, ); } + + #[test] + fn the_order_does_not_matter() { + let cfg = Some(HashMap::from([( + "".to_string(), + r#"[ + { + "match": "**/nested/**/*", + "allow_raw_access": false + }, + { + "match": ".well-known", + "ignore": false + }, + { + "match": "**/*", + "headers": { + "X-Frame-Options": "DENY", + "X-Content-Type-Options": "nosniff", + "Strict-Transport-Security": "max-age=31536000; includeSubDomains", + "Referrer-Policy": "same-origin", + "X-XSS-Protection": "1; mode=block" + } + } + ] +"# + .to_string(), + )])); + let cfg2 = Some(HashMap::from([( + "".to_string(), + r#"[ + { + "match": ".well-known", + "ignore": false + }, + { + "match": "**/*", + "headers": { + "X-Frame-Options": "DENY", + "X-Content-Type-Options": "nosniff", + "Strict-Transport-Security": "max-age=31536000; includeSubDomains", + "Referrer-Policy": "same-origin", + "X-XSS-Protection": "1; mode=block" + } + }, + { + "match": "**/nested/**/*", + "allow_raw_access": false + } + ] + "# + .to_string(), + )])); + + let x = { + let assets_temp_dir = create_temporary_assets_directory(cfg, 0); + let assets_dir = assets_temp_dir.path().canonicalize().unwrap(); + let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir).unwrap(); + assets_config + .get_asset_config(assets_dir.join("nested/deep/the-next-thing.toml").as_path()) + .unwrap() + }; + let y = { + let assets_temp_dir2 = create_temporary_assets_directory(cfg2, 0); + let assets_dir2 = assets_temp_dir2.path().canonicalize().unwrap(); + let mut assets_config2 = AssetSourceDirectoryConfiguration::load(&assets_dir2).unwrap(); + assets_config2 + .get_asset_config( + assets_dir2 + .join("nested/deep/the-next-thing.toml") + .as_path(), + ) + .unwrap() + }; + + assert_eq!(x.allow_raw_access, y.allow_raw_access); + assert_eq!(x, y); + } } From 9ce00ab5700ab9a75b19d412f7ad7636a1bee9fe Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Fri, 3 Nov 2023 17:31:23 +0100 Subject: [PATCH 2/7] fix --- src/canisters/frontend/ic-asset/src/asset/config.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/canisters/frontend/ic-asset/src/asset/config.rs b/src/canisters/frontend/ic-asset/src/asset/config.rs index 4d000cfa94..54bb6770de 100644 --- a/src/canisters/frontend/ic-asset/src/asset/config.rs +++ b/src/canisters/frontend/ic-asset/src/asset/config.rs @@ -248,16 +248,13 @@ impl AssetConfig { (_, Maybe::Null) => self.headers = None, (_, Maybe::Absent) => (), }; - if other.ignore.is_some() { self.ignore = other.ignore; } - if other.enable_aliasing.is_some() { self.enable_aliasing = other.enable_aliasing; } - - if other.allow_raw_access.is_some() { + if self.allow_raw_access.is_none() && other.allow_raw_access.is_some() { self.allow_raw_access = other.allow_raw_access; } self From ccc77c2a914692dc3e74fd56b0777cb80cde7213 Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Fri, 3 Nov 2023 17:38:16 +0100 Subject: [PATCH 3/7] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a496863420..5edf53d620 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,8 @@ Updated Motoko to [0.10.1](https://github.com/dfinity/motoko/releases/tag/0.10.1 ### Frontend canister +Fixed an issue where the `allow_raw_access` configuration value was being affected by the order of declarations in the configuration file in `.ic-assets.json`. + Defining a custom `etag` header no longer breaks certification. Fixed a certification issue where under certain conditions the fallback file (`/index.html`) was served with an incomplete certificate tree, not proving sufficiently that the fallback file may be used as a replacement. From a2207fc64e2926e312aa4cd3ed9d3ae84364bdb1 Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Mon, 6 Nov 2023 14:05:39 +0100 Subject: [PATCH 4/7] Update src/canisters/frontend/ic-asset/src/asset/config.rs Co-authored-by: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com> --- src/canisters/frontend/ic-asset/src/asset/config.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/canisters/frontend/ic-asset/src/asset/config.rs b/src/canisters/frontend/ic-asset/src/asset/config.rs index 54bb6770de..3b5cb94bd8 100644 --- a/src/canisters/frontend/ic-asset/src/asset/config.rs +++ b/src/canisters/frontend/ic-asset/src/asset/config.rs @@ -1008,7 +1008,8 @@ mod with_tempdir { .unwrap() }; - assert_eq!(x.allow_raw_access, y.allow_raw_access); + assert_eq!(x.allow_raw_access, false); + assert_eq!(y.allow_raw_access, false); assert_eq!(x, y); } } From 4124071e269d356c710c837bf5bd4d0140519f9a Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Thu, 9 Nov 2023 22:03:46 +0100 Subject: [PATCH 5/7] extend tests and debug printing --- .../frontend/ic-asset/src/asset/config.rs | 156 +++++++++++++----- 1 file changed, 117 insertions(+), 39 deletions(-) diff --git a/src/canisters/frontend/ic-asset/src/asset/config.rs b/src/canisters/frontend/ic-asset/src/asset/config.rs index 3b5cb94bd8..ce00b241a0 100644 --- a/src/canisters/frontend/ic-asset/src/asset/config.rs +++ b/src/canisters/frontend/ic-asset/src/asset/config.rs @@ -34,10 +34,6 @@ pub(crate) struct CacheConfig { pub(crate) max_age: Option, } -fn default_raw_access() -> Option { - Some(true) -} - /// A single configuration object, from `.ic-assets.json` config file #[derive(Derivative, Clone, Serialize)] #[derivative(Debug, PartialEq)] @@ -254,7 +250,7 @@ impl AssetConfig { if other.enable_aliasing.is_some() { self.enable_aliasing = other.enable_aliasing; } - if self.allow_raw_access.is_none() && other.allow_raw_access.is_some() { + if other.allow_raw_access.is_some() { self.allow_raw_access = other.allow_raw_access; } self @@ -266,6 +262,7 @@ impl AssetConfig { mod rule_utils { use super::{AssetConfig, AssetConfigRule, CacheConfig, HeadersConfig, Maybe}; use crate::error::LoadRuleError; + use derivative::Derivative; use globset::{Glob, GlobMatcher}; use serde::{Deserialize, Serializer}; use serde_json::Value; @@ -336,7 +333,7 @@ mod rule_utils { } } - #[derive(Deserialize)] + #[derive(Deserialize, Derivative)] #[serde(deny_unknown_fields)] pub(super) struct InterimAssetConfigRule { r#match: String, @@ -345,7 +342,7 @@ mod rule_utils { headers: Maybe, ignore: Option, enable_aliasing: Option, - #[serde(default = "super::default_raw_access")] + #[derivative(Default(value = "Some(true)"))] allow_raw_access: Option, } @@ -430,6 +427,16 @@ mod rule_utils { if aliasing { "enabled" } else { "disabled" } )); } + if let Some(allow_raw_access) = self.allow_raw_access { + s.push_str(&format!( + " - enable raw access: {}\n", + if allow_raw_access { + "enabled" + } else { + "disabled" + } + )); + } if let Some(ref headers) = self.headers { for (key, value) in headers { s.push_str(&format!( @@ -940,21 +947,18 @@ mod with_tempdir { "".to_string(), r#"[ { - "match": "**/nested/**/*", - "allow_raw_access": false - }, - { - "match": ".well-known", - "ignore": false + "match": "**/deep/**/*", + "allow_raw_access": false, + "cache": { + "max_age": 22 + }, + "enable_aliasing": true, + "ignore": true }, { "match": "**/*", "headers": { - "X-Frame-Options": "DENY", - "X-Content-Type-Options": "nosniff", - "Strict-Transport-Security": "max-age=31536000; includeSubDomains", - "Referrer-Policy": "same-origin", - "X-XSS-Protection": "1; mode=block" + "X-Frame-Options": "DENY" } } ] @@ -965,22 +969,93 @@ mod with_tempdir { "".to_string(), r#"[ { - "match": ".well-known", - "ignore": false + "match": "**/*", + "headers": { + "X-Frame-Options": "DENY" + } }, { - "match": "**/*", + "match": "**/deep/**/*", + "allow_raw_access": false, + "cache": { + "max_age": 22 + }, + "enable_aliasing": true, + "ignore": true + } + ] + "# + .to_string(), + )])); + + let x = { + let assets_temp_dir = create_temporary_assets_directory(cfg, 0); + let assets_dir = assets_temp_dir.path().canonicalize().unwrap(); + let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir).unwrap(); + assets_config + .get_asset_config(assets_dir.join("nested/deep/the-next-thing.toml").as_path()) + .unwrap() + }; + let y = { + let assets_temp_dir = create_temporary_assets_directory(cfg2, 0); + let assets_dir = assets_temp_dir.path().canonicalize().unwrap(); + let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir).unwrap(); + assets_config + .get_asset_config(assets_dir.join("nested/deep/the-next-thing.toml").as_path()) + .unwrap() + }; + + dbg!(&x, &y); + assert_eq!(x.allow_raw_access, Some(false)); + assert_eq!(x.enable_aliasing, Some(true)); + assert_eq!(x.ignore, Some(true)); + assert_eq!(x.cache.clone().unwrap().max_age, Some(22)); + assert_eq!( + x.headers.clone().unwrap().get("X-Frame-Options"), + Some(&"DENY".to_string()) + ); + assert_eq!(x, y); + + // same as above but with different values + let cfg = Some(HashMap::from([( + "".to_string(), + r#"[ + { + "match": "**/deep/**/*", + "allow_raw_access": true, + "enable_aliasing": false, + "ignore": false, "headers": { - "X-Frame-Options": "DENY", - "X-Content-Type-Options": "nosniff", - "Strict-Transport-Security": "max-age=31536000; includeSubDomains", - "Referrer-Policy": "same-origin", - "X-XSS-Protection": "1; mode=block" + "X-Frame-Options": "ALLOW" } }, { - "match": "**/nested/**/*", - "allow_raw_access": false + "match": "**/*", + "cache": { + "max_age": 22 + } + } + ] +"# + .to_string(), + )])); + let cfg2 = Some(HashMap::from([( + "".to_string(), + r#"[ + { + "match": "**/*", + "cache": { + "max_age": 22 + } + }, + { + "match": "**/deep/**/*", + "allow_raw_access": true, + "enable_aliasing": false, + "ignore": false, + "headers": { + "X-Frame-Options": "ALLOW" + } } ] "# @@ -996,20 +1071,23 @@ mod with_tempdir { .unwrap() }; let y = { - let assets_temp_dir2 = create_temporary_assets_directory(cfg2, 0); - let assets_dir2 = assets_temp_dir2.path().canonicalize().unwrap(); - let mut assets_config2 = AssetSourceDirectoryConfiguration::load(&assets_dir2).unwrap(); - assets_config2 - .get_asset_config( - assets_dir2 - .join("nested/deep/the-next-thing.toml") - .as_path(), - ) + let assets_temp_dir = create_temporary_assets_directory(cfg2, 0); + let assets_dir = assets_temp_dir.path().canonicalize().unwrap(); + let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir).unwrap(); + assets_config + .get_asset_config(assets_dir.join("nested/deep/the-next-thing.toml").as_path()) .unwrap() }; - assert_eq!(x.allow_raw_access, false); - assert_eq!(y.allow_raw_access, false); + dbg!(&x, &y); + assert_eq!(x.allow_raw_access, Some(true)); + assert_eq!(x.enable_aliasing, Some(false)); + assert_eq!(x.ignore, Some(false)); + assert_eq!(x.cache.clone().unwrap().max_age, Some(22)); + assert_eq!( + x.headers.clone().unwrap().get("X-Frame-Options"), + Some(&"ALLOW".to_string()) + ); assert_eq!(x, y); } } From 769a5bb71d7f6edd3556f919c01220b9da4e918c Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Thu, 9 Nov 2023 22:03:46 +0100 Subject: [PATCH 6/7] extend tests and debug printing --- .../frontend/ic-asset/src/asset/config.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/canisters/frontend/ic-asset/src/asset/config.rs b/src/canisters/frontend/ic-asset/src/asset/config.rs index ce00b241a0..f46516b39b 100644 --- a/src/canisters/frontend/ic-asset/src/asset/config.rs +++ b/src/canisters/frontend/ic-asset/src/asset/config.rs @@ -1007,14 +1007,13 @@ mod with_tempdir { dbg!(&x, &y); assert_eq!(x.allow_raw_access, Some(false)); + assert_eq!(y.allow_raw_access, Some(false)); assert_eq!(x.enable_aliasing, Some(true)); + assert_eq!(y.enable_aliasing, Some(true)); assert_eq!(x.ignore, Some(true)); + assert_eq!(y.ignore, Some(true)); assert_eq!(x.cache.clone().unwrap().max_age, Some(22)); - assert_eq!( - x.headers.clone().unwrap().get("X-Frame-Options"), - Some(&"DENY".to_string()) - ); - assert_eq!(x, y); + assert_eq!(y.cache.clone().unwrap().max_age, Some(22)); // same as above but with different values let cfg = Some(HashMap::from([( @@ -1081,13 +1080,12 @@ mod with_tempdir { dbg!(&x, &y); assert_eq!(x.allow_raw_access, Some(true)); + assert_eq!(y.allow_raw_access, Some(true)); assert_eq!(x.enable_aliasing, Some(false)); + assert_eq!(y.enable_aliasing, Some(false)); assert_eq!(x.ignore, Some(false)); + assert_eq!(y.ignore, Some(false)); assert_eq!(x.cache.clone().unwrap().max_age, Some(22)); - assert_eq!( - x.headers.clone().unwrap().get("X-Frame-Options"), - Some(&"ALLOW".to_string()) - ); - assert_eq!(x, y); + assert_eq!(y.cache.clone().unwrap().max_age, Some(22)); } } From 124e7dcbfdfa67aa4479961c6d5b66fcbc246a13 Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Tue, 14 Nov 2023 16:44:19 +0100 Subject: [PATCH 7/7] fix e2e, enum lint & imporved dbg print --- e2e/tests-dfx/assetscanister.bash | 15 ++++-------- .../frontend/ic-asset/src/asset/config.rs | 23 ++++++++----------- 2 files changed, 14 insertions(+), 24 deletions(-) diff --git a/e2e/tests-dfx/assetscanister.bash b/e2e/tests-dfx/assetscanister.bash index 628ec8ed82..a08e8bcb6e 100644 --- a/e2e/tests-dfx/assetscanister.bash +++ b/e2e/tests-dfx/assetscanister.bash @@ -1484,29 +1484,25 @@ CHERRIES" "$stdout" "match": "nevermatchme", "cache": { "max_age": 2000 - }, - "allow_raw_access": true + } }' assert_match 'WARN: 4 unmatched configurations in .*/src/e2e_project_frontend/assets/somedir/.ic-assets.json config file:' assert_contains 'WARN: { "match": "nevermatchme", "headers": {}, - "ignore": false, - "allow_raw_access": true + "ignore": false } WARN: { "match": "nevermatchmetoo", "headers": {}, - "ignore": false, - "allow_raw_access": true + "ignore": false } WARN: { "match": "non-matcher", "headers": { "x-header": "x-value" }, - "ignore": false, - "allow_raw_access": true + "ignore": false }' # splitting this up into two checks, because the order is different on macos vs ubuntu assert_contains 'WARN: { @@ -1514,8 +1510,7 @@ WARN: { "headers": { "x-header": "x-value" }, - "ignore": false, - "allow_raw_access": true + "ignore": false }' } diff --git a/src/canisters/frontend/ic-asset/src/asset/config.rs b/src/canisters/frontend/ic-asset/src/asset/config.rs index f46516b39b..286a2ae05b 100644 --- a/src/canisters/frontend/ic-asset/src/asset/config.rs +++ b/src/canisters/frontend/ic-asset/src/asset/config.rs @@ -62,19 +62,14 @@ pub struct AssetConfigRule { allow_raw_access: Option, } -#[derive(Deserialize, Debug, Clone, PartialEq, Eq)] +#[derive(Deserialize, Debug, Clone, PartialEq, Eq, Default)] enum Maybe { Null, + #[default] Absent, Value(T), } -impl Default for Maybe { - fn default() -> Self { - Self::Absent - } -} - impl AssetConfigRule { fn applies(&self, canonical_path: &Path) -> bool { // TODO: better dot files/dirs handling, awaiting upstream changes: @@ -421,12 +416,6 @@ mod rule_utils { s.push_str(&format!(" - HTTP cache max-age: {}\n", max_age)); } } - if let Some(aliasing) = self.enable_aliasing { - s.push_str(&format!( - " - URL path aliasing: {}\n", - if aliasing { "enabled" } else { "disabled" } - )); - } if let Some(allow_raw_access) = self.allow_raw_access { s.push_str(&format!( " - enable raw access: {}\n", @@ -437,6 +426,12 @@ mod rule_utils { } )); } + if let Some(aliasing) = self.enable_aliasing { + s.push_str(&format!( + " - URL path aliasing: {}\n", + if aliasing { "enabled" } else { "disabled" } + )); + } if let Some(ref headers) = self.headers { for (key, value) in headers { s.push_str(&format!( @@ -937,7 +932,7 @@ mod with_tempdir { AssetConfig { allow_raw_access: Some(true), ..Default::default() - }, + } ); }