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 USER_UID=1000
@@ -18,11 +18,11 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
&& rm -rf /var/lib/apt/lists/*
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
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",
"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]
}
-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
steps:
- name: fmt
image: rust:1.97
commands:
- rustup component add rustfmt
- cargo fmt --all -- --check
- name: clippy
image: rust:1.96
image: rust:1.97
commands:
- rustup component add clippy
- cargo clippy
- cargo clippy --workspace --all-targets --all-features -- -D warnings
- name: test
image: rust:1.96
image: rust:1.97
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",
]
[[package]]
name = "devcontainer-rs"
version = "0.1.0"
dependencies = [
"anyhow",
"serde",
"serde_json",
"tempfile",
"thiserror 2.0.18",
"tokio",
]
[[package]]
name = "dispatch2"
version = "0.3.1"
@@ -787,12 +799,13 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "ed5909b6e89a2db4456e54cd5f673791d7eca6732202bbf2a9cc504fe2f9b84a"
[[package]]
name = "herald"
version = "1.1.0"
name = "herald-server"
version = "1.2.0"
dependencies = [
"anyhow",
"axum",
"bytes",
"devcontainer-rs",
"dotenvy",
"futures-util",
"hex",
+10 -8
View File
@@ -1,12 +1,11 @@
[package]
name = "herald"
version = "1.1.0"
edition = "2024"
[workspace]
members = [
"crates/herald-server",
"crates/devcontainer-rs",
]
resolver = "3"
[profile.release]
debug = 1
[dependencies]
[workspace.dependencies]
reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] }
tokio = { version = "1.53", features = ["full"] }
tokio-stream = "0.1"
@@ -30,3 +29,6 @@ hex = "0.4"
bytes = "1.1"
metrics = "0.24"
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
COPY . .
RUN cargo build --release
COPY Cargo.toml Cargo.lock ./
COPY crates/ crates/
RUN cargo build --release --package herald-server
FROM debian:trixie-slim
WORKDIR /app
COPY --from=builder /app/target/release/herald .
CMD [ "./herald" ]
COPY --from=builder /app/target/release/herald-server .
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": {
qpismont marked this conversation as resolved Outdated
Outdated
Review

Les tests sont synchrones et utilisent std::fs plutôt que tokio::fs. C'est acceptable pour l'instant, mais si l'implémentation venait à dépendre de fonctionnalités asynchrones (comme tokio::fs), les tests pourraient ne plus être représentatifs.

Les tests sont synchrones et utilisent `std::fs` plutôt que `tokio::fs`. C'est acceptable pour l'instant, mais si l'implémentation venait à dépendre de fonctionnalités asynchrones (comme `tokio::fs`), les tests pourraient ne plus être représentatifs.
"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::{
gitea::{GiteaAPI, WebhookType}, metrics, open_router::OpenRouterClient,
gitea::{GiteaAPI, WebhookType},
metrics,
open_router::OpenRouterClient,
};
use serde::Deserialize;
use std::{collections::HashSet, sync::Arc};
@@ -18,7 +20,6 @@ pub struct ReviewResult {
pub struct ReviewItem {
pub filename: String,
pub line: Option<u64>,
pub code: String,
pub message: String,
}
@@ -4,7 +4,11 @@ use tokio_util::io::StreamReader;
use tracing::instrument;
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))]
@@ -197,7 +197,6 @@ pub struct ReviewPayload {
#[derive(Deserialize, Debug)]
pub struct PullRequest {
pub id: u64,
pub diff_url: String,
pub number: u64,
pub title: String,
@@ -207,12 +206,10 @@ pub struct PullRequest {
pub struct Comment {
pub id: u64,
pub body: String,
pub user: User,
}
#[derive(Deserialize, Debug)]
pub struct User {
pub id: u64,
pub login: String,
}
@@ -282,10 +279,8 @@ mod tests {
match result.unwrap() {
WebhookType::Review(payload) => {
assert_eq!(payload.action, "created");
assert_eq!(payload.pull_request.id, 42);
assert_eq!(payload.comment.id, 7);
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();
assert_eq!(payload.action, "created");
assert_eq!(payload.pull_request.id, 99);
assert_eq!(payload.comment.id, 12);
assert_eq!(payload.comment.body, "Needs work");
assert_eq!(payload.comment.user.id, 200);
}
#[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 tokio::signal::unix::{SignalKind, signal};
@@ -1,10 +1,9 @@
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) {
counter!("herald_webhooks_received_total", "event_type" => event_type.to_string())
.increment(1);
counter!("herald_webhooks_received_total", "event_type" => event_type.to_string()).increment(1);
}
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) {
counter!("herald_bot_tasks_failed_total", "event_type" => event_type.to_string())
.increment(1);
counter!("herald_bot_tasks_failed_total", "event_type" => event_type.to_string()).increment(1);
}
pub fn openrouter_cost_usd(cost: f64) {
@@ -40,13 +38,41 @@ pub fn openrouter_cost_usd(cost: f64) {
}
pub fn describe() {
describe_counter!("herald_webhooks_received_total", Unit::Count, "Total webhooks received");
describe_counter!("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)");
describe_counter!(
"herald_webhooks_received_total",
Unit::Count,
"Total webhooks received"
);
describe_counter!(
"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<()> {