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

Support Modern Remote Chrome #71

Merged
merged 2 commits into from
Nov 27, 2024
Merged

Support Modern Remote Chrome #71

merged 2 commits into from
Nov 27, 2024

Conversation

srvrguy
Copy link
Contributor

@srvrguy srvrguy commented Jul 1, 2024

Starting in version 66, Chrome DevTools requires the host header to be “localhost” or an IP address. Work around this by setting the header to “lcoalhost” when fetching the version endpoint.

@cla-bot cla-bot bot added the cla/signed label Jul 1, 2024
@srvrguy
Copy link
Contributor Author

srvrguy commented Jul 1, 2024

This particular issue has been a problem since Chrome version 66 in 2018 when a change was made to improve security on DevTools. If you're using a local install, you won't run into the issue. I'm guessing most users of this module are configured in this manner.

The problem surfaces when using a container version of icingaweb2. Unless you make a custom container, the official one doesn't include Chrome installed. The best solution is to spin up a container running a headless version of Chrome. While you can get around the issue by putting in an IP address, this may change across container restarts. Especially under Kubernetes, you will want to use the friendly DNS name to fix this.

This small patch works around the security issue by forcing the host header to "localhost" when fetching the version endpoint. With that set, the expected values are returned and can be used by the rest of the module.

@nilmerg
Copy link
Member

nilmerg commented Jul 25, 2024

Hi, thank your for pointing this out! However, since this is a case specific to containerized setups, I think it is safer to first try a normal request. (without spoofing the host header) If this fails with the error Host header is specified and is not an IP address or localhost., your suggested solution can safely be performed.

Starting in version 66, Chrome DevTools requires the host header to be “localhost” or an IP address. Work around this by re-running the request without a host header if we get the security error.
@srvrguy
Copy link
Contributor Author

srvrguy commented Nov 21, 2024

The updated PR now handles the issue in a different way. Since Guzzle emits an exception on a 5xx error, I've wrapped the first request in a try block and am only catching the specific error thrown when getting the security error message. Inside the catch, I just have the request try again without a Host header. It's a big larger of a change than I would have liked, but does the job just fine.

Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

Thanks! Though, phpcs complains, please have a look :)

library/Pdfexport/HeadlessChrome.php Outdated Show resolved Hide resolved
@srvrguy
Copy link
Contributor Author

srvrguy commented Nov 26, 2024

Thanks! Though, phpcs complains, please have a look :)

Simple formatting issue that shouldn't take long to fix this week. Do you want me to add as a separate commit, or should I amend and force push to keep the history clean?

@nilmerg
Copy link
Member

nilmerg commented Nov 26, 2024

You can commit my suggestion directly here. I'll squash afterwards.

Adjusted formatting of code to pass PHPCS checks.
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

LGTM!

@nilmerg nilmerg merged commit c85de46 into Icinga:main Nov 27, 2024
15 checks passed
@srvrguy srvrguy deleted the remote-chrome branch November 27, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants