Skip to content

Encode true as xFF #142

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

Merged
merged 4 commits into from
Oct 24, 2014
Merged

Encode true as xFF #142

merged 4 commits into from
Oct 24, 2014

Conversation

mtodd
Copy link
Member

@mtodd mtodd commented Oct 22, 2014

According to the spec (http://tools.ietf.org/html/rfc4511#section-5.1), we're currently encoding true incorrectly as \x01\x01\x01 instead of \x01\x01\xFF.

Fixes #31.

Wouldn't mind finding a good integration test for this, but nothing obvious or direct comes to mind.

cc @jch @schaary

mtodd added 2 commits October 22, 2014 00:18
We allow this since the library requires Ruby 1.9+
@mtodd
Copy link
Member Author

mtodd commented Oct 22, 2014

Ruby 1.8.7 has been retired for well over a year. Moving ahead, we should assume 1.9+ only.

Once this is merged and released, it will mark the first release that does not support ~1.8.7. We should make that explicit by setting appropriate Gemspec flags.

This was referenced Oct 22, 2014
@jch
Copy link
Member

jch commented Oct 22, 2014

ruby 1.9.3+ requirement enforced in #145

@jch
Copy link
Member

jch commented Oct 22, 2014

I think this is sufficiently covered by the search integration test since the default size limit is '0'.to_ber. If your encoding was wrong, then search would break. Yes, the test is implicit, but it's such a common value it'd be pretty obvious if it broke.

@mtodd
Copy link
Member Author

mtodd commented Oct 22, 2014

@jch size isn't a boolean, though, since this PR only applies to the BER encoding of the true value. This would apply for the "criticality" flag of a search control, and few other places (not much boolean usage in the spec from what I've seen).

@mtodd
Copy link
Member Author

mtodd commented Oct 22, 2014

Luckily for us, I think all we have to do is use a non-existing sort control and set the criticality to true, then assert that the response is "unavailable critical extension" (result code 12).

@jch
Copy link
Member

jch commented Oct 22, 2014

Oops, my brain just autocompleted boolean to 0 and 1, but ya what you said makes sense.

@mtodd
Copy link
Member Author

mtodd commented Oct 24, 2014

@jch ready for your review.

@jch
Copy link
Member

jch commented Oct 24, 2014

🚢

mtodd added a commit that referenced this pull request Oct 24, 2014
@mtodd mtodd merged commit 7b801fc into master Oct 24, 2014
@mtodd mtodd deleted the true-ber branch October 24, 2014 23:39
astratto pushed a commit to astratto/ruby-net-ldap that referenced this pull request Dec 18, 2015
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.

Proper BER value for "true"
2 participants