[bug]: func_curl adding duplicate header leaves request in bad format

[scgm11]

Severity

Minor

Versions

all

Components/Modules

func_curl

Operating Environment

Ubuntu 22

Frequency of Occurrence

Constant

Issue Description

using func_curl from dialplan and adding a header that already exists duplicates it and leave the request in a bad format

Example

exten = _X.,1,Set(CURLOPT(httpheader)=Authorization: basic TEST)
 same =     n,Set(CURLOPT(httpheader)=Content-Type: application/json)
 same =     n,Set(CALLERID(num)=${CURL(https://${SERVERDOMAIN}/test)}))
 same =     n,Set(CURLOPT(httpheader)=Authorization: basic TEST)
 same =     n,Set(CURLOPT(httpheader)=Content-Type: application/json)
 same =     n,Set(CALLERID(num)=${CURL(https://${SERVERDOMAIN}/test)}))

one solution could be use AST_LIST_HEAD_INIT(list); in acf_curl_helper after the unlock if this is ok for you I will make a patch here

Relevant log output

No response

Asterisk Issue Guidelines

• [X] Yes, I have read the Asterisk Issue Guidelines

[jcolp]

What does the RFC state for multiple headers of the same name? If not allowed, then I believe the code should be updated to handle that situation. If allowed then the ability should be added to have a user overwrite what already exists instead of having multiple headers.

As for your fix, I believe that wouldn't work and would eliminate all configured options.

[scgm11]

there is no method in libcurl to replace in a slist but they say you should free slist after usage

https://curl.se/libcurl/c/curl_slist_append.html

[jcolp]

You're referencing libcurl functionality, but you mentioned Asterisk linked list previously. They're different, and the slist is already freed. The slist is created at request time based on global settings as well as settings stored on the datastore, and then freed afterwards.

Initializing the list in acf_curl_helper would cause a memory leak as the various options would not be freed, and it would also go against the existing documentation that the CURLOPT settings persist for all invocations of the CURL dialplan function.

[scgm11]

ok I saw that now, but seems to be an issue withe the Asterisk list then, shouldn't it be init somewhere? as seems gets duplicated each time you call curl. As it is now if you have to call 2 WS with different options you cant

[jcolp]

The problem is isolated to httpheaders because of the code:

https://github.com/asterisk/asterisk/blob/91c8866ff3bec674666206d6e44fba5a1fd90ff1/funcs/func_curl.c#L444

Thus why I stated before:

What does the RFC state for multiple headers of the same name? If not allowed, then I believe the code should be updated to handle that situation. If allowed then the ability should be added to have a user overwrite what already exists instead of having multiple headers.

Just wiping everything clean after CURL() completes is not an option because that goes against the documented and existing behavior, so it needs to be determined what the RFC allows for httpheaders in order to determine the best way to fix this.

[scgm11]

so what about each time a header is added we traverse the list if the header is there remove it and add the new one?

[jcolp]

That's why I asked what the RFC states - what is allowed? Are multiple allowed? What are the constraints?

[jcolp]

If you don't know the answer to that or can't determine it, then I can open this issue up as we should certainly do something. There is no time frame on if/when it would get looked into.

[scgm11]

dont know the answer but is clear that if you use it as it is fails in any webservice you call

[InterLinked1]

I think this is already a known issue. I seem to recall seeing it on JIRA, or independently reported by multiple people.

What does the RFC state for multiple headers of the same name? If not allowed, then I believe the code should be updated to handle that situation. If allowed then the ability should be added to have a user overwrite what already exists instead of having multiple headers.

Multiple HTTP headers with the same name are expressly allowed by RFC 2616, 4.2:

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list

In practice, this is so uncommon that arguably any client that does this is probably taking a risk. Servers may concatenate or outright replace. RFC 7230 3.2.2 is a little clearer on this:

A sender MUST NOT generate multiple header fields with the same field name in a message unless either the entire field value for that header field is defined as a comma-separated list [i.e., #(values)] or the header field is a well-known exception (as noted below).

Thus in practice, in a valid request payload, you shouldn't need to send multiple headers with the same name.

What was suggested previously regarding this issue was a function that would clear all the existing headers, to avoid such an issue. I think replacing an existing header is probably the most intuitive default otherwise; I can't think of a good reason for sending multiple headers with the same name, based on the specification.

[scgm11]

current beheavior:

As seen on the image is wrong as content type wont change and the other variable is adding to the header.

I will propose a PR that gets this new and expected beheaviour

[InterLinked1]

current beheavior:

As seen on the image is wrong as content type wont change and the other variable is adding to the header.

I will propose a PR that gets this new and expected beheaviour

This isn't even readable, please copy and paste text, not images.

[scgm11]

not sure how you cannot read the image but here it is in text: already submitted the PR

ucontactxCLI> originate Local/1@testcurl extension 11111@nada -- Called 1@testcurl -- Executing [1@testcurl:1] Set("Local/1@testcurl-0000002e;2", "CURLOPT(httpheader)=Content-Type: application/json") in new stack -- Executing [1@testcurl:2] Set("Local/1@testcurl-0000002e;2", "CURLOPT(httpheader)=Test: Testing1") in new stack -- Executing [1@testcurl:3] Set("Local/1@testcurl-0000002e;2", "curlres={ -- "args": {}, -- "headers": { -- "x-forwarded-proto": "https", -- "x-forwarded-port": "443", -- "host": "postman-echo.com", -- "x-amzn-trace-id": "Root=1-6490af1f-076c3f711d7b5aeb40f31c0f", -- "user-agent": "asterisk-libcurl-agent/1.0", -- "accept": "/", -- "content-type": "application/json", -- "test": "Testing1" -- }, -- "url": "https://postman-echo.com/get" -- }") in new stack -- Executing [1@testcurl:4] Set("Local/1@testcurl-0000002e;2", "CURLOPT(httpheader)=Content-Type: application/xml") in new stack -- Executing [1@testcurl:5] Set("Local/1@testcurl-0000002e;2", "CURLOPT(httpheader)=Test: Testing2") in new stack -- Executing [1@testcurl:6] Set("Local/1@testcurl-0000002e;2", "curlres={ -- "args": {}, -- "headers": { -- "x-forwarded-proto": "https", -- "x-forwarded-port": "443", -- "host": "postman-echo.com", -- "x-amzn-trace-id": "Root=1-6490af1f-522c22ec135030f96e16407d", -- "user-agent": "asterisk-libcurl-agent/1.0", -- "accept": "/*", -- "content-type": "application/xml", -- "test": "Testing2" -- }, -- "url": "https://postman-echo.com/get" -- }") in new stack -- Auto fallthrough, channel 'Local/1@testcurl-0000002e;2' status is 'UNKNOWN'

[seanbright]

Set-Cookie is one of those headers that you often see multiple of. I don't think blindly replacing existing headers is the right strategy. An application that resets the curl handle to the defaults would be better.

I started working on something like this for ASTERISK-30088 but never finished it. I will see if I can find it and get it working.

[pfactum]

A use-case from me: https://community.asterisk.org/t/how-to-remove-httpheader-added-via-curlopt/99194

As duplicate headers are allowed by RFC, shouldn't the title of this bug changed to something like "Add a way to remove CURL HTTP header or reset the list of headers altogether"?