Skip to content
Merged
25 changes: 24 additions & 1 deletion apps/skit/src/permissions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,19 @@ impl Permissions {
"containers::*".to_string(),
// Transport: allow MoQ, deny HTTP fetcher by default (SSRF risk)
"transport::moq::*".to_string(),
// Core: explicitly allow safe-ish nodes; deny core::file_writer by default (arbitrary write risk)
// Transport HTTP: allow `mse` (serves over the caller's own request, no
// arbitrary-URL fetch) so least-privilege gateways can serve live casts
// without admin. `transport::http::fetcher` stays denied (SSRF risk).
// The oneshot `streamkit::http_input`/`http_output` markers are not gated by
// this allowlist (the oneshot path treats them as implicitly allowed), so
// they are intentionally not listed here.
"transport::http::mse".to_string(),
// Core: explicitly allow the safe nodes. Omitted on purpose:
// `core::file_writer` / `core::object_store_writer` (arbitrary/external write).
"core::passthrough".to_string(),
"core::file_reader".to_string(),
"core::pacer".to_string(),
"core::param_bridge".to_string(),
"core::json_serialize".to_string(),
"core::text_chunker".to_string(),
"core::script".to_string(),
Expand Down Expand Up @@ -511,6 +520,20 @@ mod tests {
assert!(user.is_node_allowed("plugin::wasm::gain_filter_rust"));
}

#[test]
fn test_default_user_http_and_core_node_policy() {
let user = Permissions::user();
// `mse` serves over the caller's own request and is safe to allow.
assert!(user.is_node_allowed("transport::http::mse"));
// Safe in-graph core nodes (no external side effects) are allowed.
assert!(user.is_node_allowed("core::param_bridge"));
// The HTTP fetcher stays denied (arbitrary-URL fetch / SSRF risk), and the
// write-capable core nodes stay denied (arbitrary / external write).
assert!(!user.is_node_allowed("transport::http::fetcher"));
assert!(!user.is_node_allowed("core::file_writer"));
assert!(!user.is_node_allowed("core::object_store_writer"));
}

#[test]
fn test_global_session_limits() {
let config = PermissionsConfig { max_concurrent_sessions: Some(10), ..Default::default() };
Expand Down
31 changes: 31 additions & 0 deletions apps/skit/tests/sample_config_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ fn samples_skit_toml_parses_and_matches_expected_defaults() {
assert!(config.permissions.roles.contains_key("admin"));
assert!(config.permissions.roles.contains_key("demo"));
assert!(config.permissions.roles.contains_key("user"));
assert!(config.permissions.roles.contains_key("gateway"));
assert!(config.permissions.roles.contains_key("readonly"));

let readonly = config.permissions.get_role("readonly");
Expand All @@ -58,5 +59,35 @@ fn samples_skit_toml_parses_and_matches_expected_defaults() {
assert!(!readonly.upload_assets);
assert!(!readonly.delete_assets);

// The user role mirrors Permissions::user(): the allowed_samples list is
// reachable (sample flags set), the HTTP `mse` sink is allowed, but the
// SSRF-risk fetcher and the write-capable core nodes stay denied.
let user = config.permissions.get_role("user");
assert!(user.list_samples && user.read_samples && user.write_samples && user.delete_samples);
assert!(user.is_node_allowed("transport::http::mse"));
assert!(user.is_node_allowed("core::param_bridge"));
assert!(!user.is_node_allowed("transport::http::fetcher"));
assert!(!user.is_node_allowed("core::file_writer"));
assert!(!user.is_node_allowed("core::object_store_writer"));
// Asset policy also mirrors the built-in: audio, images and fonts.
assert!(user.is_asset_allowed("samples/audio/system/beep.wav"));
assert!(user.is_asset_allowed("samples/images/system/logo.png"));
assert!(user.is_asset_allowed("samples/fonts/system/inter.ttf"));

// The gateway role is least-privilege but must actually be able to build the
// servo -> encode -> mux -> serve pipeline. Crucially the plugin kind has to
// pass is_node_allowed() (checked before the plugin allowlist), and the
// fetcher / file_writer stay denied.
let gateway = config.permissions.get_role("gateway");
assert!(gateway.create_sessions);
assert!(!gateway.load_plugins);
assert!(gateway.is_node_allowed("plugin::native::servo"));
assert!(gateway.is_plugin_allowed("plugin::native::servo"));
assert!(gateway.is_node_allowed("video::vp9::encoder"));
assert!(gateway.is_node_allowed("containers::webm::muxer"));
assert!(gateway.is_node_allowed("transport::http::mse"));
assert!(!gateway.is_node_allowed("transport::http::fetcher"));
assert!(!gateway.is_node_allowed("core::file_writer"));

assert!(config.script.global_fetch_allowlist.is_empty());
}
41 changes: 41 additions & 0 deletions docs/src/content/docs/guides/authorization.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,47 @@ allowed_assets = ["*"]
> [!NOTE]
> Role permissions are deny-by-default. If you define a custom role in `skit.toml`, any permission you omit defaults to `false`.

> [!NOTE]
> The built-in `user` role allows `transport::http::mse` (live-cast playback over the caller's own request) but **not** `transport::http::fetcher`, which can fetch arbitrary URLs (SSRF risk). The oneshot `streamkit::http_input` / `streamkit::http_output` markers are always permitted on the oneshot path regardless of `allowed_nodes`, so they need no allowlist entry. A trusted gateway that only serves or receives over the caller's own request therefore does not need `admin`.

## Example: Least-privilege gateway role

Trusted intermediaries (e.g. the `web-capture` or `speech-gateway` examples) build a small set of fixed pipelines and should run with a scoped token instead of `admin`. The role below grants exactly the node kinds the web-capture pipeline (`servo → encode → mux → serve`) needs and nothing more.

> [!IMPORTANT]
> A plugin must appear in **both** `allowed_nodes` and `allowed_plugins`. Enforcement calls `is_node_allowed(kind)` *before* the plugin check, so a plugin kind missing from `allowed_nodes` is rejected before `allowed_plugins` is ever consulted.

```toml
[permissions.roles.gateway]
create_sessions = true
destroy_sessions = true
modify_sessions = true
tune_nodes = true
list_sessions = true
list_nodes = true
access_all_sessions = false # Only its own sessions
load_plugins = false
delete_plugins = false
upload_assets = false
delete_assets = false
allowed_nodes = [
"plugin::native::servo", # render the page (web-capture)
"video::pixel_convert", # servo RGBA -> encoder input format
"video::vp9::encoder", # encode to VP9
"containers::webm::muxer", # mux into WebM for MSE / http_output
"transport::http::mse", # serve the live cast to the browser (MSE)
"core::pacer",
"core::sink",
# No core::file_writer (arbitrary-write risk) and no transport::http::fetcher (SSRF).
# The oneshot streamkit::http_output marker is implicitly allowed.
]
allowed_plugins = ["plugin::native::servo"] # must also be listed in allowed_nodes (see note above)
```

This role serves a **video-only** WebM cast. To also carry page audio (the `mse`
node advertises `codecs="vp9,opus"`), add the audio encoder — e.g.
`"audio::opus::encoder"` — to `allowed_nodes`.

## Permission reference

| Permission | Description |
Expand Down
82 changes: 74 additions & 8 deletions samples/skit.toml
Original file line number Diff line number Diff line change
Expand Up @@ -471,35 +471,101 @@ delete_plugins = false
list_nodes = true
access_all_sessions = false

# Users can access all samples except admin-only ones
# Sample management (mirrors the built-in Permissions::user()); without these
# flags the allowed_samples list below would be unreachable.
list_samples = true
read_samples = true
write_samples = true
delete_samples = true
Comment on lines +474 to +479

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: TOML user role now aligned with code, fixing previously-unreachable sample allowlist

Before this PR the samples/skit.toml user role omitted list_samples/read_samples/write_samples/delete_samples, which default to false. Because a TOML-defined role fully replaces the built-in Permissions::user(), the allowed_samples list was effectively unreachable for the sample config's user role. Adding these flags (samples/skit.toml:476-479) corrects that and brings the TOML in line with the code defaults in apps/skit/src/permissions.rs. This is a real fix, not a regression.

Open in Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct — this was the fix for the second validation finding. Without list/read/write/delete_samples the allowed_samples list was dead config in the sample's user role. Now the sample matches Permissions::user() and the list is reachable.


# Users can access standard samples (mirrors the built-in Permissions::user()).
allowed_samples = [
"oneshot/*.yml",
"oneshot/*.yaml",
"dynamic/*.yml",
"dynamic/*.yaml",
"demo/*.yml",
"demo/*.yaml",
"user/*.yml",
"user/*.yaml",
]
Comment on lines 488 to 489

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Sample user role allowed_samples diverges slightly from code default

The samples/skit.toml user role's allowed_samples includes demo/*.yml/demo/*.yaml (lines 480-481), while the code default Permissions::user() (apps/skit/src/permissions.rs:149-157) does not. This is a pre-existing divergence not introduced by this PR (the PR only touched allowed_nodes/allowed_plugins for the user role), so it is not a regression, but the code and sample are not a perfect mirror of each other despite the PR's stated goal of mirroring Permissions::user().

(Refers to lines 475-484)

Open in Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: removed demo/* from the sample user role's allowed_samples so it now mirrors the built-in Permissions::user() (oneshot/dynamic/user only).


# Users can use most nodes except potentially dangerous ones
# Users can use most nodes except potentially dangerous ones. Mirrors the
# built-in Permissions::user() default. Allowed: MoQ + the HTTP `mse` sink
# (serves over the caller's own request). Denied by omission:
# transport::http::fetcher (SSRF), core::file_writer / core::object_store_writer
# (arbitrary/external write). The oneshot streamkit::http_input/http_output
# markers are implicitly allowed on the oneshot path and need no entry here.
allowed_nodes = [
"audio::*",
"transport::*",
"core::*",
"video::*",
"containers::*",
"transport::moq::*",
Comment thread
staging-devin-ai-integration[bot] marked this conversation as resolved.
"transport::http::mse",
"core::passthrough",
"core::file_reader",
"core::pacer",
"core::param_bridge",
"core::json_serialize",
"core::text_chunker",
"core::script",
"core::telemetry_tap",
"core::telemetry_out",
"core::sink",
"plugin::*",
]
Comment on lines 487 to 514

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: User role node allowlist tightened from broad wildcards to explicit list

The sample user role previously allowed transport::* and core::*, which is broader than the code's Permissions::user(). This PR replaces those with an explicit list matching the code (e.g. transport::moq::*, transport::http::mse, and an enumerated set of core:: nodes), removing transport::rtmp, core::file_writer, core::object_store_writer, etc. from the user role. This is an intentional behavior change that could break existing user-created pipelines relying on the removed nodes; worth confirming there are no shipped user//oneshot/ sample pipelines that depend on them (none currently exist under samples/pipelines/user).

Open in Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the breakage surface. I checked the shipped pipelines: oneshot/tts_to_s3.yml, oneshot/transcode_to_s3.yml, dynamic/moq_s3_archive.yml use core::object_store_writer, and dynamic/moq_to_rtmp_composite.yml uses transport::rtmp::publish. A user can still read those samples but no longer run them.

That's intentional and not a regression relative to the secure default: the built-in Permissions::user() already denies object_store_writer (external write) and rtmp::publish (egress) — the old sample's core::*/transport::* wildcards were the outlier. Those pipelines remain runnable under admin (or a custom role that opts into the egress/write nodes). The point of this PR is to make the sample mirror that secure default.


# Users cannot load plugins, so this list is empty
allowed_plugins = []
# Users cannot load/delete plugins, but may use plugins an admin has already loaded.
allowed_plugins = ["plugin::*"]
Comment on lines 515 to +517

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: User role permissions widened to include loaded plugins

The built-in Permissions::user() and the samples/skit.toml user role now grant plugin::* in both allowed_nodes and allowed_plugins (previously the sample TOML had allowed_plugins = []). This is a deliberate widening so users can use already-loaded plugins, but it does change the default security posture: any plugin an admin has loaded becomes usable by all user-role callers. Worth confirming this matches the intended access model, especially given native plugins run in-process without a sandbox.

Open in Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional — this aligns the sample with the built-in Permissions::user(), which already grants plugin::* in both lists; the sample's old allowed_plugins = [] was the outlier (and was internally inconsistent, since its allowed_nodes had core::*/transport::* but no plugin grant). The access model is unchanged from the code default: load_plugins = false still means a user cannot load a plugin — only an admin can, and users may then use what the admin has vetted. The in-process/no-sandbox risk you note is real but lives at the admin's load decision (and [plugins].allow_http_management), which is exactly where the existing IMPORTANT comment in Permissions::user() points. Operators who want a stricter posture can narrow allowed_plugins to specific kinds (as the gateway role demonstrates).


# Users can list bundled system assets and their uploaded assets
# (mirrors the built-in Permissions::user(): audio, images and fonts).
allowed_assets = [
"samples/audio/system/*",
"samples/audio/user/*",
"samples/images/system/*",
"samples/images/user/*",
"samples/fonts/system/*",
"samples/fonts/user/*",
]

[permissions.roles.gateway]
# Least-privilege role for trusted intermediaries that build a small set of
# fixed pipelines, so they need not run as admin. The node/plugin lists below
# are scoped to the web-capture example; a different gateway (e.g. speech-gateway)
# would swap in its own nodes and plugins (audio nodes, whisper/kokoro, etc.).
create_sessions = true
destroy_sessions = true
list_sessions = true
modify_sessions = true
tune_nodes = true
load_plugins = false
delete_plugins = false
list_nodes = true
access_all_sessions = false # Only its own sessions
upload_assets = false
delete_assets = false

allowed_samples = []

# Exactly the node kinds the web-capture / live-cast pipeline needs, nothing more.
# A plugin kind must be allowed here too: is_node_allowed() runs before the
# allowed_plugins check, so a plugin missing from allowed_nodes is rejected first.
# No fetcher (SSRF) and no file_writer (arbitrary write). The oneshot
# streamkit::http_output marker is implicitly allowed and needs no entry.
allowed_nodes = [
"plugin::native::servo", # render the page (web-capture)
"video::pixel_convert", # servo RGBA -> encoder input format
"video::vp9::encoder", # encode to VP9
"containers::webm::muxer", # mux into WebM for MSE / http_output
"transport::http::mse", # serve the live cast to the browser (MSE)
"core::pacer",
"core::sink",
]
Comment on lines +554 to +562

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Gateway role's core::* allows arbitrary-write file_writer despite least-privilege framing

The new gateway role in samples/skit.toml:538-543 and the docs example (docs/src/content/docs/guides/authorization.md:132-137) grant core::*, which includes core::file_writer. The built-in user role (apps/skit/src/permissions.rs:172-181) deliberately enumerates individual core nodes and excludes core::file_writer because of arbitrary-write risk. So the role labelled "least-privilege" is actually broader (for core nodes) than the user role with respect to file_writer. This is a config/security-policy choice rather than a code bug, and security findings are out of scope for this review, but the inconsistency with the stated least-privilege intent may be worth a deliberate decision.

Open in Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: the gateway example (sample config + docs) now grants only the safe core plumbing nodes (passthrough, file_reader, pacer, sink) instead of core::*, so it no longer exposes core::file_writer — consistent with the least-privilege intent and the built-in user role.


# Only the specific plugin(s) the gateway needs (example: servo for web-capture).
allowed_plugins = ["plugin::native::servo"]

@staging-devin-ai-integration staging-devin-ai-integration Bot Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Gateway example references plugin::native::servo

Both the docs (docs/src/content/docs/guides/authorization.md:138) and sample config (samples/skit.toml:535) use plugin::native::servo as the gateway's allowed plugin for the web-capture example. Reviewer may want to confirm a servo native plugin kind actually exists/is the right name used by the web-capture example, otherwise the example would be non-functional as written.

Open in Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed valid — plugin::native::servo is the correct kind. The servo plugin (plugins/native/servo/plugin.yml) declares kind: native, node_kind: servo, and the existing sample pipelines (samples/pipelines/oneshot/web_capture.yml, web_pip_compositor.yml) use kind: plugin::native::servo. So the gateway example is functional as written.

Comment on lines +554 to +565

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Plugin enforcement ordering relied upon by gateway role

The gateway role lists plugin::native::servo in both allowed_nodes and allowed_plugins. This is required because enforcement checks is_node_allowed(kind) before the plugin allowlist (e.g. apps/skit/src/websocket_handlers.rs:55, apps/skit/src/server/sessions.rs:327). Verified the glob matching via glob::Pattern treats * ordinarily across :: (no / separators), so the literal plugin::native::servo entry and plugin::* entries resolve correctly. The new tests cover this ordering dependency.

Open in Devin Review (Staging)

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right — this ordering dependency (is_node_allowed runs before the plugin allowlist) was the root cause of the original gateway-role bug, where plugin::native::servo was in allowed_plugins but absent from allowed_nodes and so rejected first. The new sample_config_test assertion (gateway.is_node_allowed("plugin::native::servo")) plus the docs [!IMPORTANT] note now guard against regressing it.


allowed_assets = []

[permissions.roles.readonly]
# Read-only role - can only view, not modify
create_sessions = false
Expand Down
Loading