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

Encoding error when converting UploadedFile to json with binary tempfile #25250

Closed
maltoe opened this issue Jun 2, 2016 · 2 comments
Closed

Comments

@maltoe
Copy link

maltoe commented Jun 2, 2016

Hey,

we have an issue with ActionDispatch::Http::UploadedFile and to_json. We're using the Gelf gem for logging to a Graylog2 instance. After (successful) API calls to a file upload action, Gelf errors in to_json. We traced the problem down to an instance of UploadedFile in Gelf's parameters hash which it tries to convert to JSON to send to the server.

The UploadedFile contains a Tempfile in binary mode which seems to be included in the JSON representation, including its content. The content from read is supposed to be encoded in ASCII-8BIT, yet contains binary data.

Not really sure where the best place to fix this behaviour is (Ruby's Tempfile, Active Support's JSON, or here), but I thought UploadedFile for sure should not include the file's content in the JSON representation. Imho.

Steps to reproduce

Anyway, here's a test case:

https://gist.github.com/maltoe/a1b9bfc83be1c0eaef208ade9cbdfdad

Expected behavior

Not really sure, either catch these conversion errors in JSON (i.e. not convert the string to UTF-8, or do character replacement in there), or simply not include the tempfile's content in the json.

Actual behavior

BugTest#test_to_json_with_non_ascii_data:
Encoding::UndefinedConversionError: "\x80" from ASCII-8BIT to UTF-8

Possible fix (our current monkey-patch)

class UploadedFile
  def to_hash
    instance_values.except 'tempfile'
  end
end

System configuration

Rails version:
We use 4.2.5.2, but I tested on master.

Ruby version:
2.2.4

@maclover7
Copy link
Contributor

Hmm, I'm not sure if converting uploaded files to JSON is something that we want to support. Feel free to open a PR, and then we can continue the discussion there. :)

maltoe pushed a commit to plugintheworld/rails that referenced this issue Jun 2, 2016
This patch adds a `#to_hash` to `ActionDispatch::Http:UploadedFile`
that excludes its `tempfile` instance variable from the hash
representation.

The latter is for example used while generating JSON from an
`UploadedFile` which leads to the *content* of the temporary file
being read and included in the JSON. This behaviour is most likely
not what a user would expect. Besides, it has been reported to result
in encoding conversion errors when the temporary file is a binary file
(`Tempfile#read` returns an ASCII-encoded String containing binary data
which `JSON` tries to `encode` that to UTF-8).

Fixes rails#25250.
@sgrif
Copy link
Contributor

sgrif commented Jun 2, 2016

This isn't supported usage.

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

Successfully merging a pull request may close this issue.

3 participants