Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve JSON output when there is leading data before the actual JSON body #1130

Merged
merged 2 commits into from
Sep 21, 2021
Merged

Improve JSON output when there is leading data before the actual JSON body #1130

merged 2 commits into from
Sep 21, 2021

Conversation

BoboTiG
Copy link
Contributor

@BoboTiG BoboTiG commented Aug 18, 2021

In some special cases, to prevent against Cross Site Script Inclusion (XSSI) attacks, the JSON response body starts with a magic prefix line that must be stripped before feeding the rest of the response body to the JSON parser.
Such prefix is now simply ignored from the parser but still printed in the terminal.

Supersedes #529.

@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2021

Codecov Report

Merging #1130 (d5b9963) into master (4d7d6b6) will decrease coverage by 0.04%.
The diff coverage is 98.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1130      +/-   ##
==========================================
- Coverage   97.28%   97.24%   -0.05%     
==========================================
  Files          67       70       +3     
  Lines        4235     4279      +44     
==========================================
+ Hits         4120     4161      +41     
- Misses        115      118       +3     
Impacted Files Coverage Δ
httpie/output/formatters/colors.py 92.66% <83.33%> (-0.92%) ⬇️
httpie/__init__.py 100.00% <100.00%> (ø)
httpie/output/formatters/json.py 100.00% <100.00%> (ø)
httpie/output/lexers/http.py 100.00% <100.00%> (ø)
httpie/output/lexers/json.py 100.00% <100.00%> (ø)
httpie/output/utils.py 100.00% <100.00%> (ø)
tests/test_json.py 100.00% <100.00%> (ø)
httpie/cli/argparser.py 95.54% <0.00%> (-0.81%) ⬇️
... and 2 more

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 2731341...d5b9963. Read the comment docs.

@BoboTiG

This comment has been minimized.

@BoboTiG BoboTiG changed the title Strip off XSSI prefixes from pretty-printed JSON responses Improve pretty-printed JSON responses when there is leading data before the actual JSON body Aug 20, 2021
@BoboTiG
Copy link
Contributor Author

BoboTiG commented Aug 20, 2021

The prefix is printed before the prettified JSON:

Capture d’écran_2021-08-20_16-01-06

I am not sure yet how to set a specific color for the prefix @jakubroztocil. Do you have a clue 🙏?

@jkbrzt
Copy link
Member

jkbrzt commented Aug 20, 2021

I am not sure yet how to set a specific color for the prefix @jakubroztocil. Do you have a clue 🙏?

Take a look at what tokens Pygments works with. I’d probably experiment with comments, exceptions, errors, etc. Perhaps you could even generate an example for each token type to see what feels right?

https://github.com/pygments/pygments/blob/master/pygments/token.py#L123-L212

@BoboTiG
Copy link
Contributor Author

BoboTiG commented Aug 20, 2021

Here we are.

Style "error":
token-error

Style "keyword":
token-keyword

Style "punctuation":
token-punctuation

Style "string":
token-string

Actually, I prefer no color. But Maybe the "error" style is better to highlight data not being taken into account by the parser?

FTR there is no style for comments, and the number style is the same as "keyword" one. So it seems we have only those choices by default.

@jkbrzt
Copy link
Member

jkbrzt commented Aug 20, 2021

From those four I’d pick error. But I’m wondering how the different comment tokens would look like. Could you write a loop over all the tokens and print the name and the output for each so that we can compare them?

@BoboTiG
Copy link
Contributor Author

BoboTiG commented Aug 23, 2021

Here are all styles available (without duplicates):
tokens

@BoboTiG
Copy link
Contributor Author

BoboTiG commented Aug 23, 2021

Actually there are no "comment" token styles because JSON does not support comments.

@BoboTiG BoboTiG changed the title Improve pretty-printed JSON responses when there is leading data before the actual JSON body Improve JSON output when there is leading data before the actual JSON body Aug 23, 2021
@BoboTiG
Copy link
Contributor Author

BoboTiG commented Aug 23, 2021

I updated the PR, here is a short demo: asciicast

@BoboTiG
Copy link
Contributor Author

BoboTiG commented Aug 23, 2021

Both requests and responses are handled on purpose.

@BoboTiG
Copy link
Contributor Author

BoboTiG commented Aug 25, 2021

/packit build

@BoboTiG BoboTiG requested a review from jkbrzt August 25, 2021 09:46
@BoboTiG BoboTiG requested a review from jkbrzt September 1, 2021 13:40
@BoboTiG
Copy link
Contributor Author

BoboTiG commented Sep 2, 2021

I resolved the conflict, the patch is good for a final review :)

@BoboTiG BoboTiG requested a review from jkbrzt September 3, 2021 08:18
…JSON data

In some special cases, to prevent against Cross Site Script Inclusion (XSSI)
attacks, the JSON response body starts with a magic prefix line that must be
stripped before feeding the rest of the response body to the JSON parser.
Such prefix is now simply ignored from the parser but still printed in the
terminal.
@BoboTiG
Copy link
Contributor Author

BoboTiG commented Sep 21, 2021

Merging PR as changes are quite correct and all tests are green.

@BoboTiG BoboTiG merged commit e6c5cd3 into httpie:master Sep 21, 2021
@BoboTiG BoboTiG deleted the mickael/dev-212-add-support-for-xssi-vulnerability branch September 21, 2021 09:15
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants