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

Added sanitizer step for api helper #12685

Merged
merged 7 commits into from
Mar 29, 2022
Merged

Added sanitizer step for api helper #12685

merged 7 commits into from
Mar 29, 2022

Conversation

spylogsster
Copy link
Contributor

@spylogsster spylogsster commented Mar 21, 2022

Resolves brave/brave-browser#21831

Added JsonSanitizer to ApiRequestHelper to validate server responses. It sanitizes and normalizes JSON by parsing it in a safe environment and re-serializing it. Parsing the sanitized JSON should result in a value identical to parsing the original JSON.
For those cases where we expect data that cannot be correctly processed by the JsonSanitizer and base/json parser, a preprocessing callback for the response text was added to convert the necessary values into strings by using safe parsers.
Example can be seen in this test.

Mandatory updates in unittests:

  • data_decoder::test::InProcessDataDecoder in_process_data_decoder_; is required to be created for tests that use JsonSanitizer
  • JsonSerializer removes spaces, some testing payloads have been fixed due to the requirement.
  • For desktops added a flag to allow trailing commas in json, but it doesn't work in Android. Android implementation will treat it as invalid json.

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  • APIRequestHelper is used in BraveWallet, BraveAdaptiveCaptcha, BraveToday, need to check that these components work as expected

@spylogsster spylogsster force-pushed the sanitize-json branch 4 times, most recently from 7b75bb6 to 9e11646 Compare March 22, 2022 14:24
@spylogsster spylogsster changed the base branch from share-nonce-tracker to parse-uint64-response March 22, 2022 14:24
@spylogsster spylogsster force-pushed the sanitize-json branch 2 times, most recently from eb6806a to adc643f Compare March 22, 2022 15:08
@spylogsster spylogsster force-pushed the sanitize-json branch 2 times, most recently from d3b0cc2 to 8dfa692 Compare March 22, 2022 21:01
@spylogsster spylogsster force-pushed the sanitize-json branch 4 times, most recently from 93d1ba5 to c1411d9 Compare March 23, 2022 12:31
@spylogsster spylogsster changed the base branch from parse-uint64-response to master March 23, 2022 12:31
@spylogsster spylogsster marked this pull request as ready for review March 23, 2022 12:58
@spylogsster spylogsster requested a review from a team as a code owner March 23, 2022 12:58
@spylogsster spylogsster changed the title WIP: Added sanitizer step for api helper Added sanitizer step for api helper Mar 23, 2022
@spylogsster spylogsster added this to the 1.38.x - Nightly milestone Mar 23, 2022
@spylogsster spylogsster force-pushed the sanitize-json branch 2 times, most recently from 0a6b9ab to 21ad38a Compare March 23, 2022 13:59
@gpestana
Copy link
Contributor

Deferring my review to @emerick as he led all the client-side implementation of adaptive captcha.

@spylogsster spylogsster force-pushed the sanitize-json branch 4 times, most recently from 54de0ad to 0e8834b Compare March 24, 2022 18:39
@emerick emerick self-requested a review March 25, 2022 13:19
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM re: adaptive captcha

Copy link
Contributor

@gpestana gpestana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (cc @emerick)

@diracdeltas diracdeltas self-requested a review March 29, 2022 16:14
@spylogsster spylogsster merged commit 837a78c into master Mar 29, 2022
@spylogsster spylogsster deleted the sanitize-json branch March 29, 2022 16:15
@petemill
Copy link
Member

Just noting this broke Brave News which uses this helper to download the image from the brave news private cdn. We'll fix that by stopping it use api request helper for that request.

@bbondy
Copy link
Member

bbondy commented Mar 31, 2022

maybe sanitize by response content-type?

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.

Add a generic JSON validator that checks the JSON before it gets processed in cpp
10 participants