You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shiro.apache.org by GitBox <gi...@apache.org> on 2020/03/30 19:01:50 UTC
[GitHub] [shiro] bmhm opened a new pull request #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
bmhm opened a new pull request #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209
…f values
- Replace key-value-splitting with regex
- obscure disappearing escaping chars - please discuss!
Signed-off-by: Benjamin Marwell <bm...@gmail.com>
Now. If we would not want to break the existing (imho: obscure) way that shiro parses the `shiro.ini` file, we could introduce a parser strategy pattern.
But as the existing behaviour was not documented (other than tests), I would like to see a discussion about this.
Tests broken on purpose! See here why: [SHIRO-530](https://issues.apache.org/jira/browse/SHIRO-530)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [shiro] bmhm closed pull request #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
bmhm closed pull request #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [shiro] fpapon commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
fpapon commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209#issuecomment-606203267
We have some users that are using backslash in the key:
https://issues.apache.org/jira/browse/SHIRO-684
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [shiro] bmhm commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
bmhm commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209#issuecomment-606210911
?? Why introduce a new future? If you look at the tests, this was never supported and those always got removed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [shiro] fpapon commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
fpapon commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209#issuecomment-606454311
+1 for me, if we think it's not possible (ini specs) to have \\ in the key, just throw an exception.
@bmhm Thanks for taking care of this ;)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [shiro] bmhm edited a comment on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
bmhm edited a comment on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209#issuecomment-606678292
I am not so sure about this commit. It would be possible, in theory, that someone misused the escaping backslashes (which disappeared from the value) as indendation:
```ini
key = value\
\\1
```
In the old word, this was `"key" => "value1"`, but in the new one it would be "value\\\\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
With regards,
Apache Git Services
[GitHub] [shiro] bmhm commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
bmhm commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209#issuecomment-606678292
I am not so sure about this commit. It would be possible, in theory, that someone misused the escaping backslashes (which disappeared from the value) as indendation:
```ini
key = value\
\\1
```
In the old word, this was `"key" => "value1"`, but in the new one it would be "value\\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
With regards,
Apache Git Services
[GitHub] [shiro] bmhm edited a comment on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
bmhm edited a comment on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209#issuecomment-606678292
I am not so sure about this commit. It would be possible, in theory, that someone misused the escaping backslashes (which disappeared from the value) as indendation:
```ini
key = value\
\\1
```
In the old word, this was `"key" => "value1"`, but in the new one it would be "value\\\\1".
But then, it was never documented.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [shiro] bmhm edited a comment on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
bmhm edited a comment on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209#issuecomment-606185214
TODO:
- [x] document capture groups
- [x] remove `\\` from first capture group. It just makes no sense. Disappearing is unexpected. Or just let it disappear from the key only?
- [ ] to make it more similar to the existing behaviour, we could just strip backsashes from the key. `final String key = matcher.group(1).replaceAll("\\\\", "");`.
- [ ] Please explain this test (especially it's purpose). It makes no sense imho. This would be the only test failing then.
```java
test = "Truth\\=Beauty";
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
With regards,
Apache Git Services
[GitHub] [shiro] bmhm edited a comment on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
bmhm edited a comment on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209#issuecomment-606185214
TODO:
- [x] document capture groups
- [x] remove `\\` from first capture group. It just makes no sense. Disappearing is unexpected. Or just let it disappear from the key only?
- [x] to make it more similar to the existing behaviour, we could just strip backsashes from the key. `final String key = matcher.group(1).replaceAll("\\\\", "");`.
- [ ] Please explain this test (especially it's purpose). It makes no sense imho. This would be the only test failing then.
```java
test = "Truth\\=Beauty";
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
With regards,
Apache Git Services
[GitHub] [shiro] bmhm commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
bmhm commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209#issuecomment-606412522
> As for `"Truth\\=Beauty"`, I would _expect_ that that to be some sort of error or at least a key with no value similar to:
Thanks for the input @bdemers, I had the very same thoughts! The new force-pushed commit:
* Will throw an IllegalArgumentException for `Key\\=Value`.
* Will (as before!) remove all backslashes from the key.
* Will *not* (new behaviour) remove any backslashes from the value. The continuation backslash not included.
Actually, what @fpapon pointed out by posting the link: The XML config DOES allow backslashes in the key, where the Ini never allowed to do so. Please review my new commit which reflects these changes. I moved the tests a bit and commented them, so we can see what would change.
Please keep in mind: Neither the current nor the desired/expected behaviour is currently documented, not even in the code! It is *just* the tests!
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [shiro] bdemers commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
bdemers commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209#issuecomment-606253816
From SHIRO-530, it looks like we have:
|example (as input) | result (as java string) |
|--------|------|
| `key=value\` | `key=value\n` |
| `key=value\\` | `key=value\\` |
| `key=value\\\` | `key=value\\` |
| `key=value\\\\` | `key=value\\\\` |
I would _expect_ the result to be:
|example (as input) | result (as java string) |
|--------|------|
| `key=value\` | `key=value\n` |
| `key=value\\` | `key=value\\` |
| `key=value\\\` | `key=value\\\n` |
| `key=value\\\\` | `key=value\\\\` |
As for `"Truth\\=Beauty"`, I would _expect_ that that to be some sort of error or at least a key with no value similar to:
```ini
[foobar]
one=one
justAKeyWithoutValue
```
I'm not saying this is the way it does function, or _should_ just what I personally would expect. (not sure if that helps or hurts this conversation)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [shiro] bmhm edited a comment on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
bmhm edited a comment on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209#issuecomment-606185214
TODO:
- [ ] document capture groups
- [x] remove `\\` from first capture group. It just makes no sense. Disappearing is unexpected. Or just let it disappear from the key only?
- [ ] to make it more similar to the existing behaviour, we could just strip backsashes from the key. `final String key = matcher.group(1).replaceAll("\\\\", "");`.
- [ ] Please explain this test (especially it's purpose). It makes no sense imho. This would be the only test failing then.
```java
test = "Truth\\=Beauty";
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
With regards,
Apache Git Services
[GitHub] [shiro] bmhm commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
bmhm commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209#issuecomment-607189167
Superseeded by https://github.com/apache/shiro/pull/210
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [shiro] bmhm commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
Posted by GitBox <gi...@apache.org>.
bmhm commented on issue #209: [SHIRO-530] INI parser does not properly handled backslashes at end o…
URL: https://github.com/apache/shiro/pull/209#issuecomment-606185214
TODO:
- [ ] document capture groups
- [ ] remove `\\` from first capture group. It just makes no sense. Disappearing is unexpected. Or just let it disappear from the key only?
- [ ] to make it more similar to the existing behaviour, we could just strip backsashes from the key. `final String key = matcher.group(1).replaceAll("\\\\", "");`.
- [ ] Please explain this test (especially it's purpose). It makes no sense imho. This would be the only test failing then.
```java
test = "Truth\\=Beauty";
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
With regards,
Apache Git Services