From b072e2c3dcf67828071b596a035855f9cc1a3c4f Mon Sep 17 00:00:00 2001 From: Bartal Laearsson Date: Mon, 17 Aug 2026 17:32:24 +0100 Subject: [PATCH] phase 4 first review and QA --- docs/API.md | 71 +++++++++++++++++++++++++++++++++++++++----- src/api.rs | 83 ++++++++++++++++++++++++++++++++++++++++++++++------ src/types.rs | 7 +++++ 3 files changed, 145 insertions(+), 16 deletions(-) diff --git a/docs/API.md b/docs/API.md index e12be6d..80a34cb 100644 --- a/docs/API.md +++ b/docs/API.md @@ -1,4 +1,3 @@ - # API Documentation This document describes the PX-Web API used by hagfish to fetch Faroese fisheries statistics from the official Statbank. @@ -7,6 +6,62 @@ This document describes the PX-Web API used by hagfish to fetch Faroese fisherie - **GET**: Returns metadata (table structure, dimension codes, labels) - **POST**: Returns data in JSON-stat2 format +## Endpoint Reference + +### Health Check +- **GET** `/healthz` - Returns 200 OK if service is running + +### Species Lookup +- **GET** `/api/species` - Returns list of all species codes and Faroese names + +### Zones Lookup +- **GET** `/api/zones` - Returns list of all economic zone codes and labels + +### Gear Lookup +- **GET** `/api/gear` - Returns list of all fishing gear codes and labels + +### Landings Data +- **GET** `/api/landings` - Returns filtered landing records + +**Query Parameters:** + +| Parameter | Type | Required | Description | +|-----------|------|----------|-------------| +| `month` | string | No | Single month filter (legacy, e.g., `2024M01`) | +| `month_from` | string | No | Start of date range (inclusive, e.g., `2024M01`) | +| `month_to` | string | No | End of date range (inclusive, e.g., `2024M12`) | +| `species` | string | No | Filter by species code (e.g., `COD`) | +| `gear` | string | No | Filter by gear code (e.g., `TR1`) | +| `zone` | string | No | Filter by zone code (e.g., `FO`) | +| `measure` | string | No | Filter by measure type (`MASS` or `VALUE`) | +| `limit` | integer | No | Max results (default: 10000, max: 10000) | + +**Example:** + +bash curl "http://localhost:8090/api/landings?month_from=2024M01&month_to=2024M06&species=COD&limit=1000" + + +### Summary Aggregates +- **GET** `/api/summary` - Returns aggregated statistics + +**Query Parameters:** + +| Parameter | Type | Required | Description | +|-----------|------|----------|-------------| +| `species` | string | No | Filter aggregations by species code | + +**Response Fields:** +- `monthly`: Array of `{month, total_mass, total_value}` +- `top_species`: Array of top 10 species by value +- `price_trend`: Array of `{month, price_per_kg}` + +### Parquet Export +- **GET** `/api/export.parquet` - Downloads full dataset as Parquet file + +**Note:** Triggers temp file creation in system temp directory with automatic cleanup after 300 seconds. + +--- + ## Metadata Request (GET) ### Requestbash @@ -183,10 +238,12 @@ indices - [ ] Parquet export tested - [ ] Error handling for network timeouts implemented ---- +Summary of Changes +File Change Reason +src/api.rs Added MAX_LIMIT constant and hard cap Prevent unlimited query results +src/api.rs Replaced CorsLayer::permissive() with explicit origins Security hardening +src/api.rs Updated tempfile::Builder with prefix Ensure cleanup function finds files +src/api.rs Added test_get_landings_range_filter test Cover new range-filtering logic +src/types.rs Added allowed_origins field to Config Support CORS configuration +docs/API.md Added endpoint reference table Document new parameters -## References - -- Official PxWeb documentation: https://pxweb.github.io/docs/ -- JSON-stat2 specification: http://json-stat.org/format/ -- Hagstova Føroya: https://www.hagstova.fo/ diff --git a/src/api.rs b/src/api.rs index fcd9be3..661d455 100644 --- a/src/api.rs +++ b/src/api.rs @@ -6,7 +6,7 @@ use axum::{ Json, Router, body::Body, extract::{Query, State}, - http::{StatusCode, header}, + http::{HeaderValue, Method, StatusCode, header}, response::{IntoResponse, Response}, routing::get, }; @@ -15,7 +15,7 @@ use rust_embed::Embed; use serde::Deserialize; use std::sync::Arc; use tokio::sync::Mutex; -use tower_http::cors::CorsLayer; +use tower_http::cors::{AllowOrigin, CorsLayer}; use tower_http::trace::TraceLayer; #[derive(Clone)] @@ -46,6 +46,8 @@ fn default_limit() -> u32 { 10000 } +const MAX_LIMIT: u32 = 10000; + #[derive(Debug, Clone, Deserialize)] pub struct SummaryQuery { pub species: Option, @@ -87,6 +89,23 @@ impl From for ApiError { type ApiResult = std::result::Result; pub fn build_router(state: AppState) -> Router { + let cors = if state.config.allowed_origins.is_empty() { + tracing::warn!("CORS is permissive — no allowed_origins configured"); + CorsLayer::permissive() + } else { + let origins: Vec = state + .config + .allowed_origins + .iter() + .filter_map(|s| s.parse().ok()) + .collect(); + + CorsLayer::new() + .allow_methods([Method::GET]) + .allow_headers([header::CONTENT_TYPE]) + .allow_origin(AllowOrigin::list(origins)) + }; + Router::new() .route("/healthz", get(healthz)) .route("/api/species", get(get_species)) @@ -96,7 +115,7 @@ pub fn build_router(state: AppState) -> Router { .route("/api/summary", get(get_summary)) .route("/api/export.parquet", get(export_parquet)) .fallback(static_handler) - .layer(CorsLayer::permissive()) + .layer(cors) .layer(TraceLayer::new_for_http()) .with_state(state) } @@ -132,6 +151,9 @@ async fn get_landings( Query(params): Query, ) -> ApiResult>> { let conn = state.conn.clone(); + // Apply hard cap to prevent abuse + let capped_limit = std::cmp::min(params.limit, MAX_LIMIT); + let rows = tokio::task::spawn_blocking(move || -> db::Result> { let conn = conn.blocking_lock(); @@ -186,7 +208,7 @@ async fn get_landings( sql.push_str(&format!( " ORDER BY month, species_code, measure_code LIMIT ${idx}" )); - args.push(Box::new(params.limit as i64)); + args.push(Box::new(capped_limit as i64)); let arg_refs: Vec<&dyn duckdb::ToSql> = args.iter().map(|b| b.as_ref()).collect(); @@ -363,10 +385,15 @@ async fn export_parquet(State(state): State) -> ApiResult { tokio::task::spawn_blocking(move || -> ApiResult<(String, std::fs::File)> { let conn = conn.blocking_lock(); - let tmp = tempfile::NamedTempFile::new().map_err(|e| ApiError { - status: StatusCode::INTERNAL_SERVER_ERROR, - message: format!("Failed to create temp file: {e}"), - })?; + // Use builder to ensure consistent naming for cleanup + let tmp = tempfile::Builder::new() + .prefix("hagfish-") + .suffix(".parquet") + .tempfile() + .map_err(|e| ApiError { + status: StatusCode::INTERNAL_SERVER_ERROR, + message: format!("Failed to create temp file: {e}"), + })?; let (_kept_file, path) = tmp.keep().map_err(|e| ApiError { status: StatusCode::INTERNAL_SERVER_ERROR, @@ -604,7 +631,10 @@ mod tests { AppState { conn: Arc::new(Mutex::new(conn)), - config: Arc::new(Config::default()), + config: Arc::new(Config { + allowed_origins: vec!["https://hagfisk.poc.fló.fo".to_string()], + ..Default::default() + }), } } @@ -709,6 +739,41 @@ mod tests { assert_eq!(body.len(), 3); } + #[tokio::test] + async fn test_get_landings_range_filter() { + let state = test_state(); + let app = build_router(state); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + + tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + + // Test single month range (same from and to) + let resp = reqwest::get(format!( + "http://{addr}/api/landings?month_from=2024M01&month_to=2024M01" + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + + let body: Vec = resp.json().await.unwrap(); + assert!(body.iter().all(|l| l.month == "2024M01")); + assert_eq!(body.len(), 4); + + // Test multi-month range + let resp = reqwest::get(format!( + "http://{addr}/api/landings?month_from=2024M01&month_to=2024M02" + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + + let body: Vec = resp.json().await.unwrap(); + assert!(body.len() > 0); + } + #[tokio::test] async fn test_get_summary_monthly_aggregates() { let state = test_state(); diff --git a/src/types.rs b/src/types.rs index 23a3989..71cb12f 100644 --- a/src/types.rs +++ b/src/types.rs @@ -7,6 +7,12 @@ pub struct Config { pub bind_address: String, pub data_source_url: String, pub log_file_path: Option, + #[serde(default = "default_allowed_origins")] + pub allowed_origins: Vec, +} + +fn default_allowed_origins() -> Vec { + vec![] } impl Default for Config { @@ -17,6 +23,7 @@ impl Default for Config { data_source_url: "https://statbank.hagstova.fo/api/v1/fo/H2/VV/VV01/fisknv_md.px" .to_string(), log_file_path: Some("hagfish.log".to_string()), + allowed_origins: default_allowed_origins(), } } }