Repository navigation
fix: validate base messages the same way in sm and peer - #127
Merged
Merged
Conversation
Part 3 of the protocol contract decided in #108. The base capability, watchdog and disconnect messages were only partly checked, by parsers that differed between sm and peer, and the Manager ignored the optional validation settings. - CER, CEA, DWR, DWA, DPR and DPA are always checked against RFC 6733 §§3.2, 5.3.1-5.5.2 and 6.3: base header, required fields, types and multiplicity, Origin-Host and Origin-Realm syntax (§4.3.1), no Supported-Vendor-Id of zero (§5.3.6), and no Destination-Host or Destination-Realm in answers (§6.2). A malformed request gets one error answer with a single Failed-AVP (Verified Errata ID 4615); a malformed answer is discarded and reported. - A received answer may use the generic error grammar of §7.2 with a 3xxx or 5xxx Result-Code (§§7.1.3, 7.1.5), but not with 2xxx or 4xxx (§7.1.4). Answers this library sends still set the E bit only for 3xxx. - peer.Manager honours RejectUnknownMandatoryAVPs and ValidateRequests with the same pipeline and order as StateMachine (advertised application, then unknown mandatory AVPs per §4.1, then dictionary validation), and routes nothing after a rejection. Both stay off by default. Refs #108
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
gomaja
added a commit
that referenced
this pull request
Oct 15, 2026
Step 2 of the plan adopted in #99. sm.StateMachine, sm.Client and peer.Manager each decide the base protocol themselves; #108 unified their behaviour, and this adds the single implementation they will move onto in #110-#112. Nothing calls it yet. diam/internal/protocol is a deterministic state machine for one connection generation: New, Handle(Input) []Effect and an immutable Snapshot. It owns no sockets, goroutines, timers or callbacks and depends only on diam, diam/internal/base, diam/internal/watchdog and the dictionary types. - Phases PreCER, WaitCEA, Open, Closing, Closed and Retired, with the #108 rules: answer correlation and peer binding, base validation in the #127 order, the Closing admission transaction and publication boundary of #125, DPA handling and the in-flight 5012 (RFC 6733 §§3, 5.3-5.6, 7). - A validated CER becomes a candidate that the caller activates, so the Manager can hold the CEA during election (§5.6.4) while StateMachine activates at once. - Inputs carry received messages with their receipt time, local intents, timer expiries, allocation and write outcomes (skipped publication is reported apart from failure) and retirement. Effects ask the caller to send, admit, discard, notify, arm or stop timers, freeze admission and close. - Identifiers come from an allocator the caller supplies, so the Manager keeps one reservation space; reservations outlive answers until every writer returns. - The watchdog runs through diam/internal/watchdog with receipt-time decisions (RFC 3539 §3.4.1, Appendix A). The first validated DPR cause and a rejected CEA's parsed answer and error are kept for the front ends. Tests cover every phase and input pair against the #108 record, fuzz input sequences against an independent transition model (including delayed allocation and write completion), and mutate each correlation, Closing and watchdog rule. Refs #109
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
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.
Part 3 of the protocol contract decided in #108. The base capability,
watchdog and disconnect messages were only partly checked, by parsers
that differed between sm and peer, and the Manager ignored the optional
validation settings.
§§3.2, 5.3.1-5.5.2 and 6.3: base header, required fields, types and
multiplicity, Origin-Host and Origin-Realm syntax (§4.3.1), no
Supported-Vendor-Id of zero (§5.3.6), and no Destination-Host or
Destination-Realm in answers (§6.2). A malformed request gets one error
answer with a single Failed-AVP (Verified Errata ID 4615); a malformed
answer is discarded and reported.
3xxx or 5xxx Result-Code (§§7.1.3, 7.1.5), but not with 2xxx or 4xxx
(§7.1.4). Answers this library sends still set the E bit only for 3xxx.
with the same pipeline and order as StateMachine (advertised
application, then unknown mandatory AVPs per §4.1, then dictionary
validation), and routes nothing after a rejection. Both stay off by
default.
Refs #108
Validation
make checkpasses locally for all modules: format, test (Go 1.27.2 and1.26.9), race, vet, lint, vuln, windows, secrets and codeql.
required fields, wrong types and flags, Application-Id, malformed identities,
Supported-Vendor-Id zero, destination AVPs in answers, E-bit answers with each
result class, nested unknown mandatory AVPs and every flag combination, through
both front ends; mutating any rule fails a named test.
a few prohibited values passed; both are fixed.
Summary by cubic
Implements the base message validation from #108 so the six connection-level messages (CER/CEA, DWR/DWA, DPR/DPA) are checked identically in both sm and peer front ends. Previously these were only partly checked by parsers that differed between front ends, and
peer.Managerignored the optional validation settings.peer.Managernow honorsRejectUnknownMandatoryAVPsandValidateRequestsusing the same pipeline and order asStateMachine; both settings stay off by default.Written for commit 36ccd0f. Summary will update on new commits.