fix: auth leakage with basic authentication

[nestoracunablanco]

Issue #380

[nestoracunablanco]

PR is not complete, for more details see issue #380

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.98%. Comparing base (8a42d5c) to head (7cdb006).

❗ Current head 7cdb006 differs from pull request most recent head ba2b7d9

Please upload reports for the commit ba2b7d9 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
+ Coverage   84.94%   87.98%   +3.03%     
==========================================
  Files           9        9              
  Lines         465      441      -24     
  Branches      116      103      -13     
==========================================
- Hits          395      388       -7     
+ Misses         65       51      -14     
+ Partials        5        2       -3     

Flag	Coverage Δ
acceptance	84.12% <100.00%> (-0.82%)	⬇️
unit	61.45% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lucagiove]

That might be the idea, probably there are other headers that we want to obfuscate probably a quick research should be done to see other standard security headers exists.

Obviously this require unit tests and maybe acceptance tests if possible, I would use a dedicated function to do the obfuscation and headers to obfuscate might be configured by the user. But we can start doing this little by little in an iterative way

[nestoracunablanco]

Thank you for your prompt feedback, @lucagiove. I have a proposal for you to consider for an iterative approach:

1. Complete this PR, including unit and acceptance tests.
2. Submit another PR to enable the visualization of credentials when in debug or trace logging mode.
3. Investigate if there are other header types that may be affected.
4. If so, create a new PR with the necessary changes and introduce a dedicated function for this purpose.
5. Finally, submit another PR to incorporate user configuration options.

I believe in taking a step-by-step approach. Do you agree with these 5 steps as a plan?

[lucagiove]

I'm big fan of steps too.
I'll add my thoughts in the issue #380 better than in PR

[lucagiove]

If you didn't yet check the contribution page: https://github.com/MarketSquare/robotframework-requests/blob/master/CONTRIBUTING.md

[nestoracunablanco]

@lucagiove, I have created some unit tests. My next step will be to work on the acceptance tests. Before merging, I plan to squash the commits into one, in case you are satisfied with the current state of the code.

[nestoracunablanco]

When considering the issue of acceptance tests, it raises the following question: given that the credentials are leaked at a log level through the request headers, is there a good method to capture these logs within the robot test?

[lucagiove]

When considering the issue of acceptance tests, it raises the following question: given that the credentials are leaked at a log level through the request headers, is there a good method to capture these logs within the robot test?

That's a good point, probably it's hard to test this on RF side, maybe we could be happy to mock the log call as it's currently done in unit tests

[nestoracunablanco]

When considering the issue of acceptance tests, it raises the following question: given that the credentials are leaked at a log level through the request headers, is there a good method to capture these logs within the robot test?

That's a good point, probably it's hard to test this on RF side, maybe we could be happy to mock the log call as it's currently done in unit tests

In that case, if you agree, I can squash both commits and update/rebase the branch. Do you know why the CI status is still showing as 'expected'? Where can I debug it?

[lucagiove]

Ok to be honest I've no idea I searched whether I configured that somewhere but I couldn't find.. they are old build I removed some time ago..

[nestoracunablanco]

Thank you for the information. If you agree to these changes and they are merged, I am more than happy to proceed with the next step: 2. Submit another PR to enable the visualization of credentials when in debug or trace logging mode in an extra branch.

[nestoracunablanco]

Hello @lucagiove, are you satisfied with the current state, or do you see any necessary improvements?

[lucagiove]

Yes it might be fine, but I think would be better to offer a way to see the header for debug/testing purpose together with this change.

[nestoracunablanco]

Yes it might be fine, but I think would be better to offer a way to see the header for debug/testing purpose together with this change.

Thank you for your response. I was a bit surprised as I recall we had agreed on the following steps I outlined two weeks ago:

1. Complete this PR, including unit and acceptance tests.
2. Submit another PR to enable the visualization of credentials when in debug or trace logging mode.
3. Investigate if there are other header types that may be affected.
4. If so, create a new PR with the necessary changes and introduce a dedicated function for this purpose.
5. Finally, submit another PR to incorporate user configuration options.

I will send you an invitation to discuss and finalize these points in a Slack call. This discussion should not exceed 15 minutes of your time.

[lucagiove]

Yes it might be fine, but I think would be better to offer a way to see the header for debug/testing purpose together with this change.

Thank you for your response. I was a bit surprised as I recall we had agreed on the following steps I outlined two weeks ago:

1. Complete this PR, including unit and acceptance tests.
2. Submit another PR to enable the visualization of credentials when in debug or trace logging mode.
3. Investigate if there are other header types that may be affected.
4. If so, create a new PR with the necessary changes and introduce a dedicated function for this purpose.
5. Finally, submit another PR to incorporate user configuration options.

I will send you an invitation to discuss and finalize these points in a Slack call. This discussion should not exceed 15 minutes of your time.

Yes we agreed but I was thinking at different commits to better review the changes and iterate, or perhaps different PR but once I merge your change a new version will be released with this feature and in the meantime this might upset some user that is currently reading logs with authorization headers. That's why at least I'll propose a way to still debug those headers.

It's true that it is currently an alpha version but I mostly working keeping it stable also in term of api and behavior (as much as possible) anyway this version has big deprecations and is a major one.
Let me know what do you think.

[nestoracunablanco]

Yes it might be fine, but I think would be better to offer a way to see the header for debug/testing purpose together with this change.

Thank you for your response. I was a bit surprised as I recall we had agreed on the following steps I outlined two weeks ago:

1. Complete this PR, including unit and acceptance tests.
2. Submit another PR to enable the visualization of credentials when in debug or trace logging mode.
3. Investigate if there are other header types that may be affected.
4. If so, create a new PR with the necessary changes and introduce a dedicated function for this purpose.
5. Finally, submit another PR to incorporate user configuration options.

I will send you an invitation to discuss and finalize these points in a Slack call. This discussion should not exceed 15 minutes of your time.

Yes we agreed but I was thinking at different commits to better review the changes and iterate, or perhaps different PR but once I merge your change a new version will be released with this feature and in the meantime this might upset some user that is currently reading logs with authorization headers. That's why at least I'll propose a way to still debug those headers.

It's true that it is currently an alpha version but I mostly working keeping it stable also in term of api and behavior (as much as possible) anyway this version has big deprecations and is a major one. Let me know what do you think.

Before proceeding with any changes, just to confirm: Are steps 1 and 2 meant to be included in a single PR? Regarding my opinion, I don't have a strong preference; I believe it's important to uphold the agreements we have in place.

[lucagiove]

Are steps 1 and 2 meant to be included in a single PR?

They are meant to be released together and therefore merged at the same time.
You decide how to organize the code.
One single final PR might be useful.

[nestoracunablanco]

Thank you for your response, @lucagiove . I will consolidate everything into a single PR, but I will do it iteratively. I will notify you once I have completed step 2: 'Submit another PR to enable the visualization of credentials when in debug or trace logging mode.'

[nestoracunablanco]

Hey @lucagiove, could you please give me some feedback on my latest commit? Thanks!

[lucagiove]

It think it's fine.

What I think is missing is somewhere documentation that explain this features.