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

tool: avoid including leading spaces in the Location hyperlink #11735

Closed

Conversation

mssdvd
Copy link
Contributor

@mssdvd mssdvd commented Aug 25, 2023

This PR ensures that leading whitespaces are not marked as part of the Location URL.

Before:
image

After:
image

src/tool_cb_hdr.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented Aug 25, 2023

When I do curl -I curl.se, my terminal (Konsole) does not include the leading whitespace in the link.

@mssdvd
Copy link
Contributor Author

mssdvd commented Aug 25, 2023

alacritty, foot and kitty include the space. I believe Konsole disables escape sequences for links by default.

@bagder
Copy link
Member

bagder commented Aug 25, 2023

Ah, now I see the problem. Isn't this patch simpler?

diff --git a/src/tool_cb_hdr.c b/src/tool_cb_hdr.c
index aab96ae03..b1bad12e0 100644
--- a/src/tool_cb_hdr.c
+++ b/src/tool_cb_hdr.c
@@ -399,12 +399,12 @@ void write_linked_location(CURL *curl, const char *location, size_t loclen,
 
   if(!strcmp("http", scheme) ||
      !strcmp("https", scheme) ||
      !strcmp("ftp", scheme) ||
      !strcmp("ftps", scheme)) {
-    fprintf(stream, LINK "%s" LINKST "%.*s" LINKOFF,
-            finalurl, (int)loclen, location);
+    fprintf(stream, " " LINK "%s" LINKST "%.*s" LINKOFF,
+            finalurl, (int)loclen, loc);
     goto locdone;
   }
 
   /* Not a "safe" URL: don't linkify it */

I believe Konsole disables escape sequences for links by default.

No it doesn't. At least I get it linkified just fine.

@bagder
Copy link
Member

bagder commented Aug 25, 2023

Or maybe this is even better:

diff --git a/src/tool_cb_hdr.c b/src/tool_cb_hdr.c
index aab96ae03..878e8a563 100644
--- a/src/tool_cb_hdr.c
+++ b/src/tool_cb_hdr.c
@@ -399,12 +399,12 @@ void write_linked_location(CURL *curl, const char *location, size_t loclen,
 
   if(!strcmp("http", scheme) ||
      !strcmp("https", scheme) ||
      !strcmp("ftp", scheme) ||
      !strcmp("ftps", scheme)) {
-    fprintf(stream, LINK "%s" LINKST "%.*s" LINKOFF,
-            finalurl, (int)loclen, location);
+    fprintf(stream, " " LINK "%s" LINKST "%s" LINKOFF "\n",
+            finalurl, copyloc);
     goto locdone;
   }
 
   /* Not a "safe" URL: don't linkify it */
 

@bagder
Copy link
Member

bagder commented Aug 25, 2023

/cc @dfandrich

@mssdvd
Copy link
Contributor Author

mssdvd commented Aug 25, 2023

I had thought of fixing it like that. Is there a chance that not specifying the length will lead to bugs?

@bagder
Copy link
Member

bagder commented Aug 25, 2023

Not that I can spot since copyloc is a null-terminated string it should be fine. But I still would like to get @dfandrich to chime in, he wrote this code.

@dfandrich
Copy link
Contributor

The original intention was to preserve the whitespace so the user gets the same output with or without styling enabled (modula the styling, of course). The proposed patch strips leading whitespace and replaces it with a single character, which is often, but not always, the same. I didn't bother eliminating the leading whitespace from the linked section, but I can see how it could be annoying on some terminals.

What I would probably do instead is count the amount of whitespace stripped in the /* Strip leading whitespace of the redirect URL */ loop by adding a ++space_skipped in the loop, then change the fprintf to:

    fprintf(stream, "%.*s" LINK "%s" LINKST "%.*s" LINKOFF,
             space_skipped, location, finalurl, (int)loclen - space_skipped, loc);

This is untested but the idea is to print the leading whitespace as-is, then the rest of the linkification without that leading space.

@mssdvd mssdvd force-pushed the avoid-leading-spaces-Location-link branch from 5018ef7 to 2ec5d1b Compare August 28, 2023 22:31
Copy link
Contributor

@dfandrich dfandrich left a comment

Choose a reason for hiding this comment

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

It works for me!

src/tool_cb_hdr.c Outdated Show resolved Hide resolved
Copy link
Contributor

@dfandrich dfandrich left a comment

Choose a reason for hiding this comment

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

skip tabs

Co-authored-by: Dan Fandrich <dan@coneharvesters.com>
@mssdvd mssdvd force-pushed the avoid-leading-spaces-Location-link branch from 2ec5d1b to ad7d832 Compare August 29, 2023 09:30
@dfandrich dfandrich closed this in 226d042 Aug 29, 2023
@dfandrich
Copy link
Contributor

Thanks!

@mssdvd mssdvd deleted the avoid-leading-spaces-Location-link branch August 29, 2023 17:50
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
Co-authored-by: Dan Fandrich <dan@coneharvesters.com>

Closes curl#11735
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

3 participants