Add native TLS transport backends#428
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a cross-platform TLS transport unit and integrates it into the HTTP client, replaces OpenSSL-specific HTTPS plumbing, adds an HTTPS CLI smoke test, updates fetch docs to list per-platform TLS backends, and tightens CI to detect embedded OpenSSL symbols on Windows and set macOS SDK search paths. Changes
Sequence Diagram(s)sequenceDiagram
participant HTTPClient
participant TransportSecurity
participant TLSBackend as TLS Backend
participant Socket
HTTPClient->>TransportSecurity: StartTransportSecurity(socket, host)
activate TransportSecurity
TransportSecurity->>TLSBackend: Initialize backend & configure verification
TLSBackend->>Socket: Perform TLS handshake (I/O)
Socket-->>TLSBackend: Handshake responses
TLSBackend-->>TransportSecurity: Handshake complete
deactivate TransportSecurity
HTTPClient->>TransportSecurity: TransportSecurityWrite(data)
activate TransportSecurity
TransportSecurity->>TLSBackend: Encrypt & send
TLSBackend->>Socket: Write encrypted bytes
deactivate TransportSecurity
HTTPClient->>TransportSecurity: TransportSecurityRead(buf)
activate TransportSecurity
TLSBackend->>Socket: Read encrypted bytes
TLSBackend->>TLSBackend: Decrypt
TransportSecurity-->>HTTPClient: Return plaintext
deactivate TransportSecurity
HTTPClient->>TransportSecurity: CloseTransportSecurity()
activate TransportSecurity
TransportSecurity->>TLSBackend: Shutdown & cleanup
TLSBackend->>Socket: Close
deactivate TransportSecurity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
documentation
Contributor
Benchmark Results386 benchmarks Interpreted: 🟢 337 improved · 🔴 10 regressed · 39 unchanged · avg +9.1% arraybuffer.js — Interp: 🟢 12, 2 unch. · avg +7.5% · Bytecode: 🟢 10, 4 unch. · avg +5.3%
arrays.js — Interp: 🟢 17, 2 unch. · avg +10.1% · Bytecode: 🟢 10, 🔴 2, 7 unch. · avg +3.2%
async-await.js — Interp: 🟢 3, 3 unch. · avg +6.3% · Bytecode: 🟢 4, 2 unch. · avg +4.9%
base64.js — Interp: 🟢 10 · avg +11.2% · Bytecode: 🟢 8, 2 unch. · avg +7.7%
classes.js — Interp: 🟢 26, 5 unch. · avg +6.1% · Bytecode: 🟢 8, 🔴 4, 19 unch. · avg +0.9%
closures.js — Interp: 🟢 11 · avg +8.1% · Bytecode: 🟢 9, 2 unch. · avg +3.4%
collections.js — Interp: 🟢 12 · avg +11.4% · Bytecode: 🟢 10, 2 unch. · avg +12.9%
csv.js — Interp: 🟢 12, 1 unch. · avg +9.6% · Bytecode: 🟢 1, 🔴 9, 3 unch. · avg -2.8%
destructuring.js — Interp: 🟢 21, 1 unch. · avg +8.2% · Bytecode: 🟢 17, 5 unch. · avg +6.4%
fibonacci.js — Interp: 🟢 8 · avg +11.2% · Bytecode: 🟢 5, 3 unch. · avg +3.8%
float16array.js — Interp: 🟢 26, 🔴 4, 2 unch. · avg +3.3% · Bytecode: 🟢 16, 🔴 4, 12 unch. · avg +5.4%
for-of.js — Interp: 🟢 7 · avg +11.2% · Bytecode: 🟢 5, 2 unch. · avg +4.2%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 27, 15 unch. · avg +3.9% · Bytecode: 🟢 27, 🔴 2, 13 unch. · avg +3.9%
json.js — Interp: 🟢 20 · avg +13.0% · Bytecode: 🟢 9, 🔴 2, 9 unch. · avg +2.2%
jsx.jsx — Interp: 🟢 21 · avg +9.7% · Bytecode: 🔴 3, 18 unch. · avg -0.1%
modules.js — Interp: 🟢 9 · avg +8.9% · Bytecode: 🟢 1, 🔴 3, 5 unch. · avg -0.4%
numbers.js — Interp: 🟢 11 · avg +14.4% · Bytecode: 🟢 7, 4 unch. · avg +5.3%
objects.js — Interp: 🟢 7 · avg +10.0% · Bytecode: 🟢 1, 🔴 4, 2 unch. · avg -4.0%
promises.js — Interp: 🟢 12 · avg +8.3% · Bytecode: 🟢 8, 4 unch. · avg +4.5%
regexp.js — Interp: 🟢 11 · avg +13.0% · Bytecode: 🟢 1, 🔴 4, 6 unch. · avg -0.5%
strings.js — Interp: 🟢 11, 🔴 3, 5 unch. · avg -1.8% · Bytecode: 🟢 16, 🔴 1, 2 unch. · avg +13.4%
tsv.js — Interp: 🟢 9 · avg +11.1% · Bytecode: 🟢 1, 🔴 4, 4 unch. · avg -3.2%
typed-arrays.js — Interp: 🟢 18, 🔴 3, 1 unch. · avg +6.8% · Bytecode: 🟢 22 · avg +10.7%
uint8array-encoding.js — Interp: 🟢 16, 2 unch. · avg +38.9% · Bytecode: 🟢 8, 🔴 9, 1 unch. · avg -10.9%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@scripts/test-cli-apps.ts`:
- Around line 547-560: The HTTPS smoke test spawns LOADER with Bun.spawnSync and
lacks a timeout, so network stalls can hang the test matrix; update the
Bun.spawnSync call in the HTTPS fetch block to include a timeout (e.g., timeout:
10_000) and then check proc.exitedDueToTimeout after the call, throwing an
appropriate error if true (matching the existing timeout-handling pattern used
elsewhere in this file) before validating proc.exitCode, json.ok, and
json.value.
In `@source/shared/TransportSecurity.pas`:
- Around line 491-507: ReadOpenSSL and WriteOpenSSL must not return raw
SslRead/SslWrite values; replicate the SSL_get_error handling from
SecureTransport (the block around SecureTransport lines ~300–325) so
non-positive results are mapped: call SSL_get_error(Data.SSL, rc) and treat
SSL_ERROR_ZERO_RETURN as a graceful EOF (return 0),
SSL_ERROR_WANT_READ/SSL_ERROR_WANT_WRITE as a non-fatal retry condition
(preserve the same retry indication used by SecureTransport), and raise
ETransportSecurityError for SSL_ERROR_SSL and other fatal errors; update
ReadOpenSSL and WriteOpenSSL (using TOpenSSLData and Data.SSL) to implement this
mapping instead of returning SslRead/SslWrite directly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc78e3b0-cd41-4599-a9df-fc3440047407
📒 Files selected for processing (5)
.github/workflows/ci.ymldocs/built-ins.mdscripts/test-cli-apps.tssource/shared/HTTPClient.passource/shared/TransportSecurity.pas
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/shared/TransportSecurity.pas`:
- Around line 697-706: The function ReceiveIntoBuffer currently returns Boolean
and collapses recv==0 and recv==-1; change ReceiveIntoBuffer to return an
Integer (signed byte count) and return the raw Count from SocketReceive so
callers can see negative error codes, keep the local Temporary buffer and only
call AppendBytes(ABuffer, ...) when Count > 0; update all callers (notably
ReadSChannel) to check for Count < 0 as a transport error, Count = 0 as clean
EOF, and Count > 0 as data read. Ensure the function signature and any variable
types are updated accordingly so SocketReceive's signed result is preserved.
- Around line 414-421: The code currently skips hostname verification if the
SSL_set1_host symbol is missing; modify the logic around TSSLSetHostName
(SetHostName) lookup so that if GetProcedureAddress(SSLLibHandle,
'SSL_set1_host') returns nil you treat it as a fatal error: raise
ETransportSecurityError with a clear message (e.g., "OpenSSL library does not
provide SSL_set1_host; hostname verification unavailable") instead of silently
proceeding; keep the existing path that calls SetHostName(ASSL,
PAnsiChar(HostName)) and raises on non-1 return, and ensure HostName/ASSL are
prepared before the check so failure happens early when SetHostName is not
assigned.
- Line 440: Replace the version-locked call Data.Context :=
SslCtxNew(SslMethodTLSV1_2) with a version-flexible client method (e.g.
SslCtxNew(SslMethodTLS_CLIENT) or SslCtxNew(SslMethodTLS)) and then explicitly
set the minimum protocol to TLS1.2 on the context (use the binding for
SSL_CTX_set_min_proto_version or the equivalent wrapper) so the client
negotiates the highest supported TLS version but enforces >= TLS1.2; update
references to SslCtxNew, SslMethodTLSV1_2, and Data.Context accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ec715bf-7cfc-4424-a2a8-a7da9febbf83
📒 Files selected for processing (2)
scripts/test-cli-apps.tssource/shared/TransportSecurity.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/test-cli-apps.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/shared/TransportSecurity.pas`:
- Around line 1096-1140: TransportSecurityRead and TransportSecurityWrite must
early-return when ALength is zero (or negative) to avoid backends dereferencing
ABuffer[0]; add a guard at the start of both functions (checking if ALength <=
0) and immediately set Result := 0 and exit, so the dispatcher never forwards
zero-length operations into ReadSecureTransport/ReadSChannel/ReadOpenSSL or the
corresponding Write* functions.
- Around line 877-887: The current shutdown only calls ApplyControlToken with
SCHANNEL_SHUTDOWN and immediately DeleteSecurityContext, which prevents sending
the TLS close_notify; implement the full Schannel shutdown handshake: after
setting up ShutdownToken/ShutdownBuffer/ShutdownDesc and calling
ApplyControlToken on Data.Context, enter a loop that calls
InitializeSecurityContext (or AcceptSecurityContext on server side) with empty
input buffers and the Data.Context as the existing context, send any output
token produced to the peer, and repeat until the call returns
SEC_I_CONTEXT_EXPIRED or SEC_E_OK, then call DeleteSecurityContext; update the
code around Data.HasContext, ShutdownBuffer/ShutdownDesc, ApplyControlToken,
InitializeSecurityContext/AcceptSecurityContext and DeleteSecurityContext to
perform this sequence and ensure final output tokens are transmitted before
deleting the context.
- Around line 802-806: Add the ISC_REQ_USE_SUPPLIED_CREDS flag to the
InitializeSecurityContextW calls (e.g., the call using
Data.Credential/ExistingContext/TargetName and the second similar call around
lines 830-841) so Schannel will not implicitly select and send a client
certificate; include ISC_REQ_USE_SUPPLIED_CREDS alongside the existing ISC_REQ_*
flags. Also update the handshake-result handling that examines the Status from
InitializeSecurityContextW to treat SEC_I_INCOMPLETE_CREDENTIALS as a hard
failure (return/error) instead of mapping it into the
SEC_I_CONTINUE_NEEDED/continue path so the handshake loop does not wait for
client-cert progress that cannot occur.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3f8c38f-8451-42f6-ae7e-809ebaa6ac9c
📒 Files selected for processing (1)
source/shared/TransportSecurity.pas
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 706-715: The CI step "Check Windows TLS dependencies" currently
uses Write-Error which is non-terminating; update the PowerShell block that
reads build/GocciaScriptLoader.exe (variables $path, $bytes, $text) to fail the
step when OpenSSL symbols are found by replacing the Write-Error call with a
terminating action (e.g., use throw "GocciaScriptLoader.exe must not depend on
OpenSSL DLLs for HTTPS on Windows" or call exit 1) so the job returns a non-zero
exit code when the regex match detects libssl|libcrypto.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5e56393-c731-434d-80c0-73ba7f12898a
📒 Files selected for processing (1)
.github/workflows/ci.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #398