Permit star entry #3407

Merged
merged 3 commits into from Feb 20, 2016

Projects

None yet

3 participants

@cmouse
Contributor
cmouse commented Feb 19, 2016

No description provided.

@cmouse
Contributor
cmouse commented Feb 19, 2016

@zeha can you review please?

@zeha
Collaborator
zeha commented Feb 19, 2016

NAK, this function is called for both zone names and full qnames. To fix this properly, please have a new function that only deals with qnames and splits those first to ensure that only the left-most part can be a (single) star.

@cmouse
Contributor
cmouse commented Feb 19, 2016

@zeha is this better?

@zeha
Collaborator
zeha commented Feb 19, 2016

Yes, but I guess I'd put the whitelist in a global const and again use find_first_not_of instead of hand-coding the loop.

@cmouse
Contributor
cmouse commented Feb 19, 2016

the hand-coded loop also moves the ptr forward...

@zeha
Collaborator
zeha commented Feb 19, 2016

I don't see why you actually need that. The check could probably be "if (leftmost2 == "*.") pos += 2;" + find_first_not_of and done?

@zeha
Collaborator
zeha commented Feb 19, 2016

You've short-circuited the need for splitting with the if lefmost2 == ". anyway :)

@cmouse
Contributor
cmouse commented Feb 19, 2016

I did think of just using this alternatively, you can choose

void apiCheckQNameAllowedCharacters(const string& qname) {
  if (qname.compare(0, 2, "*.") == 0) apiCheckNameAllowedCharacters(qname.substr(2));
  else apiCheckNameAllowedCharacters(qname);
}
@zeha
Collaborator
zeha commented Feb 19, 2016

That probably works as well, but copies the string :-)

@cmouse
Contributor
cmouse commented Feb 19, 2016

so we go with a1844c1 or 860e89c? I reduced the packet copying to occur with star only.

@zeha
Collaborator
zeha commented Feb 19, 2016

LGTM

@cmouse
Contributor
cmouse commented Feb 19, 2016

Ready for merge once CI passes.

@zeha
Collaborator
zeha commented Feb 19, 2016

Also - please add a test for this...

@cmouse
Contributor
cmouse commented Feb 20, 2016

test added.

@Habbie Habbie merged commit 148b82c into PowerDNS:master Feb 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment