Skip to content

Add url encoding option to URL.resolve#1646

Merged
karlseguin merged 1 commit intomainfrom
url_encoding
Feb 25, 2026
Merged

Add url encoding option to URL.resolve#1646
karlseguin merged 1 commit intomainfrom
url_encoding

Conversation

@karlseguin
Copy link
Collaborator

Given:

a.href = "over 9000!"

Then:

a.href === BASE_URL + '/over%209000!';

This commits adds an escape: bool option to URL.resolve which will escape the path, query and fragment when true.

Also changes the Anchor, Image, Link and IFrame getSrc to escape. Escaping is also used when navigating a frame.

Given:

a.href = "over 9000!"

Then:

a.href === BASE_URL + '/over%209000!';

This commits adds an escape: bool option to URL.resolve which will escape the
path, query and fragment when true.

Also changes the Anchor, Image, Link and IFrame getSrc to escape. Escaping is
also used when navigating a frame.
@karlseguin
Copy link
Collaborator Author

@krichprollsch I didn't do it in this PR, but should the main URL we pass to navigate also be escaped? If you:

curl "https://lightpanda.io/hello world"

You'll get an error, if you ./lightpanda fetch "https://lightpanda.io/hello world" you get that same error.

If we decide not to escape the main navigation url, then we need to change https://github.com/lightpanda-io/demo/pull/117/changes#diff-15ff6b6f810cbccf0c86f043b8815c1ca9e84110c983dba1958ac3f993f311b5R286 to escape the URL..because, right now, many WPT tests are failing to navigate because it's an invalid URL.

@krichprollsch
Copy link
Member

I will update the runner to escape the URL, seems OK to refuse invalid URL input.

@krichprollsch
Copy link
Member

krichprollsch commented Feb 25, 2026

Hum, I tested with Chrome and cdpcli, and calling cdpcli dump "https://httpbin.io/get?foo bar" on chrome, it works... So I guess we should escape URLs after all...

But it's not that easy, we musn't double escape the url:

$ cdpcli dump "https://httpbin.io/get?foo bar"
// ...
  "url": "http://httpbin.io/get?foo%20bar"
$ cdpcli dump "https://httpbin.io/get?foo%20bar"
// ...
  "url": "http://httpbin.io/get?foo%20bar"

And this is not a feature from the client:

> {"method":"Page.navigate","params":{"url":"https://httpbin.io/get?foo bar","frameId":"768705AEC2AD8746E3F1AD4AAC574E3C"},"id":29,"sessionId":"FF654D0CB6DF14FD02F5EB14FF9FEECF"}
< {"method":"Page.frameStartedNavigating","params":{"frameId":"768705AEC2AD8746E3F1AD4AAC574E3C","url":"https://httpbin.io/get?foo%20bar","loaderId":"04DA9A2AF5CD30B261875EB8D0542772"
,"navigationType":"differentDocument"},"sessionId":"FF654D0CB6DF14FD02F5EB14FF9FEECF"}

@karlseguin karlseguin merged commit db8fb8b into main Feb 25, 2026
10 checks passed
@karlseguin karlseguin deleted the url_encoding branch February 25, 2026 23:02
@github-actions github-actions bot locked and limited conversation to collaborators Feb 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants