Skip to content

FEAT: Add configurable safe parameter to UrlConverter - #3402

Merged
Richard Lundeen (richlundeen) merged 2 commits into
microsoft:mainfrom
Kaap10:fix/url-converter-safe-parameter
Oct 25, 2026
Merged

Richard Lundeen (richlundeen) merged 2 commits into
microsoft:mainfrom
Kaap10:fix/url-converter-safe-parameter

Conversation

@Kaap10

Copy link
Copy Markdown
Contributor

Description

Closes #3401

Adds a configurable safe parameter to UrlConverter:

  • Default safe="": Enforces complete URL encoding of reserved characters by default, including forward slashes (/ -> %2F), ensuring path traversal (../../etc/passwd) and SSRF payloads are properly encoded for evasion testing.
  • Custom safe Support: Callers wishing to preserve specific characters (such as / or /:) can configure UrlConverter(safe="/") or any custom string.
  • Identifiable & Serialization: Overrides _build_identifier() to serialize params={"safe": self._safe}, adhering to the converter contract.
  • Input Validation: Validates that safe is a string, raising TypeError otherwise.

Tests and Documentation

  • Added test_url_converter_default_encodes_forward_slash verifying that path traversal and SSRF payloads have slashes encoded as %2F by default.
  • Added test_url_converter_custom_safe testing safe="/", safe="/:", and query formatting safe="?=".
  • Added test_url_converter_property_and_identifier testing the property and identifier parameter serialization.
  • Added test_url_converter_invalid_safe_type verifying TypeError on invalid arguments.
  • Added test_url_converter_reconstruction verifying reconstruction parameters.
  • Ran converter unit tests and contracts: all 192 tests pass; ruff check and format pass.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the behavior of every existing UrlConverter() call. urllib.parse.quote currently defaults to safe="/", but the new default is safe="", so inputs with slashes now produce different prompts without any caller opting in.

If the configurable option is needed, please preserve safe="/" as the default and let callers request safe="" explicitly. The forwarding option and identifier are enough; the extra property and manual string-type check are not needed for this change. Otherwise, I recommend closing rather than changing the default behavior for this use case.

Reviewed with GitHub Copilot.

Keep slash preservation as the default and make full encoding opt-in. Remove redundant validation and the safe property while retaining behavioral identifiers and reconstruction.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@richlundeen
Richard Lundeen (richlundeen) added this pull request to the merge queue Oct 25, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 25, 2026
@richlundeen
Richard Lundeen (richlundeen) added this pull request to the merge queue Oct 25, 2026
Merged via the queue into microsoft:main with commit 5353769 Oct 25, 2026
65 checks passed
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.

BUG: UrlConverter preserves forward slashes by default, preventing path payload evasion

2 participants