diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index fdc6cf8..bd1a26c 100644 --- a/.github/workflows/docker.yml +++ b/.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: diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index e8fdeb7..7377edf 100644 --- a/.github/workflows/rust.yml +++ b/.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: diff --git a/Dockerfile b/Dockerfile index 0796bed..7823d29 100644 --- a/Dockerfile +++ b/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 diff --git a/src/app.rs b/src/app.rs index f715410..635000e 100644 --- a/src/app.rs +++ b/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)) - .on_response(DefaultOnResponse::new().include_headers(true)), + .make_span_with(|request: &Request| { + 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; diff --git a/src/config.rs b/src/config.rs index 37afd67..51fcd27 100644 --- a/src/config.rs +++ b/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 { - let rate_limit_per_second = env_or_parse("RATE_LIMIT_PER_SECOND", 20.0)?; - let rate_limit_burst = env_or_parse("RATE_LIMIT_BURST", 40)?; + let defaults = Self::default_local(); + 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())?, - pokeapi_base_url: env_or_string("POKEAPI_BASE_URL", "https://pokeapi.co/api/v2"), + bind_addr: env_or_parse("BIND_ADDR", defaults.bind_addr)?, + 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"); diff --git a/src/error.rs b/src/error.rs index 0bc8a30..a25a0ed 100644 --- a/src/error.rs +++ b/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, diff --git a/src/pokemon.rs b/src/pokemon.rs index 98a3ddb..4814b6a 100644 --- a/src/pokemon.rs +++ b/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 { + validate_pokemon_name(name)?; let url = format!("{}/pokemon-species/{}", self.base_url, name); let response = self .http @@ -78,9 +79,9 @@ impl TryFrom 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 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::>().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;