Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

fix: escape HTML in error response body #5

Closed
wants to merge 2 commits into from

Conversation

lidel
Copy link
Member

@lidel lidel commented Nov 27, 2020

This is a basic "better safe than sorry" cleanup of unexpected error responses.

  • To avoid situation where someone uses this as a vector for HTML injection, response is escaped via template.HTMLEscapeString
  • To avoid noisy line endings and make it more clear where values start and end, payload is printed with %q which adds quotes and replaces line breaks them with \n

This adds function that returns a single batch and an int with total
count. This enables consumer of this lib to implement manual pagination
or get total pin count in efficient manner.
Better safe than sorry: misconfigured service may produce arbitrary HTTP
response, and we don't control where this response will end up.

To avoid situation where someone uses this as a vector for HTML
injection, response is escaped via template.HTMLEscapeString
Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

This seems reasonable, although is it worth doing this? i.e. we could just return "unknown error" and log this info out (e.g. in the go-ipfs daemon).

@lidel
Copy link
Member Author

lidel commented Nov 30, 2020

@aschmahmann Yeah, this is just a quick fix to unblock RC1. We need to clean up error handling before final 0.8:

I played a bit with the API and printing entire httpbody into console or returning it from /api/v0/pin/remote is pretty bad (noisy console is useful for debug but bad UX, and returning http body with error details in HTTP API is very risky security-wise, could leak some details about the backend service).

Ideally, httpbody should not be returned to the user, go-ipfs should log it as %q in "debug/trace" level instead.

@aschmahmann
Copy link
Contributor

Ideally, httpbody should not be returned to the user, go-ipfs should log it as %q in "debug/trace" level instead.

We already have a logger so seems like a few line fix to just use logger.Debug and return something like unknown pinning service error, right?

@lidel
Copy link
Member Author

lidel commented Nov 30, 2020

Yes, if we can tell between errors returned by the pinning service (error object with "reason" and "details") and unexpected ones, that would be the best way of handling it. We also need the ability to enable debug log only for remote pin operations.

@aschmahmann
Copy link
Contributor

fixed in #3

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants