CLI, Desktop: shim-init-node: add support for PQC TLS#15055
CLI, Desktop: shim-init-node: add support for PQC TLS#15055newAM wants to merge 1 commit intolaurent22:devfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdated HTTP agent configuration in the Node shim to use Changes
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
enhancement
|
I have read the CLA Document and I hereby sign the CLA |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/shim-init-node.ts (1)
704-733:⚠️ Potential issue | 🟠 MajorGate the PQC curve list behind runtime support.
ecdhCurveis part of Node/OpenSSL TLS negotiation. Whilst Electron 40.8.3 bundles OpenSSL 3.5.5 which supportsX25519MLKEM768, the codebase specifiesnode: >=18, which may use older OpenSSL versions that do not recognise this hybrid group. If an incompatible runtime is used,new https.Agent(...)will fail and break the plain-HTTP fallback.Additionally, hardcoding a fixed curve list replaces the adaptive default (
auto, which delegates to runtime selection) with a static group list, reducing interoperability for future runtimes.Recommended: detect OpenSSL/Node version support at runtime, only set this curve list when the hybrid group is available, or keep the default for unknown runtimes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/shim-init-node.ts` around lines 704 - 733, The code unconditionally sets tlsEcdhCurve = 'X25519MLKEM768:...' and passes it as ecdhCurve into new http.Agent/new https.Agent and proxyAgentConfig, which can crash on runtimes whose OpenSSL doesn't support the hybrid group; instead detect runtime support and only set ecdhCurve when available (or omit it to use the default 'auto'). Implement a runtime check (e.g. inspect process.versions.openssl or attempt a safe test creating a TLS context) and conditionally assign tlsEcdhCurve or leave it undefined, then use that variable in shim.httpAgent (inside shim.httpAgent_ initialization) and shim.proxyAgent (proxyAgentConfig) so Agents are created without the unsupported ecdhCurve when the runtime lacks support.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/lib/shim-init-node.ts`:
- Around line 704-733: The code unconditionally sets tlsEcdhCurve =
'X25519MLKEM768:...' and passes it as ecdhCurve into new http.Agent/new
https.Agent and proxyAgentConfig, which can crash on runtimes whose OpenSSL
doesn't support the hybrid group; instead detect runtime support and only set
ecdhCurve when available (or omit it to use the default 'auto'). Implement a
runtime check (e.g. inspect process.versions.openssl or attempt a safe test
creating a TLS context) and conditionally assign tlsEcdhCurve or leave it
undefined, then use that variable in shim.httpAgent (inside shim.httpAgent_
initialization) and shim.proxyAgent (proxyAgentConfig) so Agents are created
without the unsupported ecdhCurve when the runtime lacks support.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6e357d4-ecdb-4215-8f67-cb2feebc09df
📒 Files selected for processing (1)
packages/lib/shim-init-node.ts
|
Took a stab at auto-detection based on the bot feedback. As far as I can determine there's no way to tell what node uses for curves when it is set to "auto". On my system "auto" is |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lib/shim-init-node.ts`:
- Around line 586-587: The code assigns a single agent to requestOptions.agent
using resolvedProxyUrl/resolveProxyUrl and shim.proxyAgent/shim.httpAgent which
breaks cross-protocol redirects; replace this with an agents map on
requestOptions (requestOptions.agents = { http: <httpAgent>, https: <httpsAgent>
}) and populate each entry using the same proxy logic (use
shim.proxyAgent(url.href, resolvedProxyUrl) and shim.httpAgent(url.href) to
build the appropriate http and https agents) so follow-redirects can pick the
correct agent per protocol instead of reusing requestOptions.agent.
- Around line 531-533: The current assignment of options.agent uses a fixed
agent and breaks cross-protocol redirects; change options.agent to a function
callback that receives the request URL and returns the correct agent
per-redirect: if proxySettings.proxyEnabled and resolvedProxyUrl are set, call
shim.proxyAgent(url, resolvedProxyUrl) for that redirect URL, otherwise call
shim.httpAgent(url); update the code around resolvedProxyUrl, options.agent, and
the shim.fetchWithRetry call so fetchWithRetry receives this agent callback
instead of a single static agent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5f88408-0324-4427-91ef-985c29501781
📒 Files selected for processing (1)
packages/lib/shim-init-node.ts
| const resolvedProxyUrl = resolveProxyUrl(proxySettings.proxyUrl); | ||
| options.agent = (resolvedProxyUrl && proxySettings.proxyEnabled) ? shim.proxyAgent(url, resolvedProxyUrl) : null; | ||
| options.agent = (resolvedProxyUrl && proxySettings.proxyEnabled) ? shim.proxyAgent(url, resolvedProxyUrl) : shim.httpAgent(url); | ||
| return shim.fetchWithRetry(() => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/lib/shim-init-node.ts | head -750 | tail -250Repository: laurent22/joplin
Length of output: 10652
🏁 Script executed:
# Check the shim functions and agent creation
rg -A 10 "httpAgent|proxyAgent" packages/lib/shim-init-node.tsRepository: laurent22/joplin
Length of output: 3126
🏁 Script executed:
# Check package.json for node-fetch version
cat packages/lib/package.json | grep -A 2 -B 2 "node-fetch"Repository: laurent22/joplin
Length of output: 195
🌐 Web query:
node-fetch agent option callback function support documentation
💡 Result:
node-fetch does document support for an agent “callback” (i.e., a function) in fetch(url, options):
- In the official
node-fetchREADME, theagentoption is documented as either anhttp(s).Agentinstance or a function that returns an Agent. The README further explains that this function is called with the currentURL(useful for redirects that switch betweenhttp:andhttps:). [1] - The TypeScript typings for
node-fetch(DefinitelyTyped, as rendered by jsDocs) define it as:
agent?: RequestOptions['agent'] | ((parsedUrl: URL) => RequestOptions['agent']);[2]
Docs: see the node-fetch README → API → Options → Custom Agent. [1]
Sources
[1] node-fetch README (GitHub) (github.com)
[2] @types/node-fetch RequestInit typings (jsDocs) (jsdocs.io)
Citations:
Use an agent callback to support cross-protocol redirects.
node-fetch supports passing a function for the agent option, which is called with each redirect URL to allow protocol-specific agent selection. The current fixed agent on Line 532 means an http: URL that redirects to https: keeps the agent selected for the original scheme, causing the request to potentially fail.
Suggested change
- options.agent = (resolvedProxyUrl && proxySettings.proxyEnabled) ? shim.proxyAgent(url, resolvedProxyUrl) : shim.httpAgent(url);
+ options.agent = currentUrl => {
+ const currentUrlString = currentUrl.href ?? currentUrl.toString();
+ return (resolvedProxyUrl && proxySettings.proxyEnabled)
+ ? shim.proxyAgent(currentUrlString, resolvedProxyUrl)
+ : shim.httpAgent(currentUrlString);
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const resolvedProxyUrl = resolveProxyUrl(proxySettings.proxyUrl); | |
| options.agent = (resolvedProxyUrl && proxySettings.proxyEnabled) ? shim.proxyAgent(url, resolvedProxyUrl) : null; | |
| options.agent = (resolvedProxyUrl && proxySettings.proxyEnabled) ? shim.proxyAgent(url, resolvedProxyUrl) : shim.httpAgent(url); | |
| return shim.fetchWithRetry(() => { | |
| const resolvedProxyUrl = resolveProxyUrl(proxySettings.proxyUrl); | |
| options.agent = currentUrl => { | |
| const currentUrlString = currentUrl.href ?? currentUrl.toString(); | |
| return (resolvedProxyUrl && proxySettings.proxyEnabled) | |
| ? shim.proxyAgent(currentUrlString, resolvedProxyUrl) | |
| : shim.httpAgent(currentUrlString); | |
| }; | |
| return shim.fetchWithRetry(() => { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lib/shim-init-node.ts` around lines 531 - 533, The current
assignment of options.agent uses a fixed agent and breaks cross-protocol
redirects; change options.agent to a function callback that receives the request
URL and returns the correct agent per-redirect: if proxySettings.proxyEnabled
and resolvedProxyUrl are set, call shim.proxyAgent(url, resolvedProxyUrl) for
that redirect URL, otherwise call shim.httpAgent(url); update the code around
resolvedProxyUrl, options.agent, and the shim.fetchWithRetry call so
fetchWithRetry receives this agent callback instead of a single static agent.
| const resolvedProxyUrl = resolveProxyUrl(proxySettings.proxyUrl); | ||
| requestOptions.agent = (resolvedProxyUrl && proxySettings.proxyEnabled) ? shim.proxyAgent(url.href, resolvedProxyUrl) : null; | ||
| requestOptions.agent = (resolvedProxyUrl && proxySettings.proxyEnabled) ? shim.proxyAgent(url.href, resolvedProxyUrl) : shim.httpAgent(url.href); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/lib/shim-init-node.ts | sed -n '580,595p'Repository: laurent22/joplin
Length of output: 907
🏁 Script executed:
rg -A 10 "const resolvedProxyUrl = resolveProxyUrl" packages/lib/shim-init-node.tsRepository: laurent22/joplin
Length of output: 1324
🏁 Script executed:
rg "fetchBlob" packages/lib/shim-init-node.ts -B 5 -A 15Repository: laurent22/joplin
Length of output: 1086
🏁 Script executed:
rg "requestOptions" packages/lib/shim-init-node.ts -A 5 -B 2Repository: laurent22/joplin
Length of output: 1675
🏁 Script executed:
rg "http\.request\|https\.request" packages/lib/shim-init-node.ts -A 3 -B 2Repository: laurent22/joplin
Length of output: 42
🌐 Web query:
follow-redirects library agents vs agent parameter options API
💡 Result:
In follow-redirects you can use either the standard Node.js agent option or the library-specific agents option:
-
agent(standard http/https option): a single agent instance used for the request (works fine if you know you’ll stay on one protocol).follow-redirectssupports the normalhttp/httpsrequest options in addition to its own. [1] -
agents(follow-redirects-specific): lets you specify theagentper protocol because HTTP and HTTPS use different agent classes, and redirects can switch protocols (e.g.,http → https). Shape is:agents: { http?: http.Agent, https?: https.Agent }
[1][2]
Example:
const { http, https } = require('follow-redirects');
https.request({
url: 'http://example.com',
agents: {
http: new (require('http')).Agent({ keepAlive: true }),
https: new (require('https')).Agent({ keepAlive: true }),
},
});Sources: [1] follow-redirects README options docs, [2] Type definitions for agents shape.
🏁 Script executed:
rg "shim\.httpAgent\s*=|shim\.proxyAgent\s*=" packages/lib/shim-init-node.ts -A 5 -B 1Repository: laurent22/joplin
Length of output: 421
🏁 Script executed:
rg "httpAgent|proxyAgent" packages/lib/shim-init-node.ts | head -20Repository: laurent22/joplin
Length of output: 805
🏁 Script executed:
rg "shim\.httpAgent\s*=" packages/lib/shim-init-node.ts -A 15Repository: laurent22/joplin
Length of output: 443
🏁 Script executed:
rg "shim\.proxyAgent\s*=" packages/lib/shim-init-node.ts -A 25Repository: laurent22/joplin
Length of output: 1003
Use agents for follow-redirects downloads.
On line 587, a single agent is assigned to requestOptions.agent based on the initial URL scheme. However, follow-redirects requires an agents map to handle cross-protocol redirects correctly—when an HTTP download redirects to HTTPS (or vice versa), the current code reuses the wrong agent type. The library expects:
requestOptions.agents = { http: httpAgent, https: httpsAgent }This ensures each protocol gets its appropriate agent instance during the redirect chain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lib/shim-init-node.ts` around lines 586 - 587, The code assigns a
single agent to requestOptions.agent using resolvedProxyUrl/resolveProxyUrl and
shim.proxyAgent/shim.httpAgent which breaks cross-protocol redirects; replace
this with an agents map on requestOptions (requestOptions.agents = { http:
<httpAgent>, https: <httpsAgent> }) and populate each entry using the same proxy
logic (use shim.proxyAgent(url.href, resolvedProxyUrl) and
shim.httpAgent(url.href) to build the appropriate http and https agents) so
follow-redirects can pick the correct agent per protocol instead of reusing
requestOptions.agent.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/lib/shim-init-node.ts (2)
716-730:ecdhCurveonly applies to HTTPS agent.The
ecdhCurveoption is a TLS setting and has no effect onhttp.Agent. While this is harmless (the option is silently ignored), for clarity consider applying it only to the HTTPS agent.Suggested fix
shim.httpAgent = url => { if (!shim.httpAgent_) { - const AgentSettings = { + const baseSettings = { keepAlive: true, maxSockets: 1, keepAliveMsecs: 5000, - ecdhCurve: tlsEcdhCurve, }; shim.httpAgent_ = { - http: new http.Agent(AgentSettings), - https: new https.Agent(AgentSettings), + http: new http.Agent(baseSettings), + https: new https.Agent({ ...baseSettings, ecdhCurve: tlsEcdhCurve }), }; } return url.startsWith('https') ? shim.httpAgent_.https : shim.httpAgent_.http; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/shim-init-node.ts` around lines 716 - 730, The current shim.httpAgent creates a single AgentSettings object including ecdhCurve and uses it for both http and https agents; move ecdhCurve into the HTTPS-only options so the http.Agent is created without TLS-specific settings. Concretely, keep common options (keepAlive, maxSockets, keepAliveMsecs) in a shared object (used to construct shim.httpAgent_.http), then create a separate httpsOptions that spreads the common options and adds ecdhCurve, and use that to construct shim.httpAgent_.https in the shim.httpAgent factory.
704-714: Minor typo in comment.Line 706 has "Not supported on by all" which should be "Not supported by all".
The runtime detection approach for PQC TLS support is well-designed—it gracefully falls back to
'auto'when the curve isn't available on older Node.js/OpenSSL versions.Suggested fix
// X25519MLKEM768 is a post-quantum cryptography key exchange, details: // https://developers.cloudflare.com/ssl/post-quantum-cryptography/ -// Not supported on by all SSL stacks and versions, detect support at runtime. +// Not supported by all SSL stacks and versions, detect support at runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/shim-init-node.ts` around lines 704 - 714, Typo in the comment above the TLS PQC curve detection: change "Not supported on by all SSL stacks and versions, detect support at runtime." to "Not supported by all SSL stacks and versions, detect support at runtime." in the block that references tls.createSecureContext and the tlsEcdhCurve variable so the comment reads correctly while leaving the runtime detection logic (tls.createSecureContext and tlsEcdhCurve assignment/fallback) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/lib/shim-init-node.ts`:
- Around line 716-730: The current shim.httpAgent creates a single AgentSettings
object including ecdhCurve and uses it for both http and https agents; move
ecdhCurve into the HTTPS-only options so the http.Agent is created without
TLS-specific settings. Concretely, keep common options (keepAlive, maxSockets,
keepAliveMsecs) in a shared object (used to construct shim.httpAgent_.http),
then create a separate httpsOptions that spreads the common options and adds
ecdhCurve, and use that to construct shim.httpAgent_.https in the shim.httpAgent
factory.
- Around line 704-714: Typo in the comment above the TLS PQC curve detection:
change "Not supported on by all SSL stacks and versions, detect support at
runtime." to "Not supported by all SSL stacks and versions, detect support at
runtime." in the block that references tls.createSecureContext and the
tlsEcdhCurve variable so the comment reads correctly while leaving the runtime
detection logic (tls.createSecureContext and tlsEcdhCurve assignment/fallback)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06c8a790-215e-43cb-b1f5-f19ff35adfa8
📒 Files selected for processing (1)
packages/lib/shim-init-node.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/lib/shim-init-node.ts (2)
716-730: SeparateecdhCurveconfiguration forhttps.Agentonly.The
ecdhCurveoption is a TLS-specific setting that only applies tohttps.Agent. Passing it tohttp.Agentis semantically incorrect since plain HTTP connections don't use TLS. Although Node.js silently ignores unknown options (preventing runtime errors), this configuration is misleading.Suggested refactor
shim.httpAgent = url => { if (!shim.httpAgent_) { - const AgentSettings = { + const baseSettings = { keepAlive: true, maxSockets: 1, keepAliveMsecs: 5000, - ecdhCurve: tlsEcdhCurve, }; shim.httpAgent_ = { - http: new http.Agent(AgentSettings), - https: new https.Agent(AgentSettings), + http: new http.Agent(baseSettings), + https: new https.Agent({ ...baseSettings, ecdhCurve: tlsEcdhCurve }), }; } return url.startsWith('https') ? shim.httpAgent_.https : shim.httpAgent_.http; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/shim-init-node.ts` around lines 716 - 730, The AgentSettings currently includes the TLS-only option tlsEcdhCurve and is passed to both http.Agent and https.Agent via shim.httpAgent_ in shim.httpAgent; remove ecdhCurve from the settings used for the plain http.Agent and only include ecdhCurve (tlsEcdhCurve) when constructing the https.Agent. Concretely, build a base AgentSettings for both agents (keepAlive, maxSockets, keepAliveMsecs) and a separate httpsAgentSettings that spreads base and adds ecdhCurve, then instantiate shim.httpAgent_.http with new http.Agent(base) and shim.httpAgent_.https with new https.Agent(httpsAgentSettings) so tlsEcdhCurve is not passed to http.Agent.
732-752: Consider conditionally applyingecdhCurveonly toHttpsProxyAgent.The
ecdhCurveoption is only meaningful when establishing a TLS connection to the target server. WhenHttpProxyAgentis used (for HTTP server URLs), this option will not be utilised, so it can be omitted from that configuration to improve clarity.For consistency, separate the configuration objects:
Suggested change
shim.proxyAgent = (serverUrl: string, proxyUrl: string) => { - const proxyAgentConfig = { + const baseProxyConfig = { keepAlive: true, maxSockets: proxySettings.maxConcurrentConnections, keepAliveMsecs: 5000, proxy: proxyUrl, timeout: proxySettings.proxyTimeout * 1000, - ecdhCurve: tlsEcdhCurve, }; + const httpsProxyConfig = { ...baseProxyConfig, ecdhCurve: tlsEcdhCurve }; // Based on https://github.com/delvedor/hpagent#usage if (!isUrlHttps(proxyUrl) && !isUrlHttps(serverUrl)) { - return new HttpProxyAgent(proxyAgentConfig); + return new HttpProxyAgent(baseProxyConfig); } else if (isUrlHttps(proxyUrl) && !isUrlHttps(serverUrl)) { - return new HttpProxyAgent(proxyAgentConfig); + return new HttpProxyAgent(baseProxyConfig); } else if (!isUrlHttps(proxyUrl) && isUrlHttps(serverUrl)) { - return new HttpsProxyAgent(proxyAgentConfig); + return new HttpsProxyAgent(httpsProxyConfig); } else { - return new HttpsProxyAgent(proxyAgentConfig); + return new HttpsProxyAgent(httpsProxyConfig); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/shim-init-node.ts` around lines 732 - 752, The proxyAgent implementation in shim.proxyAgent currently always includes ecdhCurve (tlsEcdhCurve) in proxyAgentConfig even when returning an HttpProxyAgent; update shim.proxyAgent to build two config objects (or clone and adjust): one config for HttpProxyAgent (keepAlive, maxSockets from proxySettings.maxConcurrentConnections, keepAliveMsecs, proxy, timeout) and a separate config for HttpsProxyAgent that includes the same fields plus ecdhCurve: tlsEcdhCurve; then return HttpProxyAgent with the HTTP config when serverUrl is HTTP and/or proxyUrl is HTTP and return HttpsProxyAgent with the HTTPS config when serverUrl is HTTPS (or proxy requires TLS).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/lib/shim-init-node.ts`:
- Around line 716-730: The AgentSettings currently includes the TLS-only option
tlsEcdhCurve and is passed to both http.Agent and https.Agent via
shim.httpAgent_ in shim.httpAgent; remove ecdhCurve from the settings used for
the plain http.Agent and only include ecdhCurve (tlsEcdhCurve) when constructing
the https.Agent. Concretely, build a base AgentSettings for both agents
(keepAlive, maxSockets, keepAliveMsecs) and a separate httpsAgentSettings that
spreads base and adds ecdhCurve, then instantiate shim.httpAgent_.http with new
http.Agent(base) and shim.httpAgent_.https with new
https.Agent(httpsAgentSettings) so tlsEcdhCurve is not passed to http.Agent.
- Around line 732-752: The proxyAgent implementation in shim.proxyAgent
currently always includes ecdhCurve (tlsEcdhCurve) in proxyAgentConfig even when
returning an HttpProxyAgent; update shim.proxyAgent to build two config objects
(or clone and adjust): one config for HttpProxyAgent (keepAlive, maxSockets from
proxySettings.maxConcurrentConnections, keepAliveMsecs, proxy, timeout) and a
separate config for HttpsProxyAgent that includes the same fields plus
ecdhCurve: tlsEcdhCurve; then return HttpProxyAgent with the HTTP config when
serverUrl is HTTP and/or proxyUrl is HTTP and return HttpsProxyAgent with the
HTTPS config when serverUrl is HTTPS (or proxy requires TLS).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 871bd771-16dd-47d9-b9c9-f98823299a95
📒 Files selected for processing (2)
packages/lib/shim-init-node.tspackages/tools/cspell/dictionary2.txt
✅ Files skipped from review due to trivial changes (1)
- packages/tools/cspell/dictionary2.txt
|
@newAM Thank you for your submission. Please could you elaborate on the testing which you have done for this code change in the PR description? In particular, I'm wondering whether testing has actually been done on one of the mobile apps (eg. Android), or if you've assumed it would work because it worked on the Linux app |
|
Done! I did not test Android, only the joplin-desktop electron application on Linux. |
If you don't want to go through all the process of building the mobile app, I can build an apk with your change for you (using the Joplin dev signing certificate). Would you be willing to test if the PQC TLS works on Android on a spare device / Android emulator if I provide the apk for you? It doesn't matter so much if it doesn't actually work on Android, but I think it would be good to update the platforms which the fix addresses in the PR title accordingly if not, otherwise it is going to go in the changelog for all platforms and could be misleading. |
newAM
changed the title
|
Took some wrangling, but I built it on Android, I could not get it working. The Android app was sending I tried some changes to get this working for Android, but the APIs and call stack to the underlying system TLS libraries requires a lot of background knowledge that I do not yet have. I updated the PR title to correctly reflect the platforms this works on. Mobile is going to take more scrutiny. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/shim-init-node.ts (1)
732-752:⚠️ Potential issue | 🔴 CriticalRemove
ecdhCurvefromproxyAgentConfigor verify TLS configuration reaches the target server.The
hpagentlibrary (v1.2.0) does not support passingecdhCurvethrough to the underlyingtls.connect()call for the target server. The library'sproxyConnectOptionsis limited to proxy tunnel TLS setup, not target server TLS configuration. IncludingecdhCurveinproxyAgentConfigwill not configure PQC curves for HTTPS target servers and should be removed unlesshpagentgains this capability.Additionally,
ecdhCurveis irrelevant forHttpProxyAgent(HTTP targets do not use TLS).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/shim-init-node.ts` around lines 732 - 752, The proxyAgent function currently injects tlsEcdhCurve into proxyAgentConfig which hpagent (and HttpProxyAgent) doesn't forward to target-server TLS; remove the ecdhCurve property from proxyAgentConfig and ensure you only set TLS-specific options when constructing an HttpsProxyAgent if hpagent adds support — otherwise omit ecdhCurve entirely; update shim.proxyAgent so HttpProxyAgent never receives ecdhCurve and HttpsProxyAgent only receives options hpagent actually forwards (relying on proxySettings and tlsEcdhCurve only if you verify hpagent exposes target TLS options).
🧹 Nitpick comments (1)
packages/lib/shim-init-node.ts (1)
718-727:ecdhCurveonly applies to HTTPS connections.The
ecdhCurveoption is a TLS setting and only meaningful forhttps.Agent. Applying it tohttp.Agent(Line 725) has no effect since HTTP doesn't use TLS. While Node.js silently ignores unknown options, separating the configurations would be cleaner.Suggested change
shim.httpAgent = url => { if (!shim.httpAgent_) { - const AgentSettings = { + const baseSettings = { keepAlive: true, maxSockets: 1, keepAliveMsecs: 5000, - ecdhCurve: tlsEcdhCurve, }; shim.httpAgent_ = { - http: new http.Agent(AgentSettings), - https: new https.Agent(AgentSettings), + http: new http.Agent(baseSettings), + https: new https.Agent({ ...baseSettings, ecdhCurve: tlsEcdhCurve }), }; } return url.startsWith('https') ? shim.httpAgent_.https : shim.httpAgent_.http; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/shim-init-node.ts` around lines 718 - 727, The AgentSettings object is passing the TLS-specific tlsEcdhCurve into both http.Agent and https.Agent via shim.httpAgent_, but ecdhCurve only applies to TLS/HTTPS; update the initialization so http.Agent receives a settings object without ecdhCurve and https.Agent receives one that includes ecdhCurve (e.g., split into commonSettings and tlsSettings or create AgentSettingsHttp and AgentSettingsHttps), keeping the other properties (keepAlive, maxSockets, keepAliveMsecs) unchanged and use tlsEcdhCurve only when constructing https.Agent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/lib/shim-init-node.ts`:
- Around line 732-752: The proxyAgent function currently injects tlsEcdhCurve
into proxyAgentConfig which hpagent (and HttpProxyAgent) doesn't forward to
target-server TLS; remove the ecdhCurve property from proxyAgentConfig and
ensure you only set TLS-specific options when constructing an HttpsProxyAgent if
hpagent adds support — otherwise omit ecdhCurve entirely; update shim.proxyAgent
so HttpProxyAgent never receives ecdhCurve and HttpsProxyAgent only receives
options hpagent actually forwards (relying on proxySettings and tlsEcdhCurve
only if you verify hpagent exposes target TLS options).
---
Nitpick comments:
In `@packages/lib/shim-init-node.ts`:
- Around line 718-727: The AgentSettings object is passing the TLS-specific
tlsEcdhCurve into both http.Agent and https.Agent via shim.httpAgent_, but
ecdhCurve only applies to TLS/HTTPS; update the initialization so http.Agent
receives a settings object without ecdhCurve and https.Agent receives one that
includes ecdhCurve (e.g., split into commonSettings and tlsSettings or create
AgentSettingsHttp and AgentSettingsHttps), keeping the other properties
(keepAlive, maxSockets, keepAliveMsecs) unchanged and use tlsEcdhCurve only when
constructing https.Agent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35a62a19-ead4-4b7c-8876-5abeaa9a7fa5
📒 Files selected for processing (1)
packages/lib/shim-init-node.ts
|
Please let me know when you feel this is ready to merge. I guess we could add the change as part of 3.7 pre-release to get additional feedback |
I think it is good to go, everything works as expected when viewed from wireshark. |
3 participants
This is a new feature, adding post-quantum resistant TLS key exchange to Joplin.
Reference this forum post for details: https://discourse.joplinapp.org/t/postquantum-safe/49540/1
I tested
joplin-desktopon Linux (NixOS) with a webdav sync target behind nginx, using the nginx optionssl_ecdh_curve X25519MLKEM768;to enforce post-quantum key exchange.I also did a wireshark capture and validated that X25519MLKEM768 is sent on the wire in a TLS ClientHello.