Reject keywords#1565
Conversation
4611e9e to
6b82ec4
Compare
| "{stderr}" | ||
| ); | ||
| } else { | ||
| assert!(!stderr.is_empty(), "expected stderr for {bad_keyword}"); |
There was a problem hiding this comment.
Please keep the check for the syntax error message.
There was a problem hiding this comment.
This is a bit tricky :) Ideally I would like to keep original check:
assert_contains!(output.stderr(), "syntax error");and express more specific assertion for reserved words like so ( 7b702ea ):
let stderr = output.stderr();
if super::is_reserved_alias_keyword(bad_keyword) {
assert!(
stderr.contains("reserved alias")
|| stderr.contains("reserved word")
|| stderr.contains("syntax error"),
"{stderr}"
);
} else {
assert_contains!(stderr, "syntax error");
}But after un-ignore compliance tests they e.g.:
SUDO_UNDER_TEST=ours cargo test --manifest-path test-framework/Cargo.toml -p sudo-compliance-tests --features apparmor sudo::sudoers::host_alias::keywords -- --exact --nocapturefail with:
[sudo-compliance-tests/src/sudo/sudoers/host_alias.rs:221:9] bad_keyword = "Cmnd_Alias"
thread 'sudo::sudoers::host_alias::keywords' panicked at sudo-compliance-tests/src/sudo/sudoers/host_alias.rs:240:13:
"/etc/sudoers:2:13: expecting '=' but found 'm'\nHost_Alias Cmnd_Alias = container\n ^\nsudo: I'm sorry root. I'm afraid I can't do that" did not contain "syntax error"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test sudo::sudoers::host_alias::keywords ... FAILED
So I went into the route of having:
let stderr = output.stderr();
if super::is_reserved_alias_keyword(bad_keyword) {
assert!(
stderr.contains("reserved alias")
|| stderr.contains("reserved word")
|| stderr.contains("syntax error"),
"{stderr}"
);
} else {
assert!(!stderr.is_empty(), "expected stderr for {bad_keyword}");
}KEYWORDS_ALIAS_BAD seems to contain several classes of invalid alias names, and not all of them produce diagnostics containing the literal text "syntax error".
I am open to improve the logic of those tests, if you have a different solution in mind.
There was a problem hiding this comment.
Perhaps the best approach is to keep this PR focused on rejecting keywords and regression tests. That is what issue #700 asked for.
To fix un-ignored compliance tests, I added conditional matching against stderr - this smells like ad-hoc solution.
For enabling compliance tests (and re-design them) separate issue could be created.
dbf8f56 to
9314123
Compare
- preserve previous behavior: all bad keywords should report a syntax error - check for more specific assert rejection reson for reserved alias / keywords
9314123 to
c527d18
Compare
| } | ||
|
|
||
| for good_keyword in super::keywords_alias_good() { | ||
| for good_keyword in super::keywords_alias_good_for_cmnd_alias() { |
There was a problem hiding this comment.
All these can remain keyword_alias_good except for the Cmnd_Alias test, right?
There was a problem hiding this comment.
Good point. It should be only for Cmnd_Alias - restored in other 3 tests.
| @@ -352,12 +351,22 @@ fn user_alias_keywords() { | |||
| .build(); | |||
|
|
|||
| let output = Command::new("sudo").arg("true").output(&env); | |||
There was a problem hiding this comment.
Please add an output.assert_exit_code(1) call.
There was a problem hiding this comment.
Added (conditionally when bad keyword is not ALL.
…stronger assertion: output.assert_exit_code(1) for bad keyword
af258bf to
ffffdfd
Compare
Fix #700
CHROOT,CWD,NOTAFTER,NOTBEFORE,TIMEOUT