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

When returning an HTML error, make sure it's safe #1763

Merged
merged 4 commits into from
May 26, 2018
Merged

When returning an HTML error, make sure it's safe #1763

merged 4 commits into from
May 26, 2018

Conversation

ctennis
Copy link
Contributor

@ctennis ctennis commented May 23, 2018

When calling into an API specifying a crafted format that is HTML,
the returned error renders the HTML back to the user, causing a potential XSS
issue. For example:

http://example.com/api/endpoint?format=%3Cscript%3Ealert(document.cookie)%3C/script%3E

Renders as html:

The requested format '<script>alert(document.cookie)</script>' is not supported.

When an error generates html back to the user, make sure it's properly escaped.

Fixes issue #1762

the returned error renders the HTML back to the user, causing a potential XSS
issue.  For example:

http://example.com/api/endpoint?format=%3Cscript%3Ealert(document.cookie)%3C/script%3E

Renders as html:

The requested format '<script>alert(document.cookie)</script>' is not supported.

When an error generates html back to the user, make sure it's properly escaped.

Fixes issue #1762
@ctennis
Copy link
Contributor Author

ctennis commented May 23, 2018

I had to fix six specs to be "compliant", which really surprised me. The ones that return json in particular seem like maybe they're not setting their content-type appropriately to json and as such are returning a json blob in html code. Is this ok/correct?

@dblock
Copy link
Member

dblock commented May 23, 2018

The ones that expect JSON should indeed set the correct content type, thanks for taking care of it.

Looks like Rails 3 may be a problem. If it gets complicated we can discuss dropping support.

@dblock
Copy link
Member

dblock commented May 24, 2018

Better, but not 🍏 :(

@ctennis
Copy link
Contributor Author

ctennis commented May 25, 2018

@dblock ok specs passing now

@dblock dblock merged commit 6876b71 into ruby-grape:master May 26, 2018
@ctennis ctennis deleted the safe_html branch May 30, 2018 12:07
@rschultheis
Copy link

👋 I am a member of the GitHub team that sends security alerts. This issue was assigned CVE-2018-3769 which is now disclosed publicly therefore therefore we would like to send security alerts for it.

However, in our curation process it was noticed that the latest version of grape, 1.0.3, does not appear to include this fix. Will a new version of grape be published soon with this fix?

Thank you for any information! 🙇

@dblock
Copy link
Member

dblock commented Aug 4, 2018

I've released 1.1.0.

@Kukunin
Copy link

Kukunin commented Aug 16, 2018

I wonder why JSON error responses get escaped as well, and even specs are aligned to do that. That breaks existing JSON apis, since json doesn't understand " instead of "

I have such code:

    content_type :json, 'application/json'
    default_format :json

    rescue_from Grape::Knock::ForbiddenError do
      error!({ error: :unauthenticated }, 401)
    end

and it returns "{&quot;error&quot;:&quot;unauthenticated&quot;} instead of normal JSON

@dblock
Copy link
Member

dblock commented Aug 16, 2018

@Kukunin Reopen a bug, maybe with a fix, AFAIK that looks like a bug to me assuming the content type returned is application/json.

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.

4 participants