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

Protect from unsafe JSON parsing behavior. #325

Merged
merged 3 commits into from Oct 29, 2018
Merged

Protect from unsafe JSON parsing behavior. #325

merged 3 commits into from Oct 29, 2018

Conversation

trestletech
Copy link
Contributor

@trestletech trestletech commented Oct 29, 2018

I believe the only way that this could have been exploited would be if the API used encrypted cookies and an attacker knew the encryption key.

We were already sniffing for JSON by reading the first char and matching against '{', so we wouldn't have been vulnerable to any attack in standard usage.

I believe the only way that this could have been exploited would be if the API used encrypted cookies and an attacker knew the encryption key.

We were already sniffing for JSON by reading the first char and matching against '{', so we wouldn't have been vulnerable to any attack in standard usage.
@trestletech trestletech requested a review from schloerke Oct 29, 2018
@codecov-io
Copy link

codecov-io commented Oct 29, 2018

Codecov Report

Merging #325 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #325      +/-   ##
==========================================
+ Coverage   90.01%   90.04%   +0.02%     
==========================================
  Files          25       26       +1     
  Lines        1142     1145       +3     
==========================================
+ Hits         1028     1031       +3     
  Misses        114      114
Impacted Files Coverage Δ
R/session-cookie.R 93.02% <100%> (ø) ⬆️
R/post-body.R 100% <100%> (ø) ⬆️
R/json.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 8c25b8b...b2aea33. Read the comment docs.

@schloerke schloerke requested a review from jcheng5 Oct 29, 2018
@schloerke schloerke added the priority: high Must be fixed before next release label Oct 29, 2018
R/json.R Outdated Show resolved Hide resolved
@trestletech
Copy link
Contributor Author

trestletech commented Oct 29, 2018

Oh, look at you! One step ahead of me :) Thanks

@trestletech trestletech merged commit cccfeae into master Oct 29, 2018
5 checks passed
@trestletech trestletech deleted the jeff-json branch Oct 29, 2018
schloerke added a commit to schloerke/plumber that referenced this pull request Oct 29, 2018
* master:
  Protect from unsafe JSON parsing behavior. (rstudio#325)
  use `inherits(obj, "xxx")` and `expect_s3_class(obj, "xxx")` rather than "xxx" %in% class(obj) (rstudio#313)
  Multiline POST body collapsed (rstudio#297)
  Install plumber from CRAN in top level Docker file (rstudio#292)
@r2evans
Copy link

r2evans commented Aug 23, 2020

I know it's late in the game (congrats on 1.0.0!), is there an advantage to using this over jsonlite::parse_json()? It is safer than fromJSON and does mostly the same think (lacking jsonlite::validate). (Note, it defaults to not simplifying ... other than that, they are identical to fromJSON.) (Companion function: jsonlite::read_json for explicit file paths.

@schloerke
Copy link
Collaborator

schloerke commented Aug 23, 2020

@r2evans

parse_json(txt, simplifyVector = simplifyVector, ...)
now uses parse_json and defaults simply to FALSE. I believe this should have the same behavior as before.

Never too late to bring up discussion and to double check things! Thank you! 😊😊

@r2evans
Copy link

r2evans commented Aug 23, 2020

Thanks @schloerke. I have been bitten (local-code only, not within plumber) by the flexibility of fromJSON before, so I happened to see those functions. I've been shying away from polymorphism that includes filenames, as it can definitely be more problematic, especially cases like these.

But mostly ... I should have realized that even though I read the new-release NEWS today, that doesn't mean that this PR was at all recent (vice almost 2y ago). Oops, I see your safeFromJSON now, sorry for the noise. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high Must be fixed before next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants