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 marked this pull request as draft May 15, 2021
@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 issue 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
bagder added a commit that referenced this issue 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
lib/urlapi.c Show resolved Hide resolved
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 force-pushed the bagder/url_set-space branch from 40658cd to 313a9bd Compare Jun 1, 2021
@jay
Copy link
Member

@jay 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 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

@Karlson2k Karlson2k commented Oct 17, 2021

@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 issue 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 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants