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

Implement QU bit support in question encoder/decoder #94

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vulcainman
Copy link

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:

  • The encoder to parse properly both the class and the QU bit
  • THe decoder to encode a DNS packet with the QU bit set

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:

- The encoder to parse properly both the class and the QU bit
- THe decoder to encode a DNS packet with the QU bit set
This commit includes several fixes that addess lint issues introduced by
commit a7a0b78
The code using this constant was removed by the commit
a7a0b78
@vulcainman
Copy link
Author

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
Copy link
Collaborator

Can you add a test for it?

@vulcainman
Copy link
Author

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

@vulcainman
Copy link
Author

I've added 2 tests as suggested.

@codecov-commenter
Copy link

Codecov Report

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

❗ 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%) ⬇️

@vulcainman
Copy link
Author

Any comment, update?

@silverwind
Copy link
Collaborator

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

@vulcainman
Copy link
Author

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
Copy link
Collaborator

This seems like it does the same as #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
Copy link
Author

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
Copy link
Author

Is there any chance to merge ?

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