Skip to content

fixed PUT client endpoint behaviour for unknown fields - #2660

Merged
anushasunkada merged 1 commit into
mosip:develop-gofrom
Infosys:ES-2535
Oct 12, 2026
Merged

anushasunkada merged 1 commit into
mosip:develop-gofrom
Infosys:ES-2535

Conversation

@sacrana0

@sacrana0 sacrana0 commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #2535

Summary by CodeRabbit

  • Bug Fixes
    • Client update requests containing immutable fields such as clientId, relyingPartyId, or publicKey, or fields not supported for updates, now return an invalid_request error.
    • Invalid request bodies now return an invalid_request error with details about the decoding issue. Rejected requests do not update the client.
  • Documentation
    • Clarified that immutable and unsupported fields cause an invalid_request error in the client update documentation.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 7159e1ac-aa7a-46aa-b4e5-a470afd0c18f
📥 Commits

Reviewing files that changed from the base of the PR and between 65d39ea and d42e9ea.

📒 Files selected for processing (3)
  • esignet-service/README.md
  • esignet-service/internal/clientmgmt/handler.go
  • esignet-service/internal/clientmgmt/handler_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The client update handler now strictly decodes PUT request fields. It returns invalid_request for immutable or unknown fields and does not call the update service when decoding fails.

Changes

Client update validation

Layer / File(s) Summary
Decode and validate update requests
esignet-service/internal/clientmgmt/model.go, esignet-service/internal/clientmgmt/model_test.go
DecodeUpdateRequest decodes mutable fields and rejects unknown fields. Tests cover missing or null requests, invalid JSON, incorrect types, and rejected fields.
Apply strict decoding in the update handler
esignet-service/internal/clientmgmt/handler.go, esignet-service/internal/clientmgmt/handler_test.go, esignet-service/README.md
The handler passes the decoded request to UpdateClient and returns invalid_request on decode errors. Tests verify rejected fields do not reach the database update. The README documents the rejection behavior.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: anushasunkada

Merge Risk

Merge Risk: ⚪ Minimal · up to d42e9

The reviewed update path has no established issue that needs resolution before merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 65d39

The change rejects more invalid update requests without expanding who can update clients. The authorization settings and any client-specific access controls used in deployment still need confirmation.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected entrypoints cover the OIDC, OAuth, and generic client-update routes. An accepted update can change persisted client configuration and its cached representation; strict decoding reduces accepted body shapes without expanding those routes or the update target.

Trust Boundaries and Controls

  • observed — Scope checks depend on issuer and JWKS configuration; when disabled, the configured middleware performs request-time validation without JWT scope enforcement. The repository configuration leaves those URLs empty but identifies override settings. Live overrides, ingress controls, and client-specific authorization are not established by the available evidence.

Resilience and Maintainability Implications

  • observed — A database failure prevents cache invalidation, while an invalidation failure occurs after persistence and may leave stale configuration visible. This existing failure ordering is unchanged for requests the new decoder accepts.

Hardening Proposals

  • proposed — Confirm that deployed issuer and JWKS settings enable the intended scope checks, and document or enforce whether an authorized caller may update any client_id or only clients it owns. This addresses an unresolved existing boundary, not a demonstrated regression from the decoder change.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly describes the main change: rejecting unknown and immutable fields in the PUT client endpoint.
Linked Issues check Passed Issue #2535 requires PUT client updates to reject immutable and unknown fields and to preserve stored values. DecodeUpdateRequest uses DisallowUnknownFields, which rejects clientId, `relyingPart…
Out of Scope Changes check Passed The changes stay within issue #2535. The model adds strict PUT decoding, the handler uses the decoded request, tests verify rejection and no update, and the README documents the immutable-field behavi…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. (1 skipped: 1 …
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

A request arrives with fields in tow
The decoder checks what may pass
Unknown names receive an error
Valid updates reach their service
No rejected field changes stored state

Comment @coderabbitai help to get the list of available commands.

@codecov-commenter

codecov-commenter commented Sep 29, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (develop-go@b76c7da). Learn more about missing BASE report.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop-go    #2660   +/-   ##
=============================================
  Coverage              ?   74.96%           
=============================================
  Files                 ?      131           
  Lines                 ?     9267           
  Branches              ?      112           
=============================================
  Hits                  ?     6947           
  Misses                ?     1845           
  Partials              ?      475           
Flag Coverage Δ
go 74.08% <100.00%> (?)
npm 92.53% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread esignet-service/internal/clientmgmt/model.go
Comment thread esignet-service/internal/clientmgmt/handler.go Outdated
Signed-off-by: Sachin Rana <sacrana324@gmail.com>
@anushasunkada
anushasunkada merged commit bcb6a4f into mosip:develop-go Oct 12, 2026
31 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants