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

[🐛 Bug][java]: Switching base64 decoder to a strict mode is going to break the compatibility with various Appium drivers #11168

Closed
mykola-mokhnach opened this issue Oct 26, 2022 · 15 comments

Comments

@mykola-mokhnach
Copy link

What happened?

Inclusion of #11107 into the lib release is going to create issues with Appium driver compatibility. Some of these drivers are returning screenshots being base64-encoded with line breaks, which is against RFC4648 standard, but still complies to RFC2045. Calling Base64.getDecoder().decode would throw an exception for such blobs, while Base64.getMimeDecoder().decode would not (RFC4648-encoded blob could still be properly decoded properly by the MIME decoder though).

We did update Appium drivers to return screenshots in RFC4648, although older versions of them still use the old method. And we cannot limit newer client versions to be used only with newest driver releases. That is why my proposal would be to revert the above PR and let the client to accept both standards, even though the w3c standard is being strict about that.

How can we reproduce the issue?

See above

Relevant log output

None

Operating System

Any

Selenium version

4.5.0+

What are the browser(s) and version(s) where you see this issue?

Any

What are the browser driver(s) and version(s) where you see this issue?

Any

Are you using Selenium Grid?

No response

@mykola-mokhnach mykola-mokhnach changed the title [🐛 Bug][java]: Switching base64 decoder to a strict mode is going to break the compatibility with various Appium driver [🐛 Bug][java]: Switching base64 decoder to a strict mode is going to break the compatibility with various Appium drivers Oct 26, 2022
@github-actions
Copy link

@mykola-mokhnach, thank you for creating this issue. We will troubleshoot it as soon as we can.


Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

@titusfortner
Copy link
Member

@pujagani / @diemol is this controversial? can we just revert that commit? Do we need to discuss in meeting tomorrow?

@pujagani
Copy link
Contributor

pujagani commented Oct 27, 2022

Yes, I think we need to discuss it. Mainly around the point that w3c is being strict with the standard it accepts and changes we made in line with that but should we allow additional standards as well?

@pujagani
Copy link
Contributor

I am also thinking, we had a similar issue in terms of the minimum geckodriver version to be used with the respective Selenium version to support CDP. On similar lines, is it okay for Appium to call out which version to use if they need the RFC2045 standards? Since the newer version of Appium will be going the spec-compliant way too.

@diemol
Copy link
Member

diemol commented Oct 27, 2022

We did update Appium drivers to return screenshots in RFC4648, although older versions of them still use the old method. And we cannot limit newer client versions to be used only with newest driver releases.

@mykola-mokhnach is this meant to support users who are still with older versions? Why can't we ask users to upgrade? If we cannot ask them (for whatever reason), what is the timeline/deadline where we can use only the spec-compliant mode?

@valfirst
Copy link
Contributor

@diemol sometimes users do not have full control over their infrastructure, for examples, clouds, like, SauceLabs or BrowserStack

@diemol
Copy link
Member

diemol commented Oct 28, 2022

Well, cloud providers should have relative recent versions available. Which is why I was asking what is the timeline/deadline where we can use only the spec-compliant mode?

@mykola-mokhnach
Copy link
Author

I've only updated drivers recently. Which means we'd need to keep the legacy implementation for quite a while, at least one year. Also, the change would be a breaking one (e.g. I would expect changing of the major lib version)

@titusfortner
Copy link
Member

What driver versions are compliant and which break?
What would prevent someone from updating their driver versions?
If they can't update driver versions, can't they lock their Selenium version to an older version?
How hard would it be for Appium to create its own implementation of screenshots that allow non-compliant coding?

@titusfortner
Copy link
Member

I'm somewhat sympathetic to maintaining backwards compatibility, but we've pushed hard to be completely standards compliant. If we revert the change to the non-compliant option it has to be a temporary fix; i.e., we need a better long-term plan than "wait a year."

@mykola-mokhnach
Copy link
Author

mykola-mokhnach commented Oct 28, 2022

What driver versions are compliant and which break?

From these that I know would break: appium-xcuitest-driver, appium-uiautomator2-driver, appium-espresso-driver, appium-mac2-driver
Basically all the main mobile drivers. The base64 screenshot encoding has been changed there some days ago, so basically all the previous versions of them would stop working properly

What would prevent someone from updating their driver versions?

Many people still use Appium 1. Newer driver versions are not compatible to it.

If they can't update driver versions, can't they lock their Selenium version to an older version?

They can. Although it won't work if/when they decide to update their appium client, because we also have minimum selenium client version requirements.

How hard would it be for Appium to create its own implementation of screenshots that allow non-compliant coding?

If we are talking only about screenshots then not very hard, I would just have to override one method in the derived driver class

@titusfortner
Copy link
Member

What else besides screenshots is affected?

If we only need to update one method, and we can be reasonably assured that if they are using the latest Selenium they can use the latest Appium, this seems like the best solution for now.

For the future, we do want to see if we can determine these issues ahead of time. :)

@mykola-mokhnach
Copy link
Author

mykola-mokhnach commented Oct 28, 2022

AFAIK base64 encoding is only used for screenshots. I don't remember any other places it would be needed

@titusfortner
Copy link
Member

Thanks for raising this for us to discuss. Looks like we have a good-enough solution for now, so I'll close this.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants