Skip to content

fix(destination): reject non-digit proxy port prefixes - #159

Open
seonghobae wants to merge 5 commits into
mainfrom
fix/proxy-port-digit-contract
Open

fix(destination): reject non-digit proxy port prefixes#159
seonghobae wants to merge 5 commits into
mainfrom
fix/proxy-port-digit-contract

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Buyer/security gap

OriginWeave's explicit proxy authority parsed proxy ports with Rust integer syntax before constructing its validation origin. Rust accepts a leading +, while browser URL-style port syntax admits ASCII decimal digits only. Protected main could therefore admit identifiers such as http://proxy.example:+8080, creating a parser-authority mismatch at the proxy trust boundary.

TDD lineage

This branch starts directly from protected main 0c376acf059be9ddddddfbde1d0189e4f39ef014.

Exact test-only head a9d432044c549c1926ee56143f1b3e8ca07e9ac3 added scheme-less HTTP, explicit HTTP/HTTPS, SOCKS5, and bracketed IPv6 regressions requiring leading-plus port spellings to fail as ProxyServerError::InvalidIdentifier before production changed. The current implementation then adds the narrow parser-local ASCII-decimal check and retains zero/out-of-range rejection.

Implemented boundary

Current exact head adb09a40699a70b7c7e252cd88dec42d41833b1f:

  • requires every explicit proxy port token to contain only ASCII decimal digits before u16 parsing;
  • rejects leading-plus syntax for scheme-less HTTP, HTTP, HTTPS, SOCKS5, and bracketed IPv6 identifiers;
  • preserves digit-only ports, existing default-port canonicalization, and zero/out-of-range rejection; and
  • records the security boundary in CHANGELOG.md.

The exact three-file diff is limited to the proxy parser, its focused integration regression, and the canonical changelog entry.

Exact-head verification

On unchanged exact head adb09a40699a70b7c7e252cd88dec42d41833b1f against exact protected main 0c376acf059be9ddddddfbde1d0189e4f39ef014:

  • CI 31836561095: success;
  • Rust contracts job 94884100756: repository contracts, formatting, locked workspace/all-target checks, full tests, strict Clippy, and rustdoc success;
  • Production coverage job 94884100898: exact owned-production function, line, region, and branch coverage enforcement success;
  • SAST Semgrep 31836561124: success;
  • Security Scan 31836561125: success;
  • GitHub reports the PR Ready, open, and mergeable;
  • no formal reviews or unresolved inline review threads are currently returned.

No queued, skipped, synthetic, predecessor-head, or model-only result is promoted as current proof.

Scope boundary

This changes no DNS, destination classification, socket, TLS, PAC evaluation, workflow, secret, persistence, browser execution, or release behavior. It is independently defensive from the canonical-origin parser fix in #158.

Passing automation is not independent approval. Protected integration remains subject to the live ruleset and unchanged-head review evidence.

@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 59 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fbf68479-72ea-4313-85a8-0727268718f8

📥 Commits

Reviewing files that changed from the base of the PR and between adb09a4 and fc11916.

📒 Files selected for processing (1)
  • CHANGELOG.md

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0c2b57b3-e4d2-48ba-b72e-f7b97b95d5a0

📥 Commits

Reviewing files that changed from the base of the PR and between 0c376ac and adb09a4.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • crates/originweave-destination/src/proxy.rs
  • crates/originweave-destination/tests/proxy_port_syntax.rs

📝 Walkthrough

Walkthrough

프록시 포트 파서는 숫자 범위 검증 전에 ASCII 십진수 형식을 확인합니다. + 접두사와 u16 범위를 초과하는 포트는 InvalidIdentifier 오류로 거부됩니다. 관련 테스트와 변경 로그를 추가했습니다.

Changes

프록시 포트 검증

Layer / File(s) Summary
포트 토큰 검증 및 회귀 테스트
crates/originweave-destination/src/proxy.rs, crates/originweave-destination/tests/proxy_port_syntax.rs, CHANGELOG.md
포트 문자열이 ASCII 숫자로만 구성되는지 확인합니다. + 접두사와 65536 포트를 InvalidIdentifier 오류로 거부하는 테스트를 추가했습니다. 변경 로그에 보안 동작을 기록했습니다.

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

Merge Risk: ⚪ Minimal · up to adb09

The PR makes a localized proxy-port validation change with no actionable merge-blocking risk remaining beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 프록시 포트 접두사의 비숫자 문자를 거부하는 주요 변경 사항을 정확하고 간결하게 설명합니다.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/proxy-port-digit-contract

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.

❤️ Share

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

@seonghobae
seonghobae marked this pull request as ready for review August 14, 2026 20:45
@cursor

cursor Bot commented Aug 16, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant