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

encode string as octets before passing to md5_hex() #466

Merged
merged 4 commits into from
Apr 19, 2020

Conversation

mgage
Copy link
Member

@mgage mgage commented Apr 18, 2020

The string might have utf8 characters which md5_hex() can't handle.

See https://perldoc.perl.org/Digest/MD5.html near the bottom of the page.

@taniwallach
Copy link
Member

This is essentially the fix suggested by @dvpc in #443 and it certainly makes sense that it would fix the issue.

@mgage
Copy link
Member Author

mgage commented Apr 18, 2020

I think I like @dpvc 's fix better. Less chance of an unexpected compatibility issue with old code and old cached equations:
my $md5 = md5_hex(utf8::is_utf8($tex) ? encode_utf8($tex) : $tex);

any objections, comments?

@taniwallach
Copy link
Member

taniwallach commented Apr 18, 2020

When I enabled your fix on my server, the results table (where UTF-8 characters are used on popups) appears, but the "rendered" images are blank. There are several warnings:

Wide character in print at /opt/webwork/pg/lib/WeBWorK/PG/ImageGenerator.pm line 378

but getting a valid response page with the results table and the question shown rather than an error pages is already an improvement.

I think getting "plain" LaTeX and dvipng to handle UTF-8 encoded data is probably not that simple, and am not sure what would be a good approach to handle this.

One possible option would be to be to try to detect it any real Unicode characters are in the text (possibly after a encode to UTF-8) and if any such characters are used to replace the entire "equation string" with a warning message that this answer cannot be handled in images mode.

For better or worse, I think most sites which need answers which would end up being processed in this manner are better off recommended that MathJax be used instead of images.

@mgage
Copy link
Member Author

mgage commented Apr 18, 2020

Does restricting the encoding only to $tex strings that satisfy is_utf8 help?
What images are you rendering? Do you mean the images of the hebrew letters don't appear
in the popUp menu?

@taniwallach
Copy link
Member

Does restricting the encoding only to $tex strings that satisfy is_utf8 help?

I did not check - but this is exactly the case where we do need to encoding into UTF-8 before calling MD5, as before the change the code would fail as this is a case where Unicode characters are in the answer. The "new" issues is in the image generation process itself (in ImageGenerator.pm).

What images are you rendering? Do you mean the images of the hebrew letters don't appear
in the popUp menu?

The popUp menu in the problem I tested has Hebrew works, and in the popup itself they do appear. However, in the "rendered" answer column of the results table they do not appear, but in the "plain text" column they do appear.

The problem I tested on has popups essentially that same as those in the example given in openwebwork/webwork2#1089 which has:

$popup1 = PopUp(['נכון', 'לא נכון'], 'נכון');

and the generated image I got was a 1x1 PNG file with what seems to be a single point. The tooltip (if you click to get it) does show the correct Hebrew text of the answer selected.

As I wrote above, MathJax mode works better, and moreover the issue of the "missing" text in the image is not really critical - as the text shows up in the other column.

Upon further reading it appears that this is not useful except for internal perl checks
@mgage mgage merged commit 63f0b73 into openwebwork:develop Apr 19, 2020
@mgage mgage deleted the develop+md5_fix branch May 19, 2020 17:57
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.

3 participants