Skip to content

Fix up of: IA2: Do not treat huge base64 data as NVDA might freeze in Google Chrome (PR #10231)#10281

Merged
michaelDCurran merged 2 commits into
nvaccess:masterfrom
accessolutions:pr10231-base64Fixup
Sep 30, 2019
Merged

Fix up of: IA2: Do not treat huge base64 data as NVDA might freeze in Google Chrome (PR #10231)#10281
michaelDCurran merged 2 commits into
nvaccess:masterfrom
accessolutions:pr10231-base64Fixup

Conversation

@JulienCochuyt

Copy link
Copy Markdown
Contributor

Link to issue number:

Fix up of PR #10231

Summary of the issue:

As mentioned by @michaelDCurran in #10231 (comment) IAccessibleHandler.splitIA2Attribs might also receive IAccessibleText as argument.

Description of how this pull request fixes the issue:

Alter the regex as to ensure to truncate base64 data only in the src attribute, and not in an eventual
text content (eg. in and inline editor).

Testing performed:

Tested against the test case of #10227.

Known issues with pull request:

Change log entry:

Section: New features, Changes, Bug fixes

… Google Chrome (PR nvaccess#10231)

Ensure to truncate base64 data only in the `src` attribute, and not in an eventual
text content (eg. in and inline editor).

Re: nvaccess#10231 (comment)
@michaelDCurran

michaelDCurran commented Sep 27, 2019 via email

Copy link
Copy Markdown
Member

@michaelDCurran

Copy link
Copy Markdown
Member

If I am reading the regex correctly, it looks like the whole src:value part is totally removed. I don't think this is a good idea. I think code may want to know the src, and the fact that it contained base64. Really, we just want the bit after the comma (,) to be truncated. Apologies if I'm reading it incorrectly.

@JulienCochuyt

JulienCochuyt commented Sep 27, 2019

Copy link
Copy Markdown
Contributor Author

To be clear, this function never receives "text content". Rather, it only receives well-formatted IAccessible2 attribute strings. I think the confusion here is that we get attribute strings both from IAccessible2 objects, and from format runs of IAccessible2 text. But in both cases, we are talking about name:value;name:value; strings, not random text content.

Still, this pr does make the regex more specific, only applying to the src attribute, which is a good thing.

Thanks for this clarification.
I was conscious of the "name:value;name:value" aspect of the passed string, but was afraid one of these values might be actual text content, in which case we could have had a nasty collision if the user was indeed using an online editor to edit a web page.

If I am reading the regex correctly, it looks like the whole src:value part is totally removed. I don't think this is a good idea. I think code may want to know the src, and the fact that it contained base64. Really, we just want the bit after the comma (,) to be truncated. Apologies if I'm reading it incorrectly.

No problem, regex are always a challenge to read, and this one especially requires braille or total speech verbosity to be perceived correctly.
The first part of the replacement string is actually a reference to a capturing group.
Thus, the resulting string is of the form "data:image/jpg;base64<truncated>", that is, it preserves the "data" scheme and the original MIME type of the base64 data.
Nevertheless, I realize that it strips out the comma between "base64" and "<truncated>", but I can alter it so that this comma is preserved too.

On the contrary, my vbuf C++ proposals do not produce that same result as they also strip out the MIME type and produce results of the form "data:<truncated>". If you feel this is problematic, I'll update the proposed implementation so that the original MIME type is preserved.

EDIT: Escaped the angle brackets for clarity.

@michaelDCurran michaelDCurran merged commit 0587211 into nvaccess:master Sep 30, 2019
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Sep 30, 2019
@JulienCochuyt JulienCochuyt deleted the pr10231-base64Fixup branch September 30, 2019 08:07
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