feat: adding mock certificate support for ssl testing#123
Conversation
When only one of cert/key is provided without explicitly setting autoGenerate to true, throw an error instead of silently generating a new certificate pair and ignoring the supplied credential. https://claude.ai/code/session_01MUEtaXXUycATbbLTK13oL5
Summary of ChangesHello @jaredwray, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
===========================================
- Coverage 100.00% 99.52% -0.48%
===========================================
Files 36 37 +1
Lines 850 1042 +192
Branches 173 208 +35
===========================================
+ Hits 850 1037 +187
- Misses 0 5 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1cafcd475
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this._server = Fastify({ | ||
| ...getFastifyConfig(this._logging), | ||
| https: { | ||
| key: this._httpsCredentials.key, |
There was a problem hiding this comment.
This HTTPS branch is executed inside start()'s broad try/catch, so malformed PEM input causes Fastify({ https: ... }) to throw, get logged, and then start() still resolves; callers can observe isHttps === true even though the server is not usable. This makes TLS misconfiguration look like a successful startup and breaks readiness/error handling paths that depend on await start() rejecting when startup fails.
Useful? React with 👍 / 👎.
| if (ip.includes(".")) { | ||
| const parts = ip.split(".").map(Number); | ||
| return Buffer.from(parts); |
There was a problem hiding this comment.
The IP encoder classifies any value containing . as IPv4, so valid IPv6 text forms with embedded dotted quads (for example ::ffff:127.0.0.1) are mis-encoded as a 4-byte IPv4 SAN instead of a 16-byte IPv6 SAN. That produces certificates whose SubjectAltName does not match the requested IP representation and can cause TLS IP validation failures for those addresses.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-implemented feature for HTTPS support, including a self-contained module for generating mock SSL certificates. The code is well-structured, and the comprehensive tests for certificate generation and its integration into MockHttp are excellent. My feedback includes minor suggestions to improve code clarity and reduce duplication.
| const leftGroups = left ? left.split(":") : []; | ||
| const rightGroups = right ? right.split(":") : []; | ||
| const missing = 8 - leftGroups.length - rightGroups.length; | ||
| const middle = Array.from({ length: missing }).fill("0000") as string[]; |
There was a problem hiding this comment.
This line can be simplified. Array(n) creates a new array of length n, and .fill() can be called on it directly. This avoids the need for Array.from and the type assertion as string[], resulting in cleaner code.
| const middle = Array.from({ length: missing }).fill("0000") as string[]; | |
| const middle = Array(missing).fill("0000"); |
| if (options?.https !== undefined) { | ||
| if (options.https === true) { | ||
| this._https = { autoGenerate: true }; | ||
| } else if (options.https === false) { | ||
| this._https = undefined; | ||
| } else { | ||
| this._https = options.https; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling the https option is duplicated between the constructor and the https setter. You can simplify the constructor by calling the setter directly. This improves maintainability by centralizing the logic.
if (options?.https !== undefined) {
this.https = options.https;
}
2 participants
No description provided.