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.

Sign up for GitHub

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

Jump to bottom

fix(wasi) Allow the = sign in the environment variable value. #1762

Merged
merged 8 commits into from
Oct 27, 2020
Merged

fix(wasi) Allow the = sign in the environment variable value. #1762

merged 8 commits into from
Oct 27, 2020

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Oct 26, 2020 •
edited
Loading

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

  • Add a short description of the the change to the CHANGELOG.md file

Fixes #1708.

Hywan added 2 commits October 26, 2020 15:56
@Hywan
feat(wasi) Allow the = sign in the environment variable value.
a1457de 
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
test(wasi) Update test cases according to the previous commit.
32c08b5
@Hywan Hywan requested a review from MarkMcCaskey as a code owner October 26, 2020 15:00
@Hywan Hywan self-assigned this Oct 26, 2020
@Hywan Hywan added bug Something isn't working 📦 lib-wasi About wasmer-wasi 🧪 tests I love tests labels Oct 26, 2020
@Hywan
Copy link
Contributor Author

Hywan commented Oct 26, 2020

bors try

bors bot added a commit that referenced this pull request Oct 26, 2020
@bors
Try #1762:
b1c22b5
@Hywan Hywan mentioned this pull request Oct 26, 2020
Closed
Hywan
Hywan commented Oct 26, 2020
View reviewed changes
lib/wasi/src/state/builder.rs Outdated Show resolved Hide resolved
@ensch
Copy link

ensch commented Oct 26, 2020

Should WasiState::new("hello").env("KEY=BAD", "Value").finalize() now be allowed and set the key "KEY" to the value "BAD=Value"?

@Hywan
Copy link
Contributor Author

Hywan commented Oct 26, 2020

Yeah, the validation step happens in the build() method, not in the env() or envs() methods. I can change that a little bit. The modification implies to not flatten the env key + value in env() or envs(), but in build() after the validation. How does it sound @MarkMcCaskey?

@ensch
Copy link

ensch commented Oct 26, 2020

Maybe env() and envs() should set a flag if called with invalid input, so build() can return an error like before?

@bors
Copy link
Contributor

bors bot commented Oct 26, 2020

try

Build succeeded:

MarkMcCaskey
MarkMcCaskey approved these changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I think we can probably make a new test rather than removing one though

lib/wasi/src/state/builder.rs Outdated Show resolved Hide resolved
lib/wasi/src/state/builder.rs Show resolved Hide resolved
Hywan added 2 commits October 27, 2020 10:59
@Hywan
feat(wasi) Replace unwrap_or by from_utf8_lossy.
af9b9bd 
Since `env` contains a nul byte, it's better to get a lossy version of
it. Moreover, it removes one more `unwrap`.
@Hywan
feat(wasi) Seperate env's key and value and do different validations.
89c62e7 
This patch keeps the env's key and value separate until the `build()`
method is called. It allows to apply different validations resp. for
the key and the value: Both must not contain a nul byte, but only the
key cannot contain an equal sign.

This patch also updates the test cases accordingly.

Finally this patch updates the `envs` and `args` methods to reuse
respectively `env` and `arg` to avoid code repetition.
@Hywan
Copy link
Contributor Author

Hywan commented Oct 27, 2020

The last patch keeps the env's key and value separate until the build() method is called. It allows to apply different validations resp. for the key and the value: Both must not contain a nul byte, but only the key cannot contain an equal sign.

This patch also updates the test cases accordingly.

Finally this patch updates the envs and args methods to reuse respectively env and arg to avoid code repetition.

@Hywan
Copy link
Contributor Author

Hywan commented Oct 27, 2020

bors r+

bors bot added a commit that referenced this pull request Oct 27, 2020
@bors @Hywan
Merge #1762
9067840 
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]>
@Hywan
Copy link
Contributor Author

Hywan commented Oct 27, 2020

bors r-

@bors
Copy link
Contributor

bors bot commented Oct 27, 2020

Canceled.

@Hywan
Copy link
Contributor Author

Hywan commented Oct 27, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 27, 2020

Build succeeded:

@bors bors bot merged commit 2f01db8 into wasmerio:master Oct 27, 2020
This was referenced Jan 21, 2021
Closed
Merged
bors bot added a commit that referenced this pull request Jan 22, 2021
@bors @Hywan
Merge #2042 #2044
b7e1987 
2042: feat(cli) Improve environment variables parsing r=Hywan a=Hywan

# Description

This PR fixes #2028. In #1762 we have already improved environment variables syntax, but the same work must be done in the CLI crate.

The following environment variables are considered incorrect:

* `A`, no equal sign,
* `A=`, value is missing,
* `=A`, key is missing.

The following environment variables are considered correct:

* `A=B` with `A` for the name and `B` for the value,
* `A=B=C` with `A` for the name and `B=C` for the value.

The environment variable is trimmed before being parsed with [`str::trim`](https://doc.rust-lang.org/std/primitive.str.html#method.trim).

This PR also adds tests for the `parse_envvar` function, which was missing.

# Review

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


2044: feat(c-api) Do not build C headers if the env var `DOCS_RS` exists r=Hywan a=Hywan

# Description

Fix #1954.

According to https://docs.rs/about/builds, the `DOCS_RS` environment variable is set when the documentation is built from docs.rs. In this case, we must not build the C headers because it generates such error (see #1954 to learn more):

> Unable to copy the generated C bindings: `Os { code: 30, kind: Other, message: "Read-only file system" }`

# Review

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


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
Reviewers

@MarkMcCaskey MarkMcCaskey MarkMcCaskey approved these changes

@syrusakbary syrusakbary syrusakbary approved these changes

Assignees

@Hywan Hywan

Labels
bug Something isn't working 📦 lib-wasi About wasmer-wasi 🧪 tests I love tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

wasmer_wasi: Unable to set env values containing = (equal sign)
4 participants
@Hywan @ensch @syrusakbary @MarkMcCaskey