Conversation
The `connectWithDSN` func used `db.Exec()` which implicitly uses `context.Background()`[1]. This caused the registered "nucleitcp" dialer callback to receive a ctx missing the `executionId`, leading to a panic during type assertion. Refactor `connectWithDSN` to accept `executionId` explicitly and use it to create a `context` for `db.PingContext()` (yeah, instead of `db.Exec()`). And, add a defensive check in the dialer callback to handle nil values gracefully. Fixes #6733 regression introduced in #6296. [1]: "Exec uses `context.Background` internally" - https://pkg.go.dev/database/sql#DB.Exec. Signed-off-by: Dwi Siswanto <git@dw1.io>
Signed-off-by: Dwi Siswanto <git@dw1.io>
Signed-off-by: Dwi Siswanto <git@dw1.io>
WalkthroughAdds execution-scoped memoization keys across multiple protocol libraries, propagates executionId via context into MySQL connection paths (including signature changes and PingContext usage), adds a MySQL integration test that runs a temporary Docker MySQL container, and hardens dialer initialization to avoid panics. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/js/libs/mysql/mysql.go (1)
193-196: Signature update correctly enables context propagation.The method now accepts
ctxto extractexecutionId, aligning with the per-execution memoization and context-aware connection handling.Consider using the comma-ok idiom for the type assertion to guard against panics if
executionIdis unexpectedly missing from the context (same pattern applies to other methods in this file):🔎 Optional defensive type assertion
func (c *MySQLClient) ConnectWithDSN(ctx context.Context, dsn string) (bool, error) { - executionId := ctx.Value("executionId").(string) + executionId, ok := ctx.Value("executionId").(string) + if !ok { + return false, fmt.Errorf("executionId not found in context") + } return memoizedconnectWithDSN(executionId, dsn) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
integration_tests/protocols/javascript/mysql-connect.yamlis excluded by!**/*.yaml
📒 Files selected for processing (18)
cmd/integration-test/javascript.gopkg/js/libs/mssql/memo.mssql.gopkg/js/libs/mysql/memo.mysql.gopkg/js/libs/mysql/memo.mysql_private.gopkg/js/libs/mysql/mysql.gopkg/js/libs/mysql/mysql_private.gopkg/js/libs/oracle/memo.oracle.gopkg/js/libs/pop3/memo.pop3.gopkg/js/libs/postgres/memo.postgres.gopkg/js/libs/rdp/memo.rdp.gopkg/js/libs/redis/memo.redis.gopkg/js/libs/rsync/memo.rsync.gopkg/js/libs/smb/memo.smb.gopkg/js/libs/smb/memo.smb_private.gopkg/js/libs/smb/memo.smbghost.gopkg/js/libs/telnet/memo.telnet.gopkg/js/libs/vnc/memo.vnc.gopkg/protocols/common/protocolstate/state.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code usinggo fmt ./...
Run static analysis usinggo vet ./...on Go code
Files:
pkg/js/libs/smb/memo.smb_private.gopkg/js/libs/telnet/memo.telnet.gopkg/js/libs/redis/memo.redis.gopkg/js/libs/smb/memo.smb.gopkg/js/libs/rdp/memo.rdp.gopkg/protocols/common/protocolstate/state.gopkg/js/libs/mysql/memo.mysql_private.gopkg/js/libs/mysql/mysql.gopkg/js/libs/mysql/mysql_private.gopkg/js/libs/mysql/memo.mysql.gopkg/js/libs/vnc/memo.vnc.gopkg/js/libs/pop3/memo.pop3.gocmd/integration-test/javascript.gopkg/js/libs/rsync/memo.rsync.gopkg/js/libs/oracle/memo.oracle.gopkg/js/libs/postgres/memo.postgres.gopkg/js/libs/mssql/memo.mssql.gopkg/js/libs/smb/memo.smbghost.go
pkg/js/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use custom JavaScript runtime for code protocol templates with auto-generated bindings from pkg/js/generated/
Files:
pkg/js/libs/smb/memo.smb_private.gopkg/js/libs/telnet/memo.telnet.gopkg/js/libs/redis/memo.redis.gopkg/js/libs/smb/memo.smb.gopkg/js/libs/rdp/memo.rdp.gopkg/js/libs/mysql/memo.mysql_private.gopkg/js/libs/mysql/mysql.gopkg/js/libs/mysql/mysql_private.gopkg/js/libs/mysql/memo.mysql.gopkg/js/libs/vnc/memo.vnc.gopkg/js/libs/pop3/memo.pop3.gopkg/js/libs/rsync/memo.rsync.gopkg/js/libs/oracle/memo.oracle.gopkg/js/libs/postgres/memo.postgres.gopkg/js/libs/mssql/memo.mssql.gopkg/js/libs/smb/memo.smbghost.go
pkg/js/libs/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use custom JavaScript library implementations located in pkg/js/libs/ for JavaScript protocol support
Files:
pkg/js/libs/smb/memo.smb_private.gopkg/js/libs/telnet/memo.telnet.gopkg/js/libs/redis/memo.redis.gopkg/js/libs/smb/memo.smb.gopkg/js/libs/rdp/memo.rdp.gopkg/js/libs/mysql/memo.mysql_private.gopkg/js/libs/mysql/mysql.gopkg/js/libs/mysql/mysql_private.gopkg/js/libs/mysql/memo.mysql.gopkg/js/libs/vnc/memo.vnc.gopkg/js/libs/pop3/memo.pop3.gopkg/js/libs/rsync/memo.rsync.gopkg/js/libs/oracle/memo.oracle.gopkg/js/libs/postgres/memo.postgres.gopkg/js/libs/mssql/memo.mssql.gopkg/js/libs/smb/memo.smbghost.go
pkg/protocols/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/protocols/**/*.go: Each protocol implementation should implement the Request interface with Compile(), ExecuteWithResults(), Match(), and Extract() methods
Protocol implementations should embed Operators for matching/extraction functionality
Files:
pkg/protocols/common/protocolstate/state.go
🧬 Code graph analysis (1)
cmd/integration-test/javascript.go (1)
pkg/testutils/integration.go (2)
TestCase(247-250)RunNucleiTemplateAndGetResults(30-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (11)
pkg/js/libs/redis/memo.redis.go (1)
12-12: LGTM! Execution-scoped memoization correctly applied across all protocol libraries.The memoization key updates consistently incorporate
executionIdacross all 8 modified memo files (rsync, telnet, rdp, vnc, mssql, mysql, oracle, redis). This change ensures cache entries are isolated per execution context rather than shared globally by host/port alone, which is essential for preventing the nilexecutionIdpanic described in the PR objectives.The pattern is mechanically correct and aligns with the PR's goal of fixing issue #6733.
Also applies to: 28-28, 44-44, 60-60
pkg/js/libs/postgres/memo.postgres.go (1)
15-29: LGTM - executionId scoping applied consistently.The memoization keys now correctly include
executionIdto ensure per-execution cache isolation, which addresses the root cause of the regression.pkg/js/libs/mysql/memo.mysql_private.go (1)
11-15: LGTM - Memoization wrapper correctly updated.The function now properly accepts and propagates
executionIdto the underlyingconnectWithDSNfunction, ensuring the cache key is execution-scoped.pkg/protocols/common/protocolstate/state.go (1)
203-210: Defensive nil handling correctly prevents the panic.The guarded extraction of
executionIdfrom context prevents the type assertion panic that was causing the regression. The subsequent dialer nil check with a clear error message improves debuggability.Note: If
executionIdis nil, this will pass an empty string toGetDialersWithId(""), which will return nil and trigger the error on line 209. This is acceptable behavior since the error message will indicate"dialers not initialized for ", though the empty ID might be slightly confusing in logs.pkg/js/libs/smb/memo.smb.go (1)
13-30: LGTM - Consistent executionId scoping for SMB functions.Both memoized SMB functions now include
executionIdin their cache keys, maintaining consistency with the broader caching pattern.pkg/js/libs/smb/memo.smbghost.go (1)
12-16: LGTM - SMBGhost detection memoization updated.The cache key now correctly includes
executionIdfor execution-scoped caching.pkg/js/libs/mysql/mysql_private.go (1)
76-92: Core fix correctly propagates executionId through context.The refactor from
db.Exec("select 1")todb.PingContext(ctx)with anexecutionId-enriched context is the correct solution. This ensures the custom MySQL dialer receives the execution context needed to retrieve the appropriate dialer instance.Good additions:
defer db.Close()ensures proper resource cleanup- Using
PingContextis more appropriate for connection verification than executing a querypkg/js/libs/smb/memo.smb_private.go (1)
15-19: LGTM - SMBv2 metadata collection memoization updated.The cache key now includes
executionIdfor proper execution scoping.cmd/integration-test/javascript.go (1)
382-399: MySQL Docker container setup looks correct.The container configuration follows the established pattern with proper environment variables and expiration handling.
pkg/js/libs/mysql/mysql.go (1)
111-111: LGTM!The change correctly propagates
executionIdtoconnectWithDSN, ensuring the dialer callback receives the proper execution context and the memoization cache is scoped per execution.pkg/js/libs/pop3/memo.pop3.go (1)
12-12: LGTM! Cache key now properly scoped by executionId.The addition of
executionIdto the memoization hash key correctly ensures that cached POP3 connection results are isolated per execution, preventing unintended cache hits across different execution contexts. This aligns with the PR's objective to propagateexecutionIdthroughout the connection flow.Since this file is generated by the memogen tool from the source function signature, the change reflects that
isPoP3()now includesexecutionIdas a parameter, and the memo wrapper was regenerated accordingly.
Signed-off-by: Dwi Siswanto <git@dw1.io>
…-executionId-in-ctx
Proposed changes
The
connectWithDSNfunc useddb.Exec()whichimplicitly uses
context.Background()[1]. Thiscaused the registered "nucleitcp" dialer
callback to receive a ctx missing the
executionId, leading to a panic during typeassertion.
Refactor
connectWithDSNto acceptexecutionIdexplicitly and use it to create a
contextfordb.PingContext()(yeah, instead ofdb.Exec()).And, add a defensive check in the dialer callback
to handle nil values gracefully.
Fixes #6733 regression introduced in #6296.
[1]: "Exec uses
context.Backgroundinternally" -https://pkg.go.dev/database/sql#DB.Exec.
Steps to Reproduce
Dumped JavaScript response:
Proof
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.