Skip to content

Conversation

@ljluestc
Copy link

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

This pull request addresses a bug in the IsRequestURI function where it incorrectly validates strings like "foobar.com" as true, despite them not being valid HTTP request URIs per RFC 3986 and HTTP expectations. The change targets the master branch as it’s a bugfix.

Bug Details

  • Are you fixing a bug or providing a failing unit test to demonstrate a bug?

    • Yes, fixing a bug demonstrated by an existing failing unit test in TestIsRequestURI.
  • How do you reproduce it?

    • Run the test suite with go test -v -run TestIsRequestURI. The test case for "foobar.com" expects false but gets true with the current implementation:
      {"foobar.com", false},
  • What did you expect to happen?

    • IsRequestURI("foobar.com") should return false because it’s neither an absolute URI (e.g., http://foobar.com) nor an absolute path (e.g., /foobar), and it doesn’t qualify as a valid relative path in an HTTP request context without a scheme or leading slash.
  • What actually happened?

    • The current implementation uses url.ParseRequestURI, which is too permissive and accepts "foobar.com" as a valid opaque URI, returning true. This caused the test to fail with:
      validator_test.go:980: Expected IsRequestURI("foobar.com") to be false, got true
      

Changes Made

  • Updated IsRequestURI to:
    • Use url.Parse instead of url.ParseRequestURI for broader parsing.
    • Add stricter checks:
      • Accept absolute URIs (with a scheme like http://).
      • Accept absolute paths (starting with /).
      • Accept valid relative paths (e.g., rel/test/dir, ./rel/test/dir).
      • Reject domain-like strings without a scheme or leading slash (e.g., "foobar.com").
  • The new logic ensures "foobar.com" returns false while preserving correct behavior for other valid cases.

Testing

  • Verified locally with go test -v -run TestIsRequestURI, and the test case for "foobar.com" now passes.
  • Ran the full test suite (go test -v) to confirm no regressions.

Why This Change is Necessary

  • The bug causes IsRequestURI to misclassify invalid HTTP request URIs, potentially leading to incorrect validation in applications relying on this function. This fix aligns the function with HTTP expectations and resolves the failing test case.

Target Branch

  • master: This is a bugfix addressing an existing issue in the current codebase, not a new feature or refactor.

@asaskevich asaskevich marked this pull request as ready for review March 21, 2025 10:01
@asaskevich
Copy link
Owner

Hi, @ljluestc! Thank you! I'll take a look into it and merge

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.

2 participants