code from https://github.com/projectdiscovery/nuclei/pull/6427#6471
Merged
code from https://github.com/projectdiscovery/nuclei/pull/6427#6471
Conversation
Contributor
WalkthroughAdds Oracle support: new Oracle client library with connection, detection, and query execution; a custom dialer integrating policy checks; a JS integration test that spins up an Oracle XE Docker container and validates an auth template; and a new go-ora dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant IT as Integration Test
participant Docker as Oracle XE Container
participant Nuclei as Nuclei Runner
participant ORA as Oracle Listener (1521)
rect rgb(235, 245, 255)
note over IT,Docker: Setup
IT->>Docker: Start gvenzl/oracle-xe (ORACLE_PASSWORD=mysecret)
IT->>ORA: Retry readiness probe on localhost:1521
ORA-->>IT: Ready (port open)
end
rect rgb(240, 255, 240)
note over IT,Nuclei: Execute template
IT->>Nuclei: Run oracle-auth-test.yaml with finalURL (mapped 1521)
Nuclei-->>IT: Results
IT->>IT: Validate expectResultsCount(..., 1)
end
rect rgb(255, 240, 240)
note over IT,Docker: Teardown
IT->>Docker: Purge container
end
sequenceDiagram
autonumber
participant JS as JavaScript Runtime
participant OCI as OracleClient
participant Dial as oracleCustomDialer
participant PS as protocolstate
participant GOORA as go-ora Connector
participant DB as Oracle DB
rect rgb(235,245,255)
note over JS,OCI: Connect / ExecuteQuery Flow
JS->>OCI: Connect(host, port, service, user, pass)
OCI->>GOORA: Build DSN and create connector (with Dial)
GOORA->>Dial: DialContext(network, address)
Dial->>PS: GetDialersWithId(executionId)
PS-->>Dial: Fastdialer
Dial->>PS: IsHostAllowed(address)?
PS-->>Dial: Allowed / Denied
alt Allowed
Dial->>DB: TCP connect via Fastdialer
DB-->>GOORA: Connected
GOORA-->>OCI: Ping OK
OCI-->>JS: true
else Denied
Dial-->>GOORA: Error (host denied)
GOORA-->>OCI: Error
OCI-->>JS: false/error
end
end
rect rgb(240,255,240)
JS->>OCI: ExecuteQuery(host, port, user, pass, dbName, query)
OCI->>GOORA: Open via DSN, run query
GOORA->>DB: Execute SQL
DB-->>GOORA: Rows
GOORA-->>OCI: Rows
OCI-->>JS: utils.SQLResult
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (2 passed, 3 warnings)❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/js/libs/oracle/oracle.go (1)
81-81: Fix duplicate assignment to resp.Banner.Line 81 overwrites the value set in line 80. This appears to be a bug where both the version and metadata info should be preserved.
- resp.Banner = service.Version - resp.Banner = service.Metadata().(plugins.ServiceOracle).Info + resp.Banner = service.Version + if metadata, ok := service.Metadata().(plugins.ServiceOracle); ok && metadata.Info != "" { + resp.Banner = metadata.Info + }
🧹 Nitpick comments (4)
pkg/js/libs/oracle/oracledialer.go (2)
17-27: Consider adding connection timeout handling in dialWithCtx.The method properly integrates with the protocol state and policy checks. However, unlike
DialTimeoutwhich creates its own timeout context,dialWithCtxrelies entirely on the passed context for timeout control. Consider documenting that callers should set appropriate timeouts on the context.
29-31: Consider using context.Background() instead of context.TODO().Since this is a production implementation rather than a placeholder,
context.Background()would be more appropriate thancontext.TODO().func (o *oracleCustomDialer) Dial(network, address string) (net.Conn, error) { - return o.dialWithCtx(context.TODO(), network, address) + return o.dialWithCtx(context.Background(), network, address) }cmd/integration-test/javascript.go (1)
201-219: Consider increasing Oracle container startup timeout.Oracle XE containers typically take longer to initialize than Redis or SSH containers. The 30-second expiry might be insufficient for Oracle to fully start up, especially on slower systems. Consider extending this to 60-90 seconds.
- // by default expire after 30 sec - if err := oracleResource.Expire(30); err != nil { + // Oracle containers need more time to initialize + if err := oracleResource.Expire(90); err != nil {pkg/js/libs/oracle/oracle.go (1)
203-208: Clarify the error handling logic with a comment.The logic to return partial results when an error occurs might be confusing. Consider adding a comment to explain this behavior.
data, err := utils.UnmarshalSQLRows(rows) if err != nil { + // Return partial results if any rows were successfully parsed before the error if data != nil && len(data.Rows) > 0 { return data, nil } return nil, err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumpkg/js/generated/go/liboracle/oracle.gois excluded by!**/generated/**pkg/js/generated/ts/oracle.tsis excluded by!**/generated/**pkg/protocols/javascript/testcases/oracle-auth-test.yamlis excluded by!**/*.yaml
📒 Files selected for processing (4)
cmd/integration-test/javascript.go(3 hunks)go.mod(1 hunks)pkg/js/libs/oracle/oracle.go(4 hunks)pkg/js/libs/oracle/oracledialer.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/js/libs/oracle/oracledialer.gocmd/integration-test/javascript.gopkg/js/libs/oracle/oracle.go
🧬 Code graph analysis (3)
pkg/js/libs/oracle/oracledialer.go (2)
pkg/protocols/common/protocolstate/state.go (1)
GetDialersWithId(39-45)pkg/protocols/common/protocolstate/headless.go (2)
IsHostAllowed(124-150)ErrHostDenied(22-22)
cmd/integration-test/javascript.go (1)
pkg/testutils/integration.go (2)
TestCase(247-250)RunNucleiTemplateAndGetResults(30-32)
pkg/js/libs/oracle/oracle.go (3)
pkg/js/generated/ts/oracle.ts (8)
OracleClient(27-97)IsOracle(65-67)IsOracleResponse(13-16)Connect(40-42)ConnectWithDSN(53-55)ExecuteQuery(80-82)SQLResult(102-106)ExecuteQueryWithDSN(94-96)pkg/protocols/common/contextargs/contextargs.go (1)
Context(22-33)pkg/js/utils/util.go (1)
UnmarshalSQLRows(23-90)
⏰ 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: Integration tests (macOS-latest)
🔇 Additional comments (2)
go.mod (1)
116-116: LGTM! Oracle driver dependency added appropriately.The addition of
github.com/sijms/go-ora/v2 v2.9.0is correctly placed in the require block and aligns with the Oracle database support being added in this PR.pkg/js/libs/oracle/oracle.go (1)
113-114: Fix JSDoc syntax error.Missing parentheses in the constructor call.
// const oracle = require('nuclei/oracle'); -// const client = new oracle.OracleClient; +// const client = new oracle.OracleClient(); // client.Connect('acme.com', 1521, 'XE', 'user', 'password');Likely an incorrect or invalid review comment.
Comment on lines
+118
to
+119
| //let ssh server start | ||
| time.Sleep(3 * time.Second) |
Contributor
There was a problem hiding this comment.
Fix copy-paste comment error.
The comment mentions "ssh server" but this is the Oracle authentication test.
- //let ssh server start
+ //let oracle server start📝 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.
Suggested change
| //let ssh server start | |
| time.Sleep(3 * time.Second) | |
| //let oracle server start | |
| time.Sleep(3 * time.Second) |
🤖 Prompt for AI Agents
In cmd/integration-test/javascript.go around lines 118 to 119, the comment above
the time.Sleep call incorrectly refers to "ssh server"; update the comment to
accurately describe the delay purpose for the Oracle authentication test (e.g.,
"let Oracle auth service initialize" or "wait for Oracle authentication to be
ready") so the comment matches the code intent.
Comment on lines
+30
to
40
| // Client is a client for Oracle database. | ||
| // Internally client uses oracle/godror driver. | ||
| // @example | ||
| // ```javascript | ||
| // const oracle = require('nuclei/oracle'); | ||
| // const client = new oracle.OracleClient(); | ||
| // ``` | ||
| OracleClient struct { | ||
| connector *goora.OracleConnector | ||
| } | ||
| ) |
Contributor
There was a problem hiding this comment.
Fix JSDoc comment typo.
The comment incorrectly states "Internally client uses oracle/godror driver" but the code actually uses go-ora/v2.
// Client is a client for Oracle database.
- // Internally client uses oracle/godror driver.
+ // Internally client uses go-ora/v2 driver.
// @example📝 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.
Suggested change
| // Client is a client for Oracle database. | |
| // Internally client uses oracle/godror driver. | |
| // @example | |
| // ```javascript | |
| // const oracle = require('nuclei/oracle'); | |
| // const client = new oracle.OracleClient(); | |
| // ``` | |
| OracleClient struct { | |
| connector *goora.OracleConnector | |
| } | |
| ) | |
| // Client is a client for Oracle database. | |
| // Internally client uses go-ora/v2 driver. | |
| // @example | |
| // |
🤖 Prompt for AI Agents
In pkg/js/libs/oracle/oracle.go around lines 30 to 40, the JSDoc comment
incorrectly states the driver as "oracle/godror"; update the comment to
accurately reference go-ora/v2 (e.g., "Internally client uses go-ora/v2") so the
documentation matches the actual implementation and remove or replace any
misleading driver names.
Comment on lines
+49
to
52
| func (c *OracleClient) IsOracle(ctx context.Context, host string, port int) (IsOracleResponse, error) { | ||
| executionId := ctx.Value("executionId").(string) | ||
| return memoizedisOracle(executionId, host, port) | ||
| } |
Contributor
There was a problem hiding this comment.
Add nil check for executionId.
The method assumes executionId exists in the context and will panic if it's missing. Add a defensive check.
func (c *OracleClient) IsOracle(ctx context.Context, host string, port int) (IsOracleResponse, error) {
- executionId := ctx.Value("executionId").(string)
+ executionId, ok := ctx.Value("executionId").(string)
+ if !ok || executionId == "" {
+ return IsOracleResponse{}, fmt.Errorf("executionId not found in context")
+ }
return memoizedisOracle(executionId, host, port)
}📝 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.
Suggested change
| func (c *OracleClient) IsOracle(ctx context.Context, host string, port int) (IsOracleResponse, error) { | |
| executionId := ctx.Value("executionId").(string) | |
| return memoizedisOracle(executionId, host, port) | |
| } | |
| func (c *OracleClient) IsOracle(ctx context.Context, host string, port int) (IsOracleResponse, error) { | |
| executionId, ok := ctx.Value("executionId").(string) | |
| if !ok || executionId == "" { | |
| return IsOracleResponse{}, fmt.Errorf("executionId not found in context") | |
| } | |
| return memoizedisOracle(executionId, host, port) | |
| } |
🤖 Prompt for AI Agents
In pkg/js/libs/oracle/oracle.go around lines 49-52, the code unsafely assumes
ctx.Value("executionId") exists and panics if missing; add a nil/type check
before asserting: retrieve v := ctx.Value("executionId"), assert id, ok :=
v.(string) and if !ok || id == "" return an appropriate error (e.g., fmt.Errorf
or errors.New) and avoid calling memoizedisOracle; otherwise call
memoizedisOracle(id, host, port). Also add the necessary import for fmt/errors
if not present.
Comment on lines
+122
to
+145
| func (c *OracleClient) ConnectWithDSN(ctx context.Context, dsn string) (bool, error) { | ||
| executionId := ctx.Value("executionId").(string) | ||
|
|
||
| connector, err := c.oracleDbInstance(dsn, executionId) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| db := sql.OpenDB(connector) | ||
| defer func() { | ||
| _ = db.Close() | ||
| }() | ||
|
|
||
| db.SetMaxOpenConns(1) | ||
| db.SetMaxIdleConns(0) | ||
|
|
||
| // Test the connection | ||
| err = db.Ping() | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| return true, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Add nil check for executionId in ConnectWithDSN.
Similar to the IsOracle method, this needs defensive handling for missing executionId.
func (c *OracleClient) ConnectWithDSN(ctx context.Context, dsn string) (bool, error) {
- executionId := ctx.Value("executionId").(string)
+ executionId, ok := ctx.Value("executionId").(string)
+ if !ok || executionId == "" {
+ return false, fmt.Errorf("executionId not found in context")
+ }🤖 Prompt for AI Agents
In pkg/js/libs/oracle/oracle.go around lines 122 to 145, the code
unconditionally type-asserts ctx.Value("executionId").(string) which can panic
if executionId is missing or not a string; add a defensive nil/type check
similar to IsOracle: first retrieve v := ctx.Value("executionId"), if v == nil
return false with a clear error, then do s, ok := v.(string) and if !ok return
false with a clear error; use those early returns instead of allowing a panic so
connector creation handles missing/invalid executionId gracefully.
Comment on lines
+147
to
+155
| // ExecuteQuery connects to MS SQL database using given credentials and executes a query. | ||
| // It returns the results of the query or an error if something goes wrong. | ||
| // @example | ||
| // ```javascript | ||
| // const oracle = require('nuclei/oracle'); | ||
| // const client = new oracle.OracleClient; | ||
| // const result = client.ExecuteQuery('acme.com', 1521, 'username', 'password', 'XE', 'SELECT @@version'); | ||
| // log(to_json(result)); | ||
| // ``` |
Contributor
There was a problem hiding this comment.
Fix incorrect comment and JSDoc examples.
The comment mentions "MS SQL database" but this is for Oracle. Also, the JSDoc example incorrectly uses SELECT @@version which is MySQL/SQL Server syntax, not Oracle.
-// ExecuteQuery connects to MS SQL database using given credentials and executes a query.
+// ExecuteQuery connects to Oracle database using given credentials and executes a query.
// It returns the results of the query or an error if something goes wrong.
// @example
// ```javascript
// const oracle = require('nuclei/oracle');
-// const client = new oracle.OracleClient;
-// const result = client.ExecuteQuery('acme.com', 1521, 'username', 'password', 'XE', 'SELECT @@version');
+// const client = new oracle.OracleClient();
+// const result = client.ExecuteQuery('acme.com', 1521, 'username', 'password', 'XE', 'SELECT * FROM v$version');
// log(to_json(result));
// ```🤖 Prompt for AI Agents
In pkg/js/libs/oracle/oracle.go around lines 147 to 155, the function comment
and JSDoc example are incorrect: they reference "MS SQL" and use a non-Oracle
query; update the comment to say "Oracle database" and correct the example to
use a valid Oracle query and proper JS constructor call (e.g., use SELECT * FROM
v$version and new oracle.OracleClient() ), keeping the rest of the example
intact.
Comment on lines
+174
to
+181
| // ExecuteQueryWithDSN executes a query on an Oracle database using a DSN | ||
| // @example | ||
| // ```javascript | ||
| // const oracle = require('nuclei/oracle'); | ||
| // const client = new oracle.OracleClient; | ||
| // const result = client.ExecuteQueryWithDSN('oracle://user:password@host:port/service', 'SELECT @@version'); | ||
| // log(to_json(result)); | ||
| // ``` |
Contributor
There was a problem hiding this comment.
Fix JSDoc syntax errors.
Missing parentheses in constructor call and incorrect SQL syntax for Oracle.
// @example
// ```javascript
// const oracle = require('nuclei/oracle');
-// const client = new oracle.OracleClient;
-// const result = client.ExecuteQueryWithDSN('oracle://user:password@host:port/service', 'SELECT @@version');
+// const client = new oracle.OracleClient();
+// const result = client.ExecuteQueryWithDSN('oracle://user:password@host:port/service', 'SELECT * FROM v$version');
// log(to_json(result));
// ```🤖 Prompt for AI Agents
In pkg/js/libs/oracle/oracle.go around lines 174 to 181, the JSDoc example has
syntax errors: the constructor call is missing parentheses and the sample SQL is
not valid for Oracle; update the example to instantiate the client with new
oracle.OracleClient() and replace the query 'SELECT @@version' with a valid
Oracle query such as 'SELECT * FROM v$version' so the snippet compiles and
demonstrates a correct Oracle query.
Comment on lines
+182
to
+210
| func (c *OracleClient) ExecuteQueryWithDSN(ctx context.Context, dsn string, query string) (*utils.SQLResult, error) { | ||
| executionId := ctx.Value("executionId").(string) | ||
|
|
||
| connector, err := c.oracleDbInstance(dsn, executionId) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| db := sql.OpenDB(connector) | ||
| defer func() { | ||
| _ = db.Close() | ||
| }() | ||
|
|
||
| db.SetMaxOpenConns(1) | ||
| db.SetMaxIdleConns(0) | ||
|
|
||
| rows, err := db.Query(query) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| data, err := utils.UnmarshalSQLRows(rows) | ||
| if err != nil { | ||
| if data != nil && len(data.Rows) > 0 { | ||
| return data, nil | ||
| } | ||
| return nil, err | ||
| } | ||
| return data, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Add nil check for executionId in ExecuteQueryWithDSN.
Consistent with other methods, add defensive handling for missing executionId.
func (c *OracleClient) ExecuteQueryWithDSN(ctx context.Context, dsn string, query string) (*utils.SQLResult, error) {
- executionId := ctx.Value("executionId").(string)
+ executionId, ok := ctx.Value("executionId").(string)
+ if !ok || executionId == "" {
+ return nil, fmt.Errorf("executionId not found in context")
+ }📝 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.
Suggested change
| func (c *OracleClient) ExecuteQueryWithDSN(ctx context.Context, dsn string, query string) (*utils.SQLResult, error) { | |
| executionId := ctx.Value("executionId").(string) | |
| connector, err := c.oracleDbInstance(dsn, executionId) | |
| if err != nil { | |
| return nil, err | |
| } | |
| db := sql.OpenDB(connector) | |
| defer func() { | |
| _ = db.Close() | |
| }() | |
| db.SetMaxOpenConns(1) | |
| db.SetMaxIdleConns(0) | |
| rows, err := db.Query(query) | |
| if err != nil { | |
| return nil, err | |
| } | |
| data, err := utils.UnmarshalSQLRows(rows) | |
| if err != nil { | |
| if data != nil && len(data.Rows) > 0 { | |
| return data, nil | |
| } | |
| return nil, err | |
| } | |
| return data, nil | |
| } | |
| func (c *OracleClient) ExecuteQueryWithDSN(ctx context.Context, dsn string, query string) (*utils.SQLResult, error) { | |
| executionId, ok := ctx.Value("executionId").(string) | |
| if !ok || executionId == "" { | |
| return nil, fmt.Errorf("executionId not found in context") | |
| } | |
| connector, err := c.oracleDbInstance(dsn, executionId) | |
| if err != nil { | |
| return nil, err | |
| } | |
| db := sql.OpenDB(connector) | |
| defer func() { | |
| _ = db.Close() | |
| }() | |
| db.SetMaxOpenConns(1) | |
| db.SetMaxIdleConns(0) | |
| rows, err := db.Query(query) | |
| if err != nil { | |
| return nil, err | |
| } | |
| data, err := utils.UnmarshalSQLRows(rows) | |
| if err != nil { | |
| if data != nil && len(data.Rows) > 0 { | |
| return data, nil | |
| } | |
| return nil, err | |
| } | |
| return data, nil | |
| } |
🤖 Prompt for AI Agents
In pkg/js/libs/oracle/oracle.go around lines 182 to 210, the code directly
type-asserts ctx.Value("executionId").(string) which can panic if the key is
missing; add a nil/type-check by retrieving the value with v, ok :=
ctx.Value("executionId") and then s, ok2 := v.(string) (or combine into s, ok :=
ctx.Value("executionId").(string)); if not present/valid return a clear error
(e.g., fmt.Errorf or errors.New) instead of panicking, and only call
c.oracleDbInstance(dsn, executionId) when executionId is validated. Ensure the
function returns the error early and does not proceed to open the DB when
executionId is missing.
Member
Author
|
Merging - Already reviewed in #6427 |
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.
Proposed changes
Closes #4872
Checklist
Summary by CodeRabbit
New Features
Tests
Chores