Repository navigation
Added captcha validation service to the engine - #2235
Conversation
WalkthroughThe change centralizes outbound HTTP client configuration, wires Captcha and identity providers to the shared client, updates consent metadata parsing and essential-attribute enforcement, and adds JSON field compare-and-swap support to in-memory and Redis runtime stores. ChangesOutbound HTTP Client and Provider Wiring
Consent Metadata and Enforcement
Runtime Compare-and-Swap
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-go #2235 +/- ##
=============================================
Coverage ? 81.88%
=============================================
Files ? 59
Lines ? 4410
Branches ? 0
=============================================
Hits ? 3611
Misses ? 606
Partials ? 193
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@esignet-service/cmd/esignet/main_test.go`:
- Around line 54-59: Extend the “scope enforcement enabled wraps request time
middleware” test around getSecurityMiddleware to execute the returned middleware
with a request, rather than only asserting it is non-nil. Verify the enabled
branch produces the expected authorization rejection, or configure a test JWKS
server and valid token to assert the successful authorized path.
In `@esignet-service/cmd/esignet/main.go`:
- Around line 181-191: Update the HTTP transport created in the
client-construction function around the existing DialContext and timeout
settings to enforce a finite outbound connection limit, using MaxConnsPerHost or
equivalent admission control. Preserve the current timeout behavior, and add or
update coverage for the degraded Captcha-provider path to verify outbound
concurrency is bounded.
In `@esignet-service/internal/engine/runtimestores/inmemory/service_test.go`:
- Around line 241-248: Update the CompareFieldAndSwap test around inMemoryStore
to capture the key’s original expiresAt before the swap, then assert the
post-swap timestamp exactly matches that captured value rather than only
checking that it is non-zero. Preserve the existing read-locking pattern and
failure context.
In `@esignet-service/internal/engine/runtimestores/inmemory/service.go`:
- Around line 162-169: Update CompareFieldAndSwap in
esignet-service/internal/engine/runtimestores/inmemory/service.go and its Redis
counterpart in
esignet-service/internal/engine/runtimestores/redisstore/service.go to require a
top-level JSON object and an explicitly string-valued field before comparing;
reject null fields and scalar roots without swapping. Add matching null-field
and scalar-root parity tests in
esignet-service/internal/engine/runtimestores/inmemory/service_test.go:200-288
and
esignet-service/internal/engine/runtimestores/redisstore/service_test.go:349-400.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1a8d83b3-8a06-4ea4-aa0c-d16ad3dc4313
⛔ Files ignored due to path filters (1)
esignet-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
esignet-service/cmd/esignet/main.goesignet-service/cmd/esignet/main_test.goesignet-service/go.modesignet-service/internal/engine/runtimestores/inmemory/service.goesignet-service/internal/engine/runtimestores/inmemory/service_test.goesignet-service/internal/engine/runtimestores/redisstore/service.goesignet-service/internal/engine/runtimestores/redisstore/service_test.go
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@esignet-service/cmd/esignet/main.go`:
- Around line 186-198: The newHTTPClient function must preserve
environment-based proxy support while applying its custom transport settings.
Initialize the transport from http.DefaultTransport.Clone() or set Proxy to
http.ProxyFromEnvironment, then override only DialContext and the configured
timeout, connection, and limit fields.
In `@esignet-service/internal/config/app.go`:
- Around line 183-184: Restore the fallback assignments for
cfg.Flow.DefaultAuthFlowHandle and cfg.Flow.UserOnboardingFlowHandle in the
configuration initialization, using the existing "default" and "user-onboarding"
values. Keep YAML-provided handles authoritative while ensuring deployments
without those settings receive non-empty defaults.
In `@esignet-service/internal/engine/consent_provider.go`:
- Around line 130-135: Update the essential-claims collection around
requestedClaims so essentiality is evaluated per claim section rather than by
flattened claim name, preserving essential userinfo.email when id_token.email is
non-essential. Ensure the consent flow returns essential_consent_denied before
persistence for this combination, and add a regression case covering essential
userinfo.email with non-essential id_token.email.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5819bf2b-3e4a-44c2-a7ec-5f2899c12323
⛔ Files ignored due to path filters (1)
esignet-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
esignet-service/cmd/esignet/main.goesignet-service/cmd/esignet/main_test.goesignet-service/go.modesignet-service/internal/config/app.goesignet-service/internal/config/app_test.goesignet-service/internal/engine/consent_provider.goesignet-service/internal/engine/consent_provider_test.goesignet-service/internal/engine/idsystem_factory.goesignet-service/internal/engine/idsystem_factory_test.goesignet-service/internal/engine/mock/authenticator.goesignet-service/internal/engine/mock/authenticator_test.goesignet-service/internal/engine/mock/init.goesignet-service/internal/engine/mock/init_test.goesignet-service/internal/engine/mosip/init.goesignet-service/internal/engine/mosip/init_test.goesignet-service/internal/engine/runtimestores/inmemory/service.goesignet-service/internal/engine/runtimestores/inmemory/service_test.goesignet-service/internal/engine/runtimestores/redisstore/service.goesignet-service/internal/engine/runtimestores/redisstore/service_test.goesignet-service/internal/engine/sunbird/authenticator.goesignet-service/internal/engine/sunbird/authenticator_test.goesignet-service/internal/engine/sunbird/config.goesignet-service/internal/engine/sunbird/config_test.goesignet-service/internal/engine/sunbird/init.go
💤 Files with no reviewable changes (1)
- esignet-service/internal/engine/sunbird/config.go
| func newHTTPClient(cfg config.HTTPClientConfig) *http.Client { | ||
| return &http.Client{ | ||
| Timeout: time.Duration(cfg.TimeoutSecs) * time.Second, | ||
| Transport: &http.Transport{ | ||
| DialContext: (&net.Dialer{ | ||
| Timeout: time.Duration(cfg.DialTimeoutSecs) * time.Second, | ||
| KeepAlive: time.Duration(cfg.DialKeepAliveSecs) * time.Second, | ||
| }).DialContext, | ||
| TLSHandshakeTimeout: time.Duration(cfg.TLSHandshakeTimeoutSecs) * time.Second, | ||
| ResponseHeaderTimeout: time.Duration(cfg.ResponseHeaderTimeoutSecs) * time.Second, | ||
| IdleConnTimeout: time.Duration(cfg.IdleConnTimeoutSecs) * time.Second, | ||
| MaxConnsPerHost: cfg.MaxConnsPerHost, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -i -C 2 'ProxyFromEnvironment|HTTP_PROXY|HTTPS_PROXY|NO_PROXY' esignet-serviceRepository: mosip/esignet
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files 'esignet-service/cmd/esignet/*.go' | sed -n '1,120p'
echo
echo "== main.go relevant section =="
sed -n '150,220p' esignet-service/cmd/esignet/main.go
echo
echo "== http client config references =="
rg -n -C 3 'HTTPClientConfig|newHTTPClient|HttpClient|HttpClientConfig' esignet-serviceRepository: mosip/esignet
Length of output: 7396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, subprocess, tempfile
env = os.environ.copy()
env["NO_PROXY"] = ""
env["HTTP_PROXY"] = "http://net-proxy.example:8080"
env["HTTPS_PROXY"] = "http://net-proxy.example:8080"
prog = r"""
package main
import (
"context"
"fmt"
"net/http"
"net/url"
"time"
)
func newHTTPClient() *http.Client {
return &http.Client{
Timeout: time.Second,
Transport: &http.Transport{
DialContext: (&net.Dialer{
Timeout: time.Second,
KeepAlive: time.Second,
}).DialContext,
},
}
}
func main() {
req, _:= http.NewRequestWithContext(context.Background(), http.MethodGet, "https://example.invalid/", nil)
p, err := newHTTPClient().Transport.(*http.Transport).Proxy(req)
fmt.Println("proxy")
if p != nil {
fmt.Println("present=", p.String(), "matches=", p.String() == "http://net-proxy.example:8080")
} else {
fmt.Println("present=false")
}
fmt.Println("err=", err)
}
"""
fd, path = tempfile.mkstemp(suffix=".go")
try:
os.write(fd, prog.encode())
out = subprocess.run(["go", "run", path], env=env, text=True, capture_output=True, check=True)
print(out.stdout)
finally:
os.close(fd)
os.remove(path)
PYRepository: mosip/esignet
Length of output: 450
🌐 Web query:
Go net/http Transport Proxy field nil disables proxy default transport ProxyFromEnvironment
💡 Result:
Yes, setting the Proxy field to nil in a net/http.Transport disables all proxy usage for that transport [1][2][3]. The Transport struct's Proxy field is a function of type func(*Request) (*url.URL, error) [1][2]. According to the official Go documentation, if this field is nil (or if the function it points to returns a nil *URL), the Transport will not use a proxy for any requests [1][2]. By default, the net/http package's DefaultTransport uses ProxyFromEnvironment, which is a helper function that reads proxy settings from environment variables like HTTP_PROXY, HTTPS_PROXY, and NO_PROXY [4][3][5]. If you explicitly initialize a Transport with a nil Proxy field, you override this default behavior and effectively disable proxy discovery and usage entirely for that Transport instance [1][2].
Citations:
- 1: https://go.googlesource.com/go/+/master/src/net/http/transport.go
- 2: https://github.com/golang/go/blob/go1.20/src/net/http/transport.go
- 3: https://github.com/golang/go/blob/master/src/net/http/transport.go
- 4: https://pkg.go.dev/net/http
- 5: https://go.dev/src/net/http/transport.go
Preserve ProxyFromEnvironment in the outbound HTTP client.
newHTTPClient assigns a zero-value *http.Transport, so its Proxy field is nil and proxy env vars (HTTP_PROXY/HTTPS_PROXY/NO_PROXY) are ignored. Use http.DefaultTransport.(*http.Transport).Clone() or set Proxy: http.ProxyFromEnvironment before overriding only DialContext and the timeout/connection settings. This is required for GDPR/MOSIP-compliant deployments that must route all egress traffic through controlled proxies.
🤖 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 `@esignet-service/cmd/esignet/main.go` around lines 186 - 198, The
newHTTPClient function must preserve environment-based proxy support while
applying its custom transport settings. Initialize the transport from
http.DefaultTransport.Clone() or set Proxy to http.ProxyFromEnvironment, then
override only DialContext and the configured timeout, connection, and limit
fields.
| //cfg.Flow.DefaultAuthFlowHandle = "default" | ||
| //cfg.Flow.UserOnboardingFlowHandle = "user-onboarding" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'DefaultAuthFlowHandle|UserOnboardingFlowHandle' esignet-service
rg -n --glob '*.yaml' --glob '*.yml' 'default_auth_flow_handle|user_onboarding_flow_handle' esignet-serviceRepository: mosip/esignet
Length of output: 796
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== app config around Flow ==="
sed -n '150,200p' esignet-service/internal/config/app.go
echo
echo "=== Flow type definition ==="
rg -n -C 6 'type .*Flow|DefaultAuthFlowHandle|UserOnboardingFlowHandle|MaxVersionHistory' esignet-service/internal esignet-service | head -200
echo
echo "=== YAML config files containing flow-related keys ==="
rg -n -i --glob '*.yaml' --glob '*.yml' 'flow|auth_flow|onboarding|default_auth|user_onboarding' esignet-service || true
echo
echo "=== all occurrences ==="
rg -n 'DefaultAuthFlowHandle|UserOnboardingFlowHandle|default_auth_flow_handle|user_onboarding_flow_handle|Flow\.Default|Flow\.User' . || trueRepository: mosip/esignet
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== app.go around flow defaults ==="
sed -n '160,190p' esignet-service/internal/config/app.go
echo
echo "=== focused occurrences in tracked files ==="
git ls-files | xargs rg -n --no-heading 'DefaultAuthFlowHandle|UserOnboardingFlowHandle|default_auth_flow_handle|user_onboarding_flow_handle' || true
echo
echo "=== focused non-i18n occurrences ==="
git ls-files 'esignet-service/internal/**' 'esignet-service/*[!.yaml]!' 'esignet-service/**/![i18n]/**' | xargs rg -n --no-heading 'DefaultAuthFlowHandle|UserOnboardingFlowHandle|default_auth_flow_handle|user_onboarding_flow_handle' || trueRepository: mosip/esignet
Length of output: 5590
Restore flow-handle fallback defaults.
esignet-service/internal/config/app.go:183-184 is the only place these handles are assigned, so deployments without YAML default_auth_flow_handle / user_onboarding_flow_handle config will receive empty handles. This breaks flow resolution and can fail MOSIP/OpenID eSignet authentication/onboarding flows.
Proposed fix if this removal was unintended
- // cfg.Flow.DefaultAuthFlowHandle = "default"
- // cfg.Flow.UserOnboardingFlowHandle = "user-onboarding"
+ cfg.Flow.DefaultAuthFlowHandle = "default"
+ cfg.Flow.UserOnboardingFlowHandle = "user-onboarding"📝 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.
| //cfg.Flow.DefaultAuthFlowHandle = "default" | |
| //cfg.Flow.UserOnboardingFlowHandle = "user-onboarding" | |
| cfg.Flow.DefaultAuthFlowHandle = "default" | |
| cfg.Flow.UserOnboardingFlowHandle = "user-onboarding" |
🤖 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 `@esignet-service/internal/config/app.go` around lines 183 - 184, Restore the
fallback assignments for cfg.Flow.DefaultAuthFlowHandle and
cfg.Flow.UserOnboardingFlowHandle in the configuration initialization, using the
existing "default" and "user-onboarding" values. Keep YAML-provided handles
authoritative while ensuring deployments without those settings receive
non-empty defaults.
| essentialClaims := map[string]bool{} | ||
| for name, v := range requestedClaims(req) { | ||
| if isEssentialClaim(v) { | ||
| essentialClaims[name] = true | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Preserve essentiality across userinfo and id_token.
Line 131 uses requestedClaims, which flattens sections by claim name. A non-essential id_token.email overwrites an essential userinfo.email, allowing denied email consent to persist instead of returning essential_consent_denied.
Proposed fix
for name, v := range section {
if name == "verified_claims" {
continue
}
- out[name] = v
+ existing, exists := out[name]
+ if !exists || isEssentialClaim(v) || !isEssentialClaim(existing) {
+ out[name] = v
+ }
}Add a regression case with essential userinfo.email and non-essential id_token.email. Based on the PR objective that denied essential attributes must fail before persistence.
🤖 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 `@esignet-service/internal/engine/consent_provider.go` around lines 130 - 135,
Update the essential-claims collection around requestedClaims so essentiality is
evaluated per claim section rather than by flattened claim name, preserving
essential userinfo.email when id_token.email is non-essential. Ensure the
consent flow returns essential_consent_denied before persistence for this
combination, and add a regression case covering essential userinfo.email with
non-essential id_token.email.
Summary by CodeRabbit
New Features
Bug Fixes
Tests