Make using insecure connections opt-in (#1309)

* add insecure url checks * rename variables * add feature flags to expose Url properly * fix test compile error * fix feature errors * remove comment * add url crate and use it for url parsing * fix compile errors * satisfy the holy clippy * fix typos and host loopback * macro attribute, provide validation function in utils * fix expected output of ui tests * remove the success case for --allow-insecure because we cannot establish ws:// connection at the moment.
paritytech · Jan 9, 2024 · 7f714cb · 7f714cb
1 parent 5b35a9f
commit 7f714cb
Show file tree

Hide file tree

Showing 22 changed files with 545 additions and 396 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -96,6 +96,7 @@ tracing = "0.1.40"
 tracing-wasm = "0.2.1"
 tracing-subscriber = "0.3.18"
 trybuild = "1.0.86"
+url = "2.5.0"
 wabt = "0.10.0"
 wasm-bindgen-test = "0.3.24"
 which = "5.0.0"

diff --git a/cli/src/commands/codegen.rs b/cli/src/commands/codegen.rs
@@ -2,7 +2,7 @@
 // This file is dual-licensed as Apache-2.0 or GPL-3.0.
 // see LICENSE for license details.
 
-use crate::utils::FileOrUrl;
+use crate::utils::{validate_url_security, FileOrUrl};
 use clap::Parser as ClapParser;
 use codec::Decode;
 use color_eyre::eyre::eyre;
@@ -62,6 +62,9 @@ pub struct Opts {
     /// Defaults to `false` (default substitutions are provided).
     #[clap(long)]
     no_default_substitutions: bool,
+    /// Allow insecure URLs e.g. URLs starting with ws:// or http:// without SSL encryption
+    #[clap(long, short)]
+    allow_insecure: bool,
 }
 
 fn derive_for_type_parser(src: &str) -> Result<(String, String), String> {
@@ -89,6 +92,8 @@ fn substitute_type_parser(src: &str) -> Result<(String, String), String> {
 }
 
 pub async fn run(opts: Opts, output: &mut impl std::io::Write) -> color_eyre::Result<()> {
+    validate_url_security(opts.file_or_url.url.as_ref(), opts.allow_insecure)?;
+
     let bytes = opts.file_or_url.fetch().await?;
 
     codegen(

diff --git a/cli/src/commands/compatibility.rs b/cli/src/commands/compatibility.rs
@@ -11,6 +11,8 @@ use std::collections::HashMap;
 use subxt_codegen::fetch_metadata::MetadataVersion;
 use subxt_metadata::Metadata;
 
+use crate::utils::validate_url_security;
+
 /// Verify metadata compatibility between substrate nodes.
 #[derive(Debug, ClapParser)]
 pub struct Opts {
@@ -36,9 +38,16 @@ pub struct Opts {
     /// Defaults to latest.
     #[clap(long = "version", default_value = "latest")]
     version: MetadataVersion,
+    /// Allow insecure URLs e.g. URLs starting with ws:// or http:// without SSL encryption
+    #[clap(long, short)]
+    allow_insecure: bool,
 }
 
 pub async fn run(opts: Opts, output: &mut impl std::io::Write) -> color_eyre::Result<()> {
+    for url in opts.nodes.iter() {
+        validate_url_security(Some(url), opts.allow_insecure)?;
+    }
+
     match opts.pallet {
         Some(pallet) => {
             handle_pallet_metadata(opts.nodes.as_slice(), pallet.as_str(), opts.version, output)

diff --git a/cli/src/commands/diff.rs b/cli/src/commands/diff.rs
@@ -5,7 +5,7 @@ use frame_metadata::RuntimeMetadataPrefixed;
 use std::collections::HashMap;
 use std::hash::Hash;
 
-use crate::utils::FileOrUrl;
+use crate::utils::{validate_url_security, FileOrUrl};
 use color_eyre::owo_colors::OwoColorize;
 
 use scale_info::form::PortableForm;
@@ -29,9 +29,15 @@ pub struct Opts {
     metadata_or_url_1: FileOrUrl,
     /// metadata file or node URL
     metadata_or_url_2: FileOrUrl,
+    /// Allow insecure URLs e.g. URLs starting with ws:// or http:// without SSL encryption
+    #[clap(long, short)]
+    allow_insecure: bool,
 }
 
 pub async fn run(opts: Opts, output: &mut impl std::io::Write) -> color_eyre::Result<()> {
+    validate_url_security(opts.metadata_or_url_1.url.as_ref(), opts.allow_insecure)?;
+    validate_url_security(opts.metadata_or_url_2.url.as_ref(), opts.allow_insecure)?;
+
     let (entry_1_metadata, entry_2_metadata) = get_metadata(&opts).await?;
 
     let metadata_diff = MetadataDiff::construct(&entry_1_metadata, &entry_2_metadata);

diff --git a/cli/src/commands/explore/mod.rs b/cli/src/commands/explore/mod.rs
@@ -1,4 +1,4 @@
-use crate::utils::{print_first_paragraph_with_indent, FileOrUrl};
+use crate::utils::{print_first_paragraph_with_indent, validate_url_security, FileOrUrl};
 use clap::{Parser as ClapParser, Subcommand};
 
 use std::fmt::Write;
@@ -71,6 +71,9 @@ pub struct Opts {
     pallet: Option<String>,
     #[command(subcommand)]
     pallet_subcommand: Option<PalletSubcommand>,
+    /// Allow insecure URLs e.g. URLs starting with ws:// or http:// without SSL encryption
+    #[clap(long, short)]
+    allow_insecure: bool,
 }
 
 #[derive(Debug, Clone, Subcommand)]
@@ -81,6 +84,8 @@ pub enum PalletSubcommand {
 }
 
 pub async fn run(opts: Opts, output: &mut impl std::io::Write) -> color_eyre::Result<()> {
+    validate_url_security(opts.file_or_url.url.as_ref(), opts.allow_insecure)?;
+
     // get the metadata
     let bytes = opts.file_or_url.fetch().await?;
     let metadata = Metadata::decode(&mut &bytes[..])?;
@@ -160,48 +165,52 @@ fn print_available_pallets(metadata: &Metadata) -> String {
 
 #[cfg(test)]
 pub mod tests {
-    use super::{run, Opts};
+    use super::Opts;
 
-    async fn simulate_run(cli_command: &str) -> color_eyre::Result<String> {
-        let mut args = vec![
-            "explore",
-            "--file=../artifacts/polkadot_metadata_small.scale",
-        ];
+    async fn run(cli_command: &str) -> color_eyre::Result<String> {
+        let mut args = vec!["explore"];
         let mut split: Vec<&str> = cli_command.split(' ').filter(|e| !e.is_empty()).collect();
         args.append(&mut split);
         let opts: Opts = clap::Parser::try_parse_from(args)?;
         let mut output: Vec<u8> = Vec::new();
-        run(opts, &mut output)
+        super::run(opts, &mut output)
             .await
             .map(|_| String::from_utf8(output).unwrap())
     }
 
+    async fn run_against_file(cli_command: &str) -> color_eyre::Result<String> {
+        run(&format!(
+            "--file=../artifacts/polkadot_metadata_small.scale {cli_command}"
+        ))
+        .await
+    }
+
     #[tokio::test]
     async fn test_commands() {
         // show pallets:
-        let output = simulate_run("").await;
+        let output = run_against_file("").await;
         assert_eq!(output.unwrap(), "Usage:\n    subxt explore <PALLET>\n        explore a specific pallet\n\nAvailable <PALLET> values are:\n    Balances\n    Multisig\n    ParaInherent\n    System\n    Timestamp\n");
         // if incorrect pallet, error:
-        let output = simulate_run("abc123").await;
+        let output = run_against_file("abc123").await;
         assert!(output.is_err());
         // if correct pallet, show options (calls, constants, storage)
-        let output = simulate_run("Balances").await;
+        let output = run_against_file("Balances").await;
         assert_eq!(output.unwrap(), "Usage:\n    subxt explore Balances calls\n        explore the calls that can be made into this pallet\n    subxt explore Balances constants\n        explore the constants held in this pallet\n    subxt explore Balances storage\n        explore the storage values held in this pallet\n");
         // check that exploring calls, storage entries and constants is possible:
-        let output = simulate_run("Balances calls").await;
+        let output = run_against_file("Balances calls").await;
         assert!(output.unwrap().starts_with("Usage:\n    subxt explore Balances calls <CALL>\n        explore a specific call within this pallet\n\nAvailable <CALL>'s in the \"Balances\" pallet:\n"));
-        let output = simulate_run("Balances storage").await;
+        let output = run_against_file("Balances storage").await;
         assert!(output.unwrap().starts_with("Usage:\n    subxt explore Balances storage <STORAGE_ENTRY>\n        view details for a specific storage entry\n\nAvailable <STORAGE_ENTRY>'s in the \"Balances\" pallet:\n"));
-        let output = simulate_run("Balances constants").await;
+        let output = run_against_file("Balances constants").await;
         assert!(output.unwrap().starts_with("Usage:\n    subxt explore Balances constants <CONSTANT>\n        explore a specific call within this pallet\n\nAvailable <CONSTANT>'s in the \"Balances\" pallet:\n"));
         // check that invalid subcommands don't work:
-        let output = simulate_run("Balances abc123").await;
+        let output = run_against_file("Balances abc123").await;
         assert!(output.is_err());
         // check that we can explore a certain call:
-        let output = simulate_run("Balances calls transfer_allow_death").await;
+        let output = run_against_file("Balances calls transfer_allow_death").await;
         assert!(output.unwrap().starts_with("Usage:\n    subxt explore Balances calls transfer_allow_death <SCALE_VALUE>\n        construct the call by providing a valid argument\n\nThe call expect expects a <SCALE_VALUE> with this shape:\n    {\n        dest: enum MultiAddress"));
         // check that unsigned extrinsic can be constructed:
-        let output = simulate_run(
+        let output = run_against_file(
             "Balances calls transfer_allow_death {\"dest\":v\"Raw\"((255,255, 255)),\"value\":0}",
         )
         .await;
@@ -210,11 +219,32 @@ pub mod tests {
             "Encoded call data:\n    0x24040400020cffffff00\n"
         );
         // check that we can explore a certain constant:
-        let output = simulate_run("Balances constants ExistentialDeposit").await;
+        let output = run_against_file("Balances constants ExistentialDeposit").await;
         assert_eq!(output.unwrap(), "Description:\n    The minimum amount required to keep an account open. MUST BE GREATER THAN ZERO!\n\nThe constant has the following shape:\n    u128\n\nThe value of the constant is:\n    33333333\n");
         // check that we can explore a certain storage entry:
-        let output = simulate_run("System storage Account").await;
+        let output = run_against_file("System storage Account").await;
         assert!(output.unwrap().starts_with("Usage:\n    subxt explore System storage Account <KEY_VALUE>\n\nDescription:\n    The full account information for a particular account ID."));
         // in the future we could also integrate with substrate-testrunner to spawn up a node and send an actual storage query to it: e.g. `subxt explore System storage Digest`
     }
+
+    #[tokio::test]
+    async fn insecure_urls_get_denied() {
+        // Connection should work fine:
+        run("--url wss://rpc.polkadot.io:443").await.unwrap();
+
+        // Errors, because the --allow-insecure is not set:
+        assert!(run("--url ws://rpc.polkadot.io:443")
+            .await
+            .unwrap_err()
+            .to_string()
+            .contains("is not secure"));
+
+        // This checks, that we never prevent (insecure) requests to localhost, even if the `--allow-insecure` flag is not set.
+        // It errors, because there is no node running locally, which results in the "Request error".
+        assert!(run("--url ws://localhost")
+            .await
+            .unwrap_err()
+            .to_string()
+            .contains("Request error"));
+    }
 }
diff --git a/cli/src/commands/metadata.rs b/cli/src/commands/metadata.rs
@@ -2,7 +2,7 @@
 // This file is dual-licensed as Apache-2.0 or GPL-3.0.
 // see LICENSE for license details.
 
-use crate::utils::FileOrUrl;
+use crate::utils::{validate_url_security, FileOrUrl};
 use clap::Parser as ClapParser;
 use codec::{Decode, Encode};
 use color_eyre::eyre::{self, bail};
@@ -35,9 +35,13 @@ pub struct Opts {
     /// Write the output of the metadata command to the provided file path.
     #[clap(long, short, value_parser)]
     pub output_file: Option<PathBuf>,
+    /// Allow insecure URLs e.g. URLs starting with ws:// or http:// without SSL encryption
+    #[clap(long, short)]
+    allow_insecure: bool,
 }
 
 pub async fn run(opts: Opts, output: &mut impl Write) -> color_eyre::Result<()> {
+    validate_url_security(opts.file_or_url.url.as_ref(), opts.allow_insecure)?;
     let bytes = opts.file_or_url.fetch().await?;
     let mut metadata = RuntimeMetadataPrefixed::decode(&mut &bytes[..])?;
 

diff --git a/cli/src/utils.rs b/cli/src/utils.rs
@@ -3,7 +3,7 @@
 // see LICENSE for license details.
 
 use clap::Args;
-use color_eyre::eyre;
+use color_eyre::eyre::bail;
 
 use std::str::FromStr;
 use std::{fs, io::Read, path::PathBuf};
@@ -87,7 +87,7 @@ impl FileOrUrl {
         match (&self.file, &self.url, self.version) {
             // Can't provide both --file and --url
             (Some(_), Some(_), _) => {
-                eyre::bail!("specify one of `--url` or `--file` but not both")
+                bail!("specify one of `--url` or `--file` but not both")
             }
             // Load from --file path
             (Some(PathOrStdIn::Path(path)), None, None) => {
@@ -101,7 +101,7 @@ impl FileOrUrl {
 
                 match res {
                     Ok(bytes) => Ok(bytes),
-                    Err(err) => eyre::bail!("reading bytes from stdin (`--file -`) failed: {err}"),
+                    Err(err) => bail!("reading bytes from stdin (`--file -`) failed: {err}"),
                 }
             }
             // Cannot load the metadata from the file and specify a version to fetch.
@@ -110,7 +110,7 @@ impl FileOrUrl {
                 // but that would be involved because we'd need to convert
                 // from each metadata to the latest one and from the
                 // latest one to each metadata version. For now, disable the conversion.
-                eyre::bail!("`--file` is incompatible with `--version`")
+                bail!("`--file` is incompatible with `--version`")
             }
             // Fetch from --url
             (None, Some(uri), version) => {
@@ -144,6 +144,23 @@ pub fn with_indent(s: String, indent: usize) -> String {
         .join("\n")
 }
 
+pub fn validate_url_security(url: Option<&Url>, allow_insecure: bool) -> color_eyre::Result<()> {
+    let Some(url) = url else {
+        return Ok(());
+    };
+    match subxt::utils::url_is_secure(url.as_str()) {
+        Ok(is_secure) => {
+            if !allow_insecure && !is_secure {
+                bail!("URL {url} is not secure!\nIf you are really want to use this URL, try using --allow-insecure (-a)");
+            }
+        }
+        Err(err) => {
+            bail!("URL {url} is not valid: {err}")
+        }
+    }
+    Ok(())
+}
+
 #[cfg(test)]
 mod tests {
     use crate::utils::{FileOrUrl, PathOrStdIn};

diff --git a/codegen/src/fetch_metadata.rs b/codegen/src/fetch_metadata.rs
@@ -15,7 +15,6 @@ use jsonrpsee::{
 };
 use std::time::Duration;
 
-// Part of the public interface:
 pub use jsonrpsee::client_transport::ws::Url;
 
 /// The metadata version that is fetched from the node.