Fix/ssrf alerts target#1665
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds tenant-aware outbound policy storage and validation for alert targets, exposes admin endpoints to read and update the policy, and applies the policy during target create/update and alert delivery. It also adds supporting storage paths, metastore APIs, and routing. ChangesOutbound Alert Policy System
Estimated code review effort: 4 (Complex) | ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Cargo.toml (1)
84-91: 💤 Low value
ipnetaddition not reflected in summary; otherwise dependency changes look correct.The
netfeature ontokio("^1.43") is valid and required for DNS resolution/socket operations in the policy engine. However, line 91 also addsipnet = "2"(needed for CIDR parsing in the allow/deny policy), which contradicts the summary's claim that no other dependencies were altered.Note that
ipnetis placed under the "Async and Runtime" section though it's a networking/CIDR utility — consider moving it nearerurl/networking deps for clarity, but this is purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` around lines 84 - 91, Update the PR summary to explicitly mention the new ipnet = "2" dependency (used for CIDR parsing in allow/deny policy) in addition to the tokio feature change; in the Cargo.toml diff, ensure the ipnet addition is clearly associated with networking/url deps rather than the "Async and Runtime" block by moving the ipnet = "2" line next to other networking dependencies (the tokio features block and url dependency) so reviewers can easily see it's a networking/CIDR utility; keep the existing tokio feature changes (sync, macros, fs, rt-multi-thread, net) unchanged.src/alerts/outbound_http_policy.rs (1)
167-169: 💤 Low valueConsider propagating client build errors instead of panicking.
While the
ClientBuilderconfiguration is deterministic and unlikely to fail in practice, using.expect()in production code paths is less defensive than propagating the error. If future changes introduce fallible configuration (e.g., loading certificates), this would need updating.🛡️ Suggested fix
Add a new error variant and propagate:
+ #[error("failed to build HTTP client: {0}")] + ClientBuildFailed(String),let client = builder .build() - .expect("alert target HTTP client can be constructed"); + .map_err(|e| OutboundPolicyError::ClientBuildFailed(e.to_string()))?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/alerts/outbound_http_policy.rs` around lines 167 - 169, Replace the panic-causing .expect("alert target HTTP client can be constructed") on builder.build() with proper error propagation: add a new error variant (e.g., OutboundHttpClientBuildError or ClientBuildError) to the module's error enum, change the containing function's return type to Result<..., ErrorType>, and replace builder.build().expect(...) with builder.build().map_err(|e| ErrorType::OutboundHttpClientBuildError(e.into()))? (or equivalent) so the client construction failure is returned to callers; update any call sites and tests to handle the propagated Result accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/alerts/outbound_http_policy.rs`:
- Around line 306-323: In denied_ipv6, add explicit checks to reject IPv6
tunneling prefixes 6to4 (2002::/16) and Teredo (2001::/32) before falling back
to mapped-IPv4 handling: inspect ip.segments()[0] and return true when it equals
0x2002 (6to4) or equals 0x2001 (Teredo /32), then continue with the existing
mapped-IPv4 call to denied_ipv4 and other checks; this ensures embedded IPv4
addresses in those tunneling ranges cannot bypass the private-IP blocking.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 84-91: Update the PR summary to explicitly mention the new ipnet =
"2" dependency (used for CIDR parsing in allow/deny policy) in addition to the
tokio feature change; in the Cargo.toml diff, ensure the ipnet addition is
clearly associated with networking/url deps rather than the "Async and Runtime"
block by moving the ipnet = "2" line next to other networking dependencies (the
tokio features block and url dependency) so reviewers can easily see it's a
networking/CIDR utility; keep the existing tokio feature changes (sync, macros,
fs, rt-multi-thread, net) unchanged.
In `@src/alerts/outbound_http_policy.rs`:
- Around line 167-169: Replace the panic-causing .expect("alert target HTTP
client can be constructed") on builder.build() with proper error propagation:
add a new error variant (e.g., OutboundHttpClientBuildError or ClientBuildError)
to the module's error enum, change the containing function's return type to
Result<..., ErrorType>, and replace builder.build().expect(...) with
builder.build().map_err(|e| ErrorType::OutboundHttpClientBuildError(e.into()))?
(or equivalent) so the client construction failure is returned to callers;
update any call sites and tests to handle the propagated Result accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 307e02c4-1433-45cc-90f4-a9ee10f8c67c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlsrc/alerts/mod.rssrc/alerts/outbound_http_policy.rssrc/alerts/target.rssrc/handlers/http/alert_target_policy.rssrc/handlers/http/mod.rssrc/handlers/http/modal/query_server.rssrc/handlers/http/modal/server.rssrc/handlers/http/targets.rs
ygndotgg
force-pushed
the
fix/ssrf-alerts-target
branch
from
fa45266 to
b8f9628
Compare
parmesant
force-pushed
the
fix/ssrf-alerts-target
branch
from
b8f9628 to
a5327c8
Compare
parmesant
force-pushed
the
fix/ssrf-alerts-target
branch
from
a5327c8 to
a955914
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/alerts/target.rs (1)
662-665: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAvoid logging the full AlertManager endpoint.
self.endpointcan include userinfo, query tokens, or sensitive path data. Log only a sanitized host/scheme when credentials are blocked.Proposed fix
error!( - "Alertmanager credentials blocked by outbound policy for destination:{}", - self.endpoint + "Alertmanager credentials blocked by outbound policy for destination_host:{}", + self.endpoint.host_str().unwrap_or("<missing-host>") );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/alerts/target.rs` around lines 662 - 665, The blocked-credentials log in AlertmanagerTarget::send is exposing the full endpoint via self.endpoint, which may include sensitive userinfo or tokens. Update the error! call to log only a sanitized Alertmanager destination, keeping just the scheme and host from AlertmanagerTarget::endpoint and excluding credentials, query strings, and sensitive path data. Use the existing endpoint handling in src/alerts/target.rs to derive the sanitized value before logging.src/storage/localfs.rs (1)
504-512: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't filter out
DEFAULT_TENANThere.DEFAULT_TENANTis a literal string constant ("DEFAULT_TENANT"), and stream-name validation does not reserve it, so a stream with that name would disappear from bothlist_streamsandlist_old_streams. If the goal is to skip a stray root folder, narrow the check to the tenant directory layout instead of matching the stream name exactly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage/localfs.rs` around lines 504 - 512, The list_streams filtering currently excludes DEFAULT_TENANT by name, which can hide a valid stream. Update list_streams in localfs.rs so the ignore_dir logic no longer matches the DEFAULT_TENANT constant directly; instead, only skip the tenant root folder based on the directory layout used by the storage hierarchy. Keep the rest of the exclusion list unchanged and ensure list_old_streams follows the same rule if it shares the same filtering logic.Source: Learnings
🧹 Nitpick comments (1)
src/handlers/http/targets.rs (1)
34-37: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse
get_tenant_id_from_requesthere. This handler only needs the normalized tenant header; re-resolving tenant from auth adds an unnecessary RBAC dependency and maps those failures toCustomError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/handlers/http/targets.rs` around lines 34 - 37, The tenant lookup in tenant_from_request is using get_user_and_tenant_from_request, which pulls auth-derived tenant data and introduces an unnecessary RBAC dependency. Update tenant_from_request to use get_tenant_id_from_request instead, keep the same Result<Option<String>, AlertError> shape, and remove the CustomError mapping path so this handler only reads the normalized tenant header.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/alerts/outbound_http_policy.rs`:
- Around line 37-59: The active_policy() lookup in outbound_http_policy.rs only
returns AlertTargetPolicyConfig::default() for DEFAULT_TENANT, causing missing
tenant entries to fail with PolicyNotFound. Update active_policy() so a missing
policy for any tenant falls back to AlertTargetPolicyConfig::default() instead
of erroring, while keeping the existing read-from-ALERT_TARGET_POLICY behavior
and using OutboundPolicyError::PolicyNotFound only for truly invalid cases.
In `@src/handlers/http/modal/mod.rs`:
- Around line 237-240: The outbound policy bootstrap in load_on_init uses ?
after outbound_http_policy::load_all_policies, which makes a single bad tenant
policy abort startup unlike the other loaders. Update this block to match the
existing pattern used by correlations, filters, dashboards, alerts, and targets:
call outbound_http_policy::load_all_policies, log any error with error!
including context, and continue without returning early so one corrupt policy
does not take down the whole node.
In `@src/metastore/metastores/object_store_metastore.rs`:
- Around line 1258-1270: `get_outbound_policy` currently propagates a
missing-object storage error instead of treating it as “no policy configured,”
which makes single-tenant policy loads fail unexpectedly. Update
`ObjectStoreMetastore::get_outbound_policy` to mirror the missing-file handling
used in `get_outbound_policies` by detecting the optional-missing case from
`self.storage.get_object` and returning the appropriate default/absence behavior
instead of `?`. Keep the existing path/tenant logic intact, and make sure
callers like `load_policy_for_tenant` can resolve cleanly for tenants without a
stored policy.
---
Outside diff comments:
In `@src/alerts/target.rs`:
- Around line 662-665: The blocked-credentials log in AlertmanagerTarget::send
is exposing the full endpoint via self.endpoint, which may include sensitive
userinfo or tokens. Update the error! call to log only a sanitized Alertmanager
destination, keeping just the scheme and host from AlertmanagerTarget::endpoint
and excluding credentials, query strings, and sensitive path data. Use the
existing endpoint handling in src/alerts/target.rs to derive the sanitized value
before logging.
In `@src/storage/localfs.rs`:
- Around line 504-512: The list_streams filtering currently excludes
DEFAULT_TENANT by name, which can hide a valid stream. Update list_streams in
localfs.rs so the ignore_dir logic no longer matches the DEFAULT_TENANT constant
directly; instead, only skip the tenant root folder based on the directory
layout used by the storage hierarchy. Keep the rest of the exclusion list
unchanged and ensure list_old_streams follows the same rule if it shares the
same filtering logic.
---
Nitpick comments:
In `@src/handlers/http/targets.rs`:
- Around line 34-37: The tenant lookup in tenant_from_request is using
get_user_and_tenant_from_request, which pulls auth-derived tenant data and
introduces an unnecessary RBAC dependency. Update tenant_from_request to use
get_tenant_id_from_request instead, keep the same Result<Option<String>,
AlertError> shape, and remove the CustomError mapping path so this handler only
reads the normalized tenant header.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 688f6ea9-3f32-4f7c-8748-cd98cb9a3b6e
📒 Files selected for processing (10)
src/alerts/alert_traits.rssrc/alerts/outbound_http_policy.rssrc/alerts/target.rssrc/handlers/http/alert_target_policy.rssrc/handlers/http/modal/mod.rssrc/handlers/http/targets.rssrc/metastore/metastore_traits.rssrc/metastore/metastores/object_store_metastore.rssrc/storage/localfs.rssrc/storage/object_storage.rs
ygndotgg
force-pushed
the
fix/ssrf-alerts-target
branch
from
534cce1 to
a214381
Compare
- replace the single global policy manager with per-tenant policy state - persist outbound HTTP policy through metastore and reload it on startup - reject policies with overlapping allow/deny domain or CIDR entries - follow the existing tenant-keyed design used elsewhere in the codebase
parmesant
force-pushed
the
fix/ssrf-alerts-target
branch
from
a214381 to
f8f7a8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/alerts/mod.rs`:
- Around line 623-626: The alert policy validation error log in the target
processing path is exposing sensitive data by using Debug formatting on Target.
Update the tracing::error call in the validate_outbound_policy failure branch to
avoid target:? and instead log the masked target via target.mask() or only safe
identifiers like name/id, while still including the validation error e for
context.
In `@src/alerts/outbound_http_policy.rs`:
- Around line 461-463: The IPv4 extraction in the outbound IP denial path is too
narrow because `to_ipv4_mapped()` only handles mapped addresses and can miss
IPv4-compatible embedded forms. Update the logic in the relevant IP check to use
`to_ipv4()` instead, keeping the existing `denied_ipv4` flow so both mapped and
compatible embedded IPv4 addresses are denied consistently.
In `@src/handlers/http/targets.rs`:
- Around line 61-73: The error field in the target listing is leaking redacted
host/IP/domain details by using OutboundPolicyError::to_string() in the
validate_outbound_policy() branch. Update the targets handler to return a
generic error message or stable error code instead of e.to_string(), while
keeping target.mask() as the only source of target-identifying data.
- Around line 56-76: The list handler in targets::list is doing a live
validate_outbound_policy() call for every target, which makes GET /targets
perform sequential network lookups. Update the listing flow to avoid per-item
blocking validation by either caching the most recent validation result on each
target or running the validations concurrently before building the JSON
response. Keep the change localized to list and the validate_outbound_policy
path so target masking and enabled/error payloads still come from the same
target values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f531fb2b-3ed2-4942-a9d6-c2470c62b5f1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlsrc/alerts/alert_traits.rssrc/alerts/alerts_utils.rssrc/alerts/mod.rssrc/alerts/outbound_http_policy.rssrc/alerts/target.rssrc/handlers/http/alert_target_policy.rssrc/handlers/http/mod.rssrc/handlers/http/modal/mod.rssrc/handlers/http/modal/query_server.rssrc/handlers/http/modal/server.rssrc/handlers/http/targets.rssrc/metastore/metastore_traits.rssrc/metastore/metastores/object_store_metastore.rssrc/storage/localfs.rssrc/storage/object_storage.rssrc/validator.rs
✅ Files skipped from review due to trivial changes (1)
- src/alerts/alerts_utils.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- src/handlers/http/mod.rs
- src/alerts/alert_traits.rs
- src/handlers/http/alert_target_policy.rs
- Cargo.toml
- src/handlers/http/modal/query_server.rs
- src/metastore/metastores/object_store_metastore.rs
- src/storage/localfs.rs
- src/storage/object_storage.rs
- src/metastore/metastore_traits.rs
- src/alerts/target.rs
- src/handlers/http/modal/server.rs
| if let Err(e) = target.validate_outbound_policy().await { | ||
| tracing::error!("Target {target:?} failed alert policy check- {e}"); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -B2 'struct (Auth|OtherWebHook|SlackWebHook|AlertManager)\b|enum TargetType\b' src/alerts/target.rsRepository: parseablehq/parseable
Length of output: 782
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target type definitions, masking helpers, and the log site in context.
sed -n '460,760p' src/alerts/target.rs
printf '\n--- LOG SITE ---\n'
sed -n '615,635p' src/alerts/mod.rs
printf '\n--- MASK USAGE ---\n'
rg -n "mask\\(|Debug for|impl std::fmt::Debug|impl Debug" src/alerts/target.rs src/alerts/mod.rsRepository: parseablehq/parseable
Length of output: 9642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '180,260p' src/alerts/target.rsRepository: parseablehq/parseable
Length of output: 3278
Avoid Debug-printing Target here target:? includes TargetType and nested SlackWebHook/OtherWebHook/AlertManager/Auth fields, so webhook URLs, headers, and basic-auth credentials can end up in logs. Log target.mask() or only name/id instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/alerts/mod.rs` around lines 623 - 626, The alert policy validation error
log in the target processing path is exposing sensitive data by using Debug
formatting on Target. Update the tracing::error call in the
validate_outbound_policy failure branch to avoid target:? and instead log the
masked target via target.mask() or only safe identifiers like name/id, while
still including the validation error e for context.
| if let Some(mapped) = ip.to_ipv4_mapped() { | ||
| return denied_ipv4(mapped); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Rust std Ipv6Addr to_ipv4 vs to_ipv4_mapped IPv4-compatible addresses
💡 Result:
In Rust's standard library, Ipv6Addr::to_ipv4 and Ipv6Addr::to_ipv4_mapped both convert an IPv6 address to an IPv4 address, but they differ significantly in scope, criteria, and handling of loopback addresses [1][2]. Ipv6Addr::to_ipv4 This method is broader and attempts to convert the address if it is either an IPv4-compatible address (defined in RFC 4291 section 2.5.5.1) or an IPv4-mapped address (defined in RFC 4291 section 2.5.5.2) [1][2]. Critically, it includes the IPv6 loopback address (::1) in its conversion, mapping it to 0.0.0.1 [1][2]. It returns None for addresses that do not fit these specific definitions [1][2]. Ipv6Addr::to_ipv4_mapped This method is more restrictive and only succeeds if the address is an IPv4-mapped IPv6 address (starting with::ffff:) [1][2]. It will return None for any address that is not strictly an IPv4-mapped address, including IPv4-compatible addresses and the IPv6 loopback address (::1) [1][2]. Using this method is the recommended way to avoid the automatic conversion of the loopback address (::1) that occurs with to_ipv4 [1][2]. Summary of Key Differences Method | Criteria | Loopback Handling (::1) --- | --- | --- to_ipv4 | IPv4-compatible or IPv4-mapped | Converts to 0.0.0.1 to_ipv4_mapped | Only IPv4-mapped | Returns None When you need to ensure you are specifically working with an IPv4-mapped address and want to avoid unexpected behavior with other address types (like the loopback address), to_ipv4_mapped is the safer, more precise choice [1][2].
Citations:
- 1: https://doc.rust-lang.org/stable/std/net/struct.Ipv6Addr.html
- 2: https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section and nearby IPv6 handling.
sed -n '380,490p' src/alerts/outbound_http_policy.rs
# Find all IPv6-related checks in this file.
rg -n "to_ipv4_mapped|to_ipv4\\(|loopback|unspecified|is_loopback|is_unspecified|deny.*ipv6|denied_ipv6" src/alerts/outbound_http_policy.rsRepository: parseablehq/parseable
Length of output: 4368
Use to_ipv4() for embedded IPv4 forms
to_ipv4_mapped() misses deprecated IPv4-compatible addresses like ::a.b.c.d, so an embedded private IPv4 can still slip past this check. to_ipv4() covers both mapped and compatible forms, and loopback/unspecified are already rejected above.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/alerts/outbound_http_policy.rs` around lines 461 - 463, The IPv4
extraction in the outbound IP denial path is too narrow because
`to_ipv4_mapped()` only handles mapped addresses and can miss IPv4-compatible
embedded forms. Update the logic in the relevant IP check to use `to_ipv4()`
instead, keeping the existing `denied_ipv4` flow so both mapped and compatible
embedded IPv4 addresses are denied consistently.
| pub async fn list(req: HttpRequest) -> Result<impl Responder, AlertError> { | ||
| let tenant_id = get_tenant_id_from_request(&req); | ||
| let tenant_id = tenant_from_request(&req)?; | ||
| // add to the map | ||
| let list = TARGETS | ||
| .list(&tenant_id) | ||
| .await? | ||
| .into_iter() | ||
| .map(|t| t.mask()) | ||
| .collect_vec(); | ||
| let mut list = vec![]; | ||
| for target in TARGETS.list(&tenant_id).await? { | ||
| if let Err(e) = target.validate_outbound_policy().await { | ||
| list.push(json!({ | ||
| "target": &target.mask(), | ||
| "enabled": false, | ||
| "error": &e.to_string() | ||
| })); | ||
| } else { | ||
| list.push(json!({ | ||
| "target": &target.mask(), | ||
| "enabled": true | ||
| })); | ||
| } | ||
| } | ||
|
|
||
| Ok(web::Json(list)) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant files and symbols with line numbers.
git ls-files src/handlers/http/targets.rs src/outbound_http_policy.rs src/**/*.rs | sed -n '1,120p'
echo '--- targets.rs outline ---'
ast-grep outline src/handlers/http/targets.rs --view expanded || true
echo '--- outbound_http_policy.rs outline ---'
ast-grep outline src/outbound_http_policy.rs --view expanded || true
echo '--- targets.rs relevant lines ---'
sed -n '1,220p' src/handlers/http/targets.rs
echo '--- outbound_http_policy.rs relevant lines ---'
sed -n '1,360p' src/outbound_http_policy.rsRepository: parseablehq/parseable
Length of output: 6707
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the exact definitions for validate_outbound_policy and error display formatting.
rg -n "validate_outbound_policy|enum OutboundPolicyError|impl .*Display.*OutboundPolicyError|fmt::Display|PrivateAddress|UnsupportedScheme|resolve_endpoint_addrs|mask\(" src
# Show focused slices around the matches once located.Repository: parseablehq/parseable
Length of output: 2832
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow behavioral check: see whether list/get validation is sequential and whether it can be parallelized.
# Read-only probe using the source to inspect use of await inside loops and any join_all-like concurrency.
python3 - <<'PY'
from pathlib import Path
import re
p = Path('src/handlers/http/targets.rs')
text = p.read_text()
for name in ['list', 'get']:
m = re.search(rf'pub async fn {name}\b.*?\n\}}', text, re.S)
print(f'== {name} ==')
if m:
chunk = m.group(0)
print('await count:', chunk.count('.await'))
print('contains for-loop:', 'for ' in chunk)
print('contains validate_outbound_policy:', 'validate_outbound_policy' in chunk)
PYRepository: parseablehq/parseable
Length of output: 339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the endpoint response can leak actual host/IP information through error strings.
python3 - <<'PY'
from pathlib import Path
import re
text = Path('src/outbound_http_policy.rs').read_text()
for pat in [
r'enum\s+OutboundPolicyError\b',
r'impl\s+Display\s+for\s+OutboundPolicyError\b',
r'format!\(',
r'to_string\(\)',
r'PrivateAddress',
r'UnsupportedScheme',
r'resolve_endpoint_addrs',
]:
print(f'-- {pat} --')
for m in re.finditer(pat, text):
start = max(0, m.start()-220)
end = min(len(text), m.end()+420)
print(text[start:end])
print('---')
break
PYRepository: parseablehq/parseable
Length of output: 713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the outbound policy error definitions and display strings in the correct file.
sed -n '140,260p' src/alerts/outbound_http_policy.rs
echo '---'
sed -n '260,460p' src/alerts/outbound_http_policy.rs
echo '---'
sed -n '460,620p' src/alerts/outbound_http_policy.rsRepository: parseablehq/parseable
Length of output: 16152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target validation and masking behavior.
sed -n '160,280p' src/alerts/target.rs
echo '---'
sed -n '280,360p' src/alerts/target.rsRepository: parseablehq/parseable
Length of output: 7788
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect AlertError formatting to see how outbound-policy failures are rendered.
rg -n "enum AlertError|impl .*Display for AlertError|From<.*OutboundPolicyError|validate_outbound_policy" src/alerts src/utils
sed -n '1,220p' src/utils/error.rsRepository: parseablehq/parseable
Length of output: 1631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact error enum and Display impl sections.
grep -n "pub enum OutboundPolicyError\|impl std::fmt::Display for OutboundPolicyError\|PrivateAddressNotAllowed\|UnsupportedScheme" -n src/alerts/outbound_http_policy.rs
echo '---'
sed -n '150,230p' src/alerts/outbound_http_policy.rsRepository: parseablehq/parseable
Length of output: 2800
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact outbound policy error strings and how they are propagated.
sed -n '150,220p' src/alerts/outbound_http_policy.rs
echo '---'
sed -n '220,440p' src/alerts/outbound_http_policy.rs
echo '---'
sed -n '1,220p' src/utils/error.rsRepository: parseablehq/parseable
Length of output: 11143
Cache or batch outbound-policy validation on target listing.
list calls validate_outbound_policy() for every stored target, and that path does live DNS resolution each time. This turns GET /targets into N sequential network lookups per request; cache the last validation result or run the checks concurrently to keep latency bounded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/handlers/http/targets.rs` around lines 56 - 76, The list handler in
targets::list is doing a live validate_outbound_policy() call for every target,
which makes GET /targets perform sequential network lookups. Update the listing
flow to avoid per-item blocking validation by either caching the most recent
validation result on each target or running the validations concurrently before
building the JSON response. Keep the change localized to list and the
validate_outbound_policy path so target masking and enabled/error payloads still
come from the same target values.
| if let Err(e) = target.validate_outbound_policy().await { | ||
| list.push(json!({ | ||
| "target": &target.mask(), | ||
| "enabled": false, | ||
| "error": &e.to_string() | ||
| })); | ||
| } else { | ||
| list.push(json!({ | ||
| "target": &target.mask(), | ||
| "enabled": true | ||
| })); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'enum OutboundPolicyError { $$$ }' --lang rust src/alerts/outbound_http_policy.rs
rg -n '#\[error' -A1 src/alerts/outbound_http_policy.rsRepository: parseablehq/parseable
Length of output: 2054
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== targets.rs (relevant sections) ==\n'
sed -n '1,130p' src/handlers/http/targets.rs
printf '\n== outbound_http_policy.rs (error enum + validate paths) ==\n'
sed -n '130,260p' src/alerts/outbound_http_policy.rs
printf '\n== find uses of OutboundPolicyError display-bearing variants ==\n'
rg -n 'ResolveFailed|NoResolvedAddresses|DeniedAddress|DeniedDomain|PrivateAddressNotAllowed|InvalidSlackHost|PolicyNotFound|ConflictingCidrs|ConflictingDomains|DeniedHeader|InvalidHeaderName|InvalidHeaderValue' src/alerts/outbound_http_policy.rs src -g '*.rs'Repository: parseablehq/parseable
Length of output: 14906
Avoid returning e.to_string() in error
OutboundPolicyError messages include host/IP/domain details (ResolveFailed, DeniedAddress, DeniedDomain, PrivateAddressNotAllowed, etc.), which can undo mask()’s redaction. Return a generic error or code instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/handlers/http/targets.rs` around lines 61 - 73, The error field in the
target listing is leaking redacted host/IP/domain details by using
OutboundPolicyError::to_string() in the validate_outbound_policy() branch.
Update the targets handler to return a generic error message or stable error
code instead of e.to_string(), while keeping target.mask() as the only source of
target-identifying data.
There was a problem hiding this comment.
generic error codes/messages have been handled in AlertError
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'OutboundPolicy' src/alerts/mod.rs -A 15
echo "---"
rg -n 'enum AlertError' -A 60 src/alerts/mod.rs | head -100
echo "---error_response---"
rg -n 'fn error_response' -A 30 src/alerts/mod.rsLength of output: 9121
@parmesant, I checked AlertError's ResponseError impl for the OutboundPolicy variant (src/alerts/mod.rs:1057-1063):
Self::OutboundPolicy(_) => actix_web::HttpResponse::build(self.status_code())
.insert_header(ContentType::json())
.json(serde_json::json!({
"error": "Alert target blocked by outbound security policy",
"message": self.to_string(),
"hint": "Ask admin to allow this destination using the alert target policy."
})),This wraps a fixed "error" string, but the "message" field still uses self.to_string(), which renders as "Alert target outbound policy rejected request:{0}" where {0} is the underlying OutboundPolicyError's Display — i.e., it still contains the detailed host/IP/domain/CIDR info (e.g. DeniedAddress, PrivateAddressNotAllowed, DeniedDomain messages).
Additionally, the list/get handlers in src/handlers/http/targets.rs don't actually go through AlertError::error_response() at all — they catch the Result from validate_outbound_policy() and manually build the JSON with e.to_string():
if let Err(e) = target.validate_outbound_policy().await {
list.push(json!({
"target": &target.mask(),
"enabled": false,
"error": &e.to_string()
}));
}So the same detailed message ends up in the response either way. If the intent is to keep it generic here, it'd need a fixed string (or an error-code accessor) rather than e.to_string(), similar in spirit to the "error" field in error_response() — but even that path still leaks details via "message". Let me know if you'd like me to open a follow-up issue, or if this level of detail exposure to a super-admin-only-facing endpoint is acceptable in your threat model.
🧬 Code Graph Analysis Results
[filename: src/handlers/http/targets.rs]
- (No additional snippets in this file beyond the user-visible handler logic; see full file contents provided.)
[possibly_relevant_code_snippets]
[filename: src/alerts/target.rs]
-
Lines 178-337 (
impl Target):validate_outbound_policy(&self) -> Result<(), AlertError>- Prepares outbound HTTP policy for each
TargetType(Slack/Other/AlertManager) viaoutbound_http_policy::prepare_alert_target(...). - For
AlertManager, ifauthis present butprepared.authorization_allowedis false, returns anAlertErrorfromOutboundPolicyError::DeniedHeader(AUTHORIZATION.to_string()). - Returns
Ok(())if the outbound policy allows the target.
- Prepares outbound HTTP policy for each
-
Lines 76-88:
update(&self, target: Target) -> Result<(), AlertError>- Persists target to metastore with
put_target(&target, &target.tenant).await?. - Updates in-memory
target_configsby tenant (usestarget.tenant.as_deref().unwrap_or(DEFAULT_TENANT)). - Returns
Ok(()).
- Persists target to metastore with
-
Lines 90-98:
list(&self, tenant_id: &Option<String>) -> Result<Vec<Target>, AlertError>- Uses tenant resolved via
tenant_id.as_deref().unwrap_or(DEFAULT_TENANT). - Returns cloned list of targets from
self.target_configsor an emptyVec.
- Uses tenant resolved via
-
Lines 118-149:
delete(&self, target_id: &Ulid, tenant_id: &Option<String>) -> Result<Target, AlertError>- Checks whether the target is used by any alert; if so returns
Err(AlertError::TargetInUse). - If missing from in-memory config, returns
Err(AlertError::InvalidTargetID(target_id.to_string())). - Deletes from metastore via
PARSEABLE.metastore.delete_target(&target, tenant_id).await?. - Returns deleted
Target.
- Checks whether the target is used by any alert; if so returns
-
Lines 169-176:
pub struct Target { ... }- Includes
tenant: Option<String>and#[serde(flatten)] pub target: TargetType.
- Includes
-
Lines 222-273 (duplicate of
masklogic shown in snippets):mask(self) -> Value- Produces a JSON representation of the target with sensitive endpoint values masked.
[filename: src/alerts/mod.rs]
-
Lines 974-1023:
pub enum AlertError- Includes
CustomError(String),InvalidTargetID(String),InvalidTargetModification(String),TargetInUse,OutboundPolicy(#[from] outbound_http_policy::OutboundPolicyError), etc.
- Includes
-
Lines 1025-1069:
impl actix_web::ResponseError for AlertErrorstatus_code(&self) -> StatusCode: maps variants to HTTP status codes (notablyOutboundPolicy(_) => BAD_REQUEST,InvalidTargetID(_) => BAD_REQUEST,TargetInUse => CONFLICT, etc.).error_response(&self) -> HttpResponse: forOutboundPolicy(_)returns JSON:{"error": "...", "message": self.to_string(), "hint": "..." }
- For all other variants returns plaintext body:
self.to_string().
[filename: src/handlers/http/alert_target_policy.rs]
- Lines 15-19:
get(req: HttpRequest) -> Result<impl Responder, AlertError>- Fetches tenant via
get_tenant_id_from_request(&req), loadsactive_policy(&tenant_id).await?, returnsweb::Json(policy).
- Fetches tenant via
🧠 Learnings used
Learnt from: parmesant
Repo: parseablehq/parseable PR: 1545
File: src/handlers/http/correlation.rs:45-52
Timestamp: 2026-02-13T07:15:30.126Z
Learning: Enforce tenant scoping in HTTP handlers under src/handlers/http. Specifically: - For Action::Ingest, require and validate a tenant header, rejecting DEFAULT_TENANT and cross-check the BasicAuth user-to-tenant mapping. - For non-ingest actions, derive the tenant from the authenticated session and overwrite/inject the tenant header to prevent client-side spoofing. Downstream handlers (e.g., src/handlers/http/correlation.rs::get) should rely on get_tenant_id_from_request, since the middleware normalizes the header. Apply this guideline to all Rust files in src/handlers/http (and related middleware) to ensure consistent tenant handling across endpoints.
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1567
File: src/handlers/http/modal/query/querier_role.rs:107-116
Timestamp: 2026-03-12T15:21:18.623Z
Learning: In Rust codebases (e.g., in parseablehq/parseable), ensure that authorization checks rely on dynamically computing group-inherited permissions at auth time rather than storing them in the user session. Specifically, session permissions should only include a user's direct role permissions, while group-derived permissions are recalculated on every authorization check (e.g., via aggregate_group_permissions(username, tenant_id)). This avoids stale permissions and ensures group permissions are always up-to-date across all Rust modules that perform RBAC checks.
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1633
File: src/handlers/http/modal/ingest_server.rs:118-118
Timestamp: 2026-04-29T12:45:18.915Z
Learning: In parseablehq/parseable, it is intentional to run the startup sync entrypoint (e.g., `sync_start` in `src/sync.rs`) from another thread by doing `thread::spawn(sync_start)` in HTTP server/handler modules. Do not flag this as incorrect when `sync_start` is annotated with `#[tokio::main(flavor = "current_thread")]`, because it will create and run its own lightweight Tokio current-thread runtime on the dedicated OS thread to limit resource usage. Only flag if the spawned function is not a `#[tokio::main(...)]` entrypoint or if there is an actual runtime/ownership misuse (e.g., using a moved/invalid runtime handle).
2 participants
Fixes #XXXX.
Description
This PR hardens alert target delivery against server-side request forgery by adding an outbound policy for alert-target HTTP
requests.
Previously, authenticated users with alert target permissions could configure webhook, Slack, or AlertManager targets that
caused the Parseable server to send outbound HTTP requests to arbitrary URLs from the server’s network position. That meant
private/internal destinations, loopback services, link-local metadata endpoints, and other network-restricted HTTP services
could be reached if routable from the Parseable host.
The chosen fix is to centralize alert-target outbound networking behind a policy gate. Instead of validating only the submitted
URL string, Parseable now resolves the destination, validates the resolved IPs, applies admin-owned allow/deny policy, disables
redirects/proxies, pins the resolved DNS result into the HTTP client, and revalidates again immediately before alert delivery.
Key changes made in this patch:
Added alert target outbound policy configuration with:
Added super-admin APIs for reading, updating, and validating the alert target policy.
Applied outbound policy validation during target creation and update.
Revalidated outbound policy during alert delivery because stored targets and DNS answers can change after creation.
Blocked private, loopback, link-local, multicast, reserved, and other non-public destinations by default.
Allowed private/internal destinations only when explicitly enabled by admin policy and matched by an allowlist.
Made denied CIDRs/domains take precedence over allowlists.
Disabled redirects and proxy use for alert target delivery.
Pinned resolved DNS addresses into reqwest before sending.
Added focused unit tests for the important policy decision paths.
This PR has:
Summary by CodeRabbit
New Features
Bug Fixes