Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasmer_wasi: Unable to set env values containing = (equal sign) #1708

Closed
ensch opened this issue Oct 12, 2020 · 5 comments · Fixed by #1762
Closed

wasmer_wasi: Unable to set env values containing = (equal sign) #1708

ensch opened this issue Oct 12, 2020 · 5 comments · Fixed by #1762
Assignees
Labels
bug Something isn't working 📦 lib-wasi About wasmer-wasi

Comments

@ensch
Copy link

ensch commented Oct 12, 2020

Problem

I was trying to run CGI apps using wasmer-wasi. You need to pass the client headers to the CGI app using environment variables, but some client headers contain equal signs (e.g sec-ch-ua: "Chromium";v="86", "\"Not\\A;Brand";v="99", "Google Chrome";v="86"), which are rejected by wasmer-wasi, but work fine using wasmtime_wasi.

Bug

Setting a environment variable to a value containing an equal sign (=) is rejected by WasiStateBuilder::build, even though only keys may not contain equal signs. The first equal sign seperates the key from the value. Any other equal signs should be treated as part of the value.

[dependencies]
wasmer-wasi = "1.0.0-alpha3"
fn main() {
    wasmer_wasi::WasiState::new("hello").env("DUMMY", "X=1").finalize().expect("this should work");
}
thread 'main' panicked at 'this should work: EnvironmentVariableFormatError("found \'=\' in env var string \"DUMMY=X=1\" (key=value)")', src/main.rs:2:73

Fix

Removing the incorrect check for multiple equal signs:

diff --git a/lib/wasi/src/state/builder.rs b/lib/wasi/src/state/builder.rs
index f6694bb02..6d5118e69 100644
--- a/lib/wasi/src/state/builder.rs
+++ b/lib/wasi/src/state/builder.rs
@@ -329,22 +329,9 @@ impl WasiStateBuilder {
             }
         }
         for env in self.envs.iter() {
-            let mut eq_seen = false;
             for b in env.iter() {
                 match *b {
-                    b'=' => {
-                        if eq_seen {
-                            return Err(WasiStateCreationError::EnvironmentVariableFormatError(
-                                format!(
-                                    "found '=' in env var string \"{}\" (key=value)",
-                                    std::str::from_utf8(env)
-                                        .unwrap_or("Inner error: env var is invalid_utf8!")
-                                ),
-                            ));
-                        }
-                        eq_seen = true;
-                    }
-                    0 => {
+                     0 => {
                         return Err(WasiStateCreationError::EnvironmentVariableFormatError(
                             format!(
                                 "found nul byte in env var string \"{}\" (key=value)",

It might make sense to add a check for equal signs in the key in the function WasiStateBuilder::env, so that WasiStateBuilder::build can return an error in such cases.

Sample wasm app

fn main() {
    for (name,value) in std::env::vars() {
        println!("{} = {}", name, value);
    }
}

Output using wasmtime:

$ wasmtime run --env DUMMY=X=1 sample.wasm
DUMMY = X=1

Output using wasmer:

$ wasmer sample.wasm --env DUMMY=X=1
Error: Env vars must be of the form <var_name>=<value>. Found DUMMY=X=1
@ensch ensch added the bug Something isn't working label Oct 12, 2020
@Hywan
Copy link
Contributor

Hywan commented Oct 12, 2020

Hello,

Thanks for the bug report. I believe this might be a constraint from the specification itself. It is true however that having an equal sign inside the value part kind of make sense. I would be personally willing to authorize this. Thoughts @MarkMcCaskey?

@Hywan Hywan self-assigned this Oct 12, 2020
@syrusakbary
Copy link
Member

That makes sense for me, probably we should allow DUMMY=A=B, by splitting by the first = encountered.

Hywan added a commit to Hywan/wasmer that referenced this issue Oct 26, 2020
Having `a=b` means that the environment variable key is `a`, and its
value is `b`.

Having `a=b=c` means that the key is `a` and its value is `b=c`.

It's OK to get this, e.g. within a CGI context where values can hold
strings such as `sec-ch-ua: "Chromium;v=86"` etc. See
wasmerio#1708 for a longer
explanation.
@Hywan
Copy link
Contributor

Hywan commented Oct 26, 2020

@ensch #1762 should solve your issue. If you can try it out, that would be helpful :-).

@Hywan Hywan added the 📦 lib-wasi About wasmer-wasi label Oct 26, 2020
@ensch
Copy link
Author

ensch commented Oct 26, 2020

Works for me. Of course, .env("KEY=BAD", "Value") is now allowed and prints the unexpected KEY = BAD=Value.

@ensch ensch closed this as completed Oct 26, 2020
@Hywan
Copy link
Contributor

Hywan commented Oct 26, 2020

Discussion continues at #1762.

bors bot added a commit that referenced this issue Oct 27, 2020
1762: fix(wasi) Allow the `=` sign in the environment variable value. r=Hywan a=Hywan

# Description

Having `a=b` means that the environment variable key is `a`, and its
value is `b`.

Having `a=b=c` means that the key is `a` and its value is `b=c`.

It's OK to get this, e.g. within a CGI context where values can hold
strings such as `sec-ch-ua: "Chromium;v=86"` etc. See
#1708 for a longer
explanation.

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file

Fixes #1708.

Co-authored-by: Ivan Enderlin <[email protected]>
bors bot added a commit that referenced this issue Oct 27, 2020
1762: fix(wasi) Allow the `=` sign in the environment variable value. r=Hywan a=Hywan

# Description

Having `a=b` means that the environment variable key is `a`, and its
value is `b`.

Having `a=b=c` means that the key is `a` and its value is `b=c`.

It's OK to get this, e.g. within a CGI context where values can hold
strings such as `sec-ch-ua: "Chromium;v=86"` etc. See
#1708 for a longer
explanation.

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file

Fixes #1708.

Co-authored-by: Ivan Enderlin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📦 lib-wasi About wasmer-wasi
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants