Develop#80
Open
kweonminsung wants to merge 36 commits into
Open
Conversation
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
- build_response now copies the zone section into the response and sets QDCOUNT=1 so clients don't treat the reply as malformed - Compute new_serial once per apply_update request and thread it through the call chain; removes per-change zone reload in log_zone_change - Validate u32 TTL <= i32::MAX in add_record before casting to avoid negative TTLs being stored in the database Co-authored-by: kweonminsung <79230043+kweonminsung@users.noreply.github.com> Agent-Logs-Url: https://github.com/kweonminsung/bindizr/sessions/38cc6a44-4082-40f0-b5df-4e45ff363f4c
fix(nsupdate): proper UPDATE response, serial consistency, TTL overflow guard
72 feature add nsupdate support
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 33 medium |
🟢 Metrics 600 complexity
Metric Results Complexity 600
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates DNS responsibilities into a new dns module (UDP/TCP listener handling SOA, XFR, and NSUPDATE), refactors business logic into a new service layer, and updates configuration/docs and dependencies to support the new DNS functionality.
Changes:
- Replaces the standalone
xfrserver module with a unifieddnsserver (SOA + AXFR/IXFR + NOTIFY + RFC2136 NSUPDATE + ACL helpers). - Introduces a
servicelayer (ServiceError,RepositoryService,AuthService,TokenService) and rewires API/socket handlers to use it. - Updates configuration keys (
[xfr]→[dns]), documentation, and bumps several crate versions / release action version.
Reviewed changes
Copilot reviewed 49 out of 54 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fixture/bindizr.conf.toml | Updates test config fixture to [dns] section and adds nsupdate_tsig_key. |
| tests/common/mod.rs | Updates test router wiring to use ApiRouter. |
| src/xfr/server.rs | Removes old standalone XFR TCP server implementation. |
| src/xfr/mod.rs | Removes old XFR module bootstrap/spawn. |
| src/socket/server/zone.rs | Switches socket zone handler to service::zone::ZoneService. |
| src/socket/server/token.rs | Switches socket token operations to TokenService. |
| src/socket/server/record.rs | Switches socket record operations to service + RepositoryService lookup. |
| src/socket/server/notify.rs | Routes NOTIFY through dns::xfr::notify. |
| src/service/zone.rs | Refactors zone logic to use RepositoryService + ServiceError, and routes NOTIFY via dns. |
| src/service/token.rs | Adds TokenService for token CRUD + generation. |
| src/service/repository.rs | Adds RepositoryService facade over repositories with ServiceError mapping. |
| src/service/record.rs | Refactors record logic to ServiceError/RepositoryService and adds nsupdate-related helpers/validation. |
| src/service/mod.rs | Introduces the new service module exports. |
| src/service/error.rs | Adds ServiceError enum for service-layer errors. |
| src/service/auth.rs | Adds AuthService for API token validation and last-used updates. |
| src/main.rs | Registers new dns and service modules. |
| src/lib.rs | Exposes new dns and service modules; removes xfr export. |
| src/error/mod.rs | Repoints top-level error to dns::xfr::error::XfrError. |
| src/dns/xfr/wire.rs | Adds DNS wire encode/decode utilities and message builder for XFR/SOA. |
| src/dns/xfr/notify.rs | Refactors NOTIFY to use RepositoryService and new config key. |
| src/dns/xfr/mod.rs | Adds XFR handler entrypoints for TCP/UDP and secondary ACL validation. |
| src/dns/xfr/ixfr.rs | Updates IXFR to load zones via RepositoryService. |
| src/dns/xfr/error.rs | Adds new XFR error type in the dns module. |
| src/dns/xfr/delta.rs | Updates IXFR delta/snapshot retrieval via RepositoryService. |
| src/dns/xfr/catalog.rs | Updates catalog generation to use RepositoryService and removes catalog SOA handler. |
| src/dns/xfr/axfr.rs | Updates AXFR to use RepositoryService and removes SOA handler (now in dns::soa). |
| src/dns/soa/mod.rs | Adds dedicated SOA handler for TCP/UDP with secondary ACL enforcement. |
| src/dns/nsupdate/update.rs | Implements RFC2136-style dynamic update apply logic + journal/snapshot + NOTIFY. |
| src/dns/nsupdate/prerequisite.rs | Implements RFC2136 prerequisite evaluation against zone records. |
| src/dns/nsupdate/parser.rs | Adds a DNS UPDATE parser + TSIG extraction helpers. |
| src/dns/nsupdate/mod.rs | Adds NSUPDATE request routing and response building for TCP/UDP. |
| src/dns/nsupdate/auth.rs | Adds TSIG validation (HMAC-SHA256) gated by dns.nsupdate_tsig_key. |
| src/dns/mod.rs | Adds unified DNS server (TCP+UDP) routing between NSUPDATE/SOA/XFR. |
| src/dns/acl.rs | Adds parsing + ACL helper functions for secondary server IP allowlists. |
| src/cli/output/table.rs | Updates tabled derive attributes for the bumped tabled version. |
| src/cli/mod.rs | Switches bootstrap from xfr::initialize() to dns::initialize(). |
| src/api/zone.rs | Renames controller type and wires zone endpoints to service::* + ApiError conversion. |
| src/api/service/mod.rs | Removes old api::service module. |
| src/api/service/auth.rs | Removes old API-layer auth service (moved to service::auth). |
| src/api/router.rs | Renames ApiController to ApiRouter and rewires module imports and auth middleware layering. |
| src/api/record.rs | Renames controller type and wires record endpoints to service::record::RecordService. |
| src/api/mod.rs | Reorganizes API module layout and uses ApiRouter. |
| src/api/middleware/mod.rs | Adds new API middleware module exports. |
| src/api/middleware/body_parser.rs | Moves/keeps JSON body extractor middleware under api::middleware. |
| src/api/middleware/auth.rs | Switches auth middleware to use service::auth::AuthService. |
| src/api/error.rs | Adds From<ServiceError> conversion for consistent API error mapping. |
| src/api/controller/dns.rs | Removes DNS controller (superseded by new DNS server module). |
| README.md | Documents [dns] config and adds nsupdate usage section. |
| Cargo.toml | Bumps dependencies and adds hmac/base64 for TSIG support. |
| Cargo.lock | Updates lockfile for dependency bumps/additions. |
| bindizr.conf.toml | Updates default config to [dns] section and adds nsupdate_tsig_key. |
| .github/workflows/release.yml | Bumps softprops/action-gh-release major version. |
| .github/workflows/manual-release.yml | Bumps softprops/action-gh-release major version and tweaks release name. |
Comments suppressed due to low confidence (1)
src/dns/xfr/notify.rs:75
dns.secondary_addrsis parsed here strictly asSocketAddr(ip:port), but other code (e.g. DNS ACL parsing) accepts bare IPs and the test fixture setssecondary_addrs = "127.0.0.1". This makes NOTIFY silently skip sending when the config contains IP-only entries. Consider sharing the ACL parser and defaulting missing ports to 53, or updating configuration/fixtures to require an explicit port consistently.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for RFC 2136-style DNS dynamic updates (nsupdate) by adding a new DNS listener, implementing TSIG authentication, and refactoring the service layer to use a centralized RepositoryService. The review identified several critical issues, including the use of unstable Rust features (let_chains), lack of atomicity in database operations for DNS updates, potential stack overflow risks with large buffers, and code duplication regarding serial number generation. The reviewer provided actionable suggestions to address these issues.
There was a problem hiding this comment.
Code Review
This pull request introduces support for RFC 2136-style dynamic updates via nsupdate and performs a major architectural refactoring. DNS-related functionalities, including XFR and SOA handling, have been consolidated into a new dns module, and a dedicated service layer was introduced to decouple business logic from the API and database layers. Additionally, core dependencies were updated, and TSIG authentication was implemented for secure dynamic updates. Feedback was provided regarding the serial number generation logic, which could fallback to zero upon a parsing error and potentially cause synchronization issues.
There was a problem hiding this comment.
Code Review
This pull request introduces support for RFC 2136-style DNS dynamic updates (nsupdate) by adding a new DNS listener and associated handlers. It also refactors the codebase by introducing a service layer to centralize repository access and standardizing error handling. I have reviewed the changes and provided feedback regarding stack allocation safety and RFC compliance for the zone section type.
Owner
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces RFC 2136-style DNS dynamic update support (nsupdate) and refactors the internal architecture by separating business logic into a service layer and adding transaction support to database repositories. The xfr module is renamed to dns, and various dependencies are updated. A critical issue was identified in the TSIG authentication logic where variables are appended to the DNS message for HMAC verification instead of being prepended as required by RFC 8945, which would cause validation failures.
Owner
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for RFC 2136 dynamic updates (nsupdate) with TSIG authentication and refactors the codebase into a service layer with transaction support for MySQL, PostgreSQL, and SQLite. The review feedback highlights several critical issues, including RFC non-compliance in DNS responses and packet validation, performance bottlenecks in database interactions and catalog generation, and a security flaw in the token hashing logic. Additionally, the reviewer identified a bug in domain name matching and recommended consolidating duplicated utility functions to improve maintainability.
Owner
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements RFC 2136-style DNS dynamic updates (nsupdate) with TSIG authentication and introduces a service layer to decouple business logic from the API and DNS protocol handlers. The database layer is updated to support transactions across MySQL, PostgreSQL, and SQLite via a new RepositoryTx wrapper. Feedback highlights a potential security risk in the DNS name decompression logic due to a loose jump limit and suggests abstracting transaction operations into a trait to reduce code duplication and improve maintainability.
📝 WalkthroughWalkthroughAdds DNS TCP/UDP listeners with nsupdate (RFC2136 + TSIG), SOA/XFR routing, a service layer with transaction-aware repository APIs and implementations, API/router and socket handler rewiring, CLI/config/docs updates, and many tests. ChangesDNS runtime + services refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant DnsServer
participant NsupdateHandler
participant SoaHandler
participant XfrRouter
participant ServiceLayer
participant Repository
Client->>DnsServer: send DNS TCP/UDP packet
DnsServer->>NsupdateHandler: classify as UPDATE
NsupdateHandler->>ServiceLayer: apply_update(request, tsig)
ServiceLayer->>Repository: begin_tx / load zone / validate / mutate
Repository-->>ServiceLayer: commit result
ServiceLayer-->>NsupdateHandler: UpdateResult (RCODE)
NsupdateHandler-->>DnsServer: build response
DnsServer-->>Client: send DNS response
Client->>DnsServer: send SOA/AXFR/IXFR query
DnsServer->>SoaHandler: classify as SOA
SoaHandler->>ServiceLayer: ZoneService::find(...)
ServiceLayer->>Repository: read zone/records
Repository-->>ServiceLayer: zone data
SoaHandler-->>DnsServer: respond with SOA
DnsServer-->>Client: send response
DnsServer->>XfrRouter: AXFR/IXFR over TCP
XfrRouter->>ServiceLayer: fetch zone/records/changes
ServiceLayer->>Repository: read ops
Repository-->>ServiceLayer: data stream
XfrRouter-->>DnsServer: stream transfer
DnsServer-->>Client: stream of DNS messages
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/dns/xfr/catalog.rs (1)
81-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMatch the catalog zone case-insensitively.
DNS names are case-insensitive, but this helper only accepts the exact lowercase spelling. Queries like
Catalog.Bind.will bypass the catalog handlers and get treated as ordinary zones.Small fix
pub fn is_catalog_zone(zone_name: &str) -> bool { - zone_name == CATALOG_ZONE_NAME + zone_name + .trim_end_matches('.') + .eq_ignore_ascii_case(CATALOG_ZONE_NAME) }🤖 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 `@src/dns/xfr/catalog.rs` around lines 81 - 82, The helper is_catalog_zone currently does a case-sensitive comparison against CATALOG_ZONE_NAME so names like "Catalog.Bind." won't match; update is_catalog_zone to compare case-insensitively (e.g., use zone_name.eq_ignore_ascii_case(CATALOG_ZONE_NAME) or normalize both sides to lowercase) so DNS names match regardless of case while still referencing the same CATALOG_ZONE_NAME constant.src/dns/xfr/ixfr.rs (1)
323-351:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIXFR currently drops PTR deltas.
rr_to_record_valueaccepts PTR updates, but this serializer falls through to the default branch and silently skips them. Any zone with PTR changes will therefore produce incomplete IXFR responses.🤖 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 `@src/dns/xfr/ixfr.rs` around lines 323 - 351, The match on change.record_type currently omits "PTR" and silently skips PTR deltas; add a "PTR" arm to the match so PTR updates are serialized like other record types. Specifically, update the match in the function that converts IXFR changes (the code that calls rr_to_record_value and matches change.record_type) to include a "PTR" branch that parses change.record_value into the appropriate domain name (returning XfrError::ProtocolError on parse failure consistent with the "A"/"AAAA" branches) and calls the builder's PTR serializer (e.g., builder.add_ptr_record(&owner_name, ttl, &parsed_name) or builder.add_ptr_record(&owner_name, ttl, &change.record_value) if the builder accepts a string). Ensure you reference rr_to_record_value, change.record_type, and builder.add_ptr_record so PTR deltas are handled instead of skipped.
🤖 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 @.github/workflows/manual-release.yml:
- Line 63: The workflow currently uses softprops/action-gh-release@v3 which
requires the Node 24 Actions runtime; verify ubuntu-latest provides Node 24 in
your CI environment and if it does not either (A) revert the action to
softprops/action-gh-release@v2.6.2 to stay Node 20-compatible, or (B) add an
explicit step before the action to install Node 24 via actions/setup-node@v4
(set node-version: 24) so the softprops/action-gh-release@v3 runtime requirement
is satisfied; update the workflow to use one of these two fixes and then re-run
the workflow to confirm the release step succeeds.
In `@bindizr.conf.toml`:
- Around line 19-22: The config rename from [xfr] to [dns] is breaking upgrades;
update the config loader to detect a deprecated [xfr] section, map its keys
(listen_port, secondary_addrs, nsupdate_tsig_key) into the new [dns]
configuration, and emit a clear deprecation warning mentioning both section
names, then proceed using the mapped values; if neither [dns] nor [xfr] exist,
return a helpful error that explains the rename and points to migration steps;
finally add/update README/config docs with an explicit migration note showing
how [xfr] keys map to [dns].
In `@Cargo.toml`:
- Line 30: The Cargo.toml currently allows rand 0.10.x but the lockfile may
resolve to vulnerable 0.10.0; pin or override rand to 0.10.1+ to eliminate the
vulnerability by either adding an explicit dependency (rand = "0.10.1") or using
a [patch.crates-io] override to force 0.10.1+ (ensure this affects transitive
consumers like tabled), then regenerate Cargo.lock (cargo update -p rand
--precise 0.10.1 or cargo update) and run the test suite to confirm no breaking
API changes in code paths using rand (search for direct uses of rand::rng() or
impacted call sites).
In `@src/api/record.rs`:
- Around line 70-74: The read/delete logic is currently calling
RecordService::get with zone_id = None (producing raw_record) and later deletes
by only name+record_type, which allows cross-zone collisions; update the
handlers in src/api/record.rs to require and pass the request's zone_id into
RecordService::get and the delete call (use the same zone_id variable instead of
None) and ensure the delete path uses RecordService::delete (or its equivalent)
scoped by zone_id + name + record_type; also validate presence of zone_id and
return an error if missing to avoid accidental cross-zone reads/deletes.
In `@src/database/repository/mysql/record_repository_impl.rs`:
- Around line 137-158: The query currently lower-cases record values
(LOWER(value) = LOWER(?)), which wrongly treats all record types as
case-insensitive; change the WHERE clause in the Record fetch (the
sqlx::query_as::<_, Record> block that binds zone_id, name, record_type, value,
priority) to compare value case-sensitively by default and only do
LOWER(value)=LOWER(?) for explicitly case-insensitive record types (e.g., CNAME,
NS, PTR, MX — whatever set your domain rules require); implement this by
replacing the current value clause with a conditional expression like: AND ((?
IS NULL) OR ((record_type IN ('CNAME','NS','PTR','MX')) AND
LOWER(value)=LOWER(?)) OR ((record_type NOT IN ('CNAME','NS','PTR','MX')) AND
value = ?)) and bind value appropriately (value twice for the LOWER branch and
once for the case-sensitive branch); apply the same change to the other query at
the second location noted in this file.
In `@src/dns/nsupdate/auth.rs`:
- Around line 95-110: The code assumes bytes 10..12 exist in the rebuilt
`message` when updating ARCOUNT, but if `tsig.rr_start < 12` (or the removal
made `message.len() < 12`) this will panic; add an explicit guard before
mutating `message[10..12]` (e.g., check `tsig.rr_start >= 12 && message.len() >=
12`) and return `UpdateError::Refused("invalid TSIG envelope".to_string())` (or
a similar refusal) if the condition fails so the ARCOUNT update in `message` is
safe; update the block around `arcount`/`new_arcount` accordingly (references:
`query_data`, `tsig.rr_start`, `tsig.rr_end`, `message`, `arcount`,
`UpdateError::Refused`).
In `@src/dns/nsupdate/update.rs`:
- Around line 68-73: Move the prerequisite check and serial allocation to run
after the transaction is successfully started: call service::begin_tx(...) first
(as in the existing tx variable), then run
super::prerequisite::evaluate_prerequisites(&zone, &request.prerequisites,
query_data).await? and only after that compute let new_serial =
generate_serial(Some(zone.serial)); so both evaluation and serial generation
occur inside the transaction scope; ensure any database reads used by
evaluate_prerequisites/query_data are performed against the transaction context
if required and propagate errors into UpdateError::Internal the same way you
already do for begin_tx.
- Around line 221-246: The delete_records function is currently widening
malformed deletes because CLASS_NONE and CLASS_ANY are not validated before
deriving (target_value, target_priority); add explicit guards: check
update.class and update.rdata early in delete_records and return a
Malformed/UpdateError when CLASS_ANY has non-empty RDATA or when CLASS_NONE
would be interpreted as an RRset delete (i.e. refuse/err if update.class ==
CLASS_NONE and update.rdata.is_empty() would otherwise produce (None, None));
change the condition that sets (None, None) (currently based on is_rrset_delete
|| update.rdata.is_empty()) so it only yields an RRset delete when
is_rrset_delete is true or when rdata is empty and update.class permits it, and
keep parsing RDATA via rr_to_record_value(update, query_data) only when you have
validated the class and RDATA shape.
In `@src/service/auth.rs`:
- Around line 27-30: The token expiry check in the auth service currently uses
Utc::now() > *expires_at which allows acceptance of a token at exactly its
expiry instant; update the conditional in the stored_token.expires_at check to
use Utc::now() >= *expires_at so tokens equal to expires_at are treated as
expired and return ServiceError::Unauthorized("Token has expired".to_string())
as before.
In `@src/service/record/update.rs`:
- Around line 111-129: The update call is incorrectly setting created_at to
Utc::now(), which will reset the record's creation timestamp; change the Record
passed to RepositoryService::update_record_tx to preserve the original creation
time (use existing_record.created_at or the candidate_updated.created_at)
instead of using Utc::now(), so the update preserves the original created_at
value.
In `@src/service/token.rs`:
- Line 30: Validate expires_in_days before computing expires_at: if
expires_in_days is Some(d) require d > 0 and reject or return an error for
non-positive values rather than converting them into an immediate expiration;
only compute expires_at using Utc::now() + Duration::days(d) when d > 0,
otherwise treat as None or propagate a clear InvalidExpiration error. Update the
code path that uses the expires_in_days variable (and the calculation producing
expires_at) to perform this check and return a descriptive error (e.g.,
InvalidExpiration) when a non-positive day count is supplied.
In `@src/service/utils.rs`:
- Around line 34-47: The current is_in_bailiwick uses a raw suffix check which
treats siblings (e.g., notexample.com) as inside example.com; update
is_in_bailiwick(name, zone_name) to first compute fqdn forms via to_fqdn(name)
and to_fqdn(zone_name), lowercase them, then verify the zone is a proper
domain-label suffix: require either the name equals the zone, or the name
ends_with(zone) AND the character immediately before the matched suffix in the
name is a '.' (i.e., boundary on a label). Apply this logic in is_in_bailiwick
so sibling domains are not misclassified.
In `@src/service/zone/create.rs`:
- Around line 95-101: The structured log at the zone creation site currently
includes PII (created_zone.admin_email); update the log invocation (log_info!
macro) to remove admin_email from the format string and argument list so the
admin_email field is not emitted (keep other fields like created_zone.name,
created_zone.primary_ns, created_zone.serial, created_zone.id unchanged); ensure
no leftover formatting placeholders remain and do not add alternative plaintext
email logging—if you need email for debugging, log a redacted value or use a
separate secure/audit channel.
In `@src/service/zone/update.rs`:
- Around line 106-120: The update currently overwrites Zone.created_at with
Utc::now() inside the call to RepositoryService::update_zone_tx; instead
preserve the original creation timestamp by loading the existing zone (e.g.,
call RepositoryService::get_zone or the method that returns the current Zone)
and set created_at to that existing_zone.created_at when constructing the Zone
for update, or omit setting created_at so the update path leaves it unchanged;
update the code around the Zone construction passed into
RepositoryService::update_zone_tx (referencing Zone,
RepositoryService::update_zone_tx, and the update_zone_request fields) to use
the original created_at value.
In `@src/socket/server/record.rs`:
- Around line 16-17: The compiler error is caused by calling
RecordType::from_str(...) without importing the FromStr trait; add the trait
import (e.g. add use std::str::FromStr;) to the top of the module containing
RecordType::from_str so the method is in scope and the code will compile.
---
Outside diff comments:
In `@src/dns/xfr/catalog.rs`:
- Around line 81-82: The helper is_catalog_zone currently does a case-sensitive
comparison against CATALOG_ZONE_NAME so names like "Catalog.Bind." won't match;
update is_catalog_zone to compare case-insensitively (e.g., use
zone_name.eq_ignore_ascii_case(CATALOG_ZONE_NAME) or normalize both sides to
lowercase) so DNS names match regardless of case while still referencing the
same CATALOG_ZONE_NAME constant.
In `@src/dns/xfr/ixfr.rs`:
- Around line 323-351: The match on change.record_type currently omits "PTR" and
silently skips PTR deltas; add a "PTR" arm to the match so PTR updates are
serialized like other record types. Specifically, update the match in the
function that converts IXFR changes (the code that calls rr_to_record_value and
matches change.record_type) to include a "PTR" branch that parses
change.record_value into the appropriate domain name (returning
XfrError::ProtocolError on parse failure consistent with the "A"/"AAAA"
branches) and calls the builder's PTR serializer (e.g.,
builder.add_ptr_record(&owner_name, ttl, &parsed_name) or
builder.add_ptr_record(&owner_name, ttl, &change.record_value) if the builder
accepts a string). Ensure you reference rr_to_record_value, change.record_type,
and builder.add_ptr_record so PTR deltas are handled instead of skipped.
🪄 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: 2770ff1f-2036-4409-a431-76588529866e
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpublic/concepts.pngis excluded by!**/*.png
📒 Files selected for processing (80)
.github/workflows/manual-release.yml.github/workflows/release.ymlCargo.tomlREADME.mdbindizr.conf.tomlsrc/api/controller/dns.rssrc/api/error.rssrc/api/middleware/auth.rssrc/api/middleware/body_parser.rssrc/api/middleware/mod.rssrc/api/mod.rssrc/api/record.rssrc/api/router.rssrc/api/service/auth.rssrc/api/service/mod.rssrc/api/service/record.rssrc/api/service/zone.rssrc/api/zone.rssrc/cli/commands/token.rssrc/cli/mod.rssrc/cli/output/table.rssrc/database/repository/mod.rssrc/database/repository/mysql/record_repository_impl.rssrc/database/repository/mysql/zone_change_repository_impl.rssrc/database/repository/mysql/zone_repository_impl.rssrc/database/repository/mysql/zone_snapshot_repository_impl.rssrc/database/repository/postgres/record_repository_impl.rssrc/database/repository/postgres/zone_change_repository_impl.rssrc/database/repository/postgres/zone_repository_impl.rssrc/database/repository/postgres/zone_snapshot_repository_impl.rssrc/database/repository/sqlite/record_repository_impl.rssrc/database/repository/sqlite/zone_change_repository_impl.rssrc/database/repository/sqlite/zone_repository_impl.rssrc/database/repository/sqlite/zone_snapshot_repository_impl.rssrc/database/utils.rssrc/dns/acl.rssrc/dns/mod.rssrc/dns/nsupdate/auth.rssrc/dns/nsupdate/mod.rssrc/dns/nsupdate/parser.rssrc/dns/nsupdate/prerequisite.rssrc/dns/nsupdate/update.rssrc/dns/soa/mod.rssrc/dns/xfr/axfr.rssrc/dns/xfr/catalog.rssrc/dns/xfr/delta.rssrc/dns/xfr/error.rssrc/dns/xfr/ixfr.rssrc/dns/xfr/mod.rssrc/dns/xfr/notify.rssrc/dns/xfr/wire.rssrc/error/mod.rssrc/lib.rssrc/main.rssrc/service/auth.rssrc/service/error.rssrc/service/mod.rssrc/service/record/create.rssrc/service/record/delete.rssrc/service/record/get.rssrc/service/record/mod.rssrc/service/record/update.rssrc/service/record/validation.rssrc/service/repository.rssrc/service/token.rssrc/service/utils.rssrc/service/zone/create.rssrc/service/zone/delete.rssrc/service/zone/get.rssrc/service/zone/mod.rssrc/service/zone/snapshot.rssrc/service/zone/update.rssrc/socket/server/notify.rssrc/socket/server/record.rssrc/socket/server/token.rssrc/socket/server/zone.rssrc/xfr/mod.rssrc/xfr/server.rstests/common/mod.rstests/fixture/bindizr.conf.toml
💤 Files with no reviewable changes (8)
- src/api/service/mod.rs
- src/xfr/mod.rs
- src/api/service/auth.rs
- src/api/service/record.rs
- src/api/middleware/body_parser.rs
- src/api/controller/dns.rs
- src/xfr/server.rs
- src/api/service/zone.rs
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/socket/server/record.rs (1)
56-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDo not serialize missing zone names as empty strings.
When zone lookup fails, current fallback produces
zone_name: "". PreferNoneso responses stay semantically correct.🔧 Proposed fix
- let zone_name = ZoneService::find_by_id(record.zone_id) + let zone_name = ZoneService::find_by_id(record.zone_id) .await .ok() .flatten() - .map(|z| z.name) - .unwrap_or_default(); + .map(|z| z.name); let mut rec_response = GetRecordResponse::from_record(record); - rec_response.zone_name = Some(zone_name); + rec_response.zone_name = zone_name;- let zone_name = ZoneService::find_by_id(record.zone_id) + let zone_name = ZoneService::find_by_id(record.zone_id) .await .ok() .flatten() - .map(|z| z.name) - .unwrap_or_default(); + .map(|z| z.name); let mut response = GetRecordResponse::from_record(&record); - response.zone_name = Some(zone_name); + response.zone_name = zone_name;Also applies to: 83-91
🤖 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 `@src/socket/server/record.rs` around lines 56 - 65, The code currently forces missing zone names to an empty string by using unwrap_or_default() and then wrapping it in Some(...); change the ZoneService::find_by_id(...) chain to stop using unwrap_or_default and let it produce an Option<String> (i.e. remove .unwrap_or_default() and keep .map(|z| z.name)), then assign that Option directly to rec_response.zone_name (remove the surrounding Some(...) wrapper). Apply the same fix to the other occurrence around lines 83-91 so missing zones become None instead of "".
♻️ Duplicate comments (2)
src/service/record/update.rs (1)
112-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
created_atwhen writing the updated row.The validation candidate on Line 88 keeps
existing_record.created_at, but the actual write on Line 122 resets it toUtc::now(). That mutates creation metadata on every edit.Small fix
let updated_record = RepositoryService::update_record_tx( &mut tx, Record { id: record_id, name: update_record_request.name.clone(), record_type, value: update_record_request.value.clone(), ttl: update_record_request.ttl, priority: update_record_request.priority, zone_id: zone.id, - created_at: Utc::now(), // Will be set by the database + created_at: existing_record.created_at, }, )🤖 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 `@src/service/record/update.rs` around lines 112 - 123, The update currently overwrites the record's created_at with Utc::now() when calling RepositoryService::update_record_tx; change the call site that builds the Record for update (the Record passed into update_record_tx) to preserve the original timestamp by using existing_record.created_at instead of Utc::now() so created_at is not mutated on edits (keep other fields from update_record_request and zone.id as-is).src/dns/nsupdate/update.rs (1)
241-252:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRefuse malformed delete shapes before deriving an RRset delete.
Line 247 still turns
CLASS_NONE+ empty RDATA into(None, None), which widens an invalid exact delete into an RRset delete. The same block also ignores non-empty RDATA onCLASS_ANYdeletes. A malformed NSUPDATE can therefore delete more records than the wire request actually specifies.Suggested guardrail
let target_type = if update.rr_type == TYPE_ANY { None } else { Some(rr_type_to_record_type(update.rr_type)?) }; - let (target_value, target_priority) = if is_rrset_delete || update.rdata.is_empty() { + if is_rrset_delete { + if !update.rdata.is_empty() { + return Err(UpdateError::Refused( + "rrset deletes must not include rdata".to_string(), + )); + } + } else if update.rr_type == TYPE_ANY || update.rdata.is_empty() { + return Err(UpdateError::Refused( + "exact deletes must specify rrtype and rdata".to_string(), + )); + } + + let (target_value, target_priority) = if is_rrset_delete { (None, None) } else { let (_, value, priority) = rr_to_record_value(update, query_data)?; (Some(value), priority) };🤖 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 `@src/dns/nsupdate/update.rs` around lines 241 - 252, The code currently treats CLASS_NONE + empty RDATA and CLASS_ANY + non-empty RDATA as if they were legitimate RRset deletes, widening malformed exact deletes; add explicit validation before deriving target_value/target_priority by checking update.class and update.rdata: if update.class == CLASS_NONE && update.rdata.is_empty() or update.class == CLASS_ANY && !update.rdata.is_empty() then return an error (reject the malformed NSUPDATE) instead of falling through to the RRset-delete branch; update the logic that sets is_rrset_delete and the block around rr_to_record_value(update, query_data) / rr_type_to_record_type(update.rr_type) to rely on these guards so only valid shapes produce (None,None) for RRset deletes.
🤖 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 `@src/api/record.rs`:
- Around line 88-90: The handlers use a non-unique selector (RecordService::get
called with zone.id, name, record_type, None, None) which can match multiple
RRset rows; change the contract to operate on a stable record identifier
instead: update the GET/UPDATE/DELETE handlers to accept and pass a unique
record_id to RecordService methods (e.g., use RecordService::get_by_id /
RecordService::update_by_id / RecordService::delete_by_id) and ensure listing
endpoints return that record_id so callers can choose the exact record;
alternatively, if you cannot add record_id, require the selectors to include
value and priority (pass value and priority into
RecordService::get/update/delete) so the combination (zone_id, name,
record_type, value, priority) is unique—update all callers (the code invoking
RecordService::get and the update/delete flows) to use the new signature
(record_id or full selectors) and adjust tests accordingly.
In `@src/database/repository/sqlite/zone_repository_impl.rs`:
- Around line 104-124: The SQLite implementation of get_by_name_for_update_tx
(function get_by_name_for_update_tx, RepositoryTxKind::SQLite) needs to obtain a
reserved write lock so it matches the "for update" semantics; change the SQLite
transaction handling to begin the transaction in IMMEDIATE mode (execute "BEGIN
IMMEDIATE" on the sqlite connection when creating/initializing RepositoryTx with
kind SQLite or, if that is not possible, execute "BEGIN IMMEDIATE" on sqlite_tx
before running the SELECT in get_by_name_for_update_tx) so the reserved lock is
acquired up-front and concurrent writers cannot race.
In `@src/dns/nsupdate/prerequisite.rs`:
- Around line 120-124: The prerequisite check currently compares record.value
case-insensitively which can cause false positives; update the closure in the
zone_records.iter().any(...) used for IN-class prerequisite checks so that value
matching uses exact, case-sensitive comparison (replace
record.value.eq_ignore_ascii_case(&target_value) with a direct equality check
against target_value) while keeping the existing comparisons for name,
record_type, and priority (refer to the closure variables relative, target_type,
target_value, and target_priority to locate the change).
In `@src/dns/nsupdate/update.rs`:
- Around line 266-268: The current check using eq_ignore_ascii_case between
target_value and record.value widens exact deletes for case-sensitive RDATA
(e.g., TXT); update the comparison to be type-aware: inspect the record type
(e.g., record.rtype or RData enum) and perform case-insensitive comparison only
for name-like types (CNAME, NS, PTR, possibly MX hostnames), but use exact
case-sensitive equality for data types that are case-sensitive (e.g., TXT,
binary RDATA). Modify the block around target_value and record.value in
update.rs to match on the RData/rtype and choose the appropriate comparison
method so exact deletes behave correctly.
In `@src/service/record/delete.rs`:
- Around line 30-90: The zone lookup (RepositoryService::get_zone_by_name),
record lookup (RepositoryService::get_record), loading zone records
(RepositoryService::get_records_by_zone_id) and the
validate_record_delete_constraints call must run inside the same DB transaction
as the delete: call RepositoryService::begin_tx(...) first, then perform
get_zone_by_name, get_record and get_records_by_zone_id using that transaction
context (or transaction-aware repository APIs), run
validate_record_delete_constraints(&zone, &zone_records,
std::slice::from_ref(&existing_record)) within the tx, and only then perform the
delete and commit; ensure any early returns on NotFound or validation errors
rollback/abort the tx and convert repository errors to ServiceError::Internal as
before.
- Around line 145-152: The audit log in the record delete path currently logs
the full record value via the log_info! call in delete.rs; remove the sensitive
existing_record.value from the message and log only non-sensitive metadata
(e.g., zone.name, record_name, record_type_str_clone, existing_record.id) or
replace the value field with a fixed placeholder like "<redacted>" to avoid
exposing TXT tokens/verification strings; update the log_info! invocation that
references zone.name, record_name, record_type_str_clone, existing_record.value,
and existing_record.id accordingly.
In `@src/service/record/update.rs`:
- Around line 18-22: The update function cannot uniquely target a record in
multivalue RRsets; change the update API to use a stable identifier instead of
just zone+name+type: modify the update signature (pub async fn update) and/or
UpdateRecordRequest to include the record's unique id (record_id) or the full
old value tuple (e.g., old_value and old_priority for MX/TXT) and then change
the lookup logic that currently keys on zone.id + name + old_record_type to find
the exact record by record_id (or by zone.id + name + type +
old_value[/priority]) before applying changes; update any callers and tests that
construct UpdateRecordRequest to pass the new identifier.
---
Outside diff comments:
In `@src/socket/server/record.rs`:
- Around line 56-65: The code currently forces missing zone names to an empty
string by using unwrap_or_default() and then wrapping it in Some(...); change
the ZoneService::find_by_id(...) chain to stop using unwrap_or_default and let
it produce an Option<String> (i.e. remove .unwrap_or_default() and keep .map(|z|
z.name)), then assign that Option directly to rec_response.zone_name (remove the
surrounding Some(...) wrapper). Apply the same fix to the other occurrence
around lines 83-91 so missing zones become None instead of "".
---
Duplicate comments:
In `@src/dns/nsupdate/update.rs`:
- Around line 241-252: The code currently treats CLASS_NONE + empty RDATA and
CLASS_ANY + non-empty RDATA as if they were legitimate RRset deletes, widening
malformed exact deletes; add explicit validation before deriving
target_value/target_priority by checking update.class and update.rdata: if
update.class == CLASS_NONE && update.rdata.is_empty() or update.class ==
CLASS_ANY && !update.rdata.is_empty() then return an error (reject the malformed
NSUPDATE) instead of falling through to the RRset-delete branch; update the
logic that sets is_rrset_delete and the block around rr_to_record_value(update,
query_data) / rr_type_to_record_type(update.rr_type) to rely on these guards so
only valid shapes produce (None,None) for RRset deletes.
In `@src/service/record/update.rs`:
- Around line 112-123: The update currently overwrites the record's created_at
with Utc::now() when calling RepositoryService::update_record_tx; change the
call site that builds the Record for update (the Record passed into
update_record_tx) to preserve the original timestamp by using
existing_record.created_at instead of Utc::now() so created_at is not mutated on
edits (keep other fields from update_record_request and zone.id as-is).
🪄 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: e0513ffb-b5e6-45ea-a8cc-c8d6a5c7468f
📒 Files selected for processing (21)
clippy.tomlsrc/api/dto.rssrc/api/record.rssrc/cli/commands/delete.rssrc/cli/commands/get.rssrc/database/repository/mod.rssrc/database/repository/mysql/record_repository_impl.rssrc/database/repository/mysql/zone_repository_impl.rssrc/database/repository/postgres/record_repository_impl.rssrc/database/repository/postgres/zone_repository_impl.rssrc/database/repository/sqlite/record_repository_impl.rssrc/database/repository/sqlite/zone_repository_impl.rssrc/dns/nsupdate/auth.rssrc/dns/nsupdate/prerequisite.rssrc/dns/nsupdate/update.rssrc/service/record/delete.rssrc/service/record/update.rssrc/service/repository.rssrc/service/zone/get.rssrc/socket/server/record.rstests/api/record.rs
✅ Files skipped from review due to trivial changes (1)
- clippy.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/service/zone/get.rs
- src/service/repository.rs
Comment on lines
+30
to
+90
| let zone = match RepositoryService::get_zone_by_name(zone_name).await { | ||
| Ok(Some(zone)) => zone, | ||
| Ok(None) => { | ||
| return Err(ServiceError::NotFound(format!( | ||
| "Zone with name '{}' not found", | ||
| zone_name | ||
| ))); | ||
| } | ||
| Err(e) => { | ||
| log_error!("Failed to fetch zone: {}", e); | ||
| return Err(ServiceError::Internal("Failed to fetch zone".to_string())); | ||
| } | ||
| }; | ||
|
|
||
| // Check if record exists in the requested zone. | ||
| let existing_record = match RepositoryService::get_record( | ||
| Some(zone.id), | ||
| name, | ||
| &record_type, | ||
| None, | ||
| None, | ||
| false, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(Some(record)) => record, | ||
| Ok(None) => { | ||
| return Err(ServiceError::NotFound(format!( | ||
| "Record with name '{}' and type '{}' not found", | ||
| name, record_type_str | ||
| ))); | ||
| } | ||
| Err(e) => { | ||
| log_error!("Failed to fetch record: {}", e); | ||
| return Err(ServiceError::Internal("Failed to fetch record".to_string())); | ||
| } | ||
| }; | ||
|
|
||
| let record_id = existing_record.id; | ||
| let record_name = existing_record.name.clone(); | ||
| let record_type_str_clone = record_type_str.to_string(); | ||
|
|
||
| let zone_records = match RepositoryService::get_records_by_zone_id(zone.id).await { | ||
| Ok(records) => records, | ||
| Err(e) => { | ||
| log_error!("Failed to load zone records: {}", e); | ||
| return Err(ServiceError::Internal( | ||
| "Failed to delete record".to_string(), | ||
| )); | ||
| } | ||
| }; | ||
|
|
||
| validate_record_delete_constraints( | ||
| &zone, | ||
| &zone_records, | ||
| std::slice::from_ref(&existing_record), | ||
| )?; | ||
|
|
||
| // Delete record | ||
| let mut tx = RepositoryService::begin_tx("Failed to delete record").await?; | ||
|
|
There was a problem hiding this comment.
Move existence/constraint checks into the same transaction as delete.
Zone lookup, record lookup, and delete-constraint validation currently run before begin_tx, so concurrent writes can invalidate assumptions before the delete is committed.
🤖 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 `@src/service/record/delete.rs` around lines 30 - 90, The zone lookup
(RepositoryService::get_zone_by_name), record lookup
(RepositoryService::get_record), loading zone records
(RepositoryService::get_records_by_zone_id) and the
validate_record_delete_constraints call must run inside the same DB transaction
as the delete: call RepositoryService::begin_tx(...) first, then perform
get_zone_by_name, get_record and get_records_by_zone_id using that transaction
context (or transaction-aware repository APIs), run
validate_record_delete_constraints(&zone, &zone_records,
std::slice::from_ref(&existing_record)) within the tx, and only then perform the
delete and commit; ensure any early returns on NotFound or validation errors
rollback/abort the tx and convert repository errors to ServiceError::Internal as
before.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/commands/get.rs (1)
116-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid panic in CLI output mapping for single-item responses.
Line 123 and Line 185 panic on parse errors, which can crash the CLI on malformed or shape-shifted daemon responses. Prefer graceful handling and return an empty row set or propagated command error.
Proposed fix
- vec![ - ZoneRow::from_json(data).unwrap_or_else(|_| panic!("Failed to parse zone")), - ] + ZoneRow::from_json(data).map_or_else(|_| vec![], |row| vec![row]) ... - vec![ - RecordRow::from_json(data) - .unwrap_or_else(|_| panic!("Failed to parse record")), - ] + RecordRow::from_json(data).map_or_else(|_| vec![], |row| vec![row])Also applies to: 177-186
🤖 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 `@src/cli/commands/get.rs` around lines 116 - 125, The CLI currently panics when ZoneRow::from_json fails (inside print_output_with_table closure and at the other similar site), so replace the unwrap_or_else panic paths with graceful handling: call ZoneRow::from_json and either filter_map the Result to skip failures (e.g., use .ok()) for array branches or match the single-item branch to return an empty vec (or propagate a CommandError) instead of panicking; update both occurrences that use ZoneRow::from_json (the closure passed to print_output_with_table and the similar block around lines 177-186) to either collect only successful ZoneRow parses or convert parse errors into an Err result that the command returns. Ensure you reference ZoneRow::from_json and print_output_with_table when making the change.
♻️ Duplicate comments (2)
src/service/record/delete.rs (1)
20-69:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRun existence/constraint checks in the same transaction as delete.
Line 20 through Line 67 performs record/zone reads plus delete-constraint validation before Line 69 starts the transaction. Concurrent writes can invalidate those assumptions before the delete is applied.
🤖 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 `@src/service/record/delete.rs` around lines 20 - 69, The reads and validate_record_delete_constraints call must run inside the same DB transaction as the delete to avoid race conditions; begin the transaction earlier by calling RepositoryService::begin_tx("Failed to delete record").await? before fetching the record/zone/zone_records, then use the transaction-scoped repository methods (e.g. tx.get_record_by_id, tx.get_zone_by_id, tx.get_records_by_zone_id) to load existing_record, zone and zone_records, run validate_record_delete_constraints(&zone, &zone_records, std::slice::from_ref(&existing_record)) while still holding tx, and only then perform the delete and commit/rollback on tx so all existence/constraint checks and the delete occur atomically.src/dns/nsupdate/update.rs (1)
268-270:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse type-aware value matching for exact deletes.
eq_ignore_ascii_casestill widens exact deletes for case-sensitive data like TXT. A delete forToken=abccan matchToken=ABCand remove the wrong record.🎯 Narrow exact-delete matching
- if let Some(ref value) = target_value - && !record.value.eq_ignore_ascii_case(value) - { - continue; - } + if let Some(ref value) = target_value { + let same_value = match record.record_type { + RecordType::TXT => record.value == *value, + _ => record.value.eq_ignore_ascii_case(value), + }; + if !same_value { + continue; + } + }🤖 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 `@src/dns/nsupdate/update.rs` around lines 268 - 270, The current delete check uses eq_ignore_ascii_case between target_value and record.value which incorrectly treats case-sensitive RDATA (e.g., TXT) as case-insensitive; change the comparison to be type-aware: when the record type (e.g., record.rtype / record.record_type) is a case-sensitive type like TXT, compare using exact equality (record.value == target_value), otherwise use case-insensitive comparison for name-like types; update the conditional around target_value and record.value to branch on the record type and perform the appropriate equality check to ensure exact deletes only remove truly matching RDATA.
🧹 Nitpick comments (1)
src/socket/server/record.rs (1)
45-57: ⚡ Quick winCache zone lookups while building the list response.
This does one
ZoneService::find_by_idcall per record, so listing many records turns into N+1 lookups. Memoizing byzone_idinside the handler avoids the repeated round-trips.♻️ Localized fix
Ok(records) => { let mut response: Vec<GetRecordResponse> = Vec::new(); + let mut zone_names = std::collections::HashMap::new(); for record in records.iter() { - let zone_name = ZoneService::find_by_id(record.zone_id) - .await - .ok() - .flatten() - .map(|z| z.name) - .unwrap_or_default(); + let zone_name = if let Some(zone_name) = zone_names.get(&record.zone_id) { + zone_name.clone() + } else { + let zone_name = ZoneService::find_by_id(record.zone_id) + .await + .ok() + .flatten() + .map(|z| z.name) + .unwrap_or_default(); + zone_names.insert(record.zone_id, zone_name.clone()); + zone_name + }; let mut rec_response = GetRecordResponse::from_record(record); rec_response.zone_name = Some(zone_name); response.push(rec_response); }🤖 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 `@src/socket/server/record.rs` around lines 45 - 57, The handler currently calls ZoneService::find_by_id for every record causing N+1 lookups; fix by memoizing zone names in a local HashMap keyed by record.zone_id and reuse cached names when building each GetRecordResponse (i.e. before calling ZoneService::find_by_id check the cache for the zone_id, only call ZoneService::find_by_id when missing, store the returned zone.name in the cache, then set rec_response.zone_name). Update the loop that creates GetRecordResponse::from_record to consult this cache (use the record.zone_id and rec_response.zone_name fields) so each zone is fetched at most once.
🤖 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 `@src/dns/nsupdate/parser.rs`:
- Around line 324-339: The parser currently follows compression pointers without
rejecting forward or self-referential pointers; update the pointer-handling
logic in the name-parsing loop to validate that the decoded ptr is strictly less
than the position where the pointer was read (i.e., pointer_target <
pointer_offset) to prevent self-referential/forward pointers, keep the existing
data bounds check for ptr < data.len(), and preserve the consumed/jumped/jumps
bookkeeping and jump cap; on violation return ParseError::InvalidName (use the
same symbols: ptr, pos, consumed, jumped, jumps, data, ParseError::InvalidName).
In `@src/service/record/update.rs`:
- Around line 62-91: The constraint reads and validation (calls to
RepositoryService::get_records_by_zone_id and
validate_record_update_constraints) must run inside the same database
transaction as the update to avoid TOCTOU races: begin the transaction first
with RepositoryService::begin_tx("Failed to update record").await?, then fetch
zone records using the transactional API (e.g., a tx-scoped method or
RepositoryService method that accepts the tx) instead of the pre-tx
get_records_by_zone_id call, build the candidate Record and call
validate_record_update_constraints(&zone, &zone_records, &existing_record,
&candidate_updated) while the tx is open, and only then perform the update and
commit; update references to use the tx object for all DB reads/writes around
the update.
- Around line 149-150: When building the IXFR ADD change row you currently use
update_record_request.record_type directly which can preserve mixed-case input;
instead parse/normalize it to the canonical enum representation and use the
enum's canonical string when populating zone_change. Locate the place
constructing the change row (where update_record_request.record_type is used)
and replace the direct field with a canonicalized value by parsing into your
RecordType enum (e.g., RecordType::from_str / TryFrom<&str>) and then using the
enum's to_string()/as_str() for the stored value, with a safe fallback (e.g.,
original string) on parse failure so you don’t leak mixed-case types into
zone_change.
In `@src/service/zone/create.rs`:
- Around line 20-30: The preflight uniqueness check using
RepositoryService::get_zone_by_name on create_zone_request is racy and must not
be relied on; remove or ignore that check and instead enforce uniqueness by
catching the database unique-constraint error at the insert/transaction point
(the code path that performs the zone insert) and map that specific DB violation
to ServiceError::BadRequest ("Zone with this name already exists") while
preserving other DB errors as ServiceError::Internal; update the create-zone
code paths (including the block around the insert transaction referenced in the
diff and the similar logic at lines 41-64) to detect the DB unique-violation
error returned by your DB client/ORM and return BadRequest accordingly.
---
Outside diff comments:
In `@src/cli/commands/get.rs`:
- Around line 116-125: The CLI currently panics when ZoneRow::from_json fails
(inside print_output_with_table closure and at the other similar site), so
replace the unwrap_or_else panic paths with graceful handling: call
ZoneRow::from_json and either filter_map the Result to skip failures (e.g., use
.ok()) for array branches or match the single-item branch to return an empty vec
(or propagate a CommandError) instead of panicking; update both occurrences that
use ZoneRow::from_json (the closure passed to print_output_with_table and the
similar block around lines 177-186) to either collect only successful ZoneRow
parses or convert parse errors into an Err result that the command returns.
Ensure you reference ZoneRow::from_json and print_output_with_table when making
the change.
---
Duplicate comments:
In `@src/dns/nsupdate/update.rs`:
- Around line 268-270: The current delete check uses eq_ignore_ascii_case
between target_value and record.value which incorrectly treats case-sensitive
RDATA (e.g., TXT) as case-insensitive; change the comparison to be type-aware:
when the record type (e.g., record.rtype / record.record_type) is a
case-sensitive type like TXT, compare using exact equality (record.value ==
target_value), otherwise use case-insensitive comparison for name-like types;
update the conditional around target_value and record.value to branch on the
record type and perform the appropriate equality check to ensure exact deletes
only remove truly matching RDATA.
In `@src/service/record/delete.rs`:
- Around line 20-69: The reads and validate_record_delete_constraints call must
run inside the same DB transaction as the delete to avoid race conditions; begin
the transaction earlier by calling RepositoryService::begin_tx("Failed to delete
record").await? before fetching the record/zone/zone_records, then use the
transaction-scoped repository methods (e.g. tx.get_record_by_id,
tx.get_zone_by_id, tx.get_records_by_zone_id) to load existing_record, zone and
zone_records, run validate_record_delete_constraints(&zone, &zone_records,
std::slice::from_ref(&existing_record)) while still holding tx, and only then
perform the delete and commit/rollback on tx so all existence/constraint checks
and the delete occur atomically.
---
Nitpick comments:
In `@src/socket/server/record.rs`:
- Around line 45-57: The handler currently calls ZoneService::find_by_id for
every record causing N+1 lookups; fix by memoizing zone names in a local HashMap
keyed by record.zone_id and reuse cached names when building each
GetRecordResponse (i.e. before calling ZoneService::find_by_id check the cache
for the zone_id, only call ZoneService::find_by_id when missing, store the
returned zone.name in the cache, then set rec_response.zone_name). Update the
loop that creates GetRecordResponse::from_record to consult this cache (use the
record.zone_id and rec_response.zone_name fields) so each zone is fetched at
most once.
🪄 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: 7aa86b3c-4676-49b2-8178-0b8b1f90628c
📒 Files selected for processing (22)
src/api/record.rssrc/api/zone.rssrc/cli/commands/delete.rssrc/cli/commands/get.rssrc/cli/output/table.rssrc/database/model/record.rssrc/dns/nsupdate/parser.rssrc/dns/nsupdate/update.rssrc/service/auth.rssrc/service/record/create.rssrc/service/record/delete.rssrc/service/record/get.rssrc/service/record/update.rssrc/service/repository.rssrc/service/token.rssrc/service/utils.rssrc/service/zone/create.rssrc/service/zone/get.rssrc/service/zone/update.rssrc/socket/server/record.rssrc/socket/server/zone.rstests/api/record.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/api/zone.rs
- src/service/auth.rs
- src/service/token.rs
- src/service/zone/get.rs
- src/service/utils.rs
- src/service/record/create.rs
- src/service/zone/update.rs
- src/service/repository.rs
Comment on lines
+324
to
+339
| let ptr = (((len as u16 & 0x3F) << 8) | data[pos + 1] as u16) as usize; | ||
| if ptr >= data.len() { | ||
| return Err(ParseError::InvalidName); | ||
| } | ||
|
|
||
| if !jumped { | ||
| consumed += 2; | ||
| jumped = true; | ||
| } | ||
|
|
||
| pos = ptr; | ||
| jumps += 1; | ||
| if jumps > data.len() { | ||
| return Err(ParseError::InvalidName); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
Reject forward compression pointers.
ptr is only bounds-checked here, so self-referential or forward pointers are still followed until the jump cap trips. DNS compression pointers must target a prior name, so this currently accepts malformed wire data and does extra work on invalid input.
🔧 Tighten pointer validation
let ptr = (((len as u16 & 0x3F) << 8) | data[pos + 1] as u16) as usize;
- if ptr >= data.len() {
+ if ptr >= data.len() || ptr >= pos {
return Err(ParseError::InvalidName);
}📝 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
| let ptr = (((len as u16 & 0x3F) << 8) | data[pos + 1] as u16) as usize; | |
| if ptr >= data.len() { | |
| return Err(ParseError::InvalidName); | |
| } | |
| if !jumped { | |
| consumed += 2; | |
| jumped = true; | |
| } | |
| pos = ptr; | |
| jumps += 1; | |
| if jumps > data.len() { | |
| return Err(ParseError::InvalidName); | |
| } | |
| continue; | |
| let ptr = (((len as u16 & 0x3F) << 8) | data[pos + 1] as u16) as usize; | |
| if ptr >= data.len() || ptr >= pos { | |
| return Err(ParseError::InvalidName); | |
| } | |
| if !jumped { | |
| consumed += 2; | |
| jumped = true; | |
| } | |
| pos = ptr; | |
| jumps += 1; | |
| if jumps > data.len() { | |
| return Err(ParseError::InvalidName); | |
| } | |
| continue; |
🤖 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 `@src/dns/nsupdate/parser.rs` around lines 324 - 339, The parser currently
follows compression pointers without rejecting forward or self-referential
pointers; update the pointer-handling logic in the name-parsing loop to validate
that the decoded ptr is strictly less than the position where the pointer was
read (i.e., pointer_target < pointer_offset) to prevent self-referential/forward
pointers, keep the existing data bounds check for ptr < data.len(), and preserve
the consumed/jumped/jumps bookkeeping and jump cap; on violation return
ParseError::InvalidName (use the same symbols: ptr, pos, consumed, jumped,
jumps, data, ParseError::InvalidName).
Comment on lines
+62
to
+91
| let zone_records = match RepositoryService::get_records_by_zone_id(zone.id).await { | ||
| Ok(records) => records, | ||
| Err(e) => { | ||
| log_error!("Failed to load zone records: {}", e); | ||
| return Err(ServiceError::Internal( | ||
| "Failed to update record".to_string(), | ||
| )); | ||
| } | ||
| }; | ||
|
|
||
| let candidate_updated = Record { | ||
| id: record_id, | ||
| name: update_record_request.name.clone(), | ||
| record_type: record_type.clone(), | ||
| value: update_record_request.value.clone(), | ||
| ttl: update_record_request.ttl, | ||
| priority: update_record_request.priority, | ||
| zone_id: zone.id, | ||
| created_at: existing_record.created_at, | ||
| }; | ||
|
|
||
| validate_record_update_constraints( | ||
| &zone, | ||
| &zone_records, | ||
| &existing_record, | ||
| &candidate_updated, | ||
| )?; | ||
|
|
||
| // Update record | ||
| let mut tx = RepositoryService::begin_tx("Failed to update record").await?; |
There was a problem hiding this comment.
Move constraint reads/validation into the same transaction as the update.
Line 62 through Line 88 compute update constraints before Line 91 starts the transaction. This creates a TOCTOU window where concurrent writes can invalidate the checked state before commit.
🤖 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 `@src/service/record/update.rs` around lines 62 - 91, The constraint reads and
validation (calls to RepositoryService::get_records_by_zone_id and
validate_record_update_constraints) must run inside the same database
transaction as the update to avoid TOCTOU races: begin the transaction first
with RepositoryService::begin_tx("Failed to update record").await?, then fetch
zone records using the transactional API (e.g., a tx-scoped method or
RepositoryService method that accepts the tx) instead of the pre-tx
get_records_by_zone_id call, build the candidate Record and call
validate_record_update_constraints(&zone, &zone_records, &existing_record,
&candidate_updated) while the tx is open, and only then perform the update and
commit; update references to use the tx object for all DB reads/writes around
the update.
Comment on lines
+149
to
+150
| record_type: update_record_request.record_type.clone(), | ||
| record_value: update_record_request.value.clone(), |
There was a problem hiding this comment.
Use canonical record type for IXFR ADD change rows.
Line 149 writes update_record_request.record_type directly. Mixed-case input can leak non-canonical type strings into zone_change, diverging from enum-normalized values.
Proposed fix
- record_type: update_record_request.record_type.clone(),
+ record_type: updated_record.record_type.to_string(),🤖 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 `@src/service/record/update.rs` around lines 149 - 150, When building the IXFR
ADD change row you currently use update_record_request.record_type directly
which can preserve mixed-case input; instead parse/normalize it to the canonical
enum representation and use the enum's canonical string when populating
zone_change. Locate the place constructing the change row (where
update_record_request.record_type is used) and replace the direct field with a
canonicalized value by parsing into your RecordType enum (e.g.,
RecordType::from_str / TryFrom<&str>) and then using the enum's
to_string()/as_str() for the stored value, with a safe fallback (e.g., original
string) on parse failure so you don’t leak mixed-case types into zone_change.
Comment on lines
+20
to
+30
| match RepositoryService::get_zone_by_name(&create_zone_request.name).await { | ||
| Ok(Some(_)) => { | ||
| log_error!("Zone with name {} already exists", create_zone_request.name); | ||
| return Err(ServiceError::BadRequest( | ||
| "Zone with this name already exists".to_string(), | ||
| )); | ||
| } | ||
| Ok(None) => (), | ||
| Err(e) => { | ||
| log_error!("Failed to check existing zone: {}", e); | ||
| return Err(ServiceError::Internal("Failed to create zone".to_string())); |
There was a problem hiding this comment.
Don't rely on the preflight existence check for uniqueness.
get_zone_by_name runs before the insert transaction, so two concurrent creates can both pass this check. The losing request will then either create a duplicate or surface as Internal depending on the database constraint. Enforce uniqueness at insert time and map that violation to BadRequest.
Also applies to: 41-64
🤖 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 `@src/service/zone/create.rs` around lines 20 - 30, The preflight uniqueness
check using RepositoryService::get_zone_by_name on create_zone_request is racy
and must not be relied on; remove or ignore that check and instead enforce
uniqueness by catching the database unique-constraint error at the
insert/transaction point (the code path that performs the zone insert) and map
that specific DB violation to ServiceError::BadRequest ("Zone with this name
already exists") while preserving other DB errors as ServiceError::Internal;
update the create-zone code paths (including the block around the insert
transaction referenced in the diff and the similar logic at lines 41-64) to
detect the DB unique-violation error returned by your DB client/ORM and return
BadRequest accordingly.
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.
Summary by CodeRabbit
New Features
CLI / UX
Configuration
[xfr]section replaced by[dns]; new nsupdate_tsig_key and updated secondary_addrs format.Documentation
Chores