You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@shiro.apache.org by GitBox <gi...@apache.org> on 2021/03/13 07:38:30 UTC
[GitHub] [shiro] fpapon opened a new pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
fpapon opened a new pull request #286:
URL: https://github.com/apache/shiro/pull/286
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] fpapon merged pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
fpapon merged pull request #286:
URL: https://github.com/apache/shiro/pull/286
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] fpapon commented on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
fpapon commented on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-821057486
@todmorrison thanks for your feedback, I will take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] fpapon edited a comment on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
fpapon edited a comment on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-797884192
Fix this https://github.com/apache/shiro/commit/0c424d37e5d542844c9ed65cf962498211d2334b#commitcomment-47294324
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] fpapon commented on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
fpapon commented on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-798839427
I have one test failed but do we want this to be possible?
```
@Test
public void testSplitKeyValueEscapedEquals() {
String test = "Truth\\=Beauty";
String[] kv = Ini.Section.splitKeyValue(test);
assertEquals("Truth", kv[0]);
assertEquals("Beauty", kv[1]);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] charliemblack commented on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
charliemblack commented on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-812679826
I tested out the change with an LDAP integration I have with Apache Geode. The fix works like a champ! Thanks for doing this.
If I could request this to be back-ported to 1.7.x since that is what Apache Geode is on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] fpapon commented on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
fpapon commented on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-816712040
Ok I will cherrypick to 1.7.x branch and add info in the release note.
Thanks for your feedback!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] fpapon commented on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
fpapon commented on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-797884192
Fix this https://github.com/apache/shiro/commit/0c424d37e5d542844c9ed65cf962498211d2334b
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] todmorrison edited a comment on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
todmorrison edited a comment on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-819829577
This is a cleaner way and should fix up the "escaped escape-char" case (i.e. handle "//" as a single "/")
```
if (buildingKey) {
// Given that isCharEscaped actually means isEscapeChar, the middle case is always
// false.
// if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i) && !isCharEscaped(line, i-1)) {
if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i-1)) {
buildingKey = false;//now start building the value
// Allow escaped escape character -- adjusting for the misleading naming of isCharEscaped
} else if (!isCharEscaped(line, i) || !isCharEscaped(line, i-1) ){
keyBuffer.append(c);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] todmorrison edited a comment on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
todmorrison edited a comment on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-819829577
This is a cleaner way and should fix up the "escaped escape-char" case (i.e. handle "//" as a single "/")
```
if (buildingKey) {
// Given that isCharEscaped actually means isEscapeChar, the middle case is always false.
if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i) && !isCharEscaped(line, i-1)) {
if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i-1)) {
buildingKey = false;//now start building the value
} else if (!isCharEscaped(line, i) || !isCharEscaped(line, i-1) ){ // Allow escaped escape character
keyBuffer.append(c);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bmarwell commented on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-798887811
> I have one test failed but do we want this to be possible?
>
> ```
> @Test
> public void testSplitKeyValueEscapedEquals() {
> String test = "Truth\\=Beauty";
> String[] kv = Ini.Section.splitKeyValue(test);
> assertEquals("Truth", kv[0]);
> assertEquals("Beauty", kv[1]);
> }
> ```
For 2.0: no. For me the equals is escaped.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] todmorrison commented on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
todmorrison commented on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-819829577
This is a cleaner way and should fix up the "escaped escape-char" case (i.e. handle "//" as a single "/")
` if (buildingKey) {
// Given that isCharEscaped actually means isEscapeChar, the middle case is always
// false.
// if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i) && !isCharEscaped(line, i-1)) {
if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i-1)) {
buildingKey = false;//now start building the value
// Allow escaped escape character -- adjusting for the misleading naming of isCharEscaped
} else if (!isCharEscaped(line, i) || !isCharEscaped(line, i-1) ){
keyBuffer.append(c);
}
`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] todmorrison edited a comment on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
todmorrison edited a comment on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-819829577
This is a cleaner way and should fix up the "escaped escape-char" case (i.e. handle "//" as a single "/")
```
if (buildingKey) {
// Given that isCharEscaped actually means isEscapeChar, the middle case is always true.
// if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i) && !isCharEscaped(line, i-1)) {
if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i-1)) {
buildingKey = false;//now start building the value
} else if (!isCharEscaped(line, i) || !isCharEscaped(line, i-1) ){ // Allow escaped escape character
keyBuffer.append(c);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
bdemers commented on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-816702021
IIRC, @bmarwell added some tests with the current behavior in: b50b829a285b106666b688a4b69a3ebee94f51b4
```java
public void testSplitKeyValueEscapedEquals() {
String test = "Truth\\=Beauty";
String[] kv = Ini.Section.splitKeyValue(test);
assertEquals("Truth", kv[0]);
assertEquals("Beauty", kv[1]);
}
```
The above was existing behavior. Though the more I look at it this must have been a side effect/bug. I cannot imagine why you would want this behavior, I would have expected `Truth\\=Beauty` to be evaluated to `Truth\=Beauty` with a `null` value, or possibly a runtime parsing error if the value is required.
I'm guessing this might have been the result of the key and value logic sharing the same escape logic.
TL;DR, I think this is a bug, and we should fix it in any version. (We do need to make this clear in the release notes, anyone caught by this _should_ be able to process the INI file/string to strip out the odd `\\=` if needed.
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] fpapon commented on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
fpapon commented on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-823354595
@todmorrison I'm not sure to understand your code because our test failed. Did you take a look on our utests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bmarwell commented on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-816707696
+1. Cherry pick into 1.7.x, remove the test. This behaviour can be restored easily by removing one backslash and does not look intentional.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bmarwell edited a comment on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
bmarwell edited a comment on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-798887811
> I have one test failed but do we want this to be possible?
>
> ```
> @Test
> public void testSplitKeyValueEscapedEquals() {
> String test = "Truth\\=Beauty";
> String[] kv = Ini.Section.splitKeyValue(test);
> assertEquals("Truth", kv[0]);
> assertEquals("Beauty", kv[1]);
> }
> ```
For 2.0: no, we do not want to retain such irrational behaviour. For me, the equals sign is escaped.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bmarwell commented on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-816469684
> If I could request this to be back-ported to 1.7.x since that is what Apache Geode is on.
I am not sure we can backport this, as this is a breaking change. See https://github.com/apache/shiro/pull/286#issuecomment-798887811.
OTOH, I cannot imagine someone has ever used a kv-pair like this intentionally.
@fpapon WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] todmorrison commented on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
todmorrison commented on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-823424307
Hi @fpapon,
You're right. I partially amend my comment. The first part, changing:
```if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i) && !isCharEscaped(line, i-1)) ```
to
```if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i-1))```
is correct. The middle case is redundant since `isKeyValueSeparatorChar(c)` where `c = line.charAt(i)` means the character at `i` is "=" and certainly not "\".
For the second part, `(!isCharEscaped(line, i) || !isCharEscaped(line, i-1) `, this fails the test for "Tru\\th=Beauty" = "Truth", which still doesn't look right to me but my "fix" doesn't solve the problem either. Specifically, I feel that backslashes should be allowed in the key (and possibly other escape sequences), but this is harder than I originally thought. (A project for another day).
Anyway, I do feel my observation that the middle case in the first conditional is redundant is worth fixing. I withdraw the second change though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] todmorrison edited a comment on pull request #286: [SHIRO-812] Key value separator in config is broken with escape char
Posted by GitBox <gi...@apache.org>.
todmorrison edited a comment on pull request #286:
URL: https://github.com/apache/shiro/pull/286#issuecomment-819829577
This is a cleaner way and should fix up the "escaped escape-char" case (i.e. handle "//" as a single "/")
```
if (buildingKey) {
// Given that isCharEscaped actually means isEscapeChar, the middle case is always true.
if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i) && !isCharEscaped(line, i-1)) {
if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i-1)) {
buildingKey = false;//now start building the value
} else if (!isCharEscaped(line, i) || !isCharEscaped(line, i-1) ){ // Allow escaped escape character
keyBuffer.append(c);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org