Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TXT record encoding #36

Merged
merged 2 commits into from
Nov 27, 2023
Merged

Fix TXT record encoding #36

merged 2 commits into from
Nov 27, 2023

Conversation

szekelyisz
Copy link
Contributor

♻️ Current situation

When the advertised service has no TXT data, the RDATA generated is invalid. RFC 1035 3.3.14. mandates TXT RDATA be "One or more <character-string>s". Currently ciao generates zero <character-string>s if there is no TXT data associated with the service.

In this case other server implementations seem to return a TXT RDATA containing one <character-string> of zero length which is valid according to the RFC.

Most client implementations gracefully handle the invalid data. iOS 17 does not, so currently services advertised by ciao are not discoverable on iOS 17.

💡 Proposed solution

Follow other server implementations' behavior: return one <character-string> having length of 0. this.txt is assigned accordingly in TXTRecord constructor.

⚙️ Release Notes

Fix TXT record encoding according to RFC 1035.

Testing

No changes.

Reviewer Nudging

Oneliner! ;)

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7009623595

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 35.636%

Totals Coverage Status
Change from base Build 7009617504: 0.03%
Covered Lines: 1200
Relevant Lines: 3048

💛 - Coveralls

Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 🚀

@Supereg Supereg merged commit a7a4d66 into homebridge:master Nov 27, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants