fix: Accurate session count to avoid constant upward drift#1348
Open
jentfoo wants to merge 13 commits intoprojectdiscovery:devfrom
Open
fix: Accurate session count to avoid constant upward drift#1348jentfoo wants to merge 13 commits intoprojectdiscovery:devfrom
jentfoo wants to merge 13 commits intoprojectdiscovery:devfrom
Conversation
…ion-strategy feat(server) added eviction strategy
…-1275-feature/eviction-strategy Revert "feat(server) added eviction strategy"
interactsh v1.3.0
Bumps [github.com/refraction-networking/utls](https://github.com/refraction-networking/utls) from 1.8.0 to 1.8.2. - [Release notes](https://github.com/refraction-networking/utls/releases) - [Commits](refraction-networking/utls@v1.8.0...v1.8.2) --- updated-dependencies: - dependency-name: github.com/refraction-networking/utls dependency-version: 1.8.2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
…abot/go_modules/github.com/refraction-networking/utls-1.8.2 chore(deps): bump github.com/refraction-networking/utls from 1.8.0 to 1.8.2
release interactsh v1.3.1
The register handler incremented the session counter before `SetIDPublicKey` could fail, leaking +1 on duplicate IDs or bad keys. Now only incremented after successful registration. The deregister handler decremented the counter before validating the request, leaking -1 on malformed or unauthorized requests. Removed the explicit decrement entirely (handled in logic below). Cache eviction and TTL expiry silently removed sessions without decrementing the counter, causing monotonic growth. Unified all session decrements into a single cache removal callback that fires on deregistration, eviction, and cache close, filtering to only count entries with a SecretKey (true client sessions).
Neo - PR Security ReviewNo security issues found Highlights
Hardening Notes
Comment |
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAtomic session metrics were added and tracked via a storage-layer OnRemoval hook; registration now increments Sessions and SessionsTotal after successful key setup; storage removal callback was hardened, restricted to session entries, and always attached to the cache. Changes
sequenceDiagram
participant Client
participant HTTP as HTTP Server
participant Storage
participant Disk as Disk DB
participant Metrics
Client->>HTTP: POST /register (pubkey)
HTTP->>Storage: SetIDPublicKey(...)
alt SetID success
HTTP->>Metrics: atomic.AddInt64(&Stats.Sessions, +1)
HTTP->>Metrics: atomic.AddInt64(&Stats.SessionsTotal, +1)
HTTP-->>Client: 200 OK (id)
else failure
HTTP-->>Client: error
end
Note right of Storage: Later removal (deregister / TTL / eviction)
Storage->>Storage: onCacheRemoval(key,value)
alt value contains SecretKey (session)
Storage->>Disk: delete(key) if UseDisk() && db != nil
Storage->>Metrics: invoke Options.OnRemoval -> atomic.AddInt64(&Stats.Sessions, -1)
else non-session entry
Storage-->>Storage: no OnRemoval invocation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/storage/roundtrip_test.go (1)
493-507: Make goroutine shutdown fail-safe on test timeout.If
waitRemovalhitst.Fatalf, Line 507 is not reached and the ticker goroutine can leak. Defer channel close immediately after creation.Proposed test-hardening diff
stop := make(chan struct{}) + defer close(stop) go func() { ticker := time.NewTicker(10 * time.Millisecond) defer ticker.Stop() for { select { case <-stop: return case <-ticker.C: db.cache.GetIfPresent(cid2) } } }() waitRemoval("TTL eviction") - close(stop)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/storage/roundtrip_test.go` around lines 493 - 507, The goroutine started to poll db.cache.GetIfPresent(cid2) can leak if waitRemoval triggers t.Fatalf because close(stop) is after waitRemoval; change the test to close the stop channel via defer immediately after creating stop (the stop chan in the test and the goroutine that reads from it) so the goroutine will always be signaled to exit even on test timeout or failure—i.e., after stop := make(chan struct{}) add defer close(stop) and remove the later explicit close(stop) at the end.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/storage/roundtrip_test.go`:
- Around line 493-507: The goroutine started to poll db.cache.GetIfPresent(cid2)
can leak if waitRemoval triggers t.Fatalf because close(stop) is after
waitRemoval; change the test to close the stop channel via defer immediately
after creating stop (the stop chan in the test and the goroutine that reads from
it) so the goroutine will always be signaled to exit even on test timeout or
failure—i.e., after stop := make(chan struct{}) add defer close(stop) and remove
the later explicit close(stop) at the end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6095a8a6-a4d1-4a4d-ae47-66dbb8353b21
📒 Files selected for processing (5)
cmd/interactsh-server/main.gopkg/server/http_server.gopkg/storage/option.gopkg/storage/roundtrip_test.gopkg/storage/storagedb.go
With the prior change fixing the `session` count to be the current count, it can be harder to see short session activity. This change adds a `session_total` that constantly increases so that even short session activity is captured.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/server/http_server_test.go (1)
139-140: Misleading comment:OnRemovalfires synchronously, not asynchronously.Per
pkg/storage/storagedb.go:78-93, theonCacheRemovalcallback (which invokesOnRemoval) is triggered synchronously by the cache removal listener, not via a background goroutine. The sleep is harmless but unnecessary, and the comment could mislead future maintainers.💡 Optional fix
- // Small delay for async eviction callback. - time.Sleep(50 * time.Millisecond) -Or if you prefer to keep a small buffer for any internal cache bookkeeping:
- // Small delay for async eviction callback. + // Brief yield to allow any internal cache housekeeping to complete. time.Sleep(50 * time.Millisecond)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/http_server_test.go` around lines 139 - 140, The test currently uses time.Sleep(50 * time.Millisecond) with a comment claiming OnRemoval fires asynchronously; instead, update the test to remove the sleep and change the comment to state that onCacheRemoval (which invokes OnRemoval) is triggered synchronously by the cache removal listener, or if you deliberately want a safety buffer keep the sleep but change the comment to say it's an optional small buffer for internal bookkeeping; reference OnRemoval and onCacheRemoval when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/server/http_server_test.go`:
- Around line 139-140: The test currently uses time.Sleep(50 * time.Millisecond)
with a comment claiming OnRemoval fires asynchronously; instead, update the test
to remove the sleep and change the comment to state that onCacheRemoval (which
invokes OnRemoval) is triggered synchronously by the cache removal listener, or
if you deliberately want a safety buffer keep the sleep but change the comment
to say it's an optional small buffer for internal bookkeeping; reference
OnRemoval and onCacheRemoval when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20b31e4d-96d3-4aae-9277-5ea506244aaa
📒 Files selected for processing (3)
pkg/server/http_server.gopkg/server/http_server_test.gopkg/server/metrics.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/server/http_server_test.go (1)
118-122: Capture and assertjsoniter.Marshalerrors in test setup.Lines 118 and 132 discard errors from
jsoniter.Marshalcalls. If payload setup fails, test failures become harder to diagnose. Capture the error and assert it withrequire.NoError(t, err).Proposed fix
- regBody, _ := jsoniter.Marshal(&RegisterRequest{ + regBody, err := jsoniter.Marshal(&RegisterRequest{ PublicKey: pubB64, SecretKey: secretKey, CorrelationID: correlationID, }) + require.NoError(t, err) @@ - deregBody, _ := jsoniter.Marshal(&DeregisterRequest{ + deregBody, err := jsoniter.Marshal(&DeregisterRequest{ SecretKey: secretKey, CorrelationID: correlationID, }) + require.NoError(t, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/http_server_test.go` around lines 118 - 122, The test currently discards errors from jsoniter.Marshal when building RegisterRequest (regBody) — update the test to capture the returned error from jsoniter.Marshal for RegisterRequest (and the other marshal call at line 132) and assert success with require.NoError(t, err) before using regBody; locate the jsoniter.Marshal calls for RegisterRequest and the other payload, assign both results to (body, err), and call require.NoError(t, err) to fail fast and surface marshal failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/server/http_server_test.go`:
- Around line 109-112: The PEM block type used when encoding the PKIX-formatted
public key is incorrect: x509.MarshalPKIXPublicKey produces PKIX bytes but the
code creates a pem.Block with Type "RSA PUBLIC KEY" (PKCS#1). Update the PEM
block Type to "PUBLIC KEY" where pubPem is created (referencing
x509.MarshalPKIXPublicKey, pubBytes, pubPem) so the encoded PEM matches the PKIX
format.
- Around line 87-94: The test's OnRemoval closure closes the channel variable
removed directly, which will panic if invoked multiple times; wrap the close in
a sync.Once to ensure it's only closed once while keeping the
atomic.AddInt64(&stats.Sessions, -1) call on every invocation. Concretely,
create a sync.Once (e.g., once := &sync.Once{}) near where removed is declared
and change the OnRemoval callback in storage.New(&storage.Options{...}) to call
atomic.AddInt64(&stats.Sessions, -1) and then once.Do(func(){ close(removed) })
so the channel close is idempotent.
---
Nitpick comments:
In `@pkg/server/http_server_test.go`:
- Around line 118-122: The test currently discards errors from jsoniter.Marshal
when building RegisterRequest (regBody) — update the test to capture the
returned error from jsoniter.Marshal for RegisterRequest (and the other marshal
call at line 132) and assert success with require.NoError(t, err) before using
regBody; locate the jsoniter.Marshal calls for RegisterRequest and the other
payload, assign both results to (body, err), and call require.NoError(t, err) to
fail fast and surface marshal failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83bec887-fff2-42a5-aed9-5d30b2c37cde
📒 Files selected for processing (1)
pkg/server/http_server_test.go
Comment on lines
+109
to
+112
| pubBytes, err := x509.MarshalPKIXPublicKey(&priv.PublicKey) | ||
| require.NoError(t, err) | ||
| pubPem := pem.EncodeToMemory(&pem.Block{Type: "RSA PUBLIC KEY", Bytes: pubBytes}) | ||
| pubB64 := base64.StdEncoding.EncodeToString(pubPem) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify what PEM types are accepted by register path key parsing.
rg -n -C3 'SetIDPublicKey|pem\.Decode|ParsePKIXPublicKey|ParsePKCS1PublicKey|block\.Type|PUBLIC KEY|RSA PUBLIC KEY'Repository: projectdiscovery/interactsh
Length of output: 9821
Use PKIX-consistent PEM block type on line 111.
x509.MarshalPKIXPublicKey produces PKIX-formatted bytes and should be encoded with PEM type "PUBLIC KEY", not "RSA PUBLIC KEY". The latter is the PKCS#1 type and is semantically incorrect for PKIX format, creating parser inconsistencies.
Proposed fix
- pubPem := pem.EncodeToMemory(&pem.Block{Type: "RSA PUBLIC KEY", Bytes: pubBytes})
+ pubPem := pem.EncodeToMemory(&pem.Block{Type: "PUBLIC KEY", Bytes: pubBytes})📝 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
| pubBytes, err := x509.MarshalPKIXPublicKey(&priv.PublicKey) | |
| require.NoError(t, err) | |
| pubPem := pem.EncodeToMemory(&pem.Block{Type: "RSA PUBLIC KEY", Bytes: pubBytes}) | |
| pubB64 := base64.StdEncoding.EncodeToString(pubPem) | |
| pubBytes, err := x509.MarshalPKIXPublicKey(&priv.PublicKey) | |
| require.NoError(t, err) | |
| pubPem := pem.EncodeToMemory(&pem.Block{Type: "PUBLIC KEY", Bytes: pubBytes}) | |
| pubB64 := base64.StdEncoding.EncodeToString(pubPem) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/server/http_server_test.go` around lines 109 - 112, The PEM block type
used when encoding the PKIX-formatted public key is incorrect:
x509.MarshalPKIXPublicKey produces PKIX bytes but the code creates a pem.Block
with Type "RSA PUBLIC KEY" (PKCS#1). Update the PEM block Type to "PUBLIC KEY"
where pubPem is created (referencing x509.MarshalPKIXPublicKey, pubBytes,
pubPem) so the encoded PEM matches the PKIX format.
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
4 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.
The register handler incremented the session counter before
SetIDPublicKeycould fail, leaking +1 on duplicate IDs or bad keys. Now only incremented after successful registration.The deregister handler decremented the counter before validating the request, leaking -1 on malformed or unauthorized requests. Removed the explicit decrement entirely (handled in logic below).
Cache eviction and TTL expiry silently removed sessions without decrementing the counter, causing monotonic growth. Unified all session decrements into a single cache removal callback that fires on deregistration, eviction, and cache close, filtering to only count entries with a SecretKey (true client sessions).
Summary by CodeRabbit
New Features
Bug Fixes
Tests