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

Unhandled exception when checking if URL exists #101

Open
BansheeHero opened this issue Mar 12, 2024 · 8 comments
Open

Unhandled exception when checking if URL exists #101

BansheeHero opened this issue Mar 12, 2024 · 8 comments
Labels
enhancement New feature or request fixed The issue is fixed good first issue Good for newcomers please test Fix was pushed and should be tested stale No news, closing

Comments

@BansheeHero
Copy link

BansheeHero commented Mar 12, 2024

Impact: Cannot build or serve on some network isolated machines.
Issue: Code expects that request is always made and only evaluates the final HTTP code:

def url_exists(url:str, local_base_dir:str='') -> bool:
    "Checks that a url exists"
    if url.startswith('http'):
        request = requests.get(url)
        return request.status_code == 200

Discovered on ZScaler networks, where the product fails to inject SSL certificates. This part is not why this issue is raised.
Based on reading the code I would expect there to be a try and error message: "Cannot find Mermaid library: %s" % javascript

I would like to also request an option to skip this check - this URL is available in the browser itself due to the way ZScaler works differently to python environment. This seems to align with the intention to just load the remote URL later with:
import mermaid from "https://unpkg.com/[email protected]/dist/mermaid.esm.min.mjs";

I was able to reproduce the request, confirming the SSL issue is not related to this project.

>>> import requests
>>> request = requests.get("https://unpkg.com/mermaid@%s/dist/mermaid.esm.min.mjs")
cadmwXcAs6MGMHWs
Traceback (most recent call last):
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\urllib3\connectionpool.py", line 468, in _make_request
    self._validate_conn(conn)
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\urllib3\connectionpool.py", line 1097, in _validate_conn
    conn.connect()
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\urllib3\connection.py", line 642, in connect
    sock_and_verified = _ssl_wrap_socket_and_match_hostname(
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\urllib3\connection.py", line 783, in _ssl_wrap_socket_and_match_hostname
    ssl_sock = ssl_wrap_socket(
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\urllib3\util\ssl_.py", line 471, in ssl_wrap_socket
    ssl_sock = _ssl_wrap_socket_impl(sock, context, tls_in_tls, server_hostname)
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\urllib3\util\ssl_.py", line 515, in _ssl_wrap_socket_impl
    return ssl_context.wrap_socket(sock, server_hostname=server_hostname)
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\ssl.py", line 513, in wrap_socket
    return self.sslsocket_class._create(
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\ssl.py", line 1071, in _create
    self.do_handshake()
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\ssl.py", line 1342, in do_handshake
    self._sslobj.do_handshake()
ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1007)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\urllib3\connectionpool.py", line 791, in urlopen
    response = self._make_request(
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\urllib3\connectionpool.py", line 492, in _make_request
    raise new_e
urllib3.exceptions.SSLError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1007)

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\requests\adapters.py", line 486, in send
    resp = conn.urlopen(
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\urllib3\connectionpool.py", line 845, in urlopen
    retries = retries.increment(
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\urllib3\util\retry.py", line 515, in increment
    raise MaxRetryError(_pool, url, reason) from reason  # type: ignore[arg-type]
urllib3.exceptions.MaxRetryError: HTTPSConnectionPool(host='unpkg.com', port=443): Max retries exceeded with url: /mermaid@%25s/dist/mermaid.esm.min.mjs (Caused by SSLError(SSLCertVerificationError(1, '[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1007)')))

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\requests\api.py", line 73, in get
    return request("get", url, params=params, **kwargs)
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\requests\api.py", line 59, in request
    return session.request(method=method, url=url, **kwargs)
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\requests\sessions.py", line 589, in request
    resp = self.send(prep, **send_kwargs)
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\requests\sessions.py", line 703, in send
    r = adapter.send(request, **kwargs)
  File "C:\Users\User\AppData\Local\Programs\Python\Python310\lib\site-packages\requests\adapters.py", line 517, in send
    raise SSLError(e, request=request)
requests.exceptions.SSLError: HTTPSConnectionPool(host='unpkg.com', port=443): Max retries exceeded with url: /mermaid@%25s/dist/mermaid.esm.min.mjs (Caused by SSLError(SSLCertVerificationError(1, '[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1007)')))

https://github.com/fralau/mkdocs-mermaid2-plugin/pull/102 - Pull request for handling the exception,

Copy link

Thank you for your contribution! This is very appreciated.

@EvaSDK
Copy link

EvaSDK commented Apr 4, 2024

Came for the same issue. I wanted to remove use of extra_javascript and extra_css but since I'm building our documentation in an isolated network, the build fails checking the URL of mermaid on unpkg.
I had a look at #102 @BansheeHero and it looks good but I would recommend a simpler strategy to allow not even checking the URL and warn that it might lead to non-functional graphs. The reason is that it would avoid doing any connections at all thus no exception handling, timeouts, etc to deal with. What do you think?

@fralau fralau added the good first issue Good for newcomers label Apr 4, 2024
@fralau
Copy link
Owner

fralau commented Apr 4, 2024

Thank you. The business case makes sense and the approach appears sensible.

@BansheeHero
Copy link
Author

BansheeHero commented Jun 3, 2024

I would advise against implementing a skip without making sure the admin is aware of the issue.
There is a gap between ignoring the network problem and not being aware of it at all.
(EDIT: Remember that we are dealing with injecting remote javascript to client's browsers. This is not something to leave to as a warning that is skipped by default.)

Anyway, there are two PRs I tried to made in the same style as the original repo. One is correct reporting and the other is the way I worked around the problem. I split them because I consider one to be a bug and the other enhancement.

Let me know if there is something I can do better.

@fralau fralau linked a pull request Jun 3, 2024 that will close this issue
@fralau
Copy link
Owner

fralau commented Jun 3, 2024

@BansheeHero Your approach has some good points.You complemented it with two procedures warning() and critical(), something I had just done with Mkdocs-Macros (fralau/mkdocs-macros-plugin@4f96dd5), in a slightly more general way.

Maybe you wish to compare?

The question we need to ask at this point, is how we want this issue to initially appear in the build process: as a warning to the admin (which would break only if the --strict option is used), or as an error?

(For more background, see fralau/mkdocs-macros-plugin#226.)

@EvaSDK
Copy link

EvaSDK commented Aug 16, 2024

Since our documentation has a CI/CD pipeline, it is easy ok for us to make it fail in case there is an error in the documentation (missing documents, broken links, etc). As long as we can skip some specific errors that are expected due to network isolation.

@fralau
Copy link
Owner

fralau commented Aug 20, 2024

Thanks for all the input and PRs. To sum it up, we have an issue of

  1. Unhandled exception (bug).
  2. A mechanism for gracefully failing or not (enhancement).

@BansheeHero Is that your analysis too?

In general, this puts into question the mechanism for calling the library. In principle, if there is a local file, it should not make an http call., and that might solve the problem, I believe.

Or, in the normal case just issue a warning and continue. If there is an --strict option (as mentioned) we could make it fail.

@EvaSDK Would that second solution work for you?

fralau pushed a commit that referenced this issue Sep 5, 2024
  - Fixed behavior of the `url_exists()` function
  - Added log levels (Warning)
  - Documented the change
@fralau fralau added fixed The issue is fixed please test Fix was pushed and should be tested labels Sep 5, 2024
@fralau
Copy link
Owner

fralau commented Sep 5, 2024

@BansheeHero @EvaSDK I took the idea of the pull request (#102), simplified it a little, and documented.

Now a WARNING is issued when no Internet access is available and mkdocs continues (unless the option --strict is used).

Could you please test whether it works for you?

@fralau fralau added the stale No news, closing label Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed The issue is fixed good first issue Good for newcomers please test Fix was pushed and should be tested stale No news, closing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants