fix: address review findings

.github/workflows/docker.yml

@@ -2,9 +2,9 @@ name: Docker
 
 on:
   push:
-    branches: [main]
+    branches: [main, master]
   pull_request:
-    branches: [main]
+    branches: [main, master]
   workflow_dispatch:
 
 concurrency:

.github/workflows/rust.yml

@@ -2,9 +2,9 @@ name: Rust
 
 on:
   push:
-    branches: [main]
+    branches: [main, master]
   pull_request:
-    branches: [main]
+    branches: [main, master]
   workflow_dispatch:
 
 concurrency:

Dockerfile

@@ -66,7 +66,9 @@ FROM debian:trixie-slim
 
 RUN apt-get update && apt-get install -y --no-install-recommends \
     ca-certificates curl \
-    && rm -rf /var/lib/apt/lists/*
+    && rm -rf /var/lib/apt/lists/* \
+    && groupadd --system app \
+    && useradd --system --gid app --home-dir /nonexistent --shell /usr/sbin/nologin app
 
 COPY --from=builder /usr/local/bin/pokedex-api /usr/local/bin/pokedex-api
 
@@ -75,6 +77,7 @@ ENV GIT_SHA=$GIT_SHA \
     BIND_ADDR=0.0.0.0:5000
 
 EXPOSE 5000
+USER app
 HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \
     CMD curl -fsS http://127.0.0.1:5000/health || exit 1
 

src/app.rs

@@ -16,7 +16,7 @@ use tower::ServiceBuilder;
 use tower_http::{
     ServiceBuilderExt,
     request_id::MakeRequestUuid,
-    trace::{DefaultMakeSpan, DefaultOnResponse, TraceLayer},
+    trace::{DefaultOnResponse, TraceLayer},
 };
 
 #[derive(Clone)]
@@ -56,8 +56,20 @@ pub fn create_app(state: AppState) -> Router {
         .set_x_request_id(MakeRequestUuid)
         .layer(
             TraceLayer::new_for_http()
-                .make_span_with(DefaultMakeSpan::new().include_headers(true))
+                .make_span_with(|request: &Request| {
-                .on_response(DefaultOnResponse::new().include_headers(true)),
+                    let request_id = request
+                        .headers()
+                        .get("x-request-id")
+                        .and_then(|value| value.to_str().ok())
+                        .unwrap_or("unknown");
+                    tracing::info_span!(
+                        "http.request",
+                        method = %request.method(),
+                        uri = %request.uri(),
+                        request_id = %request_id,
+                    )
+                })
+                .on_response(DefaultOnResponse::new()),
         )
         .propagate_x_request_id();
 
@@ -201,6 +213,24 @@ mod tests {
         );
     }
 
+    #[tokio::test]
+    async fn pokemon_endpoint_rejects_invalid_names_before_calling_upstream() {
+        let pokeapi = MockServer::start().await;
+        let translations = MockServer::start().await;
+        let app = create_app(state_for(&pokeapi, &translations));
+
+        let response = app
+            .oneshot(request("/pokemon/foo%3Fbar=baz"))
+            .await
+            .expect("request should be handled");
+
+        assert_eq!(response.status(), StatusCode::BAD_REQUEST);
+        assert_eq!(
+            json_body(response).await["error"],
+            "pokemon name must contain only ASCII letters, digits, or hyphens"
+        );
+    }
+
     #[tokio::test]
     async fn translated_endpoint_returns_translated_description() {
         let pokeapi = MockServer::start().await;
@@ -227,6 +257,27 @@ mod tests {
         );
     }
 
+    #[tokio::test]
+    async fn translated_endpoint_falls_back_to_original_description_when_translation_fails() {
+        let pokeapi = MockServer::start().await;
+        let translations = MockServer::start().await;
+        mount_species(&pokeapi, "zubat", false, "cave", "It has no eyes.").await;
+        Mock::given(matchers::method("POST"))
+            .and(matchers::path("/translate/yoda.json"))
+            .respond_with(ResponseTemplate::new(500))
+            .mount(&translations)
+            .await;
+        let app = create_app(state_for(&pokeapi, &translations));
+
+        let response = app
+            .oneshot(request("/pokemon/translated/zubat"))
+            .await
+            .expect("request should be handled");
+
+        assert_eq!(response.status(), StatusCode::OK);
+        assert_eq!(json_body(response).await["description"], "It has no eyes.");
+    }
+
     #[tokio::test]
     async fn every_response_has_a_request_id() {
         let pokeapi = MockServer::start().await;

src/config.rs

@@ -12,22 +12,40 @@ pub struct AppConfig {
 }
 
 impl AppConfig {
+    #[must_use]
+    pub fn default_local() -> Self {
+        Self {
+            bind_addr: default_bind_addr(),
+            pokeapi_base_url: "https://pokeapi.co/api/v2".to_owned(),
+            translations_base_url: "https://funtranslations.mercxry.me".to_owned(),
+            request_timeout: Duration::from_secs(5),
+            rate_limit_per_second: 20.0,
+            rate_limit_burst: 40,
+            service_name: env!("CARGO_PKG_NAME").to_owned(),
+        }
+    }
+
     pub fn from_env() -> Result<Self, ConfigError> {
-        let rate_limit_per_second = env_or_parse("RATE_LIMIT_PER_SECOND", 20.0)?;
+        let defaults = Self::default_local();
-        let rate_limit_burst = env_or_parse("RATE_LIMIT_BURST", 40)?;
+        let rate_limit_per_second =
+            env_or_parse("RATE_LIMIT_PER_SECOND", defaults.rate_limit_per_second)?;
+        let rate_limit_burst = env_or_parse("RATE_LIMIT_BURST", defaults.rate_limit_burst)?;
         validate_rate_limit(rate_limit_per_second, rate_limit_burst)?;
 
         Ok(Self {
-            bind_addr: env_or_parse("BIND_ADDR", default_bind_addr())?,
+            bind_addr: env_or_parse("BIND_ADDR", defaults.bind_addr)?,
-            pokeapi_base_url: env_or_string("POKEAPI_BASE_URL", "https://pokeapi.co/api/v2"),
+            pokeapi_base_url: env_or_string("POKEAPI_BASE_URL", &defaults.pokeapi_base_url),
             translations_base_url: env_or_string(
                 "FUN_TRANSLATIONS_BASE_URL",
-                "https://funtranslations.mercxry.me",
+                &defaults.translations_base_url,
             ),
-            request_timeout: Duration::from_secs(env_or_parse("REQUEST_TIMEOUT_SECONDS", 5)?),
+            request_timeout: Duration::from_secs(env_or_parse(
+                "REQUEST_TIMEOUT_SECONDS",
+                defaults.request_timeout.as_secs(),
+            )?),
             rate_limit_per_second,
             rate_limit_burst,
-            service_name: env_or_string("OTEL_SERVICE_NAME", env!("CARGO_PKG_NAME")),
+            service_name: env_or_string("OTEL_SERVICE_NAME", &defaults.service_name),
         })
     }
 }
@@ -81,7 +99,7 @@ mod tests {
 
     #[test]
     fn default_config_is_valid_for_local_development() {
-        let config = AppConfig::from_env().expect("default env should parse");
+        let config = AppConfig::default_local();
 
         assert_eq!(config.bind_addr.port(), 5000);
         assert_eq!(config.pokeapi_base_url, "https://pokeapi.co/api/v2");

src/error.rs

@@ -3,6 +3,8 @@ use serde::Serialize;
 
 #[derive(Debug, thiserror::Error)]
 pub enum AppError {
+    #[error("bad request: {0}")]
+    BadRequest(String),
     #[error("pokemon not found")]
     NotFound,
     #[error("upstream service returned invalid data: {0}")]
@@ -19,6 +21,7 @@ impl AppError {
     #[must_use]
     pub const fn status_code(&self) -> StatusCode {
         match self {
+            Self::BadRequest(_) => StatusCode::BAD_REQUEST,
             Self::NotFound => StatusCode::NOT_FOUND,
             Self::InvalidUpstreamData(_) | Self::Upstream(_) | Self::Timeout => {
                 StatusCode::BAD_GATEWAY
@@ -32,12 +35,24 @@ impl IntoResponse for AppError {
     fn into_response(self) -> axum::response::Response {
         let status = self.status_code();
         let body = ErrorBody {
-            error: self.to_string(),
+            error: self.public_message(),
         };
         (status, Json(body)).into_response()
     }
 }
 
+impl AppError {
+    fn public_message(&self) -> String {
+        match self {
+            Self::BadRequest(message) => message.clone(),
+            Self::NotFound => "pokemon not found".to_owned(),
+            Self::InvalidUpstreamData(_) => "upstream service returned invalid data".to_owned(),
+            Self::Upstream(_) | Self::Timeout => "upstream service failed".to_owned(),
+            Self::Internal => "internal server error".to_owned(),
+        }
+    }
+}
+
 #[derive(Serialize)]
 struct ErrorBody {
     error: String,

src/pokemon.rs

@@ -24,6 +24,7 @@ impl PokeApiClient {
 
     #[instrument(skip(self), fields(pokemon.name = %name))]
     pub async fn get_pokemon_info(&self, name: &str) -> Result<PokemonInfo, AppError> {
+        validate_pokemon_name(name)?;
         let url = format!("{}/pokemon-species/{}", self.base_url, name);
         let response = self
             .http
@@ -78,9 +79,9 @@ impl TryFrom<PokemonSpeciesResponse> for PokemonInfo {
         let description = species
             .flavor_text_entries
             .iter()
-            .find(|entry| entry.language.name == "en")
+            .filter(|entry| entry.language.name == "en")
             .map(|entry| normalize_description(&entry.flavor_text))
-            .filter(|description| !description.is_empty())
+            .find(|description| !description.is_empty())
             .ok_or_else(|| {
                 AppError::InvalidUpstreamData("missing English flavor text".to_owned())
             })?;
@@ -96,6 +97,21 @@ impl TryFrom<PokemonSpeciesResponse> for PokemonInfo {
     }
 }
 
+pub fn validate_pokemon_name(name: &str) -> Result<(), AppError> {
+    let is_valid = !name.is_empty()
+        && name
+            .bytes()
+            .all(|byte| byte.is_ascii_alphanumeric() || byte == b'-');
+
+    if is_valid {
+        Ok(())
+    } else {
+        Err(AppError::BadRequest(
+            "pokemon name must contain only ASCII letters, digits, or hyphens".to_owned(),
+        ))
+    }
+}
+
 #[must_use]
 pub fn normalize_description(description: &str) -> String {
     description.split_whitespace().collect::<Vec<_>>().join(" ")
@@ -115,7 +131,7 @@ fn map_reqwest_error(error: &reqwest::Error) -> AppError {
 
 #[cfg(test)]
 mod tests {
-    use super::{PokeApiClient, normalize_description};
+    use super::{PokeApiClient, normalize_description, validate_pokemon_name};
     use crate::error::AppError;
     use std::time::Duration;
     use wiremock::{Mock, MockServer, ResponseTemplate, matchers};
@@ -127,6 +143,13 @@ mod tests {
         assert_eq!(normalized, "Line one line two line three");
     }
 
+    #[test]
+    fn rejects_names_that_could_change_the_upstream_url_shape() {
+        let error = validate_pokemon_name("foo?bar=baz").expect_err("name should be rejected");
+
+        assert!(matches!(error, AppError::BadRequest(_)));
+    }
+
     #[tokio::test]
     async fn fetches_basic_pokemon_information_from_species_endpoint() {
         let server = MockServer::start().await;
@@ -199,6 +222,32 @@ mod tests {
         assert!(matches!(error, AppError::NotFound));
     }
 
+    #[tokio::test]
+    async fn skips_empty_english_descriptions_until_it_finds_a_non_empty_one() {
+        let server = MockServer::start().await;
+        Mock::given(matchers::method("GET"))
+            .and(matchers::path("/pokemon-species/eevee"))
+            .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({
+                "name": "eevee",
+                "is_legendary": false,
+                "habitat": { "name": "urban" },
+                "flavor_text_entries": [
+                    { "flavor_text": " \n \t", "language": { "name": "en" } },
+                    { "flavor_text": "Can evolve in many ways.", "language": { "name": "en" } }
+                ]
+            })))
+            .mount(&server)
+            .await;
+        let client = client_for(&server);
+
+        let pokemon = client
+            .get_pokemon_info("eevee")
+            .await
+            .expect("second English description should be used");
+
+        assert_eq!(pokemon.description, "Can evolve in many ways.");
+    }
+
     #[tokio::test]
     async fn rejects_species_without_english_description() {
         let server = MockServer::start().await;