Skip to content

Conversation

simonbru
Copy link
Contributor

@simonbru simonbru commented Aug 20, 2021

Some special URL characters (such as +, = and &) were not escaped in query parameters. This would introduce a risk of data corruption or URL injection.

Note: I did not notice any breakage when testing against SAP. However, I only tested a few use cases (e.g., I barely tested batched requests).

@CLAassistant
Copy link

CLAassistant commented Aug 20, 2021

CLA assistant check
All committers have signed the CLA.

@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2021

Codecov Report

Merging #175 (145c26f) into master (1396809) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
+ Coverage   92.63%   92.70%   +0.06%     
==========================================
  Files           6        6              
  Lines        2675     2672       -3     
==========================================
- Hits         2478     2477       -1     
+ Misses        197      195       -2     
Impacted Files Coverage Δ
pyodata/v2/service.py 90.66% <100.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1396809...145c26f. Read the comment docs.

@filak-sap
Copy link
Contributor

Good catch! Thank you very much. Can you accept the CLA, please? I cannot merge your patch without it.

@simonbru
Copy link
Contributor Author

simonbru commented Aug 20, 2021

Can you accept the CLA, please?

Done!

@phanak-sap
Copy link
Contributor

CLA signed, PR looking good. I will update changelog in follow-up commit.

@phanak-sap phanak-sap merged commit cf3108b into SAP:master Aug 22, 2021
phanak-sap added a commit that referenced this pull request Aug 22, 2021
add fix merged from PR #175
@simonbru
Copy link
Contributor Author

Thanks for the quick response!

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.

5 participants