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

Skip empty headers key if they present in the opts.Headers #1203

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dmitryk-dk
Copy link

What this PR does / why we need it:
This PR updated the logic in the CustomHeadersMiddleware. The issue may happen when user created datasource with the empty headers value when setting Custom HTTP Headers.

Which issue(s) this PR fixes:
Related issue: #1202

Fixes #

Special notes for your reviewer:

add test to handle empty key
@dmitryk-dk dmitryk-dk requested a review from a team as a code owner January 22, 2025 19:29
@dmitryk-dk dmitryk-dk requested review from andresmgot, oshirohugo and xnyo and removed request for a team January 22, 2025 19:29
@CLAassistant
Copy link

CLAassistant commented Jan 22, 2025

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@andresmgot
Copy link
Contributor

Hello @dmitryk-dk,

Thank you for the contribution! I believe the fix should be elsewhere, not in this SDK. With this fix, the datasource will silently skip setting the header, which could be confusing. A better approach would be to avoid saving the datasource settings if the data within it is invalid. In this case, there should be some validation in the form so it's not allowed to set an empty header key.

The component you are using is the DatasourceHTTPSettings which can be replaced with the new Auth component. Feel free to open the issue in one of those repositories or submit a PR there (I'd try to set the property required in the input).

@dmitryk-dk
Copy link
Author

Hello @dmitryk-dk,

Thank you for the contribution! I believe the fix should be elsewhere, not in this SDK. With this fix, the datasource will silently skip setting the header, which could be confusing. A better approach would be to avoid saving the datasource settings if the data within it is invalid. In this case, there should be some validation in the form so it's not allowed to set an empty header key.

The component you are using is the DatasourceHTTPSettings which can be replaced with the new Auth component. Feel free to open the issue in one of those repositories or submit a PR there (I'd try to set the property required in the input).

Hi! Thank you for your reply.

The SDK currently allows the use of an empty header key, which could lead to issues. To avoid problems, requests should not include any headers with empty keys. Implementing proper validation in the SDK to prevent this scenario would be beneficial. I’ve added a fix for the issue I identified during my tests, and I believe the problem should be addressed on both sides.

Why Empty Header Keys Are Problematic:
HTTP headers are key-value pairs. If a key is empty, it violates the structure expected by servers or intermediaries, leading to unpredictable behavior.

Suggested Improvement:
The SDK should validate headers to ensure that keys are not empty before making requests.

Feel free to close the PR and issue if you find them unnecessary. :)

@andresmgot
Copy link
Contributor

Right, I agree that having empty headers are problematic, I just think that fixing this issue before the request reach the SDK would be a better solution and more user friendly.

Please reach out to the teams owning the components I mentioned above. If, for some reason, the fix cannot be applied there, it should be fine to adapt the behavior here in the SDK and at least avoid that the empty headers reach the final service. The priority should be to avoid storing invalid datasource settings.

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

Successfully merging this pull request may close these issues.

3 participants