Not being able to interpolate a header value is no longer a fatal error

Author: Michael-F-Bryan

README.md

@@ -95,22 +95,25 @@ cache-timeout = 43200
 warning-policy = "warn"
 
 # Extra HTTP headers that must be send to certain web sites
-# in order to link check to succeed
+# in order to link check to succeed.
 #
 # This is a dictionary (map), with keys being regexes
 # matching a set of web sites, and values being an array of
 # the headers.
-[http-headers]
+[output.linkcheck.http-headers]
 # Any hyperlink that contains this regexp will be sent
 # the "Accept: text/html" header
 'crates\.io' = ["Accept: text/html"]
 
-# mdbook-linkcheck will interpolate environment variables
-# into your header via $IDENT.
+# mdbook-linkcheck will interpolate environment variables into your header via
+# $IDENT.
 #
-# If this is not what you want
-# you must escape the `$` symbol, like `\$TOKEN`. `\` itself can also be escaped
-# via `\\`.
+# If this is not what you want you must escape the `$` symbol, like `\$TOKEN`.
+# `\` itself can also be escaped via `\\`.
+#
+# Note: If interpolation fails, the header will be skipped and the failure will
+# be logged. This can be useful if a particular header isn't always necessary,
+# but may be helpful (e.g. when working with rate limiting).
 'website\.com' = ["Authorization: Basic $TOKEN"]
 ```
 

src/config.rs

@@ -1,10 +1,15 @@
 use crate::hashed_regex::HashedRegex;
 use anyhow::Error;
 use http::header::{HeaderName, HeaderValue};
+use log::Level;
 use reqwest::Client;
 use serde_derive::{Deserialize, Serialize};
 use std::{
-    collections::HashMap, convert::TryFrom, str::FromStr, time::Duration,
+    collections::HashMap,
+    convert::TryFrom,
+    fmt::{self, Display, Formatter},
+    str::FromStr,
+    time::Duration,
 };
 
 /// The configuration options available with this backend.
@@ -40,11 +45,18 @@ pub struct Config {
 pub struct HttpHeader {
     pub name: HeaderName,
     pub value: String,
+}
+
+impl HttpHeader {
+    pub(crate) fn interpolate(&self) -> Result<HeaderValue, Error> {
+        interpolate_env(&self.value)
+    }
+}
 
-    // This is a separate field because interpolated env vars
-    // may contain some secrets that should not be revealed
-    // in logs, config, error messages and the like.
-    pub(crate) interpolated_value: HeaderValue,
+impl Display for HttpHeader {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        write!(f, "{}: {}", self.name, self.value)
+    }
 }
 
 impl Config {
@@ -67,6 +79,44 @@ impl Config {
             .insert(http::header::USER_AGENT, self.user_agent.parse().unwrap());
         Client::builder().default_headers(headers).build().unwrap()
     }
+
+    pub(crate) fn interpolate_headers(
+        &self,
+        warning_policy: WarningPolicy,
+    ) -> Vec<(HashedRegex, Vec<(HeaderName, HeaderValue)>)> {
+        let mut all_headers = Vec::new();
+        let log_level = warning_policy.to_log_level();
+
+        for (pattern, headers) in &self.http_headers {
+            let mut interpolated = Vec::new();
+
+            for header in headers {
+                match header.interpolate() {
+                    Ok(value) => {
+                        interpolated.push((header.name.clone(), value))
+                    },
+                    Err(e) => {
+                        // We don't want failed interpolation (i.e. due to a
+                        // missing env variable) to abort the whole
+                        // linkchecking, so emit a warning and keep going.
+                        //
+                        // If it was important, the user would notice a "broken"
+                        // link and read back through the logs.
+                        log::log!(
+                            log_level,
+                            "Unable to interpolate \"{}\" because {}",
+                            header,
+                            e
+                        );
+                    },
+                }
+            }
+
+            all_headers.push((pattern.clone(), interpolated));
+        }
+
+        all_headers
+    }
 }
 
 impl Default for Config {
@@ -91,16 +141,14 @@ impl FromStr for HttpHeader {
             Some(idx) => {
                 let name = s[..idx].parse()?;
                 let value = s[idx + 2..].to_string();
-                let interpolated_value = interpolate_env(&value)?;
                 Ok(HttpHeader {
                     name,
                     value,
-                    interpolated_value,
                 })
             },
 
             None => Err(Error::msg(format!(
-                "The `{}` HTTP header must contain `: ` but it doesn't",
+                "The `{}` HTTP header must be in the form `key: value` but it isn't",
                 s
             ))),
         }
@@ -209,6 +257,16 @@ pub enum WarningPolicy {
     Error,
 }
 
+impl WarningPolicy {
+    pub(crate) fn to_log_level(self) -> Level {
+        match self {
+            WarningPolicy::Error => Level::Error,
+            WarningPolicy::Warn => Level::Warn,
+            WarningPolicy::Ignore => Level::Debug,
+        }
+    }
+}
+
 impl Default for WarningPolicy {
     fn default() -> WarningPolicy { WarningPolicy::Warn }
 }
@@ -269,16 +327,14 @@ https = ["accept: html/text", "authorization: Basic $TOKEN"]
 
     #[test]
     fn interpolation() {
-        std::env::set_var("TOKEN", "QWxhZGRpbjpPcGVuU2VzYW1l");
-        let should_be = HttpHeader {
+        std::env::set_var("TOKEN", "abcdefg123456");
+        let header = HttpHeader {
             name: "Authorization".parse().unwrap(),
             value: "Basic $TOKEN".into(),
-            interpolated_value: "Basic QWxhZGRpbjpPcGVuU2VzYW1l"
-                .parse()
-                .unwrap(),
         };
+        let should_be: HeaderValue = "Basic abcdefg123456".parse().unwrap();
 
-        let got = HttpHeader::try_from("Authorization: Basic $TOKEN").unwrap();
+        let got = header.interpolate().unwrap();
 
         assert_eq!(got, should_be);
     }

src/context.rs

@@ -1,6 +1,6 @@
-use crate::Config;
+use crate::{Config, HashedRegex};
 use codespan::Files;
-use http::HeaderMap;
+use http::header::{HeaderMap, HeaderName, HeaderValue};
 use linkcheck::{
     validation::{Cache, Options},
     Link,
@@ -20,6 +20,8 @@ pub struct Context<'a> {
     pub(crate) files: &'a Files<String>,
     pub(crate) client: Client,
     pub(crate) filesystem_options: Options,
+    pub(crate) interpolated_headers:
+        Vec<(HashedRegex, Vec<(HeaderName, HeaderValue)>)>,
 }
 
 impl<'a> linkcheck::validation::Context for Context<'a> {
@@ -42,26 +44,12 @@ impl<'a> linkcheck::validation::Context for Context<'a> {
         let url = url.to_string();
         let mut headers = HeaderMap::new();
 
-        let extra_headers = self
-            .cfg
-            .http_headers
-            .iter()
-            .filter_map(|(re, extra)| {
-                if re.find(&url).is_some() {
-                    Some(extra)
-                } else {
-                    None
+        for (pattern, matching_headers) in &self.interpolated_headers {
+            if pattern.find(&url).is_some() {
+                for (name, value) in matching_headers {
+                    headers.insert(name.clone(), value.clone());
                 }
-            })
-            .flatten();
-
-        for header in extra_headers {
-            let crate::config::HttpHeader {
-                name,
-                interpolated_value,
-                ..
-            } = header;
-            headers.insert(name.clone(), interpolated_value.clone());
+            }
         }
 
         headers

src/validate.rs

@@ -43,13 +43,16 @@ fn lc_validate(
         .set_default_file("README.md")
         .set_custom_validation(ensure_included_in_book(src_dir, file_names));
 
+    let interpolated_headers = cfg.interpolate_headers(cfg.warning_policy);
+
     let ctx = Context {
         client: cfg.client(),
         filesystem_options: options,
         cfg,
         src_dir,
         cache: Mutex::new(cache.clone()),
         files,
+        interpolated_headers,
     };
     let links = collate_links(links, src_dir, files);