Skip to content

Commit

Permalink
Replace mandatory extension with validation
Browse files Browse the repository at this point in the history
Rather than restricting the extension to .wasm and later failing
when loading the file, accept any file but validate its content.

As a bonus, disambiguate OS specific errors (e.g., file does not
exist) from invalid format.

Fixes #2.
  • Loading branch information
fgsch committed Jul 13, 2021
1 parent e296534 commit 15ef9a6
Show file tree
Hide file tree
Showing 7 changed files with 61 additions and 26 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ tracing-subscriber = "0.2.19"
tracing-futures = "0.2.5"
hyper = {version = "0.14.0", features = ["full"]}
hyper-tls = "0.5.0"
wat = "1.0.38"

[dev-dependencies]
anyhow = "1.0.31"
Expand Down
77 changes: 54 additions & 23 deletions cli/src/opts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,8 @@ pub struct Opts {
/// The IP address that the service should be bound to.
#[structopt(long = "addr", default_value = "127.0.0.1:7878")]
socket_addr: SocketAddr,
/// The path to the service's Wasm binary.
#[structopt(
parse(
try_from_str = check_binary_extension
)
)]
/// The path to the service's Wasm module.
#[structopt(parse(try_from_str = check_module))]
input: PathBuf,
/// The path to a TOML file containing `local_server` configuration.
#[structopt(short = "C", long = "config")]
Expand Down Expand Up @@ -81,15 +77,16 @@ impl Opts {
}
}

/// A parsing function used by [`Opts`][opts] to check that the input is a Wasm binary.
/// A parsing function used by [`Opts`][opts] to check that the input is a valid Wasm module in
/// binary or text format.
///
/// [opts]: struct.Opts.html
fn check_binary_extension(s: &str) -> Result<PathBuf, Error> {
fn check_module(s: &str) -> Result<PathBuf, Error> {
let path = PathBuf::from(s);
match path.extension().and_then(|ext| ext.to_str()) {
Some("wasm") => Ok(path),
_ => Err(Error::FileExtension),
}
let contents = std::fs::read(&path)?;
wat::parse_bytes(&contents)
.map(|_| path)
.map_err(|_| Error::FileFormat)
}

/// A collection of unit tests for our CLI argument parsing.
Expand All @@ -104,19 +101,29 @@ mod opts_tests {
use {
super::Opts,
std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr},
std::path::PathBuf,
structopt::{
clap::{Error, ErrorKind},
StructOpt,
},
};

fn test_file(name: &str) -> String {
let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"))
.join("tests")
.join("wasm");
path.push(name);
assert!(path.exists(), "test file does not exist");
path.to_str().unwrap().to_string()
}

/// A small type alias for test results, with a boxed error type.
type TestResult = Result<(), anyhow::Error>;

/// Test that the default address works as expected.
#[test]
fn default_addr_works() -> TestResult {
let empty_args: &[&str] = &["dummy-program-name", "path/to/a/guest-program.wasm"];
let empty_args = &["dummy-program-name", &test_file("minimal.wat")];
let opts = Opts::from_iter_safe(empty_args)?;
let expected = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 7878);
assert_eq!(opts.addr(), expected);
Expand All @@ -130,7 +137,7 @@ mod opts_tests {
"dummy-program-name",
"--addr",
"999.0.0.1:7878",
"path/to/a/guest-program.wasm",
&test_file("minimal.wat"),
];
match Opts::from_iter_safe(args_with_bad_addr) {
Err(Error {
Expand All @@ -149,7 +156,7 @@ mod opts_tests {
"dummy-program-name",
"--addr",
"[::1]:7878",
"path/to/a/guest-program.wasm",
&test_file("minimal.wat"),
];
let opts = Opts::from_iter_safe(args_with_ipv6_addr)?;
let addr_v6 = IpAddr::V6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1));
Expand All @@ -158,12 +165,26 @@ mod opts_tests {
Ok(())
}

/// Test that a file with a non-wasm file extension will be rejected properly.
/// Test that a nonexistent file is rejected properly.
#[test]
fn nonexistent_file_is_rejected() -> TestResult {
let args_with_nonexistent_file = &["dummy-program-name", "path/to/a/nonexistent/file"];
match Opts::from_iter_safe(args_with_nonexistent_file) {
Err(Error {
kind: ErrorKind::ValueValidation,
message,
..
}) if message.contains("No such file or directory") => Ok(()),
res => panic!("unexpected result: {:?}", res),
}
}

/// Test that an invalid file is rejected.
#[test]
fn invalid_extension_is_rejected() -> TestResult {
let args_with_bad_ext = &["dummy-program-name", "path/to/a/invalid-binary.exe"];
let expected_msg = format!("{}", viceroy_lib::Error::FileExtension);
match Opts::from_iter_safe(args_with_bad_ext) {
fn invalid_file_is_rejected() -> TestResult {
let args_with_invalid_file = &["dummy-program-name", &test_file("invalid.wat")];
let expected_msg = format!("{}", viceroy_lib::Error::FileFormat);
match Opts::from_iter_safe(args_with_invalid_file) {
Err(Error {
kind: ErrorKind::ValueValidation,
message,
Expand All @@ -173,10 +194,20 @@ mod opts_tests {
}
}

/// Test that a file with a wasm file extension will be accepted.
/// Test that a Wasm module in text format is accepted.
#[test]
fn text_format_is_accepted() -> TestResult {
let args = &["dummy-program-name", &test_file("minimal.wat")];
match Opts::from_iter_safe(args) {
Ok(_) => Ok(()),
res => panic!("unexpected result: {:?}", res),
}
}

/// Test that a Wasm module in binary format is accepted.
#[test]
fn valid_extension_is_accepted() -> TestResult {
let args = &["dummy-program-name", "path/to/a/valid-binary.wasm"];
fn binary_format_is_accepted() -> TestResult {
let args = &["dummy-program-name", &test_file("minimal.wasm")];
match Opts::from_iter_safe(args) {
Ok(_) => Ok(()),
res => panic!("unexpected result: {:?}", res),
Expand Down
1 change: 1 addition & 0 deletions cli/tests/wasm/invalid.wat
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Not a valid wat file
Binary file added cli/tests/wasm/minimal.wasm
Binary file not shown.
1 change: 1 addition & 0 deletions cli/tests/wasm/minimal.wat
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
(module)
6 changes: 3 additions & 3 deletions lib/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ pub enum Error {
#[error("Fatal error: [{0}]")]
FatalError(String),

/// Error when viceroy has been given a file with an unexpected extension.
#[error("expected a file with a `.wasm` file extension")]
FileExtension,
/// Error when viceroy has been given an invalid file.
#[error("expected a valid Wasm file")]
FileFormat,

#[error(transparent)]
FastlyConfig(#[from] FastlyConfigError),
Expand Down

0 comments on commit 15ef9a6

Please sign in to comment.