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

x/text/secure/precis: Bidi rule is applied incorrectly #17383

Closed
SamWhited opened this issue Oct 8, 2016 · 4 comments
Closed

x/text/secure/precis: Bidi rule is applied incorrectly #17383

SamWhited opened this issue Oct 8, 2016 · 4 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@SamWhited
Copy link
Member

SamWhited commented Oct 8, 2016

Currently in PRECIS profiles that use the bidi rule (RFC 5893 §2) such as the username profiles, the bidi rule is always applied regardless of whether RTL characters exist in the input string. However, RFC 7613 §3.3.2 seems to state that the bidi rule should not be applied unless RTL characters exist in the string:

Directionality Rule: Applications MUST apply the "Bidi Rule"
defined in [RFC5893] to strings that contain right-to-left
characters
(i.e., each of the six conditions of the Bidi Rule
must be satisfied).

This wording is a bit confusing (I started looking into this when someone asked on the IETF list for clarification of the above paragraph), but the recent draft-ietf-precis-7613bis-03 clarifies the language:

Directionality Rule: Apply the "Bidi Rule" defined in [RFC5893]
to strings that contain right-to-left characters (i.e., each of
the six conditions of the Bidi Rule must be satisfied); for
strings that do not contain right-to-left characters, there is no
special processing for directionality.

However, I think we may be doing it right and changing it to match the RFC is not entirely advisable because it means usernames that include RTL characters are not consistent with usernames that do not (eg. "!Username" is valid, but "!User<RTLChar>name" is not valid because the bidi rule does not allow punctuation [category NO] at the beginning of a label). I've written to the working group to see if this should be changed in the draft to match our implementation. I'm not sure if it would be better to go ahead and update to be compliant with the current RFC, or if it would be better to wait and see if the rule changes (or if I'm just completely wrong; someone with more knowledge about this than me will likely reply on the list and explain why it doesn't make sense to apply the bidi rule all the time).

/cc @mpvl @nigeltao

@quentinmit quentinmit added this to the Unreleased milestone Oct 9, 2016
@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 9, 2016
@SamWhited
Copy link
Member Author

SamWhited commented Oct 10, 2016

Got some feedback about this from one of the PRECIS authors today; I was misunderstanding the punctuation rules and it sounds like there's a lot of punctuation is itself an RTL character (which is the only punctuation allowed at the beginning or end of an RTL label), so it definitely makes sense to only apply the bidi rule for labels that contain RTL characters; my mistake.

@SamWhited
Copy link
Member Author

This is covered by https://go-review.googlesource.com/c/30553 (which I apparently looked at, reviewed, and forgot about before filing this issue).

@SamWhited
Copy link
Member Author

Going ahead and closing this since gobot doesn't appear to want to cross link it and I don't think the issue will close automatically when the CL is merged. Sorry for the noise.

@gopherbot
Copy link

CL https://golang.org/cl/30553 mentions this issue.

gopherbot pushed a commit to golang/text that referenced this issue Oct 12, 2016
This also fixes issues in secure/precis (the tests make a lot more
sense now).

- Rules only apply to Bidi Domain names. Currently the user had
to test this. This test is now embedded in the bidi test.

- Direction* now reports the direction as defined in the RFC,
instead of using the Unicode definition.

- Added Valid and ValidString functions. This is much more useful
and intuitive if the user does not require a transformer.

Fixes golang/go#17383

Change-Id: Id7be15e48bbf36581ce23347575eb88226e12841
Reviewed-on: https://go-review.googlesource.com/30553
Run-TryBot: Marcel van Lohuizen <mpvl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Nigel Tao <nigeltao@golang.org>
@golang golang locked and limited conversation to collaborators Oct 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

3 participants