mirror of
https://github.com/getnora-io/nora.git
synced 2026-04-12 06:50:31 +00:00
fix: code quality hardening — unwrap removal, unsafe forbid, Go/Raw tests (#72)
* fix: remove unwrap() from production code, improve error handling - Replace unwrap() with proper error handling in npm, mirror, validation - Add input validation to cargo registry (crate name + version) - Improve expect() messages with descriptive context in metrics, rate_limit - Remove unnecessary clone() in error.rs, docker.rs, npm.rs, dashboard_metrics - Add #![deny(clippy::unwrap_used)] to prevent future unwrap in prod code - Add let-else pattern for safer null checks in validation.rs * docs: update SECURITY.md — add 0.3.x to supported versions * security: forbid unsafe code at crate level Add #![forbid(unsafe_code)] to both lib.rs and main.rs. NORA has zero unsafe blocks — this prevents future additions without removing the forbid attribute (stronger than deny). * build: add rust-toolchain.toml, Dockerfile HEALTHCHECK - Pin toolchain to stable with clippy + rustfmt components - Add Docker HEALTHCHECK for standalone deployments (wget /health) * test: add Go proxy and Raw registry integration tests Go proxy tests: list, .info, .mod, @latest, path traversal, 404 Raw registry tests: upload/download, HEAD, 404, path traversal, overwrite, delete, binary data (10KB)
This commit is contained in:
@@ -76,7 +76,6 @@ impl DashboardMetrics {
|
||||
pub fn with_persistence(storage_path: &str) -> Self {
|
||||
let path = Path::new(storage_path).join("metrics.json");
|
||||
let mut metrics = Self::new();
|
||||
metrics.persist_path = Some(path.clone());
|
||||
|
||||
// Load existing metrics if file exists
|
||||
if path.exists() {
|
||||
@@ -108,6 +107,7 @@ impl DashboardMetrics {
|
||||
}
|
||||
}
|
||||
|
||||
metrics.persist_path = Some(path);
|
||||
metrics
|
||||
}
|
||||
|
||||
|
||||
@@ -51,11 +51,11 @@ struct ErrorResponse {
|
||||
|
||||
impl IntoResponse for AppError {
|
||||
fn into_response(self) -> Response {
|
||||
let (status, message) = match &self {
|
||||
AppError::NotFound(msg) => (StatusCode::NOT_FOUND, msg.clone()),
|
||||
AppError::BadRequest(msg) => (StatusCode::BAD_REQUEST, msg.clone()),
|
||||
AppError::Unauthorized(msg) => (StatusCode::UNAUTHORIZED, msg.clone()),
|
||||
AppError::Internal(msg) => (StatusCode::INTERNAL_SERVER_ERROR, msg.clone()),
|
||||
let (status, message) = match self {
|
||||
AppError::NotFound(msg) => (StatusCode::NOT_FOUND, msg),
|
||||
AppError::BadRequest(msg) => (StatusCode::BAD_REQUEST, msg),
|
||||
AppError::Unauthorized(msg) => (StatusCode::UNAUTHORIZED, msg),
|
||||
AppError::Internal(msg) => (StatusCode::INTERNAL_SERVER_ERROR, msg),
|
||||
AppError::Storage(e) => match e {
|
||||
StorageError::NotFound => (StatusCode::NOT_FOUND, "Resource not found".to_string()),
|
||||
StorageError::Validation(v) => (StatusCode::BAD_REQUEST, v.to_string()),
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
#![deny(clippy::unwrap_used)]
|
||||
#![forbid(unsafe_code)]
|
||||
//! NORA Registry — library interface for fuzzing and testing
|
||||
|
||||
pub mod validation;
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
// Copyright (c) 2026 Volkov Pavel | DevITWay
|
||||
// SPDX-License-Identifier: MIT
|
||||
|
||||
#![deny(clippy::unwrap_used)]
|
||||
#![forbid(unsafe_code)]
|
||||
mod activity_log;
|
||||
mod audit;
|
||||
mod auth;
|
||||
|
||||
@@ -26,7 +26,7 @@ lazy_static! {
|
||||
"nora_http_requests_total",
|
||||
"Total number of HTTP requests",
|
||||
&["registry", "method", "status"]
|
||||
).expect("metric can be created");
|
||||
).expect("failed to create HTTP_REQUESTS_TOTAL metric at startup");
|
||||
|
||||
/// HTTP request duration histogram
|
||||
pub static ref HTTP_REQUEST_DURATION: HistogramVec = register_histogram_vec!(
|
||||
@@ -34,28 +34,28 @@ lazy_static! {
|
||||
"HTTP request latency in seconds",
|
||||
&["registry", "method"],
|
||||
vec![0.001, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1.0, 2.5, 5.0, 10.0]
|
||||
).expect("metric can be created");
|
||||
).expect("failed to create HTTP_REQUEST_DURATION metric at startup");
|
||||
|
||||
/// Cache requests counter (hit/miss)
|
||||
pub static ref CACHE_REQUESTS: IntCounterVec = register_int_counter_vec!(
|
||||
"nora_cache_requests_total",
|
||||
"Total cache requests",
|
||||
&["registry", "result"]
|
||||
).expect("metric can be created");
|
||||
).expect("failed to create CACHE_REQUESTS metric at startup");
|
||||
|
||||
/// Storage operations counter
|
||||
pub static ref STORAGE_OPERATIONS: IntCounterVec = register_int_counter_vec!(
|
||||
"nora_storage_operations_total",
|
||||
"Total storage operations",
|
||||
&["operation", "status"]
|
||||
).expect("metric can be created");
|
||||
).expect("failed to create STORAGE_OPERATIONS metric at startup");
|
||||
|
||||
/// Artifacts count by registry
|
||||
pub static ref ARTIFACTS_TOTAL: IntCounterVec = register_int_counter_vec!(
|
||||
"nora_artifacts_total",
|
||||
"Total artifacts stored",
|
||||
&["registry"]
|
||||
).expect("metric can be created");
|
||||
).expect("failed to create ARTIFACTS_TOTAL metric at startup");
|
||||
}
|
||||
|
||||
/// Routes for metrics endpoint
|
||||
|
||||
@@ -64,7 +64,7 @@ pub fn create_progress_bar(total: u64) -> ProgressBar {
|
||||
.template(
|
||||
"{spinner:.green} [{elapsed_precise}] [{bar:40.cyan/blue}] {pos}/{len} ({eta}) {msg}",
|
||||
)
|
||||
.unwrap()
|
||||
.expect("static progress bar template is valid")
|
||||
.progress_chars("=>-"),
|
||||
);
|
||||
pb
|
||||
@@ -220,7 +220,7 @@ fn parse_requirements_txt(content: &str) -> Vec<MirrorTarget> {
|
||||
.lines()
|
||||
.filter(|l| !l.trim().is_empty() && !l.starts_with('#') && !l.starts_with('-'))
|
||||
.filter_map(|line| {
|
||||
let line = line.split('#').next().unwrap().trim();
|
||||
let line = line.split('#').next().unwrap_or(line).trim();
|
||||
if let Some((name, version)) = line.split_once("==") {
|
||||
Some(MirrorTarget {
|
||||
name: name.trim().to_string(),
|
||||
|
||||
@@ -200,7 +200,11 @@ async fn mirror_npm_packages(
|
||||
let mut handles = Vec::new();
|
||||
|
||||
for target in targets {
|
||||
let permit = sem.clone().acquire_owned().await.unwrap();
|
||||
let permit = sem
|
||||
.clone()
|
||||
.acquire_owned()
|
||||
.await
|
||||
.expect("semaphore closed unexpectedly");
|
||||
let client = client.clone();
|
||||
let pb = pb.clone();
|
||||
let fetched = fetched.clone();
|
||||
|
||||
@@ -25,7 +25,7 @@ pub fn auth_rate_limiter(
|
||||
.burst_size(config.auth_burst)
|
||||
.use_headers()
|
||||
.finish()
|
||||
.expect("Failed to build auth rate limiter");
|
||||
.expect("failed to build auth rate limiter: invalid RateLimitConfig");
|
||||
|
||||
tower_governor::GovernorLayer::new(gov_config)
|
||||
}
|
||||
@@ -46,7 +46,7 @@ pub fn upload_rate_limiter(
|
||||
.burst_size(config.upload_burst)
|
||||
.use_headers()
|
||||
.finish()
|
||||
.expect("Failed to build upload rate limiter");
|
||||
.expect("failed to build upload rate limiter: invalid RateLimitConfig");
|
||||
|
||||
tower_governor::GovernorLayer::new(gov_config)
|
||||
}
|
||||
@@ -65,7 +65,7 @@ pub fn general_rate_limiter(
|
||||
.burst_size(config.general_burst)
|
||||
.use_headers()
|
||||
.finish()
|
||||
.expect("Failed to build general rate limiter");
|
||||
.expect("failed to build general rate limiter: invalid RateLimitConfig");
|
||||
|
||||
tower_governor::GovernorLayer::new(gov_config)
|
||||
}
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
|
||||
use crate::activity_log::{ActionType, ActivityEntry};
|
||||
use crate::audit::AuditEntry;
|
||||
use crate::validation::validate_storage_key;
|
||||
use crate::AppState;
|
||||
use axum::{
|
||||
extract::{Path, State},
|
||||
@@ -26,6 +27,10 @@ async fn get_metadata(
|
||||
State(state): State<Arc<AppState>>,
|
||||
Path(crate_name): Path<String>,
|
||||
) -> Response {
|
||||
// Validate input to prevent path traversal
|
||||
if validate_storage_key(&crate_name).is_err() {
|
||||
return StatusCode::BAD_REQUEST.into_response();
|
||||
}
|
||||
let key = format!("cargo/{}/metadata.json", crate_name);
|
||||
match state.storage.get(&key).await {
|
||||
Ok(data) => (StatusCode::OK, data).into_response(),
|
||||
@@ -37,6 +42,10 @@ async fn download(
|
||||
State(state): State<Arc<AppState>>,
|
||||
Path((crate_name, version)): Path<(String, String)>,
|
||||
) -> Response {
|
||||
// Validate inputs to prevent path traversal
|
||||
if validate_storage_key(&crate_name).is_err() || validate_storage_key(&version).is_err() {
|
||||
return StatusCode::BAD_REQUEST.into_response();
|
||||
}
|
||||
let key = format!(
|
||||
"cargo/{}/{}/{}-{}.crate",
|
||||
crate_name, version, crate_name, version
|
||||
|
||||
@@ -346,7 +346,7 @@ async fn start_upload(Path(name): Path<String>) -> Response {
|
||||
(
|
||||
StatusCode::ACCEPTED,
|
||||
[
|
||||
(header::LOCATION, location.clone()),
|
||||
(header::LOCATION, location),
|
||||
(HeaderName::from_static("docker-upload-uuid"), uuid),
|
||||
],
|
||||
)
|
||||
|
||||
@@ -176,8 +176,7 @@ async fn handle_request(State(state): State<Arc<AppState>>, Path(path): Path<Str
|
||||
} else {
|
||||
// Metadata: rewrite tarball URLs to point to NORA
|
||||
let nora_base = nora_base_url(&state);
|
||||
let rewritten = rewrite_tarball_urls(&data, &nora_base, proxy_url)
|
||||
.unwrap_or_else(|_| data.clone());
|
||||
let rewritten = rewrite_tarball_urls(&data, &nora_base, proxy_url).unwrap_or(data);
|
||||
|
||||
data_to_cache = rewritten.clone();
|
||||
data_to_serve = rewritten;
|
||||
@@ -217,8 +216,7 @@ async fn refetch_metadata(state: &Arc<AppState>, path: &str, key: &str) -> Optio
|
||||
.ok()?;
|
||||
|
||||
let nora_base = nora_base_url(state);
|
||||
let rewritten =
|
||||
rewrite_tarball_urls(&data, &nora_base, proxy_url).unwrap_or_else(|_| data.clone());
|
||||
let rewritten = rewrite_tarball_urls(&data, &nora_base, proxy_url).unwrap_or(data);
|
||||
|
||||
let storage = state.storage.clone();
|
||||
let key_clone = key.to_string();
|
||||
@@ -346,7 +344,9 @@ async fn handle_publish(
|
||||
}
|
||||
|
||||
// Merge versions
|
||||
let meta_obj = metadata.as_object_mut().unwrap();
|
||||
let Some(meta_obj) = metadata.as_object_mut() else {
|
||||
return (StatusCode::INTERNAL_SERVER_ERROR, "invalid metadata format").into_response();
|
||||
};
|
||||
let stored_versions = meta_obj.entry("versions").or_insert(serde_json::json!({}));
|
||||
if let Some(sv) = stored_versions.as_object_mut() {
|
||||
for (ver, ver_data) in new_versions {
|
||||
|
||||
@@ -178,7 +178,12 @@ pub fn validate_docker_name(name: &str) -> Result<(), ValidationError> {
|
||||
"empty path segment".to_string(),
|
||||
));
|
||||
}
|
||||
let first = segment.chars().next().unwrap();
|
||||
// Safety: segment.is_empty() checked above, but use match for defense-in-depth
|
||||
let Some(first) = segment.chars().next() else {
|
||||
return Err(ValidationError::InvalidDockerName(
|
||||
"empty path segment".to_string(),
|
||||
));
|
||||
};
|
||||
if !first.is_ascii_alphanumeric() {
|
||||
return Err(ValidationError::InvalidDockerName(
|
||||
"segment must start with alphanumeric".to_string(),
|
||||
@@ -292,7 +297,10 @@ pub fn validate_docker_reference(reference: &str) -> Result<(), ValidationError>
|
||||
}
|
||||
|
||||
// Validate as tag
|
||||
let first = reference.chars().next().unwrap();
|
||||
// Safety: empty check at function start, but use let-else for defense-in-depth
|
||||
let Some(first) = reference.chars().next() else {
|
||||
return Err(ValidationError::EmptyInput);
|
||||
};
|
||||
if !first.is_ascii_alphanumeric() {
|
||||
return Err(ValidationError::InvalidReference(
|
||||
"tag must start with alphanumeric".to_string(),
|
||||
|
||||
Reference in New Issue
Block a user