extend js smb for #4707 - #7565
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughChangesSMB and DCERPC expansion
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
pkg/js/libs/dcerpc/transport_init.go (1)
16-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant "ensure init runs" reference.
Go executes every imported package's
init()regardless of symbol usage, andgptransportis already imported and used by the wrapper above (line 13). Thisvar _ = gptransport.NewExecDialerline adds nothing.♻️ Proposed cleanup
- -// Ensure gptransport init() runs whenever dcerpc is imported. -var _ = gptransport.NewExecDialer🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/js/libs/dcerpc/transport_init.go` around lines 16 - 17, Remove the redundant blank assignment referencing gptransport.NewExecDialer in the dcerpc transport initialization code. Keep the existing gptransport import and wrapper usage unchanged so package initialization continues through the normal import mechanism.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/js/libs/dcerpc/enum.go`:
- Line 32: Add TSTS and tsts to the repository’s typos allow-list configuration,
preserving the legitimate technical abbreviation and the gptsts package
references in the surrounding code and comments.
In `@pkg/js/libs/smb/memo.smb.go`:
- Around line 45-90: Update memoizedlistDir, memoizedreadFile, and
memoizedlistTree so their memoization keys no longer use colon-concatenated raw
inputs or expose passwords. Encode the complete structured input fields
unambiguously, hash the encoded payload, and use the resulting digest (with an
operation discriminator) as the key passed to protocolstate.Memoizer.Do,
preserving distinct keys for each operation and input combination.
In `@pkg/js/libs/smb/smb_share.go`:
- Around line 12-59: The `@memo` annotations on listShares, listDir, readFile, and
listTree cause password values to enter generated memoization keys. Remove
memoization from these credential-bearing functions, or configure the generated
wrappers to exclude user/password while preserving correct cache isolation for
authenticated calls; do not allow plaintext passwords in memoizer keys.
In `@pkg/js/libs/smbsession/session.go`:
- Around line 59-87: The Dial function currently ignores its ctx argument and
can block indefinitely in client.Connect. Add a context-aware cancellation and
timeout wrapper around gpsmb.Client.Connect, ensuring Dial returns promptly when
ctx is canceled or its deadline expires while preserving the existing connection
and error behavior otherwise.
- Around line 185-210: The readFile function currently calls shareBackend.Cat,
buffering the entire remote file before enforcing maxBytes. Extend shareBackend
with Open and Read support, then update readFile to open the normalized path and
stream it through io.LimitReader with a maxBytes+1 limit, preserving the
existing oversized-file error and returning only content within the limit.
In `@pkg/protocols/file/find.go`:
- Around line 23-36: Update getInputPaths to accept a context parameter and use
it for enumerateSMBInputs instead of context.Background(). In
ExecuteWithResults, pass input.Context() into getInputPaths, and remove the
no-op request.options branch while preserving the existing SMB path handling and
callback deduplication.
In `@pkg/protocols/file/smb_bridge.go`:
- Around line 65-135: Eliminate redundant SMB authentication by reusing a single
smbsession.Session across enumeration and file reads for one scan, keyed by
execution and connection credentials (execID, host, port, and resolved creds).
Update enumerateSMBInputs and readSMBFile, plus their request-processing
callers, to obtain and reuse the shared session for both directory entries and
single-file targets, closing it only after the scan completes; avoid dialing
independently per expanded file.
- Around line 116-135: Update readSMBFile after obtaining execID from
request.executionID() to return the same “smb file target requires an
initialized execution id” error when it is empty, before calling
smbsession.Dial. Keep the existing session dialing and file-reading flow
unchanged for initialized execution IDs.
- Around line 65-114: Update enumerateSMBInputs to use
isDirectorySMBTarget(input), or an equivalent trailing-slash directory check,
instead of relying on target.Path != ".". Ensure SMB targets with subdirectory
paths ending in a slash are enumerated through the existing directory-listing
flow rather than returned as files.
---
Nitpick comments:
In `@pkg/js/libs/dcerpc/transport_init.go`:
- Around line 16-17: Remove the redundant blank assignment referencing
gptransport.NewExecDialer in the dcerpc transport initialization code. Keep the
existing gptransport import and wrapper usage unchanged so package
initialization continues through the normal import mechanism.
🪄 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: 53867c2f-d217-4983-bd07-62cbe46334ea
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumpkg/js/generated/ts/dcerpc.tsis excluded by!**/generated/**pkg/js/generated/ts/smb.tsis excluded by!**/generated/**
📒 Files selected for processing (26)
go.modpkg/js/libs/dcerpc/dcerpc.gopkg/js/libs/dcerpc/enum.gopkg/js/libs/dcerpc/enum_policy_test.gopkg/js/libs/dcerpc/enum_test.gopkg/js/libs/dcerpc/transport_init.gopkg/js/libs/dcerpc/wmiexec.gopkg/js/libs/gptransport/dialer.gopkg/js/libs/gptransport/dialer_test.gopkg/js/libs/smb/memo.smb.gopkg/js/libs/smb/smb.gopkg/js/libs/smb/smb_path.gopkg/js/libs/smb/smb_policy_test.gopkg/js/libs/smb/smb_share.gopkg/js/libs/smb/smb_share_test.gopkg/js/libs/smbsession/path.gopkg/js/libs/smbsession/path_test.gopkg/js/libs/smbsession/session.gopkg/js/libs/smbsession/session_test.gopkg/protocols/file/file.gopkg/protocols/file/find.gopkg/protocols/file/request.gopkg/protocols/file/smb_bridge.gopkg/protocols/file/smb_bridge_test.gopkg/protocols/file/smb_path.gopkg/protocols/file/smb_path_test.go
💤 Files with no reviewable changes (1)
- go.mod
Open
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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
3 participants
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.
Closes #4707
Related #6142
Grow nuclei SMB support on the JS stack with a shared goimpacket session, RPC enums, and an optional file-protocol UNC/smb:// bridge.
Summary by CodeRabbit