feat: add scheme length validation to prevent invalid protocols#39
feat: add scheme length validation to prevent invalid protocols#39
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds defensive validation on parsed proxy URL schemes by enforcing a reasonable length range before proceeding with protocol-specific handling, returning ErrInvalidProxy when the scheme is empty or excessively long. Sequence diagram for proxy URL parsing with scheme length validationsequenceDiagram
participant Caller
participant Parser as ParseProxyURL
participant URLPkg as url_Parse
Caller->>Parser: ParseProxyURL(proto, proxyURL)
Parser->>URLPkg: Parse(proxyURL)
URLPkg-->>Parser: URL u or error
alt url parse error
Parser-->>Caller: return nil, ErrInvalidProxy
else url parse ok
Parser->>Parser: scheme = toLower(u.Scheme)
alt invalid scheme length
Parser-->>Caller: return nil, ErrInvalidProxy
else valid scheme length
Parser->>Parser: switch scheme
Parser-->>Caller: return Proxy, nil
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughA new exported constant Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ Finishing touches
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 |
f6237cb to
99d9238
Compare
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The hardcoded
15for maximum scheme length is a magic number; consider extracting it into a named constant (with a brief comment on why 15) so future maintainers understand and can safely adjust the limit. - There is an extra blank line added before the
defaultcase body in the switch; consider removing it to keep the formatting consistent with the other cases.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hardcoded `15` for maximum scheme length is a magic number; consider extracting it into a named constant (with a brief comment on why 15) so future maintainers understand and can safely adjust the limit.
- There is an extra blank line added before the `default` case body in the switch; consider removing it to keep the formatting consistent with the other cases.
## Individual Comments
### Comment 1
<location> `internal/parser.go:52-53` </location>
<code_context>
scheme := strings.ToLower(u.Scheme)
+ // Validate scheme length to prevent invalid protocols
+ if len(scheme) == 0 || len(scheme) > 15 {
+ return nil, ErrInvalidProxy
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The hard-coded 15-character scheme length limit may reject valid or future proxy schemes.
Since RFC 3986 does not specify a length limit for schemes and some valid schemes exceed 15 characters, this cap risks rejecting legitimate or future protocols. If the goal is input validation, consider checking only that the scheme is non-empty and matches the allowed character pattern (e.g. `[A-Za-z][A-Za-z0-9+.-]*`), or, if a product-specific limit is required, document it and extract the limit into a named constant for easier adjustment.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ve extra blank line
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.