[Silvertree2010]

Fixes: none (found while testing crates/fetch/src/http.rs:140 (Url::parse); no existing issue)

Problem

The URL parser accepts a signed port such as :+80. A URL like http://example.com:+80/x is accepted as port 80, and so is a redirect Location that goes through Url::join.

Cause: Url::parse read the port with p.parse::<u16>(), and Rust's integer parser accepts a leading +.

Change

The port text must now be all ASCII digits before it is parsed. Anything else returns BadUrl("bad port"). Because Url::join calls Url::parse, redirects get the same check.

Not in this PR: Port range and zero handling are unchanged: 0 and values above 65535 are still rejected. Host validation is unchanged.

How it was checked

• cargo xtask ci passes on this branch, rebased on current main. Note: it passes except accessibility::every_interactive_node_has_a_name_in_every_view_and_tool, which also fails on main without this change.
• A test that fails without this change: regression test http::tests::rejects_signed_ports fails on main without the fix and passes with it (cargo test -p lightcraft-fetch rejects_signed_ports)
• User-visible change: ran the app and looked at the result

Merging

Checklist

• No new panic path in non-test code: no unwrap / expect / panic!, no indexing or unchecked arithmetic on a number from a file, a person or an agent (get, checked_mul, NaN and char boundaries handled)
• New text on screen goes through tr() and has its key in crates/ui-egui/locales/*.json; a new shortcut is free in docs/library-shortcuts.md
• The feature's rows in docs/parity-checklist.md and its entry in docs/gaps.md are updated, honestly (🟡 when it is partial), in the same commit
• New code has a caller: no command, module or setting that nothing reaches yet
• Every new image, icon or other asset has its entry in assets/ATTRIBUTION.md; no font or media files added
• Ready to merge (otherwise open it as a draft)

🤖 Generated with AI by ApolloOffice, see the Co-Authored-By trailers for the models

Summary by CodeRabbit

• Bug Fixes
  • URLs and absolute redirects with ports written using a + or - sign are now rejected. Valid unsigned ports, including port 80, continue to be accepted.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: storytold/lightcraft
• Branch: main
• File: .coderabbit.yaml
• Commit: 59a5d00
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 44c32363-5745-458b-b12a-867e019ba3cb

Commits

Reviewing files that changed from the base of the PR and between 59a5d00 and 1384930.

Files selected for processing: 1

• crates/fetch/src/http.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.

Walkthrough

URL parsing now rejects ports that contain characters other than ASCII digits. Tests cover signed ports in HTTP and bracketed IPv6 URLs, signed ports in absolute redirects, and acceptance of port 80.

Changes: URL port parsing

Layer / File(s)	Summary
Port validation and tests crates/fetch/src/http.rs	Url::parse checks that port text contains only ASCII digits before parsing. Existing nonzero and u16 range checks remain. Tests cover signed ports in URLs and redirects, and acceptance of port 80.

Priority: Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: echelon, endthestart

Merge Risk: Minimal · up to 13849

Signed URL ports are rejected as intended, including redirects, without an identified merge-blocking risk.

Security architecture risk: Minimal · up to 13849

The change rejects signed ports before requests proceed. It preserves existing port limits, redirect protections, and request destinations for URLs that remain valid. No material security risk was found introduced or worsened by this change.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The changed exposure is limited to rejecting previously accepted signed-port URLs in the inspected fetch path. URLs that remain accepted retain the same host, port, TLS selection, and path; no additional destination or privilege becomes reachable.

Trust Boundaries and Controls

• observed — Redirects that introduce an authority, including absolute and network-path references, re-enter Url::parse. Relative references retain the existing parsed authority, so they cannot introduce signed port text around the new gate.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: rejecting signed ports in Url.parse.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 1 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}