Conversation
📝 WalkthroughWalkthroughAdds a shared helper to detect and format unknown critical CoAP options, then updates TCP and UDP client request handling to reject matching requests with Bad Option responses and diagnostic payloads. Adds unit and integration tests covering the rejection behavior and unchanged stored state. ChangesUnknown critical option rejection
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses RFC 7252 handling of unrecognized critical CoAP options by detecting them early in the UDP/TCP receive paths and returning 4.02 Bad Option (with a diagnostic payload) before application handlers run, preventing unintended resource mutation (Fixes #666).
Changes:
- Add shared utilities to detect unknown critical options and format a diagnostic message.
- Reject inbound requests containing unknown critical options in UDP and TCP processing paths by responding with
4.02 Bad Option. - Add regression tests for UDP and TCP that reproduce the reported
/storemutation scenario and assert it no longer occurs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| udp/client/conn.go | Adds early rejection and immediate 4.02 Bad Option response for unknown critical options (UDP). |
| udp/client/conn_test.go | Adds UDP regression test using raw datagrams to verify rejection and no /store mutation. |
| tcp/client/session.go | Adds early rejection and 4.02 Bad Option response for unknown critical options (TCP). |
| tcp/client_test.go | Adds TCP regression test verifying rejection and no /store mutation. |
| net/client/criticalOptions.go | Introduces shared logic to detect unknown critical options and format a diagnostic string. |
| net/client/criticalOptions_test.go | Adds unit tests for the new critical-options helper functions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #674 +/- ##
=======================================
Coverage 76.60% 76.60%
=======================================
Files 77 78 +1
Lines 6095 6172 +77
=======================================
+ Hits 4669 4728 +59
- Misses 1042 1053 +11
- Partials 384 391 +7 ☔ 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: 1
🧹 Nitpick comments (1)
udp/client/conn_test.go (1)
944-959: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the raw hex payloads.
The crafted
rawRequestHexvalues encode specific option layouts (Content-Format then option 99 / option 13) that are easy to get wrong when edited. A short comment breaking down the byte layout (header, token, URI-Path, Content-Format, the injected option) would help future maintainers avoid needing to hand-decode CoAP wire bytes.🤖 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 `@udp/client/conn_test.go` around lines 944 - 959, Add a brief comment near the test cases in conn_test.go explaining how each rawRequestHex is structured at the CoAP wire level, including the header, token, URI-Path, Content-Format, and the injected unknown option (99 or 13). Keep the existing test data, but document the byte layout around the testCases definition so future edits can verify the intended option ordering in the unknown critical option cases.
🤖 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 `@udp/client/conn.go`:
- Around line 938-945: The unknown critical option rejection in
cc.respondUnknownCriticalOptions is only applied in the req.Type() ==
message.Confirmable branch, so NonConfirmable requests can still reach dispatch.
Update the request handling in udp/client/conn.go to check
client.GetUnknownCriticalOptions(req.Options()) for both Confirmable and
NonConfirmable messages before any handler dispatch, and keep the existing
ReleaseMessage/return flow when rejecting.
---
Nitpick comments:
In `@udp/client/conn_test.go`:
- Around line 944-959: Add a brief comment near the test cases in conn_test.go
explaining how each rawRequestHex is structured at the CoAP wire level,
including the header, token, URI-Path, Content-Format, and the injected unknown
option (99 or 13). Keep the existing test data, but document the byte layout
around the testCases definition so future edits can verify the intended option
ordering in the unknown critical option cases.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 495ff772-52b7-4f63-b57f-df5a00035cc5
📒 Files selected for processing (6)
net/client/criticalOptions.gonet/client/criticalOptions_test.gotcp/client/session.gotcp/client_test.goudp/client/conn.goudp/client/conn_test.go
Fixes #666 Enforce RFC 7252 behavior for unrecognized critical options by returning 4.02 Bad Option before application handlers are invoked, preventing unintended resource mutation.t
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tcp/client/session.go (2)
230-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the error branches flagged by coverage.
Codecov reports 2 missing and 2 partial lines in this file. Likely candidates are the header-decode error path, the max-message-size-exceeded path in
decodeMessageFromBuffer(Lines 172-177), and/or theWriteMessagefailure path inrespondUnknownCriticalOptions(Line 261-263). Adding targeted unit tests for these branches would close the coverage gap on this newly refactored, security-relevant code path.🤖 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 `@tcp/client/session.go` around lines 230 - 262, Add targeted tests for the uncovered error branches in Session.processBuffer, decodeMessageFromBuffer, and respondUnknownCriticalOptions. Cover the header-decode failure path and the max-message-size-exceeded branch in decodeMessageFromBuffer, and also force WriteMessage to fail in respondUnknownCriticalOptions so the wrapped error path is exercised. Use the existing Session, decodeMessageFromBuffer, processBuffer, and respondUnknownCriticalOptions symbols to locate the code and assert the returned errors and cleanup behavior.
194-206: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCritical-option check excludes RFC 8132 extended methods.
The check only runs for codes in
[codes.GET, codes.DELETE](1–4), soFETCH/PATCH/iPATCH(RFC 8132 extended methods, codes 5–7) bypass unknown-critical-option rejection and would still reach the handler. Since RFC 7252 §5.4.1 requires this behavior for any request with a critical option it doesn't recognize/handle, not just the base four methods, consider widening the guard if these extended methods are supported by this server.♻️ Possible widening (verify extended method support first)
- if req.Code() < codes.GET || req.Code() > codes.DELETE { + if req.Code() < codes.GET || req.Code() > codes.IPATCH { return false, nil }🤖 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 `@tcp/client/session.go` around lines 194 - 206, The critical-option gate in shouldDropRequest only covers the base CoAP methods and skips RFC 8132 extended methods, so requests like FETCH, PATCH, and iPATCH can bypass unknown-critical-option rejection. Update the method-range guard in Session.shouldDropRequest to include any supported extended methods as well, while still preserving the existing unknown-critical-option response path via respondUnknownCriticalOptions and GetUnknownCriticalOptions.
🤖 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.
Nitpick comments:
In `@tcp/client/session.go`:
- Around line 230-262: Add targeted tests for the uncovered error branches in
Session.processBuffer, decodeMessageFromBuffer, and
respondUnknownCriticalOptions. Cover the header-decode failure path and the
max-message-size-exceeded branch in decodeMessageFromBuffer, and also force
WriteMessage to fail in respondUnknownCriticalOptions so the wrapped error path
is exercised. Use the existing Session, decodeMessageFromBuffer, processBuffer,
and respondUnknownCriticalOptions symbols to locate the code and assert the
returned errors and cleanup behavior.
- Around line 194-206: The critical-option gate in shouldDropRequest only covers
the base CoAP methods and skips RFC 8132 extended methods, so requests like
FETCH, PATCH, and iPATCH can bypass unknown-critical-option rejection. Update
the method-range guard in Session.shouldDropRequest to include any supported
extended methods as well, while still preserving the existing
unknown-critical-option response path via respondUnknownCriticalOptions and
GetUnknownCriticalOptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8992432-23fa-4605-8faf-8c3f93db0146
📒 Files selected for processing (6)
net/client/criticalOptions.gonet/client/criticalOptions_test.gotcp/client/session.gotcp/client_test.goudp/client/conn.goudp/client/conn_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- tcp/client_test.go
- net/client/criticalOptions_test.go
- net/client/criticalOptions.go
- udp/client/conn_test.go
- udp/client/conn.go
|
Fixes #666
Enforce RFC 7252 behavior for unrecognized critical options by returning 4.02 Bad Option before application handlers are invoked, preventing unintended resource mutation.
Summary by CodeRabbit
Bad Optionresponse.Bad Optionand do not modify stored data.