-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
fix(core): Throw NodeSSLError
only for nodes that allow ignoring SSL issues
#6928
Conversation
Great PR! Please pay attention to the following items before merging: Files matching
Files matching
Make sure to check off this list before asking for review. |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #6928 +/- ##
==========================================
- Coverage 24.85% 24.84% -0.01%
==========================================
Files 3144 3144
Lines 191233 191233
Branches 21091 21088 -3
==========================================
- Hits 47524 47521 -3
- Misses 142730 142733 +3
Partials 979 979
☔ View full report in Codecov by Sentry. |
@@ -720,7 +720,7 @@ export async function proxyRequestToAxios( | |||
error: responseData, | |||
response: pick(response, ['headers', 'status', 'statusText']), | |||
}); | |||
} else if (error instanceof Error && error.message.includes('SSL routines')) { | |||
} else if ('rejectUnauthorized' in configObject && error.code?.includes('CERT')) { |
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.
Yes, rejectUnauthorized limits the error to HTTP Request node and a couple of other nodes.
Is there why we need to change error.message.includes('SSL routines')
? Does the error.code?.includes('CERT')
matches more SSL issue errors?
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 tested this using a few urls from https://badssl.com/ and looks like axios catches some SSL issues and rewrites error.message
. error.code
was a lot more reliable as a check.
Passing run #1977 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
✅ All Cypress E2E specs passed |
# [1.5.0](https://github.com/n8n-io/n8n/compare/[email protected]@1.5.0) (2023-08-31) ### Bug Fixes * **Agile CRM Node:** Fix issue with company address not working ([#6997](#6997)) ([2f81652](2f81652)) * **Code Node:** Switch over to vm2 fork ([#7018](#7018)) ([dfe0fa6](dfe0fa6)) * **core:** Invalid NODES_INCLUDE should not crash the app ([#7038](#7038)) ([04e3178](04e3178)), closes [#6683](#6683) * **core:** Setup websocket keep-live messages ([#6866](#6866)) ([8bdb07d](8bdb07d)), closes [#6757](#6757) * **core:** Throw `NodeSSLError` only for nodes that allow ignoring SSL issues ([#6928](#6928)) ([a01c3fb](a01c3fb)) * **Date & Time Node:** Dont parse date if it's not set (null or undefined) ([#7050](#7050)) ([d72f79f](d72f79f)) * **editor:** Fix sending of Ask AI tracking events ([#7002](#7002)) ([fb05afa](fb05afa)) * **Microsoft Excel 365 Node:** Support for more extensions in workbook rlc ([#7020](#7020)) ([d6e1cf2](d6e1cf2)) * **MongoDB Node:** Stringify response ObjectIDs ([#6990](#6990)) ([9ca990b](9ca990b)) * **MongoDB Node:** Upgrade mongodb package to address CVE-2021-32050 ([#7054](#7054)) ([d3f6356](d3f6356)) * **Postgres Node:** Empty return data fix for Postgres and MySQL ([#7016](#7016)) ([176ccd6](176ccd6)) * **Webhook Node:** Fix URL params for webhooks ([#6986](#6986)) ([596b569](596b569)) ### Features * **core:** External Secrets storage for credentials ([#6477](#6477)) ([ed927d3](ed927d3)) * **core:** Add MFA ([#4767](#4767)) ([2b7ba6f](2b7ba6f)) * **core:** Add filtering, selection and pagination to users ([#6994](#6994)) ([b716241](b716241)) * **editor:** Debug executions in the editor ([#6834](#6834)) ([c833078](c833078)) * **RSS Read Node:** Add support for self signed certificates ([#7039](#7039)) ([3b9f0fe](3b9f0fe)) Co-authored-by: netroy <[email protected]>
Got released with |
No description provided.