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 support for XSSI Vulnerability Protected JSON #529

Closed
wants to merge 1 commit into from

Conversation

yashsriv
Copy link

XSSI Vulnerability protection usually involves prefixing all JSON
data with a particular prefix to make it non-executable.
HTTPie does not display the resulting JSON in pretty print format.
My changes add support for specifying a prefix so that HTTPie can strip
it before parsing the data as JSON.

@yashsriv
Copy link
Author

Angular js supports this(See this) as well as some json parsing frameworks.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 93.57% when pulling 136fbcf on yashsriv:feat/xssi into 64f6f69 on jkbrzt:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 93.57% when pulling 136fbcf on yashsriv:feat/xssi into 64f6f69 on jkbrzt:master.

@jkbrzt
Copy link
Member

jkbrzt commented Oct 14, 2016

@yashsriv Thanks. As far as I understand it though, this vulnerability isn't relevant to our case: HTTPie doesn't have a JavaScript runtime so "overriding native JavaScript object constructors" doesn't apply. Also:

This attack is only successful if the returned JSON is executable as JavaScript.

We treat JSON purely as data, and it doesn't get interpreted beyond parsing.

@sigmavirus24
Copy link

@jkbrzt I think @yashsriv may have done a poor job explaining their usecase. As I understand their change, they're trying to communicate with a service that returns XSSI protected JSON. Now, with today's HTTPie they lose JSON syntax highlighting because of this. If they were able to specify the prefix, however, that would be stripped, then they could see the JSON as they would see normal JSON via HTTPie.

While I agree with you that this doesn't strengthen HTTPie's security posture, I think this serves to help its users of these types of APIs.

@jkbrzt
Copy link
Member

jkbrzt commented Oct 14, 2016

@yashsriv Now I see what this is about — I have done a poor job reading the description the first time around. Thanks, @sigmavirus24.

So I understand that )]}',\n is the de facto standard prefix.

What if we simply tweaked HTTPie's JSON handling so that when JSON is expected but the content is not valid, the parser would then try again while skipping anything before the first [ or {? And then, if the remainder turns out to be valid JSON, it would format it the usual way and print out prefix + formatted_json.

Do you have an example URL?

@yashsriv
Copy link
Author

yashsriv commented Oct 14, 2016

@jkbrzt I don't have an example URL because I encountered this issue when I implemented XSSI protection on a REST api I'm working on for a private project. Also, I added the option to specify a prefix because I read online that some services use while(1); and other prefixes too. So, creating a generalised handler seems difficult. In my way, if you encounter an api with a different prefix, you just specify the prefix and it works.

I'll try and find a public api on which you can test this.

@yashsriv
Copy link
Author

This is the difference on my machine:

img-json-example

@adarshaj
Copy link

I'm interested in having this feature. Would like to see the feature merged! Is there anything which can be improved to get this done?
/cc @jkbrzt

@@ -17,6 +17,10 @@ def format_body(self, body, mime):
]
if (self.kwargs['explicit_json'] or
any(token in mime for token in maybe_json)):
if 'xssi_prefix' in self.kwargs:
token = self.kwargs["xssi_prefix"]
if body[0:len(token)] == token:

Choose a reason for hiding this comment

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

Would it not be simpler to do:

if body.startswith(token):

It looks as though token being '' won't get through to here because otherwise you'll always find this condition to be true and you'll always make a copy.

That said, even if you specify the common value that @jkbrzt found, this should be a relatively cheap check (to use startswith).

@yashsriv
Copy link
Author

Rebased and updated as per @sigmavirus24 's recommendation

@fearphage
Copy link

Is this waiting on anything else?

@adarshaj
Copy link

I'm still having to fallback to http $URL | tail -n1 | jq .. can this diff be pls merged? 🙏
/cc @jakubroztocil

@sakshamsharma
Copy link

Bump. Can we hope that this PR gets merged soon?
/cc @jakubroztocil

httpie/cli.py Outdated
@@ -168,6 +168,17 @@ def _split_lines(self, text, width):

"""
)
content_type.add_argument(

Choose a reason for hiding this comment

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

This is not a content type. This feels like it should be with the output options.

@@ -17,6 +17,10 @@ def format_body(self, body, mime):
]
if (self.kwargs['explicit_json'] or
any(token in mime for token in maybe_json)):
if 'xssi_prefix' in self.kwargs:

Choose a reason for hiding this comment

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

This will always be true since you defaulted it in the cli arguments. I think it'd be better to reverse this logic a bit:

xssi_prefix = self.kwargs["xssi_prefix"]

if xssi_prefix and body.startswith(xssi_prefix):
  body = body[len(xssi_prefix):]

XSSI Vulnerability protection usually involves prefixing all JSON
data with a particular prefix to make it non-executable.
HTTPie does not display the resulting JSON in pretty print format.
My changes add support for specifying a prefix so that HTTPie can strip
it before parsing the data as JSON.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 93.768% when pulling 6be881b on yashsriv:feat/xssi into 0f4dce9 on jakubroztocil:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 93.768% when pulling 6be881b on yashsriv:feat/xssi into 0f4dce9 on jakubroztocil:master.

@BoboTiG
Copy link
Contributor

BoboTiG commented Sep 22, 2021

Done with #1130.

Thanks for the original patch @yashsriv 🥧

@BoboTiG BoboTiG closed this Sep 22, 2021
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

8 participants