feat(v1.1.3-modules): reject module scripts from routes + triggers; tighten cross-app trigger check
- `POST /api/v1/admin/scripts/{id}/routes` returns 400 when the
target script is `kind=module`. Modules have no entry point — they
are imported, not invoked.
- `POST /api/v1/admin/apps/{id}/triggers/{kv,docs,dead_letter}` gain
a shared `validate_trigger_target` that loads the target script
and rejects when:
- the script doesn't exist
- the script belongs to a different app (latent v1.1.1/v1.1.2 gap
where triggers could target a script in any app — closed here)
- the script is `kind=module`
- `TriggersState` grows a `scripts: Arc<dyn ScriptRepository>` field
so handlers can load the target script.
- Trigger-create test helpers split into `state_with` (empty script
repo — for tests asserting upstream errors) and
`state_with_endpoint` (pre-populated — for tests asserting
successful creation). `InMemoryScriptRepo` added to the test
module.
Workspace builds; full test suite (~440 tests) green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -184,6 +184,17 @@ async fn create_route<RR: RouteRepository, SR: ScriptRepository>(
|
|||||||
)
|
)
|
||||||
.await?;
|
.await?;
|
||||||
|
|
||||||
|
// v1.1.3: module scripts have no executable entry point — they're
|
||||||
|
// libraries imported by other scripts. Reject route bindings here
|
||||||
|
// before we touch the routes table.
|
||||||
|
if script.kind == picloud_shared::ScriptKind::Module {
|
||||||
|
return Err(RouteApiError::BadRequest(format!(
|
||||||
|
"script {script_id} has kind=module; modules are imported, \
|
||||||
|
not bound to routes — switch the script to kind=endpoint \
|
||||||
|
or attach this route to a different script"
|
||||||
|
)));
|
||||||
|
}
|
||||||
|
|
||||||
// Validate the route's host is consistent with one of the app's
|
// Validate the route's host is consistent with one of the app's
|
||||||
// domain claims. `HostKind::Any` is always permitted (catches every
|
// domain claims. `HostKind::Any` is always permitted (catches every
|
||||||
// host the app already owns). Specific hosts must match a claim.
|
// host the app already owns). Specific hosts must match a claim.
|
||||||
|
|||||||
@@ -16,12 +16,13 @@ use axum::http::StatusCode;
|
|||||||
use axum::response::{IntoResponse, Json, Response};
|
use axum::response::{IntoResponse, Json, Response};
|
||||||
use axum::routing::{delete, get, post};
|
use axum::routing::{delete, get, post};
|
||||||
use axum::{Extension, Router};
|
use axum::{Extension, Router};
|
||||||
use picloud_shared::{AppId, DocsEventOp, KvEventOp, Principal, ScriptId, TriggerId};
|
use picloud_shared::{AppId, DocsEventOp, KvEventOp, Principal, ScriptId, ScriptKind, TriggerId};
|
||||||
use serde::{Deserialize, Serialize};
|
use serde::{Deserialize, Serialize};
|
||||||
use serde_json::json;
|
use serde_json::json;
|
||||||
|
|
||||||
use crate::app_repo::AppRepository;
|
use crate::app_repo::AppRepository;
|
||||||
use crate::authz::{require, AuthzDenied, AuthzError, AuthzRepo, Capability};
|
use crate::authz::{require, AuthzDenied, AuthzError, AuthzRepo, Capability};
|
||||||
|
use crate::repo::{ScriptRepository, ScriptRepositoryError};
|
||||||
use crate::trigger_config::{BackoffShape, TriggerConfig};
|
use crate::trigger_config::{BackoffShape, TriggerConfig};
|
||||||
use crate::trigger_repo::{
|
use crate::trigger_repo::{
|
||||||
CreateDeadLetterTrigger, CreateDocsTrigger, CreateKvTrigger, Trigger, TriggerDispatchMode,
|
CreateDeadLetterTrigger, CreateDocsTrigger, CreateKvTrigger, Trigger, TriggerDispatchMode,
|
||||||
@@ -33,6 +34,11 @@ pub struct TriggersState {
|
|||||||
pub triggers: Arc<dyn TriggerRepo>,
|
pub triggers: Arc<dyn TriggerRepo>,
|
||||||
pub apps: Arc<dyn AppRepository>,
|
pub apps: Arc<dyn AppRepository>,
|
||||||
pub authz: Arc<dyn AuthzRepo>,
|
pub authz: Arc<dyn AuthzRepo>,
|
||||||
|
/// v1.1.3: trigger creation must verify the target script (1) exists,
|
||||||
|
/// (2) belongs to this app, and (3) is `kind = endpoint` — modules
|
||||||
|
/// cannot be invoked. The script-load lives in the handler, so the
|
||||||
|
/// state needs a repo handle.
|
||||||
|
pub scripts: Arc<dyn ScriptRepository>,
|
||||||
/// Defaults applied to created triggers when the request omits
|
/// Defaults applied to created triggers when the request omits
|
||||||
/// retry settings. Kept on the state struct so tests can swap
|
/// retry settings. Kept on the state struct so tests can swap
|
||||||
/// in a stricter / looser config without env tinkering.
|
/// in a stricter / looser config without env tinkering.
|
||||||
@@ -146,6 +152,44 @@ async fn list_triggers(
|
|||||||
Ok(Json(TriggerListResponse { triggers }))
|
Ok(Json(TriggerListResponse { triggers }))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// v1.1.3: shared check used by every trigger-create handler. Returns
|
||||||
|
/// `Ok(())` when the target script exists, lives in the same app, and
|
||||||
|
/// is `kind = endpoint`. Wrong app surfaces as 422 (not 404) so we
|
||||||
|
/// don't leak whether a script id exists in some other app.
|
||||||
|
async fn validate_trigger_target(
|
||||||
|
scripts: &dyn ScriptRepository,
|
||||||
|
app_id: AppId,
|
||||||
|
script_id: ScriptId,
|
||||||
|
) -> Result<(), TriggersApiError> {
|
||||||
|
let script = scripts
|
||||||
|
.get(script_id)
|
||||||
|
.await
|
||||||
|
.map_err(map_script_repo_err)?
|
||||||
|
.ok_or_else(|| {
|
||||||
|
TriggersApiError::Invalid(format!("script {script_id} not found in this app"))
|
||||||
|
})?;
|
||||||
|
if script.app_id != app_id {
|
||||||
|
return Err(TriggersApiError::Invalid(format!(
|
||||||
|
"script {script_id} does not belong to this app"
|
||||||
|
)));
|
||||||
|
}
|
||||||
|
if script.kind == ScriptKind::Module {
|
||||||
|
return Err(TriggersApiError::Invalid(format!(
|
||||||
|
"script {script_id} has kind=module; modules cannot be trigger targets — \
|
||||||
|
switch the script to kind=endpoint or attach this trigger to a different script"
|
||||||
|
)));
|
||||||
|
}
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
|
fn map_script_repo_err(e: ScriptRepositoryError) -> TriggersApiError {
|
||||||
|
// Surface as Invalid so the wire shape (422 with `error` field)
|
||||||
|
// stays consistent with the other trigger-validation failures.
|
||||||
|
// The underlying DB error is still logged through the manager's
|
||||||
|
// tracing instrumentation.
|
||||||
|
TriggersApiError::Invalid(format!("script lookup failed: {e}"))
|
||||||
|
}
|
||||||
|
|
||||||
async fn create_kv_trigger(
|
async fn create_kv_trigger(
|
||||||
State(s): State<TriggersState>,
|
State(s): State<TriggersState>,
|
||||||
Extension(principal): Extension<Principal>,
|
Extension(principal): Extension<Principal>,
|
||||||
@@ -165,6 +209,7 @@ async fn create_kv_trigger(
|
|||||||
"collection_glob must not be empty".into(),
|
"collection_glob must not be empty".into(),
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
validate_trigger_target(&*s.scripts, app_id, input.script_id).await?;
|
||||||
|
|
||||||
let req = CreateKvTrigger {
|
let req = CreateKvTrigger {
|
||||||
script_id: input.script_id,
|
script_id: input.script_id,
|
||||||
@@ -201,6 +246,7 @@ async fn create_docs_trigger(
|
|||||||
"collection_glob must not be empty".into(),
|
"collection_glob must not be empty".into(),
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
validate_trigger_target(&*s.scripts, app_id, input.script_id).await?;
|
||||||
|
|
||||||
let req = CreateDocsTrigger {
|
let req = CreateDocsTrigger {
|
||||||
script_id: input.script_id,
|
script_id: input.script_id,
|
||||||
@@ -231,6 +277,7 @@ async fn create_dl_trigger(
|
|||||||
Capability::AppManageTriggers(app_id),
|
Capability::AppManageTriggers(app_id),
|
||||||
)
|
)
|
||||||
.await?;
|
.await?;
|
||||||
|
validate_trigger_target(&*s.scripts, app_id, input.script_id).await?;
|
||||||
let req = CreateDeadLetterTrigger {
|
let req = CreateDeadLetterTrigger {
|
||||||
script_id: input.script_id,
|
script_id: input.script_id,
|
||||||
source_filter: input.source_filter,
|
source_filter: input.source_filter,
|
||||||
@@ -628,6 +675,123 @@ mod tests {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Minimal `ScriptRepository` impl backing the trigger-create
|
||||||
|
/// handler's `validate_trigger_target` check. Tests insert one or
|
||||||
|
/// more scripts via [`InMemoryScriptRepo::with_endpoint`] /
|
||||||
|
/// [`with_module`] and pass it into `TriggersState`.
|
||||||
|
struct InMemoryScriptRepo {
|
||||||
|
existing: Mutex<HashMap<ScriptId, picloud_shared::Script>>,
|
||||||
|
}
|
||||||
|
|
||||||
|
impl InMemoryScriptRepo {
|
||||||
|
fn empty() -> Arc<Self> {
|
||||||
|
Arc::new(Self {
|
||||||
|
existing: Mutex::new(HashMap::new()),
|
||||||
|
})
|
||||||
|
}
|
||||||
|
fn with_endpoint(app_id: AppId, script_id: ScriptId) -> Arc<Self> {
|
||||||
|
Self::with(app_id, script_id, ScriptKind::Endpoint)
|
||||||
|
}
|
||||||
|
fn with_module(app_id: AppId, script_id: ScriptId) -> Arc<Self> {
|
||||||
|
Self::with(app_id, script_id, ScriptKind::Module)
|
||||||
|
}
|
||||||
|
fn with(app_id: AppId, script_id: ScriptId, kind: ScriptKind) -> Arc<Self> {
|
||||||
|
let now = Utc::now();
|
||||||
|
let mut existing = HashMap::new();
|
||||||
|
existing.insert(
|
||||||
|
script_id,
|
||||||
|
picloud_shared::Script {
|
||||||
|
id: script_id,
|
||||||
|
app_id,
|
||||||
|
name: format!(
|
||||||
|
"{}_{}",
|
||||||
|
match kind {
|
||||||
|
ScriptKind::Endpoint => "endpoint",
|
||||||
|
ScriptKind::Module => "module",
|
||||||
|
},
|
||||||
|
script_id
|
||||||
|
),
|
||||||
|
description: None,
|
||||||
|
version: 1,
|
||||||
|
source: String::new(),
|
||||||
|
kind,
|
||||||
|
timeout_seconds: 30,
|
||||||
|
sandbox: Default::default(),
|
||||||
|
memory_limit_mb: 256,
|
||||||
|
created_at: now,
|
||||||
|
updated_at: now,
|
||||||
|
},
|
||||||
|
);
|
||||||
|
Arc::new(Self {
|
||||||
|
existing: Mutex::new(existing),
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[async_trait]
|
||||||
|
impl ScriptRepository for InMemoryScriptRepo {
|
||||||
|
async fn get(
|
||||||
|
&self,
|
||||||
|
id: ScriptId,
|
||||||
|
) -> Result<Option<picloud_shared::Script>, crate::repo::ScriptRepositoryError> {
|
||||||
|
Ok(self.existing.lock().await.get(&id).cloned())
|
||||||
|
}
|
||||||
|
async fn list(
|
||||||
|
&self,
|
||||||
|
) -> Result<Vec<picloud_shared::Script>, crate::repo::ScriptRepositoryError> {
|
||||||
|
Ok(self.existing.lock().await.values().cloned().collect())
|
||||||
|
}
|
||||||
|
async fn list_for_app(
|
||||||
|
&self,
|
||||||
|
_app_id: AppId,
|
||||||
|
) -> Result<Vec<picloud_shared::Script>, crate::repo::ScriptRepositoryError> {
|
||||||
|
unimplemented!()
|
||||||
|
}
|
||||||
|
async fn list_for_user(
|
||||||
|
&self,
|
||||||
|
_user_id: AdminUserId,
|
||||||
|
) -> Result<Vec<picloud_shared::Script>, crate::repo::ScriptRepositoryError> {
|
||||||
|
unimplemented!()
|
||||||
|
}
|
||||||
|
async fn create(
|
||||||
|
&self,
|
||||||
|
_input: crate::repo::NewScript,
|
||||||
|
) -> Result<picloud_shared::Script, crate::repo::ScriptRepositoryError> {
|
||||||
|
unimplemented!()
|
||||||
|
}
|
||||||
|
async fn update(
|
||||||
|
&self,
|
||||||
|
_id: ScriptId,
|
||||||
|
_patch: crate::repo::ScriptPatch,
|
||||||
|
) -> Result<picloud_shared::Script, crate::repo::ScriptRepositoryError> {
|
||||||
|
unimplemented!()
|
||||||
|
}
|
||||||
|
async fn delete(
|
||||||
|
&self,
|
||||||
|
_id: ScriptId,
|
||||||
|
) -> Result<(), crate::repo::ScriptRepositoryError> {
|
||||||
|
unimplemented!()
|
||||||
|
}
|
||||||
|
async fn count_routes_for_script(
|
||||||
|
&self,
|
||||||
|
_script_id: ScriptId,
|
||||||
|
) -> Result<i64, crate::repo::ScriptRepositoryError> {
|
||||||
|
Ok(0)
|
||||||
|
}
|
||||||
|
async fn count_triggers_for_script(
|
||||||
|
&self,
|
||||||
|
_script_id: ScriptId,
|
||||||
|
) -> Result<i64, crate::repo::ScriptRepositoryError> {
|
||||||
|
Ok(0)
|
||||||
|
}
|
||||||
|
async fn list_imports(
|
||||||
|
&self,
|
||||||
|
_script_id: ScriptId,
|
||||||
|
) -> Result<Vec<picloud_shared::Script>, crate::repo::ScriptRepositoryError> {
|
||||||
|
Ok(vec![])
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
struct AlwaysAllowAuthzRepo;
|
struct AlwaysAllowAuthzRepo;
|
||||||
#[async_trait]
|
#[async_trait]
|
||||||
impl AuthzRepo for AlwaysAllowAuthzRepo {
|
impl AuthzRepo for AlwaysAllowAuthzRepo {
|
||||||
@@ -666,6 +830,24 @@ mod tests {
|
|||||||
triggers: Arc::new(InMemoryTriggerRepo::default()),
|
triggers: Arc::new(InMemoryTriggerRepo::default()),
|
||||||
apps: InMemoryAppRepo::with(app_id),
|
apps: InMemoryAppRepo::with(app_id),
|
||||||
authz,
|
authz,
|
||||||
|
scripts: InMemoryScriptRepo::empty(),
|
||||||
|
config: TriggerConfig::conservative(),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Like [`state_with`] but pre-populates the script repo with a
|
||||||
|
/// single endpoint script (so the v1.1.3 `validate_trigger_target`
|
||||||
|
/// check passes and tests can exercise downstream behavior).
|
||||||
|
fn state_with_endpoint(
|
||||||
|
authz: Arc<dyn AuthzRepo>,
|
||||||
|
app_id: AppId,
|
||||||
|
script_id: ScriptId,
|
||||||
|
) -> TriggersState {
|
||||||
|
TriggersState {
|
||||||
|
triggers: Arc::new(InMemoryTriggerRepo::default()),
|
||||||
|
apps: InMemoryAppRepo::with(app_id),
|
||||||
|
authz,
|
||||||
|
scripts: InMemoryScriptRepo::with_endpoint(app_id, script_id),
|
||||||
config: TriggerConfig::conservative(),
|
config: TriggerConfig::conservative(),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -718,7 +900,8 @@ mod tests {
|
|||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn kv_trigger_uses_env_defaults_when_omitted() {
|
async fn kv_trigger_uses_env_defaults_when_omitted() {
|
||||||
let app_id = AppId::new();
|
let app_id = AppId::new();
|
||||||
let mut state = state_with(Arc::new(AlwaysAllowAuthzRepo), app_id);
|
let script_id = ScriptId::new();
|
||||||
|
let mut state = state_with_endpoint(Arc::new(AlwaysAllowAuthzRepo), app_id, script_id);
|
||||||
// Tweak the config so we can detect that defaults were used.
|
// Tweak the config so we can detect that defaults were used.
|
||||||
state.config.retry_max_attempts = 7;
|
state.config.retry_max_attempts = 7;
|
||||||
state.config.retry_base_ms = 12_345;
|
state.config.retry_base_ms = 12_345;
|
||||||
@@ -727,7 +910,7 @@ mod tests {
|
|||||||
Extension(member_principal()),
|
Extension(member_principal()),
|
||||||
Path(app_id),
|
Path(app_id),
|
||||||
Json(CreateKvTriggerRequest {
|
Json(CreateKvTriggerRequest {
|
||||||
script_id: ScriptId::new(),
|
script_id,
|
||||||
collection_glob: "widgets".into(),
|
collection_glob: "widgets".into(),
|
||||||
ops: vec![KvEventOp::Insert],
|
ops: vec![KvEventOp::Insert],
|
||||||
dispatch_mode: TriggerDispatchMode::Async,
|
dispatch_mode: TriggerDispatchMode::Async,
|
||||||
@@ -769,13 +952,14 @@ mod tests {
|
|||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn docs_trigger_create_succeeds() {
|
async fn docs_trigger_create_succeeds() {
|
||||||
let app_id = AppId::new();
|
let app_id = AppId::new();
|
||||||
let state = state_with(Arc::new(AlwaysAllowAuthzRepo), app_id);
|
let script_id = ScriptId::new();
|
||||||
|
let state = state_with_endpoint(Arc::new(AlwaysAllowAuthzRepo), app_id, script_id);
|
||||||
let (status, Json(trigger)) = create_docs_trigger(
|
let (status, Json(trigger)) = create_docs_trigger(
|
||||||
State(state),
|
State(state),
|
||||||
Extension(member_principal()),
|
Extension(member_principal()),
|
||||||
Path(app_id),
|
Path(app_id),
|
||||||
Json(CreateDocsTriggerRequest {
|
Json(CreateDocsTriggerRequest {
|
||||||
script_id: ScriptId::new(),
|
script_id,
|
||||||
collection_glob: "users".into(),
|
collection_glob: "users".into(),
|
||||||
ops: vec![DocsEventOp::Create, DocsEventOp::Update],
|
ops: vec![DocsEventOp::Create, DocsEventOp::Update],
|
||||||
dispatch_mode: TriggerDispatchMode::Async,
|
dispatch_mode: TriggerDispatchMode::Async,
|
||||||
|
|||||||
@@ -217,7 +217,7 @@ pub async fn build_app(pool: PgPool, auth: AuthDeps) -> anyhow::Result<Router> {
|
|||||||
};
|
};
|
||||||
let route_admin = RouteAdminState {
|
let route_admin = RouteAdminState {
|
||||||
routes: route_repo.clone(),
|
routes: route_repo.clone(),
|
||||||
scripts: Arc::new(PostgresScriptRepoHandle(script_repo)),
|
scripts: Arc::new(PostgresScriptRepoHandle(script_repo.clone())),
|
||||||
domains: domains_repo.clone(),
|
domains: domains_repo.clone(),
|
||||||
table: route_table.clone(),
|
table: route_table.clone(),
|
||||||
authz: authz.clone(),
|
authz: authz.clone(),
|
||||||
@@ -245,6 +245,7 @@ pub async fn build_app(pool: PgPool, auth: AuthDeps) -> anyhow::Result<Router> {
|
|||||||
triggers: trigger_repo,
|
triggers: trigger_repo,
|
||||||
apps: apps_repo.clone(),
|
apps: apps_repo.clone(),
|
||||||
authz: authz.clone(),
|
authz: authz.clone(),
|
||||||
|
scripts: Arc::new(PostgresScriptRepoHandle(script_repo.clone())),
|
||||||
config: trigger_config,
|
config: trigger_config,
|
||||||
};
|
};
|
||||||
let dead_letters_state = DeadLettersState {
|
let dead_letters_state = DeadLettersState {
|
||||||
|
|||||||
Reference in New Issue
Block a user