1.2: Sandboxing #7

Open
qpismont wants to merge 7 commits from 1.2 into main
24 changed files with 330 additions and 57 deletions
+6 -6
View File
@@ -1,4 +1,4 @@
FROM debian:trixie FROM rust:1.97-trixie
ARG USERNAME=dev ARG USERNAME=dev
ARG USER_UID=1000 ARG USER_UID=1000
@@ -18,11 +18,11 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
&& rm -rf /var/lib/apt/lists/* && rm -rf /var/lib/apt/lists/*
RUN groupadd --gid ${USER_GID:-1000} $USERNAME \ RUN groupadd --gid ${USER_GID:-1000} $USERNAME \
&& useradd --uid ${USER_UID:-1000} --gid ${USER_GID:-1000} -m $USERNAME && useradd --uid ${USER_UID:-1000} --gid ${USER_GID:-1000} -m $USERNAME \
&& rustup component add clippy \
&& rustup component add rustfmt
USER $USERNAME USER $USERNAME
WORKDIR /home/$USERNAME WORKDIR /home/$USERNAME
ENV PATH="/home/${USERNAME}/.cargo/bin:${PATH}"
RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --default-toolchain stable
+1
View File
@@ -19,5 +19,6 @@
}, },
"workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/herald,type=bind", "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/herald,type=bind",
"workspaceFolder": "/workspaces/herald", "workspaceFolder": "/workspaces/herald",
"runArgs": ["--userns=keep-id", "--security-opt", "label=disable"],
qpismont marked this conversation as resolved
Review

L'option --security-opt label=disable désactive la sécurité SELinux, ce qui réduit l'isolation du conteneur. Pour un environnement de développement, cela peut être acceptable, mais il faut être conscient des implications de sécurité. Envisagez d'utiliser --security-opt label=type:container_runtime_t ou d'autres options moins permissives si possible.

L'option `--security-opt label=disable` désactive la sécurité SELinux, ce qui réduit l'isolation du conteneur. Pour un environnement de développement, cela peut être acceptable, mais il faut être conscient des implications de sécurité. Envisagez d'utiliser `--security-opt label=type:container_runtime_t` ou d'autres options moins permissives si possible.
"appPort": [3000] "appPort": [3000]
} }
-4
View File
@@ -1,4 +0,0 @@
target/
.env
.devcontainer/
docs/
-3
View File
@@ -1,3 +0,0 @@
{
"rust-analyzer.check.command": "clippy"
}
+18 -4
View File
@@ -3,13 +3,27 @@ when:
- push - push
steps: steps:
- name: fmt
image: rust:1.97
commands:
- rustup component add rustfmt
- cargo fmt --all -- --check
- name: clippy - name: clippy
image: rust:1.96 image: rust:1.97
commands: commands:
- rustup component add clippy - rustup component add clippy
- cargo clippy - cargo clippy --workspace --all-targets --all-features -- -D warnings
- name: test - name: test
image: rust:1.96 image: rust:1.97
commands: commands:
- cargo test - cargo test --workspace --all-targets
- name: container-build
image: quay.io/buildah/stable
privileged: true
volumes:
- /data/woodpecker-builds:/data
commands:
- buildah bud -f Containerfile -t herald-ci .
+22
View File
@@ -0,0 +1,22 @@
{
"languages": {
"Rust": {
"format_on_save": "on",
"formatter": "language_server"
}
},
"lsp": {
"rust-analyzer": {
"initialization_options": {
"check": {
"command": "clippy",
"extraArgs": [
"--",
"-D",
"warnings"
]
}
}
}
}
}
Generated
+15 -2
View File
@@ -490,6 +490,18 @@ dependencies = [
"unicode-xid", "unicode-xid",
] ]
[[package]]
name = "devcontainer-rs"
version = "0.1.0"
dependencies = [
"anyhow",
"serde",
"serde_json",
"tempfile",
"thiserror 2.0.18",
"tokio",
]
[[package]] [[package]]
name = "dispatch2" name = "dispatch2"
version = "0.3.1" version = "0.3.1"
@@ -787,12 +799,13 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "ed5909b6e89a2db4456e54cd5f673791d7eca6732202bbf2a9cc504fe2f9b84a" checksum = "ed5909b6e89a2db4456e54cd5f673791d7eca6732202bbf2a9cc504fe2f9b84a"
[[package]] [[package]]
name = "herald" name = "herald-server"
version = "1.1.0" version = "1.2.0"
dependencies = [ dependencies = [
"anyhow", "anyhow",
"axum", "axum",
"bytes", "bytes",
"devcontainer-rs",
"dotenvy", "dotenvy",
"futures-util", "futures-util",
"hex", "hex",
+10 -8
View File
@@ -1,12 +1,11 @@
[package] [workspace]
name = "herald" members = [
version = "1.1.0" "crates/herald-server",
edition = "2024" "crates/devcontainer-rs",
]
resolver = "3"
[profile.release] [workspace.dependencies]
debug = 1
[dependencies]
reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] } reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] }
tokio = { version = "1.53", features = ["full"] } tokio = { version = "1.53", features = ["full"] }
tokio-stream = "0.1" tokio-stream = "0.1"
@@ -30,3 +29,6 @@ hex = "0.4"
bytes = "1.1" bytes = "1.1"
metrics = "0.24" metrics = "0.24"
metrics-exporter-prometheus = { version = "0.18", default-features = false, features = ["http-listener"] } metrics-exporter-prometheus = { version = "0.18", default-features = false, features = ["http-listener"] }
[profile.release]
debug = 1
+8 -5
View File
@@ -1,12 +1,15 @@
FROM rust:1.96 as builder FROM rust:1.97-trixie as builder
WORKDIR /app WORKDIR /app
COPY . .
RUN cargo build --release COPY Cargo.toml Cargo.lock ./
COPY crates/ crates/
RUN cargo build --release --package herald-server
FROM debian:trixie-slim FROM debian:trixie-slim
WORKDIR /app WORKDIR /app
COPY --from=builder /app/target/release/herald . COPY --from=builder /app/target/release/herald-server .
CMD [ "./herald" ] CMD [ "./herald-server" ]
+14
View File
@@ -0,0 +1,14 @@
[package]
name = "devcontainer-rs"
version = "0.1.0"
edition = "2024"
[dependencies]
tokio = { workspace = true }
serde = { workspace = true }
serde_json = { workspace = true }
anyhow = { workspace = true }
thiserror = { workspace = true }
[dev-dependencies]
tempfile = "3"
+152
View File
@@ -0,0 +1,152 @@
use std::{
collections::HashMap,
path::{Path, PathBuf},
};
use serde::Deserialize;
#[derive(Debug, Deserialize)]
pub struct DevContainerBuildSchema {
#[serde(default)]
pub dockerfile: String,
#[serde(default)]
pub args: HashMap<String, String>,
}
#[derive(Debug, Deserialize)]
pub struct DevContainerSchema {
qpismont marked this conversation as resolved
Review

Le champ build est obligatoire, mais le schéma standard devcontainer.json permet également d'utiliser image à la place de build. Si quelqu'un utilise image, la désérialisation échouera. Envisagez de rendre build optionnel avec #[serde(default)] et de gérer le cas où ni build ni image ne sont présents.

Le champ `build` est obligatoire, mais le schéma standard devcontainer.json permet également d'utiliser `image` à la place de `build`. Si quelqu'un utilise `image`, la désérialisation échouera. Envisagez de rendre `build` optionnel avec `#[serde(default)]` et de gérer le cas où ni `build` ni `image` ne sont présents.
#[serde(default)]
pub name: Option<String>,
pub build: DevContainerBuildSchema,
#[serde(rename = "workspaceFolder", default)]
pub workspace_folder: Option<String>,
#[serde(rename = "containerEnv", default)]
pub container_env: HashMap<String, String>,
#[serde(rename = "postCreateCommand", default)]
pub post_create_command: Option<String>,
#[serde(rename = "postStartCommand", default)]
pub post_start_command: Option<String>,
}
#[derive(Debug)]
pub struct DevContainer {
pub container_file_path: PathBuf,
pub name: Option<String>,
pub build_args: HashMap<String, String>,
pub container_env: HashMap<String, String>,
pub workspace_folder: Option<String>,
pub post_create_command: Option<String>,
pub post_start_command: Option<String>,
}
#[derive(Debug, thiserror::Error)]
pub enum ParseError {
#[error("failed to read devcontainer file `{path}`: {source}")]
Read {
path: PathBuf,
source: std::io::Error,
},
#[error("invalid devcontainer JSON in `{path}`: {source}")]
Json {
path: PathBuf,
source: serde_json::Error,
},
#[error("container file `{0}` does not exist or is not a regular file")]
ContainerFileNotFound(PathBuf),
#[error("the devcontainer file path has no parent directory: `{0}`")]
InvalidDevContainerPath(PathBuf),
}
impl TryFrom<(DevContainerSchema, PathBuf)> for DevContainer {
type Error = ParseError;
fn try_from(
(schema, devcontainer_path): (DevContainerSchema, PathBuf),
) -> Result<Self, Self::Error> {
let base_dir = devcontainer_path
.parent()
.ok_or_else(|| ParseError::InvalidDevContainerPath(devcontainer_path.clone()))?;
let container_file_path = base_dir.join(schema.build.dockerfile);
if !container_file_path.is_file() {
return Err(ParseError::ContainerFileNotFound(container_file_path));
}
Ok(Self {
container_file_path,
name: schema.name,
build_args: schema.build.args,
container_env: schema.container_env,
workspace_folder: schema.workspace_folder,
post_create_command: schema.post_create_command,
post_start_command: schema.post_start_command,
})
}
}
pub async fn parse(path: impl AsRef<Path>) -> Result<DevContainer, ParseError> {
let path = path.as_ref().to_path_buf();
let contents = tokio::fs::read_to_string(&path)
qpismont marked this conversation as resolved
Review

Si aucun fichier Dockerfile ou Containerfile n'existe, la fonction retourne base_dir.join("Dockerfile"), ce qui échouera plus tard avec ContainerFileNotFound. Cela pourrait être plus explicite en retournant une Option<PathBuf> et en laissant l'appelant gérer l'absence de fichier. Sinon, le message d'erreur actuel est suffisant.

Si aucun fichier `Dockerfile` ou `Containerfile` n'existe, la fonction retourne `base_dir.join("Dockerfile")`, ce qui échouera plus tard avec `ContainerFileNotFound`. Cela pourrait être plus explicite en retournant une `Option<PathBuf>` et en laissant l'appelant gérer l'absence de fichier. Sinon, le message d'erreur actuel est suffisant.
.await
.map_err(|source| ParseError::Read {
path: path.clone(),
source,
})?;
let schema = serde_json::from_str::<DevContainerSchema>(&contents).map_err(|source| {
ParseError::Json {
qpismont marked this conversation as resolved
Review

La fonction parse est asynchrone, mais les tests unitaires ne la testent pas directement (ils testent TryFrom). Il serait bon d'ajouter un test asynchrone pour parse afin de couvrir le flux complet (lecture du fichier, désérialisation, conversion).

La fonction `parse` est asynchrone, mais les tests unitaires ne la testent pas directement (ils testent `TryFrom`). Il serait bon d'ajouter un test asynchrone pour `parse` afin de couvrir le flux complet (lecture du fichier, désérialisation, conversion).
path: path.clone(),
source,
}
})?;
DevContainer::try_from((schema, path))
}
#[cfg(test)]
mod tests {
use super::*;
use std::fs;
#[tokio::test]
async fn parses_devcontainer_file() {
let dir = tempfile::tempdir().unwrap();
let devcontainer_path = dir.path().join("devcontainer.json");
let dockerfile_path = dir.path().join("Dockerfile");
fs::write(&dockerfile_path, "FROM alpine\n").unwrap();
fs::write(
&devcontainer_path,
r#"{
"name": "test",
"build": {
"dockerfile": "Dockerfile",
"args": {
"VERSION": "1"
}
},
"workspaceFolder": "/workspace",
"containerEnv": {
"RUST_LOG": "debug"
}
}"#,
)
.unwrap();
let config = parse(&devcontainer_path).await.unwrap();
assert_eq!(config.name.as_deref(), Some("test"));
assert_eq!(config.container_file_path, dockerfile_path);
assert_eq!(config.build_args.get("VERSION").unwrap(), "1");
assert_eq!(config.container_env.get("RUST_LOG").unwrap(), "debug");
assert_eq!(config.workspace_folder.as_deref(), Some("/workspace"));
}
}
+30
View File
@@ -0,0 +1,30 @@
[package]
name = "herald-server"
version = "1.2.0"
edition = "2024"
[dependencies]
reqwest = { workspace = true }
tokio = { workspace = true }
tokio-stream = { workspace = true }
tokio-util = { workspace = true }
futures-util = { workspace = true }
serde_json = { workspace = true }
serde = { workspace = true }
sentry = { workspace = true }
sentry-anyhow = { workspace = true }
openrouter-rs = { workspace = true }
dotenvy = { workspace = true }
tower = { workspace = true }
tower-http = { workspace = true }
tracing = { workspace = true }
tracing-subscriber = { workspace = true }
axum = { workspace = true }
anyhow = { workspace = true }
thiserror = { workspace = true }
ring = { workspace = true }
hex = { workspace = true }
bytes = { workspace = true }
metrics = { workspace = true }
metrics-exporter-prometheus = { workspace = true }
devcontainer-rs = { path = "../devcontainer-rs" }
@@ -1,5 +1,7 @@
use crate::{ use crate::{
gitea::{GiteaAPI, WebhookType}, metrics, open_router::OpenRouterClient, gitea::{GiteaAPI, WebhookType},
metrics,
open_router::OpenRouterClient,
}; };
use serde::Deserialize; use serde::Deserialize;
use std::{collections::HashSet, sync::Arc}; use std::{collections::HashSet, sync::Arc};
@@ -18,7 +20,6 @@ pub struct ReviewResult {
pub struct ReviewItem { pub struct ReviewItem {
pub filename: String, pub filename: String,
pub line: Option<u64>, pub line: Option<u64>,
pub code: String,
pub message: String, pub message: String,
} }
@@ -4,7 +4,11 @@ use tokio_util::io::StreamReader;
use tracing::instrument; use tracing::instrument;
use crate::{ use crate::{
bot::ReviewResult, consts::{BOT_PROCESS_MSG, MAX_DIFF_SIZE, REVIEW_PROMPT}, gitea::{GiteaAPI, ReviewPayload}, metrics, open_router::OpenRouterClient, bot::ReviewResult,
consts::{BOT_PROCESS_MSG, MAX_DIFF_SIZE, REVIEW_PROMPT},
gitea::{GiteaAPI, ReviewPayload},
metrics,
open_router::OpenRouterClient,
}; };
#[instrument(skip(gitea_api, open_router_client, http_client, review_payload))] #[instrument(skip(gitea_api, open_router_client, http_client, review_payload))]
@@ -197,7 +197,6 @@ pub struct ReviewPayload {
#[derive(Deserialize, Debug)] #[derive(Deserialize, Debug)]
pub struct PullRequest { pub struct PullRequest {
pub id: u64,
pub diff_url: String, pub diff_url: String,
pub number: u64, pub number: u64,
pub title: String, pub title: String,
@@ -207,12 +206,10 @@ pub struct PullRequest {
pub struct Comment { pub struct Comment {
pub id: u64, pub id: u64,
pub body: String, pub body: String,
pub user: User,
} }
#[derive(Deserialize, Debug)] #[derive(Deserialize, Debug)]
pub struct User { pub struct User {
pub id: u64,
pub login: String, pub login: String,
} }
@@ -282,10 +279,8 @@ mod tests {
match result.unwrap() { match result.unwrap() {
WebhookType::Review(payload) => { WebhookType::Review(payload) => {
assert_eq!(payload.action, "created"); assert_eq!(payload.action, "created");
assert_eq!(payload.pull_request.id, 42);
assert_eq!(payload.comment.id, 7); assert_eq!(payload.comment.id, 7);
assert_eq!(payload.comment.body, "@test_bot LGTM"); assert_eq!(payload.comment.body, "@test_bot LGTM");
assert_eq!(payload.comment.user.id, 100);
} }
} }
} }
@@ -375,10 +370,8 @@ mod tests {
let payload: ReviewPayload = serde_json::from_value(json).unwrap(); let payload: ReviewPayload = serde_json::from_value(json).unwrap();
assert_eq!(payload.action, "created"); assert_eq!(payload.action, "created");
assert_eq!(payload.pull_request.id, 99);
assert_eq!(payload.comment.id, 12); assert_eq!(payload.comment.id, 12);
assert_eq!(payload.comment.body, "Needs work"); assert_eq!(payload.comment.body, "Needs work");
assert_eq!(payload.comment.user.id, 200);
} }
#[test] #[test]
@@ -1,4 +1,9 @@
use crate::{bot::Bot, gitea::{GiteaAPI, WebhookType}, open_router::OpenRouterClient, state::AppState}; use crate::{
bot::Bot,
gitea::{GiteaAPI, WebhookType},
open_router::OpenRouterClient,
state::AppState,
};
use dotenvy::dotenv; use dotenvy::dotenv;
use tokio::signal::unix::{SignalKind, signal}; use tokio::signal::unix::{SignalKind, signal};
@@ -1,10 +1,9 @@
use std::{net::SocketAddr, str::FromStr}; use std::{net::SocketAddr, str::FromStr};
use metrics::{Unit, describe_counter, describe_gauge, counter, gauge}; use metrics::{Unit, counter, describe_counter, describe_gauge, gauge};
pub fn webhook_received(event_type: &str) { pub fn webhook_received(event_type: &str) {
counter!("herald_webhooks_received_total", "event_type" => event_type.to_string()) counter!("herald_webhooks_received_total", "event_type" => event_type.to_string()).increment(1);
.increment(1);
} }
pub fn webhook_duplicate(event_type: &str) { pub fn webhook_duplicate(event_type: &str) {
@@ -31,8 +30,7 @@ pub fn task_completed(event_type: &str) {
} }
pub fn task_failed(event_type: &str) { pub fn task_failed(event_type: &str) {
counter!("herald_bot_tasks_failed_total", "event_type" => event_type.to_string()) counter!("herald_bot_tasks_failed_total", "event_type" => event_type.to_string()).increment(1);
.increment(1);
} }
pub fn openrouter_cost_usd(cost: f64) { pub fn openrouter_cost_usd(cost: f64) {
@@ -40,13 +38,41 @@ pub fn openrouter_cost_usd(cost: f64) {
} }
pub fn describe() { pub fn describe() {
describe_counter!("herald_webhooks_received_total", Unit::Count, "Total webhooks received"); describe_counter!(
describe_counter!("herald_webhooks_duplicate_total", Unit::Count, "Webhooks rejected as duplicates"); "herald_webhooks_received_total",
describe_counter!("herald_webhooks_channel_full_total", Unit::Count, "Webhooks dropped because the bot channel was full"); Unit::Count,
describe_gauge!("herald_bot_tasks_active", Unit::Count, "Bot tasks currently in progress"); "Total webhooks received"
describe_counter!("herald_bot_tasks_completed_total", Unit::Count, "Bot tasks completed successfully"); );
describe_counter!("herald_bot_tasks_failed_total", Unit::Count, "Bot tasks that failed"); describe_counter!(
describe_counter!("herald_openrouter_cost_cents_total", Unit::Count, "Total OpenRouter cost in cents (divide by 100 for USD)"); "herald_webhooks_duplicate_total",
Unit::Count,
"Webhooks rejected as duplicates"
);
describe_counter!(
"herald_webhooks_channel_full_total",
Unit::Count,
"Webhooks dropped because the bot channel was full"
);
describe_gauge!(
"herald_bot_tasks_active",
Unit::Count,
"Bot tasks currently in progress"
);
describe_counter!(
"herald_bot_tasks_completed_total",
Unit::Count,
"Bot tasks completed successfully"
);
describe_counter!(
"herald_bot_tasks_failed_total",
Unit::Count,
"Bot tasks that failed"
);
describe_counter!(
"herald_openrouter_cost_cents_total",
Unit::Count,
"Total OpenRouter cost in cents (divide by 100 for USD)"
);
} }
pub fn install(bind_addr: &str) -> anyhow::Result<()> { pub fn install(bind_addr: &str) -> anyhow::Result<()> {