-
-
Notifications
You must be signed in to change notification settings - Fork 99
Fix CI errors and tear down socket errors #770
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
base: main
Are you sure you want to change the base?
Changes from all commits
f9bbfed
e28d855
015bfd4
70bfa38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -286,11 +286,19 @@ def wrap(self, sock): | |
raise errors.FatalSSLAlert( | ||
*tls_connection_drop_error.args, | ||
) from tls_connection_drop_error | ||
except ssl.SSLError as generic_tls_error: | ||
peer_speaks_plain_http_over_https = ( | ||
generic_tls_error.errno == ssl.SSL_ERROR_SSL | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What other error codes can this case have? |
||
and _assert_ssl_exc_contains(generic_tls_error, 'http request') | ||
) | ||
except ( | ||
ssl.SSLError, | ||
OSError, | ||
) as generic_tls_error: | ||
# When the client speaks plain HTTP into a TLS-only connection, | ||
# Python's builtin ssl raises an SSLError with `http request` | ||
# in its message. Sometimes, due to a race condition, the socket | ||
# is closed by the time we try to handle the error, resulting in an | ||
# OSError: [Errno 9] Bad file descriptor. | ||
peer_speaks_plain_http_over_https = isinstance( | ||
generic_tls_error, | ||
ssl.SSLError, | ||
) and _assert_ssl_exc_contains(generic_tls_error, 'http request') | ||
if peer_speaks_plain_http_over_https: | ||
reraised_connection_drop_exc_cls = errors.NoSSLError | ||
else: | ||
|
@@ -299,10 +307,6 @@ def wrap(self, sock): | |
raise reraised_connection_drop_exc_cls( | ||
*generic_tls_error.args, | ||
) from generic_tls_error | ||
except OSError as tcp_connection_drop_error: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why you felt the need to move this into another except-block. That makes it more complicated to follow. And AFAICS the lotig is about the same just more branchy. I was keeping it separate so that it's simple. |
||
raise errors.FatalSSLAlert( | ||
*tcp_connection_drop_error.args, | ||
) from tcp_connection_drop_error | ||
|
||
return s, self.get_environ(s) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extracted this into
main
. I have this typo across a bunch of repos, actually. Good catch!@sirosen any insight into why jsonschema didn't catch this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to work up a fix I can submit to schemastore for this, but I did... sort of figure it out.
The definition of a
pull_request
event mixes explicit properties with a$ref
usage (the target of the JSON Schema$ref
is also named"ref"
, which is a little quirky but fine). When$ref
loading is mixed with an explicit schema, as in this case, I don't have any clear expectation about what should happen. It might be covered by the spec, but it's definitely going to trip up implementations.The definition of
"ref"
looks fine at a glance:https://github.com/SchemaStore/schemastore/blame/71c836f7a50aa0f796c990c433307dc7b87e300e/src/schemas/json/github-workflow.json#L340-L386
But I notice that it doesn't set
"additionalProperties": false
, and I think that's the issue. If I remove that$ref
usage in a copy of the schema, I get the appropriate error.It looks like this is the only usage site for
#/definitions/ref
, so I think I'll just inline it and add a test which demonstrates the issue. JSON Schema is always a headtrip! 😵💫There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did end up getting a PR posted!
It ate a lot of my free time for FOSS work this evening, but I think it's worth it. There's a pretty large class of mistakes which
check-jsonschema
can't catch until this is fixed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sirosen nice, thanks!