Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

urlapi: reject spaces in URLs, allow if flagbit set #7073

Closed
wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented May 15, 2021

They were never officially allowed and slipped in only due to sloppy
parsing. Spaces (ascii 32) should be correctly encoded (to %20) before
being part of a URL.

The new flag bit CURLU_ALLOW_SPACE when a full URL is set, makes libcurl allow spaces.

@bagder bagder added URL libcurl API feature-window A merge of this requires an open feature window labels May 15, 2021
@bagder bagder marked this pull request as draft May 15, 2021 16:09
@bagder bagder changed the title urlapi: reject spaces in URLs urlapi: reject spaces in URLs, allow if flagbit set May 15, 2021
bagder added a commit that referenced this pull request May 15, 2021
They were never officially allowed and slipped in only due to sloppy
parsing. Spaces (ascii 32) should be correctly encoded (to %20) before
being part of a URL.

The new flag bit CURLU_ALLOW_SPACE when a full URL is set, makes libcurl
allow spaces.

Closes #7073
@bagder bagder marked this pull request as ready for review May 24, 2021 14:20
bagder added a commit that referenced this pull request May 24, 2021
They were never officially allowed and slipped in only due to sloppy
parsing. Spaces (ascii 32) should be correctly encoded (to %20) before
being part of a URL.

The new flag bit CURLU_ALLOW_SPACE when a full URL is set, makes libcurl
allow spaces.

Updated test 1560 to verify.

Closes #7073
@bagder bagder removed the feature-window A merge of this requires an open feature window label May 31, 2021
They were never officially allowed and slipped in only due to sloppy
parsing. Spaces (ascii 32) should be correctly encoded (to %20) before
being part of a URL.

The new flag bit CURLU_ALLOW_SPACE when a full URL is set, makes libcurl
allow spaces.

Updated test 1560 to verify.

Closes #7073
@jay
Copy link
Member

jay commented Jun 13, 2021

@bagder did you intend to merge this? I notice you removed next-feature-window a while back.

@bagder
Copy link
Member Author

bagder commented Jun 14, 2021

I did mean to do that. I've hesitated a little but I'll move forward on this again in a bit.

@Karlson2k
Copy link
Contributor

@bagder Maybe mark in the documentation the first version where CURLU_ALLOW_SPACE is available?
Currently one reading documentation may think that CURLU_ALLOW_SPACE is available with the very first implementation of curl_url().

spdk-bot pushed a commit to spdk/spdk that referenced this pull request Dec 14, 2021
Latest curl versions started to treat such URLs as invalid. See:
curl/curl#7073

Signed-off-by: Michal Berger <michalx.berger@intel.com>
Change-Id: Ic5ab34a566f89f411ec40cbcb8de57a8d2f3ea88
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/10607
Reviewed-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>
Reviewed-by: Maciej Wawryk <maciejx.wawryk@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
@bagder
Copy link
Member Author

bagder commented Jan 7, 2022

Chromium often leaves spaces in the URL bar alone

The browsers' "URL bars" don't show URLs and don't follow the URL syntax. Normally however, if you copy the data in the URL bar they tend to store that copy in the clipboard in a better URL format.

I don't suppose curl could be made to check to see if there's an environment variable named CURLU_ALLOW_SPACE set to 1?

Everything is always debatable, but having the argument in an old closed pull-request is not the most suitable place for this discussion.

keithalucas pushed a commit to longhorn/spdk that referenced this pull request Jun 30, 2022
Latest curl versions started to treat such URLs as invalid. See:
curl/curl#7073

Signed-off-by: Michal Berger <michalx.berger@intel.com>
Change-Id: Ic5ab34a566f89f411ec40cbcb8de57a8d2f3ea88
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/10607
Reviewed-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>
Reviewed-by: Maciej Wawryk <maciejx.wawryk@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants