security: path traversal via link_sid in delete_person_company_link

casper-stevens commented

2026-05-07 08:28:52 +00:00

Member

Summary

delete_person_company_link constructs a file path directly from the link_sid URL parameter without any validation:

pub async fn person_link_delete(
    Path((context, person_id, link_sid)): Path<(String, String, String)>,
    ...
) -> impl IntoResponse {
    let path = self.link_dir(context).join(format!("{}.json", link_sid));
    // deletes the file at this path
}

A link_sid of ../../Cargo.toml or ../../../etc/passwd would delete arbitrary files on the filesystem that the process has write access to.

Fix

Validate link_sid matches ^[a-zA-Z0-9_-]+$ before using it in a path, or use the same safe_join helper as the fix for #41.

Affected code

crates/hero_biz_ui/src/services/mod.rs — delete_person_company_link
crates/hero_biz_ui/src/web/handlers/mod.rs — person_link_delete

Found in

PR #21 review

## Summary `delete_person_company_link` constructs a file path directly from the `link_sid` URL parameter without any validation: ```rust pub async fn person_link_delete( Path((context, person_id, link_sid)): Path<(String, String, String)>, ... ) -> impl IntoResponse { let path = self.link_dir(context).join(format!("{}.json", link_sid)); // deletes the file at this path } ``` A `link_sid` of `../../Cargo.toml` or `../../../etc/passwd` would delete arbitrary files on the filesystem that the process has write access to. ## Fix Validate `link_sid` matches `^[a-zA-Z0-9_-]+$` before using it in a path, or use the same `safe_join` helper as the fix for #41. ## Affected code - `crates/hero_biz_ui/src/services/mod.rs` — `delete_person_company_link` - `crates/hero_biz_ui/src/web/handlers/mod.rs` — `person_link_delete` ## Found in PR #21 review

casper-stevens commented

2026-05-07 09:49:19 +00:00

Author

Member

Implementation Spec for Issue #42

Objective

Prevent a directory traversal attack where a crafted link_sid URL parameter (e.g. ../../Cargo.toml) can be used to delete arbitrary files on the filesystem through the person_link_delete HTTP handler.

Current State

The service layer is already fully fixed. validate_path_component() at services/mod.rs:43-54 is called on both context and link_sid inside delete_person_company_link and link_dir. No filesystem operation will be performed on a traversal-containing path.

The remaining gap is in the HTTP handler person_link_delete in handlers/mod.rs, which discards the error with let _ = .... This means:

A traversal attempt is silently ignored (attacker gets no signal, operators get no log trace)
The handler redirects as if the operation succeeded
HeroBizError::Validation is never surfaced as HTTP 400

Requirements

The handler must propagate HeroBizError::Validation as HTTP 400, not a silent redirect
Follow the existing pattern: (StatusCode::BAD_REQUEST, e.to_string()).into_response()
No changes needed to services/mod.rs — validation is complete
Existing unit tests for validate_path_component must continue to pass

Files to Modify

crates/hero_biz_ui/src/web/handlers/mod.rs — handle the Result from delete_person_company_link

Implementation Plan

Step 1: Fix `person_link_delete` handler

File: crates/hero_biz_ui/src/web/handlers/mod.rs

Replace the let _ = ... discard with a match that returns HTTP 400 on error:

pub async fn person_link_delete(
    State(state): State<Arc<AppState>>,
    Path((context, person_id, link_sid)): Path<(String, String, String)>,
) -> impl IntoResponse {
    if let Err(e) = state.store.delete_person_company_link(&context, &link_sid) {
        return (StatusCode::BAD_REQUEST, e.to_string()).into_response();
    }
    Redirect::to(&format!(
        "{}/c/{}/contacts/{}",
        effective_base_path(&state),
        context,
        person_id
    ))
    .into_response()
}

StatusCode is already imported. No new imports needed.

Dependencies: none

Acceptance Criteria

cargo build -p hero_biz_ui passes with no errors
cargo test -p hero_biz_ui passes with no regressions
person_link_delete returns HTTP 400 when link_sid contains .., /, %2F, spaces, or characters outside [a-zA-Z0-9_-]
person_link_delete returns HTTP 303 redirect on a well-formed link_sid
The let _ = ... discard pattern is removed from person_link_delete

Notes

Service-layer validation (added for #41) is already complete and covers both context and link_sid
Issue #42 is exclusively about the handler not surfacing the validation error
HeroBizError does not implement IntoResponse, so per-handler boilerplate is consistent with existing patterns (e.g. line 423)

## Implementation Spec for Issue #42 ### Objective Prevent a directory traversal attack where a crafted `link_sid` URL parameter (e.g. `../../Cargo.toml`) can be used to delete arbitrary files on the filesystem through the `person_link_delete` HTTP handler. ### Current State The service layer is already fully fixed. `validate_path_component()` at `services/mod.rs:43-54` is called on both `context` and `link_sid` inside `delete_person_company_link` and `link_dir`. No filesystem operation will be performed on a traversal-containing path. The remaining gap is in the HTTP handler `person_link_delete` in `handlers/mod.rs`, which discards the error with `let _ = ...`. This means: - A traversal attempt is silently ignored (attacker gets no signal, operators get no log trace) - The handler redirects as if the operation succeeded - `HeroBizError::Validation` is never surfaced as HTTP 400 ### Requirements - The handler must propagate `HeroBizError::Validation` as HTTP 400, not a silent redirect - Follow the existing pattern: `(StatusCode::BAD_REQUEST, e.to_string()).into_response()` - No changes needed to `services/mod.rs` — validation is complete - Existing unit tests for `validate_path_component` must continue to pass ### Files to Modify - `crates/hero_biz_ui/src/web/handlers/mod.rs` — handle the `Result` from `delete_person_company_link` ### Implementation Plan #### Step 1: Fix `person_link_delete` handler File: `crates/hero_biz_ui/src/web/handlers/mod.rs` Replace the `let _ = ...` discard with a match that returns HTTP 400 on error: ```rust pub async fn person_link_delete( State(state): State<Arc<AppState>>, Path((context, person_id, link_sid)): Path<(String, String, String)>, ) -> impl IntoResponse { if let Err(e) = state.store.delete_person_company_link(&context, &link_sid) { return (StatusCode::BAD_REQUEST, e.to_string()).into_response(); } Redirect::to(&format!( "{}/c/{}/contacts/{}", effective_base_path(&state), context, person_id )) .into_response() } ``` `StatusCode` is already imported. No new imports needed. Dependencies: none ### Acceptance Criteria - [ ] `cargo build -p hero_biz_ui` passes with no errors - [ ] `cargo test -p hero_biz_ui` passes with no regressions - [ ] `person_link_delete` returns HTTP 400 when `link_sid` contains `..`, `/`, `%2F`, spaces, or characters outside `[a-zA-Z0-9_-]` - [ ] `person_link_delete` returns HTTP 303 redirect on a well-formed `link_sid` - [ ] The `let _ = ...` discard pattern is removed from `person_link_delete` ### Notes - Service-layer validation (added for #41) is already complete and covers both `context` and `link_sid` - Issue #42 is exclusively about the handler not surfacing the validation error - `HeroBizError` does not implement `IntoResponse`, so per-handler boilerplate is consistent with existing patterns (e.g. line 423)

casper-stevens commented

2026-05-07 09:58:48 +00:00

Author

Member

Test Results

Total: 7
Passed: 7
Failed: 0

All tests in hero_biz_ui passed successfully.

Test suite: cargo test -p hero_biz_ui

Tests run:

parser::tests::test_name_fix
services::tests::dot_dot_is_rejected
services::tests::empty_string_is_rejected
services::tests::null_byte_is_rejected
services::tests::path_traversal_sequences_rejected
services::tests::slash_is_rejected
services::tests::valid_components_pass

## Test Results - Total: 7 - Passed: 7 - Failed: 0 All tests in `hero_biz_ui` passed successfully. **Test suite:** `cargo test -p hero_biz_ui` **Tests run:** - `parser::tests::test_name_fix` - `services::tests::dot_dot_is_rejected` - `services::tests::empty_string_is_rejected` - `services::tests::null_byte_is_rejected` - `services::tests::path_traversal_sequences_rejected` - `services::tests::slash_is_rejected` - `services::tests::valid_components_pass`

casper-stevens commented

2026-05-07 10:00:55 +00:00

Author

Member

Fix Summary

File changed: crates/hero_biz_ui/src/web/handlers/mod.rs

The person_link_delete handler previously discarded the Result from delete_person_company_link with let _ = ..., silently redirecting even when the service rejected a traversal-containing link_sid. This meant:

Attackers received no error signal
Operators had no log trace of rejected attempts
The HTTP response was misleading (303 redirect instead of 400)

The service layer (validate_path_component added in #41) was already blocking the filesystem operation. This fix surfaces that rejection at the HTTP layer.

Change: replaced the silent discard with an explicit error check — the handler now returns HTTP 400 Bad Request with the validation error message when link_sid contains path traversal characters. A valid link_sid continues to redirect as before.

Tests: 7/7 pass (cargo test -p hero_biz_ui), including 6 tests specifically covering path component validation.

Closes #42

## Fix Summary **File changed:** `crates/hero_biz_ui/src/web/handlers/mod.rs` The `person_link_delete` handler previously discarded the `Result` from `delete_person_company_link` with `let _ = ...`, silently redirecting even when the service rejected a traversal-containing `link_sid`. This meant: - Attackers received no error signal - Operators had no log trace of rejected attempts - The HTTP response was misleading (303 redirect instead of 400) The service layer (`validate_path_component` added in #41) was already blocking the filesystem operation. This fix surfaces that rejection at the HTTP layer. **Change:** replaced the silent discard with an explicit error check — the handler now returns `HTTP 400 Bad Request` with the validation error message when `link_sid` contains path traversal characters. A valid `link_sid` continues to redirect as before. **Tests:** 7/7 pass (`cargo test -p hero_biz_ui`), including 6 tests specifically covering path component validation. Closes #42

casper-stevens referenced this issue from a commit

2026-05-07 10:12:48 +00:00

fix(security): return HTTP 400 when link_sid fails path validation in person_link_delete

casper-stevens closed this issue

2026-05-07 10:13:07 +00:00

Rows
Columns

security: path traversal via link_sid in delete_person_company_link #42

Summary

Fix

Affected code

Found in

Implementation Spec for Issue #42

Objective

Current State

Requirements

Files to Modify

Implementation Plan

Step 1: Fix person_link_delete handler

Acceptance Criteria

Notes

Test Results

Fix Summary

Step 1: Fix `person_link_delete` handler