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

Fix encoding/decoding of query parameters and cookies #462

Merged
merged 5 commits into from Jul 17, 2019

Conversation

antoine-sachet
Copy link
Contributor

@antoine-sachet antoine-sachet commented Jul 17, 2019

Plumber should use decodeURIComponent instead of decodeURI for query parameters, otherwise some characters are not correctly decoded in strings (#460 ).

Plumber should also use encode/decodeURIComponent when setting and reading cookies (#461). Otherwise, invalid characters such as ; can appear in a cookie which breaks the cookie string. This is best practice as per MDN .

Note that the latter impacted an unrelated cookie test in test-sessions.R which was parsing cookies manually without URL-decoding. Using parseCookies solved the issue, however I am not familiar with this part of the codebase so please do double-check!

PR task list:

  • Update NEWS
  • Add tests
  • Update documentation with devtools::document()

@antoine-sachet antoine-sachet changed the title Fix uri encoding Fix encoding/decoding of query parameters and cookies Jul 17, 2019
@codecov-io
Copy link

codecov-io commented Jul 17, 2019

Codecov Report

Merging #462 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #462      +/-   ##
==========================================
+ Coverage   89.84%   89.91%   +0.06%     
==========================================
  Files          29       29              
  Lines        1527     1527              
==========================================
+ Hits         1372     1373       +1     
+ Misses        155      154       -1
Impacted Files Coverage Δ
R/cookie-parser.R 100% <100%> (+6.25%) ⬆️
R/query-string.R 91.13% <100%> (ø) ⬆️
R/response.R 100% <100%> (ø) ⬆️

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 a159aa6...9ca6185. Read the comment docs.

R/cookie-parser.R Outdated Show resolved Hide resolved
@schloerke schloerke requested review from schloerke and wch Jul 17, 2019
Copy link
Collaborator

@schloerke schloerke left a comment

Remove @importFrom from roxygen comments.

Otherwise LGTM.

@schloerke schloerke added this to the v0.5.0 - Next CRAN release milestone Jul 17, 2019
wch
wch approved these changes Jul 17, 2019
Copy link
Collaborator

@wch wch left a comment

Looks good!

@schloerke schloerke merged commit 5a5223f into rstudio:master Jul 17, 2019
5 checks passed
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.

None yet

4 participants