fix: .ic-assets.json configuration entries no longer overwrite the de…

…fault for allow_raw_access (#3635)

For for all entries in .ic-assets.json being treated as if they specify allow_raw_access: true if not explicitly set

Fixes: https://dfinity.atlassian.net/browse/SDK-1238

CHANGELOG.md

@@ -2,6 +2,34 @@
 
 # UNRELEASED
 
+# fix: .ic-assets.json configuration entries no longer overwrite the default for `allow_raw_access`
+
+Previously, any configuration element in .ic-assets.json functioned as if a setting of
+`"allow_raw_access": true` were present in the json object.
+
+For example, given the following configuration, all files would be configured
+with `allow_raw_access` set to `true`, as if the second entry specified
+`"allow_raw_access": true` (which is the default), even though it does not.
+
+```json
+[
+  {
+    "match": "**/*",
+    "allow_raw_access": false
+  },
+  {
+    "match": "**/*",
+    "headers": {
+      "X-Anything": "Something"
+    }
+  }
+]
+```
+
+Now, given the same configuration, all files would be configured with `allow_raw_access` set to false, as expected.
+
+Note that the default value of `allow_raw_access` is still `true`.
+
 # 0.18.0
 
 ### fix!: removed the `dfx upgrade` command

e2e/tests-dfx/assetscanister.bash

@@ -1163,6 +1163,54 @@ CHERRIES" "$stdout"
   #   /.well-known/thing.json 1/1 (0 bytes)
 }
 
+@test "asset configuration via .ic-assets.json5 - default properties" {
+  install_asset assetscanister
+  rm src/e2e_project_frontend/assets/.ic-assets.json5
+  dfx_start
+  assert_command dfx deploy
+  assert_command dfx canister call e2e_project_frontend get_asset_properties '("/binary/noise.txt")'
+  assert_contains '(
+  record {
+    headers = null;
+    is_aliased = null;
+    allow_raw_access = opt true;
+    max_age = null;
+  },
+)'
+}
+
+@test "asset configuration via .ic-assets.json5 - property override order does not matter" {
+  install_asset assetscanister
+  # Ensure that setting one property does not overwrite a different property set earlier
+  cat > src/e2e_project_frontend/assets/.ic-assets.json5 <<EOF
+[
+  {
+    "match": "**/binary/**/*",
+    "allow_raw_access": false
+  },
+  {
+    "match": "**/*",
+    "headers": {
+      "X-Anything": "Something"
+    }
+  }
+]
+EOF
+  dfx_start
+  assert_command dfx deploy
+  assert_command dfx canister call e2e_project_frontend get_asset_properties '("/binary/noise.txt")'
+  assert_contains '(
+  record {
+    headers = opt vec { record { "X-Anything"; "Something" } };
+    is_aliased = null;
+    allow_raw_access = opt false;
+    max_age = null;
+  },
+)'
+}
+
+
+
 @test "asset configuration via .ic-assets.json5 - nested dot directories" {
   install_asset assetscanister
 
@@ -1490,38 +1538,33 @@ 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: {
   "match": "/thanks-for-not-stripping-forward-slash",
   "headers": {
     "x-header": "x-value"
   },
-  "ignore": false,
-  "allow_raw_access": true
+  "ignore": false
 }'
 }
 

src/canisters/frontend/ic-asset/src/asset/config.rs

@@ -34,10 +34,6 @@ pub(crate) struct CacheConfig {
     pub(crate) max_age: Option<u64>,
 }
 
-fn default_raw_access() -> Option<bool> {
-    Some(true)
-}
-
 /// A single configuration object, from `.ic-assets.json` config file
 #[derive(Derivative, Clone, Serialize)]
 #[derivative(Debug, PartialEq)]
@@ -348,7 +344,6 @@ mod rule_utils {
         headers: Maybe<HeadersConfig>,
         ignore: Option<bool>,
         enable_aliasing: Option<bool>,
-        #[serde(default = "super::default_raw_access")]
         allow_raw_access: Option<bool>,
     }
 
@@ -427,6 +422,16 @@ mod rule_utils {
                     s.push_str(&format!("  - HTTP cache max-age: {}\n", max_age));
                 }
             }
+            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(aliasing) = self.enable_aliasing {
                 s.push_str(&format!(
                     "  - URL path aliasing: {}\n",
@@ -936,4 +941,152 @@ mod with_tempdir {
             },
         );
     }
+
+    #[test]
+    fn the_order_does_not_matter() {
+        let cfg = Some(HashMap::from([(
+            "".to_string(),
+            r#"[
+                {
+                    "match": "**/deep/**/*",
+                    "allow_raw_access": false,
+                    "cache": {
+                      "max_age": 22
+                    },
+                    "enable_aliasing": true,
+                    "ignore": true
+                },
+                {
+                    "match": "**/*",
+                    "headers": {
+                        "X-Frame-Options": "DENY"
+                    }
+                }
+            ]
+"#
+            .to_string(),
+        )]));
+        let cfg2 = Some(HashMap::from([(
+            "".to_string(),
+            r#"[
+                {
+                    "match": "**/*",
+                    "headers": {
+                        "X-Frame-Options": "DENY"
+                    }
+                },
+                {
+                    "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!(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!(y.cache.clone().unwrap().max_age, Some(22));
+
+        // 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": "ALLOW"
+                    }
+                },
+                {
+                    "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"
+                    }
+                }
+            ]
+            "#
+            .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(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!(y.cache.clone().unwrap().max_age, Some(22));
+    }
 }