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

lib: add CURLFOLLOW_OBEYCODE and *FIRSTONLY #16473

Closed
wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Feb 25, 2025

With this change, the argument passed to the CURLOPT_FOLLOWLOCATION option is treated as a mode option instead of a boolean.

CURLFOLLOW_ALL

This is the old value and makes redirects with custom methods work exactly like before: it changes the method for all requests, independently of what response codes are returned.

CURLFOLLOW_OBEYCODE

libcurl obeys the HTTP server instruction in the response code: for 301, 302, 303 it changes to GET. For 307 and 308 it keeps the custom method even in the redirect.

CURLFOLLOW_FIRSTONLY

If this is set, the method is stored to either GET or POST/PUT on the first redirect. The custom method is only ever used in the first outgoing request.


This change is forward compatible because CURLOPT_FOLLOWLOCATION has been documented to accept the exact value of '1' to enable redirect following and therefore all other bits were left unused and undefined. We now add two other non-zero values. Starting in 8.13.0, the values 2 and 3 add more meaning.

This change is backward compatible in the following way: setting the modes in a program that still uses an older libcurl installation at runtime will have no effect. This is because older libcurl code checked if the value was non-zero and then enabled redirect following. Of course older libcurl will always let the set CURLOPT_CUSTOMMETHOD string override the method, disregarding what the HTTP response code suggests.

libtest 1571 to to 1581 verify

@bagder bagder added the HTTP label Feb 25, 2025
@bagder bagder force-pushed the bagder/CURLFOLLOW_OBEYCODE branch from 21433bc to 9e8cad3 Compare February 25, 2025 10:00
@github-actions github-actions bot added the CI Continuous Integration label Feb 25, 2025
@bagder

This comment was marked as outdated.

@vszakats

This comment was marked as outdated.

@testclutch

This comment was marked as outdated.

@bagder
Copy link
Member Author

bagder commented Feb 25, 2025

#16487 and #16482 should ideally fix them. I will rebase on top of master once they both have been merged.

@vszakats
Copy link
Member

Confirmed this also affects gcc-13 and gcc-14, thus gcc-specific in general (and not gcc-12 in particular):
https://github.com/curl/curl/actions/runs/13534146044/job/37822591439?pr=16492

bagder added a commit that referenced this pull request Feb 26, 2025
With this change, the argument passed to the CURLOPT_FOLLOWLOCATION
option is treated as a bitmask instead of a boolean. If the new
CURLFOLLOW_OBEYCODE bit is set in the bitmask, it means that libcurl
rather obeys the HTTP server instruction in the response code: for 301,
302, 303 it changes to GET. For 307 and 308 it keeps the custom method
even in the redirect.

Without CURLFOLLOW_OBEYCODE set, libcurl sticks to sending the custom
method for every redirect disregarding the response code.

This change is forward compatible because CURLOPT_FOLLOWLOCATION has
been documented to accept the exact value of '1' to enable redirect
following and therefore all other bits were left unused and undefined.
We now add value to another bit. Starting in 8.13.0, the value 1 and the
first bit still enables plain redirect following but the second bit adds
more meaning.

This change is backward compatible in the following way: setting the
CURLFOLLOW_OBEYCODE bit in a program that still uses an older libcurl
installation at run-tim will have no effect. This is because older
libcurl code checked if the value was non-zero and then enabled redirect
following. Of course older libcurl will always let the set
CURLOPT_CUSTOMMETHOD string override the method, disregarding what the
HTTP response code suggests.

libtest 1571 and 1572 verify

Closes #16473
@bagder bagder force-pushed the bagder/CURLFOLLOW_OBEYCODE branch from c355f27 to f74a277 Compare February 26, 2025 07:00
@bagder
Copy link
Member Author

bagder commented Feb 27, 2025

There's perhaps one question to decide:

If we replace GET as in curl -X MOO -L localhost and that 301-redirects to a new URL.

With *OBEYCODE set, should it change the follow-up method to GET or not? The current code does not, since the user has replaced the method for GET.

@bagder
Copy link
Member Author

bagder commented Feb 27, 2025

Maybe we need another bit for ONLY-FOR-FIRST-REQUEST that then resets back unconditionally on the subsequent... Maybe that's the behavior people actually want?

@bagder bagder changed the title lib: add CURLFOLLOW_OBEYCODE lib: add CURLFOLLOW_OBEYCODE and *FIRSTONLY Feb 27, 2025
bagder added a commit that referenced this pull request Feb 27, 2025
With this change, the argument passed to the CURLOPT_FOLLOWLOCATION
option is treated as a bitmask instead of a boolean. If the new
CURLFOLLOW_OBEYCODE bit is set in the bitmask, it means that libcurl
rather obeys the HTTP server instruction in the response code: for 301,
302, 303 it changes to GET. For 307 and 308 it keeps the custom method
even in the redirect.

Without CURLFOLLOW_OBEYCODE set, libcurl sticks to sending the custom
method for every redirect disregarding the response code.

This change is forward compatible because CURLOPT_FOLLOWLOCATION has
been documented to accept the exact value of '1' to enable redirect
following and therefore all other bits were left unused and undefined.
We now add value to another bit. Starting in 8.13.0, the value 1 and the
first bit still enables plain redirect following but the second bit adds
more meaning.

This change is backward compatible in the following way: setting the
CURLFOLLOW_OBEYCODE bit in a program that still uses an older libcurl
installation at run-tim will have no effect. This is because older
libcurl code checked if the value was non-zero and then enabled redirect
following. Of course older libcurl will always let the set
CURLOPT_CUSTOMMETHOD string override the method, disregarding what the
HTTP response code suggests.

libtest 1571 and 1572 verify

Closes #16473
@bagder bagder force-pushed the bagder/CURLFOLLOW_OBEYCODE branch from f74a277 to af7e48f Compare February 27, 2025 12:12
bagder added a commit that referenced this pull request Feb 27, 2025
With this change, the argument passed to the CURLOPT_FOLLOWLOCATION
option is now instead a "mode" instead of just a boolean. Documentation
is extended to describe the two new modes.

libtest 1571 to 1575 verify.

Closes #16473
@bagder bagder force-pushed the bagder/CURLFOLLOW_OBEYCODE branch from b23f4c7 to b1afa0b Compare February 27, 2025 12:22
bagder added a commit that referenced this pull request Feb 27, 2025
With this change, the argument passed to the CURLOPT_FOLLOWLOCATION
option is now instead a "mode" instead of just a boolean. Documentation
is extended to describe the two new modes.

Test 1571 to 1580 verify.

Closes #16473
@bagder bagder force-pushed the bagder/CURLFOLLOW_OBEYCODE branch from c784add to d5bc6da Compare February 27, 2025 13:08
@jay
Copy link
Member

jay commented Feb 27, 2025

Maybe we need another bit for ONLY-FOR-FIRST-REQUEST that then resets back unconditionally on the subsequent... Maybe that's the behavior people actually want?

I think there are websites that generate curl code and use -X POST and -X GET so there's some code out there using that even though it's wrong. What if there was just one bit CURLFOLLOW_IGNORE_CUSTOMREQ where it would not pass on the custom request in any redirects no matter what? Is there need for a more refined behavior like what you are doing?

(actually I see now you have added CURLFOLLOW_FIRSTONLY which is same? however I think the name FIRSTONLY implies like that it's only going to follow the first redirect and that may be harder to understand for someone just reading the code without a lot of libcurl knowledge)

edit: CURLFOLLOW_IGNORE_CUSTOMREQ looks like a flag not a separate enum so maybe when seen by itself would be hard to understand as well hm

@bagder
Copy link
Member Author

bagder commented Feb 27, 2025

What if there was just one bit CURLFOLLOW_IGNORE_CUSTOMREQ where it would not pass on the custom request in any redirects no matter what?

(actually I see now you have added CURLFOLLOW_FIRSTONLY which is same? however I think the name FIRSTONLY implies like that it's only going to follow the first redirect and that may be harder to understand for someone just reading the code without a lot of libcurl knowledge)

Names are hard, but I think your name could easily be read to imply the other way around: that it disables the custom method completely. I think it is hard for us to pick a name that will explain it clearly without the user having to read the documentation for it.

I wouldn't mind other names if we come up with better!

bagder added a commit that referenced this pull request Feb 27, 2025
With this change, the argument passed to the CURLOPT_FOLLOWLOCATION
option is now instead a "mode" instead of just a boolean. Documentation
is extended to describe the two new modes.

Test 1571 to 1580 verify.

Closes #16473
@bagder bagder force-pushed the bagder/CURLFOLLOW_OBEYCODE branch from a61ceaf to 2c7d8de Compare February 27, 2025 21:14
bagder added a commit that referenced this pull request Feb 27, 2025
With this change, the argument passed to the CURLOPT_FOLLOWLOCATION
option is now instead a "mode" instead of just a boolean. Documentation
is extended to describe the two new modes.

Test 1571 to 1580 verify.

Closes #16473
@bagder bagder force-pushed the bagder/CURLFOLLOW_OBEYCODE branch from 2c7d8de to a810dfa Compare February 27, 2025 21:32
bagder added a commit that referenced this pull request Feb 28, 2025
With this change, the argument passed to the CURLOPT_FOLLOWLOCATION
option is now instead a "mode" instead of just a boolean. Documentation
is extended to describe the two new modes.

Test 1571 to 1580 verify.

Closes #16473
@bagder bagder force-pushed the bagder/CURLFOLLOW_OBEYCODE branch from a810dfa to 1cddf7e Compare February 28, 2025 21:51
With this change, the argument passed to the CURLOPT_FOLLOWLOCATION
option is now instead a "mode" instead of just a boolean. Documentation
is extended to describe the two new modes.

Test 1571 to 1581 verify.

Closes #16473
@bagder bagder force-pushed the bagder/CURLFOLLOW_OBEYCODE branch from 1cddf7e to 50831d4 Compare March 1, 2025 22:25
@bagder
Copy link
Member Author

bagder commented Mar 2, 2025

Any further remarks, ideas, complaints or name improvements?

@bagder bagder closed this in fb13923 Mar 3, 2025
@bagder bagder deleted the bagder/CURLFOLLOW_OBEYCODE branch March 3, 2025 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration HTTP libcurl API tests
Development

Successfully merging this pull request may close these issues.

5 participants