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

implement stricter env filter parsing #1542

Closed
wants to merge 2 commits into from
Closed

implement stricter env filter parsing #1542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
789f504
Rewrite env filter parsing to be stricter
CAD97 Sep 21, 2021
7f97a85
Remove use of nested-or-patterns
CAD97 Sep 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions tracing-subscriber/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ keywords = ["logging", "tracing", "metrics", "subscriber"]
[features]

default = ["env-filter", "smallvec", "fmt", "ansi", "chrono", "tracing-log", "json"]
env-filter = ["matchers", "regex", "lazy_static", "tracing"]
env-filter = ["matchers", "tracing"]
fmt = ["registry"]
ansi = ["fmt", "ansi_term"]
registry = ["sharded-slab", "thread_local"]
Expand All @@ -36,9 +36,7 @@ tracing-core = { path = "../tracing-core", version = "0.2" }
# only required by the filter feature
tracing = { optional = true, path = "../tracing", version = "0.2", default-features = false, features = ["std"] }
matchers = { optional = true, version = "0.1.0" }
regex = { optional = true, version = "1", default-features = false, features = ["std"] }
smallvec = { optional = true, version = "1" }
lazy_static = { optional = true, version = "1" }

# fmt
tracing-log = { path = "../tracing-log", version = "0.2", optional = true, default-features = false, features = ["log-tracer", "std"] }
Expand Down
123 changes: 23 additions & 100 deletions tracing-subscriber/src/filter/env/directive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use super::super::level::{self, LevelFilter};
use super::{field, FieldMap, FilterVec};
use lazy_static::lazy_static;
use regex::Regex;
use super::{
field,
parsing::{parse_one_directive, ParseErrorKind},
FieldMap, FilterVec,
};
use std::{cmp::Ordering, error::Error, fmt, iter::FromIterator, str::FromStr};
use tracing_core::{span, Level, Metadata};

Expand All @@ -10,8 +12,8 @@ use tracing_core::{span, Level, Metadata};
#[derive(Debug, Eq, PartialEq)]
#[cfg_attr(docsrs, doc(cfg(feature = "env-filter")))]
pub struct Directive {
in_span: Option<String>,
fields: FilterVec<field::Match>,
pub(super) in_span: Option<String>,
pub(super) fields: FilterVec<field::Match>,
pub(crate) target: Option<String>,
pub(crate) level: LevelFilter,
}
Expand Down Expand Up @@ -58,13 +60,6 @@ pub struct ParseError {
kind: ParseErrorKind,
}

#[derive(Debug)]
enum ParseErrorKind {
Field(Box<dyn Error + Send + Sync>),
Level(level::ParseError),
Other,
}

impl Directive {
pub(super) fn has_name(&self) -> bool {
self.in_span.is_some()
Expand Down Expand Up @@ -176,94 +171,12 @@ impl Match for Directive {
impl FromStr for Directive {
type Err = ParseError;
fn from_str(from: &str) -> Result<Self, Self::Err> {
lazy_static! {
static ref DIRECTIVE_RE: Regex = Regex::new(
r"(?x)
^(?P<global_level>(?i:trace|debug|info|warn|error|off|[0-5]))$ |
# ^^^.
# `note: we match log level names case-insensitively
^
(?: # target name or span name
(?P<target>[\w:-]+)|(?P<span>\[[^\]]*\])
){1,2}
(?: # level or nothing
=(?P<level>(?i:trace|debug|info|warn|error|off|[0-5]))?
# ^^^.
# `note: we match log level names case-insensitively
)?
$
"
)
.unwrap();
static ref SPAN_PART_RE: Regex =
Regex::new(r#"(?P<name>[^\]\{]+)?(?:\{(?P<fields>[^\}]*)\})?"#).unwrap();
static ref FIELD_FILTER_RE: Regex =
// TODO(eliza): this doesn't _currently_ handle value matchers that include comma
// characters. We should fix that.
Regex::new(r#"(?x)
(
# field name
[[:word:]][[[:word:]]\.]*
# value part (optional)
(?:=[^,]+)?
)
# trailing comma or EOS
(?:,\s?|$)
"#).unwrap();
}

let caps = DIRECTIVE_RE.captures(from).ok_or_else(ParseError::new)?;

if let Some(level) = caps
.name("global_level")
.and_then(|s| s.as_str().parse().ok())
{
return Ok(Directive {
level,
..Default::default()
});
let (parsed, after) = parse_one_directive(from);
if after != from.len() {
Err(ParseError::new())
} else {
Ok(parsed?)
}

let target = caps.name("target").and_then(|c| {
let s = c.as_str();
if s.parse::<LevelFilter>().is_ok() {
None
} else {
Some(s.to_owned())
}
});

let (in_span, fields) = caps
.name("span")
.and_then(|cap| {
let cap = cap.as_str().trim_matches(|c| c == '[' || c == ']');
let caps = SPAN_PART_RE.captures(cap)?;
let span = caps.name("name").map(|c| c.as_str().to_owned());
let fields = caps
.name("fields")
.map(|c| {
FIELD_FILTER_RE
.find_iter(c.as_str())
.map(|c| c.as_str().parse())
.collect::<Result<FilterVec<_>, _>>()
})
.unwrap_or_else(|| Ok(FilterVec::new()));
Some((span, fields))
})
.unwrap_or_else(|| (None, Ok(FilterVec::new())));

let level = caps
.name("level")
.and_then(|l| l.as_str().parse().ok())
// Setting the target without the level enables every level for that target
.unwrap_or(LevelFilter::TRACE);

Ok(Directive {
level,
target,
in_span,
fields: fields?,
})
}
}

Expand Down Expand Up @@ -643,6 +556,9 @@ impl fmt::Display for ParseError {
ParseErrorKind::Other => f.pad("invalid filter directive"),
ParseErrorKind::Level(ref l) => l.fmt(f),
ParseErrorKind::Field(ref e) => write!(f, "invalid field filter: {}", e),
ParseErrorKind::UnexpectedSyntax(ref c) => {
write!(f, "unexpected semantic character: {}", c)
}
}
}
}
Expand All @@ -657,6 +573,7 @@ impl Error for ParseError {
ParseErrorKind::Other => None,
ParseErrorKind::Level(ref l) => Some(l),
ParseErrorKind::Field(ref n) => Some(n.as_ref()),
ParseErrorKind::UnexpectedSyntax(_) => None,
}
}
}
Expand All @@ -677,6 +594,12 @@ impl From<level::ParseError> for ParseError {
}
}

impl From<ParseErrorKind> for ParseError {
fn from(kind: ParseErrorKind) -> Self {
Self { kind }
}
}

// ===== impl DynamicMatch =====

impl CallsiteMatcher {
Expand Down Expand Up @@ -1106,7 +1029,7 @@ mod test {

#[test]
fn parse_directives_with_special_characters_in_span_name() {
let span_name = "!\"#$%&'()*+-./:;<=>?@^_`|~[}";
let span_name = r##"!#$%&'()*+-.:;<>?@^_`|~"##; // forbidden: "/[]{}=

let dirs = parse_directives(format!("target[{}]=info", span_name));
assert_eq!(dirs.len(), 1, "\nparsed: {:#?}", dirs);
Expand Down
40 changes: 25 additions & 15 deletions tracing-subscriber/src/filter/env/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@ pub use self::{
};
mod directive;
mod field;
mod parsing;

use crate::{
filter::LevelFilter,
subscribe::{Context, Subscribe},
sync::RwLock,
};
use std::{cell::RefCell, collections::HashMap, env, error::Error, fmt, str::FromStr};
use std::{cell::RefCell, collections::HashMap, env, error::Error, fmt, iter, str::FromStr};
use tracing_core::{
callsite,
collect::{Collect, Interest},
Expand Down Expand Up @@ -151,24 +152,32 @@ impl EnvFilter {
/// Returns a new `EnvFilter` from the directives in the given string,
/// ignoring any that are invalid.
pub fn new<S: AsRef<str>>(dirs: S) -> Self {
let directives = dirs.as_ref().split(',').filter_map(|s| match s.parse() {
Ok(d) => Some(d),
Err(err) => {
eprintln!("ignoring `{}`: {}", s, err);
None
let mut dirs = dirs.as_ref();
let directives = iter::from_fn(move || {
if dirs.is_empty() {
return None;
}
});
let (parsed, after) = parsing::parse_one_directive(dirs);
dirs = &dirs[after..];
Some(parsed)
})
.filter_map(Result::ok);
Self::from_directives(directives)
}

/// Returns a new `EnvFilter` from the directives in the given string,
/// or an error if any are invalid.
pub fn try_new<S: AsRef<str>>(dirs: S) -> Result<Self, ParseError> {
let directives = dirs
.as_ref()
.split(',')
.map(|s| s.parse())
.collect::<Result<Vec<_>, _>>()?;
let mut dirs = dirs.as_ref();
let directives = iter::from_fn(move || {
if dirs.is_empty() {
return None;
}
let (parsed, after) = parsing::parse_one_directive(dirs);
dirs = &dirs[after..];
Some(parsed)
})
.collect::<Result<Vec<_>, _>>()?;
Ok(Self::from_directives(directives))
}

Expand Down Expand Up @@ -679,7 +688,8 @@ mod tests {

#[test]
fn callsite_enabled_includes_span_directive_multiple_fields() {
let filter = EnvFilter::new("app[mySpan{field=\"value\",field2=2}]=debug")
let filter = EnvFilter::try_new(r#"app[mySpan{field="value",field2=2}]=debug"#)
.unwrap()
.with_collector(NoCollector);
static META: &Metadata<'static> = &Metadata::new(
"mySpan",
Expand All @@ -693,13 +703,13 @@ mod tests {
);

let interest = filter.register_callsite(META);
assert!(interest.is_never());
assert!(interest.is_sometimes());
}

#[test]
fn roundtrip() {
let f1: EnvFilter =
"[span1{foo=1}]=error,[span2{bar=2 baz=false}],crate2[{quux=\"quuux\"}]=debug"
"[span1{foo=1}]=error,[span2{bar=2, baz=false}],crate2[{quux=\"quuux\"}]=debug"
.parse()
.unwrap();
let f2: EnvFilter = format!("{}", f1).parse().unwrap();
Expand Down
Loading