mafintosh / dns-packet

An abstract-encoding compliant module for encoding / decoding DNS packets
MIT License
201 stars 70 forks source link

Implement QU bit support in question encoder/decoder #94

Open vulcainman opened 11 months ago

vulcainman commented 11 months ago

This commit fix current QU bit implementation that may be used in mDNS protocol. Indeed, until now, if a packet with QU bit set is received, it is decoded as:

{

...

questions: [ { name: '...', type: 'PTR', class: 'UNKNOWN_32769' }, { name: '...', type: 'PTR', class: 'UNKNOWN_32769' } ],

...

}

Instead of :

{

...

questions: [ { name: '...', type: 'PTR', class: 'IN' }, { name: '...', type: 'PTR', class: 'IN' } ],

...

}

This commit adds a proper QU bit support via the qu_bit field. It enables:

vulcainman commented 11 months ago

Hi @mafintosh,

First of all, thanks for this great module that saved me a lot of time! Is there any change to merge this pull request?

Best

silverwind commented 11 months ago

Can you add a test for it?

vulcainman commented 11 months ago

Of course, I'll propose something today or early next week.

vulcainman commented 11 months ago

I've added 2 tests as suggested.

codecov-commenter commented 11 months ago

Codecov Report

Merging #94 (52a86ce) into master (7b66620) will decrease coverage by 0.01%. The diff coverage is 100.00%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
- Coverage   97.99%   97.99%   -0.01%     
==========================================
  Files           6        6              
  Lines        2046     2043       -3     
==========================================
- Hits         2005     2002       -3     
  Misses         41       41              
Files Coverage Δ
index.js 98.06% <100.00%> (-0.01%) :arrow_down:
vulcainman commented 10 months ago

Any comment, update?

silverwind commented 10 months ago

I don't feel qualified to say whether this is correct, maybe @mafintosh.

vulcainman commented 10 months ago

The only thing I can say is that it works as expected in our professional project. Basically, it is used to forward and modify on the fly mdns traffic between 2 lans (one modification is to enable qu_bit flag).

silverwind commented 10 months ago

This seems like it does the same as https://github.com/mafintosh/dns-packet/pull/96, except that PR uses QU instead of qu_bit. I think I would prefer it to be named just qu.

Any opinion on which PR is better @XiXiangFiles, @vulcainman?

vulcainman commented 10 months ago

I would say that, for code lint reasons, it would be better to have a lowercased member. Except that point, IMO it doesn't matter since it something like qu, qu_bit qu_flag, bit_qu flag_qu.

vulcainman commented 9 months ago

Is there any chance to merge ?