#4856 #4825: cname record validations by DaisyGuti · Pull Request #4955 · cisagov/manage.get.gov

DaisyGuti · 2026-04-27T13:52:33Z

Ticket

Changes

Added CNAME name ≠ hostname validation to DnsRecord.clean() — previously only enforced at the form level
Refactored clean() to resolve the zone domain name once instead of once per validator (eliminates duplicate DB hit)
Fixed logger.error(f"...") → logger.exception(...) in vendor data methods so tracebacks are preserved
Added model- and form-level tests for all shorthand cases (@, bare label, FQDN)
Add multi-field validations for CNAME record form #4825 Banner: Name and target can't be the same.
Add multi-field validations for CNAME record form #4825 Inline name: Name can't be the same as the target.
Add multi-field validations for CNAME record form #4825 Inline target: Target can't be the same as the record name.

Context for reviewers

This branch builds from work already done in dg/4779-no-dup-name-dns-records, which added form-level CNAME name != hostname validation. That PR left a gap: DnsRecord.clean() never enforced the same constraint, so it could be bypassed at the model level. This PR does that. No migrations needed.

Setup

Ensure the DNS hosting waffle flag is ON and the test domain is enrolled. On sandbox (e.g. DG) or local with DNS_MOCK_EXTERNAL_APIS=False:

Go to /domain/<id>/dns/records on a DNS-enrolled domain (e.g. daisyepp.gov if on DG)
Create a CNAME record where the name resolves to the same hostname as the content (e.g. name www, content www.daisyepp.gov)
Confirm Add multi-field validations for CNAME record form #4825:
- A single banner appears at the top: Name and target can't be the same.
- The name field shows: Name can't be the same as the target.
- The target field shows: Target can't be the same as the record name.
Repeat using shorthand forms — @ with content daisyepp.gov, and bare label sub with content sub.daisyepp.gov — and confirm each is rejected

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

Met the acceptance criteria, or will meet them in a subsequent PR
Created/modified automated tests
Update documentation in READMEs and/or onboarding guide

Ensured code standards are met (Original Developer)

If any updated dependencies on Pipfile, also update dependencies in requirements.txt.
Interactions with external systems are wrapped in try/except
Error handling exists for unusual or missing values

Validated user-facing changes (if applicable)

Follow the process for requesting a design review. If code is not user-facing, delete design reviewer checklist
Verify new pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
Checked keyboard navigability
Reviewed accessibility checklist using screen reader (such as NVDA with Chrome or Voiceover with Safari), ANDI, or WAVE:
- Tested general usability
- Page header structure
- Landmarks
- Links and buttons
Checked for errors or warnings with an a11y browser tool (such as ANDI or WAVE)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Pulled this branch locally and tested it
Verified code meets all checks above. Address any checks that are not satisfied
Reviewed this code and left comments. Indicate if comments must be addressed before code is merged
Checked that all code is adequately covered by tests
Verify migrations are valid and do not conflict with existing migrations

Validated user-facing changes as a developer

Note: Multiple code reviewers can share the checklists above, a second reviewer should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A.

As a designer reviewer, I have

Verified that the changes match the design intention

Checked that the design translated visually
Checked behavior. Comment any found issues or broken flows.
Checked different states (empty, one, some, error)
Checked for landmarks, page heading structure, and links

Validated user-facing changes as a designer

References

Code review best practices

Screenshots

erinysong · 2026-05-01T00:02:19Z

Tried making a CNAME record with the same name and hostname and was blocked by the new clean() updates 🎉 . Interestingly the error appears at the name field whereas when the error is caught at the form field, the error appears for the hostname field.

erinysong · 2026-05-04T19:11:34Z

@@ -1,6 +1,5 @@
 """Forms for domain management."""

-import logging


didn't realize we weren't logging anything in forms - thank you for removing this!

… for cname on the form. Cleaned up the file a bit. Tiny refactor to minimize db calls.

DaisyGuti · 2026-05-06T12:41:34Z

Wires the existing CNAME "name must not equal hostname" check into the model's clean() so it runs on every save. Also handles the shorthand (@ expands to the domain, www expands to www.example.gov) so users can't bypass the check by typing the name in a different format. Pulls the zone's domain name lookup into a single helper (_resolve_domain_name) so it isn't queried twice per clean().

DaisyGuti · 2026-05-06T12:42:08Z

Adds a test class covering the form-level CNAME self-loop check across the three name shapes users can submit: FQDN, bare label, and @, plus the happy path. Also enables the previously-disabled CNAME entries in the shared test fixtures so the generic form-validation tests now cover CNAME alongside A/AAAA/MX.

DaisyGuti · 2026-05-06T12:42:56Z

Mirrors the form tests at the model layer by exercising DnsRecord.clean() directly. Confirms CNAME self-loops are rejected for every name shape and that the check is skipped when it shouldn't fire — empty content, or non-CNAME types where name == content is ok. Guards against regressions if a future code path saves a record while bypassing the form.

Love the tests!

DaisyGuti · 2026-05-06T12:44:28Z

                    vendor_dns_record=vendor_dns_record,
                )

-        except Exception as e:


logger.exception automatically attaches the traceback of the currently-handled exception. The old logger.error(f"...{str(e)}") only logged the exception's string form (e.g. "connection refused") — you got the what but not the where. With .exception you can see the exact line that blew up, which is what you actually need to debug a vendor-API failure.

chaswick · 2026-05-07T20:52:30Z

When testing I noticed that if i didn't use the same case between the two fields, I would get no message (I do see a response error 400 in the javascript console). This was on -dg with all-trade-president.gov

chaswick

Per comments below and the behavior i observed in sb, I believe those items should be addressed before approval.

chaswick · 2026-05-07T17:40:45Z

+        """CNAME record whose FQDN name equals its content should fail clean()."""
+        from django.core.exceptions import ValidationError
+
+        record = DnsRecord(


Nice. Would it make sense to also or in addition test case sensitivity, e.g. 'name=' and 'content=' are the same but different cases? name="sub.dns-test.gov" content="Sub.dns-test.gov"

chaswick · 2026-05-07T17:41:06Z

Love the tests!

chaswick · 2026-05-07T17:43:21Z

+
+    HOSTNAME_ERROR = "CNAME record hostname must not match record name."
+
+    def test_cname_fqdn_name_matches_content_raises_content_error(self):


Nice. Would it make sense to also or in addition test case sensitivity, e.g. data["name"] and "CNAME", <> are the same but different cases? Like "sub.dns-test.gov" and "Sub.dns-test.gov" for example.

erinysong · 2026-05-07T22:18:01Z

            errors["priority"] = [DNS_RECORD_PRIORITY_REQUIRED_ERROR_MESSAGE]

-    def _validate_exclusive_names(self, record_type, errors):
+    def _resolve_domain_name(self) -> str | None:


erinysong · 2026-05-07T22:19:22Z

        self.VALID_CONTENT_BY_TYPE = {
            "A": "192.0.2.10",
            "AAAA": "2001:db8::1234:5678",
+            "CNAME": "www.example.com",


glad we can use our unit tests for these now! thank you for getting them to work!

erinysong

Tested and LGTM on my end! Small observation that when name == hostname, the validation error happens on the name field on admin but on the content field on the registrar. I don't think it's significant enough to block though but happy to consider otherwise

DaisyGuti · 2026-05-08T12:14:18Z

Nice catch, thanks! @chaswick Fixed. Ready for rev.

…orm + model

DaisyGuti · 2026-05-08T12:24:18Z

When testing I noticed that if i didn't use the same case between the two fields, I would get no message (I do see a response error 400 in the javascript console). This was on -dg with all-trade-president.gov

Just a heads up this work is not on dg. Notice no [dg] in the PR title. You can test locally or on your env. dg is currently being used by design to verify a different pr.

…sg for

DaisyGuti · 2026-05-08T13:44:31Z

It was a simple add that built on this work this PR Resolves #4856 & #4825 @erinysong @chaswick

github-project-automation Bot added this to .gov Product Board Apr 27, 2026

DaisyGuti marked this pull request as draft April 27, 2026 14:09

DaisyGuti marked this pull request as ready for review April 27, 2026 14:26

DaisyGuti moved this to 🎯 Ready in .gov Product Board Apr 27, 2026

erinysong self-assigned this Apr 27, 2026

katypies removed the status in .gov Product Board Apr 28, 2026

katypies moved this to 👀 In review in .gov Product Board Apr 28, 2026

lizpearl assigned chaswick May 4, 2026

erinysong reviewed May 4, 2026

View reviewed changes

Added _validate_cname_name_not_hostname and tests. Completed the todo…

3d2162e

… for cname on the form. Cleaned up the file a bit. Tiny refactor to minimize db calls.

DaisyGuti force-pushed the dg/4856_cname_record_validations branch from ba337aa to 3d2162e Compare May 6, 2026 12:38

DaisyGuti commented May 6, 2026

View reviewed changes

Merge branch 'main' into dg/4856_cname_record_validations

2c77470

chaswick requested changes May 7, 2026

View reviewed changes

erinysong reviewed May 7, 2026

View reviewed changes

erinysong approved these changes May 7, 2026

View reviewed changes

DaisyGuti and others added 2 commits May 8, 2026 08:15

Fix case-sensitive CNAME hostname check; lowercase normalization at f…

073b655

…orm + model

Merge branch 'main' into dg/4856_cname_record_validations

0dae472

#4825: Updated the error messaing and condensed the top banner to 1 m…

19c0b0c

…sg for

DaisyGuti mentioned this pull request May 8, 2026

Add multi-field validations for CNAME record form #4825

Open

4 tasks

DaisyGuti changed the title ~~#4856: cname record validations~~ #4856 #4825: cname record validations May 8, 2026

DaisyGuti requested a review from erinysong May 8, 2026 13:44

chaswick approved these changes May 8, 2026

View reviewed changes

		@@ -1,6 +1,5 @@
		"""Forms for domain management."""

		import logging


		HOSTNAME_ERROR = "CNAME record hostname must not match record name."

		def test_cname_fqdn_name_matches_content_raises_content_error(self):

Conversation

DaisyGuti commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

Changes

Context for reviewers

Setup

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

Ensured code standards are met (Original Developer)

Validated user-facing changes (if applicable)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Validated user-facing changes as a developer

As a designer reviewer, I have

Verified that the changes match the design intention

Validated user-facing changes as a designer

References

Screenshots

Uh oh!

erinysong commented May 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chaswick commented May 7, 2026

Uh oh!

chaswick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erinysong left a comment

Choose a reason for hiding this comment

Uh oh!

DaisyGuti commented May 8, 2026

Uh oh!

DaisyGuti commented May 8, 2026

Uh oh!

DaisyGuti commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DaisyGuti commented Apr 27, 2026 •

edited

Loading

DaisyGuti commented May 8, 2026 •

edited

Loading