[Chibey-max]

The OHTTP relay parsed gateway URL prefixes using fixed byte
slices. A short malformed path such as /http:/ could therefore
panic instead of returning a gateway parsing error.

This change replaces the unchecked slices with starts_with checks
and adds a regression test for the malformed path.

Reviewer: @DanGould

AI disclosure: I only used AI assistance to inspect the codebase. I reviewed ,investigated the edge case and wrote the code and test results myself.

[codaMW]

tACK 77617108

Confirmed the fix and the panic it removes.

The bug: parse_gateway_uri_from_path guards the empty and "/" cases, but a short non-empty path still reaches &path[..7] / &path[..8]. Byte-indexing panics when the string is shorter than the index (and would also panic on a non-char boundary), so a path like /http:/ (6 bytes after the leading slash) blows up. The caller uses parse_gateway_uri_from_path(path, ...).ok(), so this turns a malformed request into a panic in the handler rather than a graceful None a robustness fix, not just style.

The fix: starts_with("http://") / starts_with("https://") returns false safely for any length and never splits a UTF-8 char, so the short-path and boundary cases both fall through to the Authority::from_str branch (which returns Err). Correct and idiomatic.

Verified locally (nightly-2026-09-28):

• cargo test -p payjoin-mailroom short_gateway_path -> 1 passed on this branch.
• Mutation: reverting only the condition back to &path[..7]/&path[..8] makes the same test panic with end byte index 7 is out of bounds for string of length 6, so the test is load-bearing and pins the exact bug.

LGTM.

---

Review drafted with AI assistance; I ran the test and the mutation myself.

[Manuel1234477]

Tested locally after rebasing onto master: the fix is correct, and cargo fmt, clippy -D warnings, codespell and the payjoin-mailroom tests all pass. I confirmed the new test fails (panics at mod.rs:251) without the fix.

The old slicing panicked on more than malformed paths. Any path shorter than 8 bytes after the / that doesn't start with http:// hit it, including valid short gateway hosts such as /a.io or /1.2.3.4. This PR fixes those too, and valid paths still parse the same as before.

What should be done before merge:

1. Broaden the test. Add cases for:
   • /a.io returns Ok (a valid short host that used to panic)
   • /https:/ returns Err (covers the old [..8] slice)
   • /https://payjo.in still parses
2. Add a commit body. The commit has only a subject line. CONTRIBUTING asks the body to explain what and why, and the PR description would work well there.
3. Rebase onto current master. The branch is behind, but it rebases cleanly.

Optional: &path[1..] still assumes the path starts with /. Hyper always gives one, so it's safe, but path.strip_prefix('/') would make that explicit.

For context on severity: connections are served in their own tokio::spawn task, so the panic only dropped that one connection (the client was disconnected instead of getting a 400 error). It did not crash the relay.

[Chibey-max]

Tested locally after rebasing onto master: the fix is correct, and cargo fmt, clippy -D warnings, codespell and the payjoin-mailroom tests all pass. I confirmed the new test fails (panics at mod.rs:251) without the fix.

The old slicing panicked on more than malformed paths. Any path shorter than 8 bytes after the / that doesn't start with http:// hit it, including valid short gateway hosts such as /a.io or /1.2.3.4. This PR fixes those too, and valid paths still parse the same as before.

What should be done before merge:

1. Broaden the test. Add cases for:

   • /a.io returns Ok (a valid short host that used to panic)
   • /https:/ returns Err (covers the old [..8] slice)
   • /https://payjo.in still parses

2. Add a commit body. The commit has only a subject line. CONTRIBUTING asks the body to explain what and why, and the PR description would work well there.

3. Rebase onto current master. The branch is behind, but it rebases cleanly.

Optional: &path[1..] still assumes the path starts with /. Hyper always gives one, so it's safe, but path.strip_prefix('/') would make that explicit.

For context on severity: connections are served in their own tokio::spawn task, so the panic only dropped that one connection (the client was disconnected instead of getting a 400 error). It did not crash the relay.

Thanks for the thorough review @Manuel1234477 . I just added the requested regression cases for /a.io, /https:/, and /https://payjo.in, then rebased the branch onto the latest master. The focused payjoin-mailroom tests and formatting checks still pass.

I also updated the commit message with the rationale and test coverage, Thanks !.

[Chibey-max]

tACK 77617108

Confirmed the fix and the panic it removes.

The bug: parse_gateway_uri_from_path guards the empty and "/" cases, but a short non-empty path still reaches &path[..7] / &path[..8]. Byte-indexing panics when the string is shorter than the index (and would also panic on a non-char boundary), so a path like /http:/ (6 bytes after the leading slash) blows up. The caller uses parse_gateway_uri_from_path(path, ...).ok(), so this turns a malformed request into a panic in the handler rather than a graceful None a robustness fix, not just style.

The fix: starts_with("http://") / starts_with("https://") returns false safely for any length and never splits a UTF-8 char, so the short-path and boundary cases both fall through to the Authority::from_str branch (which returns Err). Correct and idiomatic.

Verified locally (nightly-2026-09-28):

• cargo test -p payjoin-mailroom short_gateway_path -> 1 passed on this branch.
• Mutation: reverting only the condition back to &path[..7]/&path[..8] makes the same test panic with end byte index 7 is out of bounds for string of length 6, so the test is load-bearing and pins the exact bug.

LGTM.

Review drafted with AI assistance; I ran the test and the mutation myself.

Thanks for the review @codaMW !

[codaMW]

Re-tACK 0641a87 after the rebase onto master.

The fix line is unchanged from my earlier tACK (7761710): starts_with("http://") || starts_with("https://"), which never slices and never splits a UTF-8 char, so the correctness argument and the mutation I posted before still hold. What changed is the rebase plus the broadened coverage @Manuel1234477 asked for, so I re-ran it to confirm the new state.

Verified on 0641a87, repo-pinned nightly, --all-features, running the whole ohttp_relay::tests module so every case fires:

• short_gateway_path_returns_error: /http:/ and /https:/ return Err (the short non-empty paths that used to index past the end)
• short_gateway_host_and_full_gateway_path_parse: /a.io returns Ok (a valid short host that previously panicked) and /https://payjo.in parses to the expected GatewayUri

Both pass. That closes the gap I care about: the regression now pins not just the panic cases but also that a valid short host and a full URI still parse, so a future refactor cannot silently narrow the accepted set.

On the optional note about &path[1..]: agreed it is safe as-is, since hyper always supplies the leading slash, so strip_prefix would be cosmetic. No objection either way.

LGTM. Review drafted with AI assistance; I ran the tests myself.

[caarloshenriq]

Concept ACK 0641a87

Reproduced locally: both new tests pass, and both panic at the old slice when the fix is reverted. starts_with is the right minimal fix, and the remaining &path[1..] is safe since hyper paths always start with /.

One thing before ACK: payjoin-mailroom is a release crate, so this needs a changelog line (inline). Happy to ACK once that’s in.

[Chibey-max]

Concept ACK 0641a87

Reproduced locally: both new tests pass, and both panic at the old slice when the fix is reverted. starts_with is the right minimal fix, and the remaining &path[1..] is safe since hyper paths always start with /.

One thing before ACK: payjoin-mailroom is a release crate, so this needs a changelog line (inline). Happy to ACK once that’s in.

Thanks !! i have addressed this. Added the release note under payjoin-mailroom/CHANGELOG.md under ## Unreleased.

[caarloshenriq]

ACK b7de016

[caarloshenriq]

the branch now has a merge commit from master (99cc84e), and resolving it dropped your changelog line. On the PR head, payjoin-mailroom/CHANGELOG.md only has the #1983 entry under Unreleased, so the changelog check will fail again. The project prefers a linear history, so could you rebase instead of merging?

[Chibey-max]

the branch now has a merge commit from master (99cc84e), and resolving it dropped your changelog line. On the PR head, payjoin-mailroom/CHANGELOG.md only has the #1983 entry under Unreleased, so the changelog check will fail again. The project prefers a linear history, so could you rebase instead of merging?

Fixed , thanks !!

[caarloshenriq]

re-ACK 162b308