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

If you keep writing weird runes, I'll be back... #5539

Merged

Conversation

rustyrussell
Copy link
Contributor

First commit is a drive-by fix for msggen Makefiles @cdecker (since it doesn't cover commando-rune yet, it's not required, but I was trying to figure out why msggen didn't re-run before I understtood that).

Fixes #5474

The last commit deprecates the specification of alternatives in restrictions as "a|b", requiring a ["a", "b"] JSON array. This also means no more having to escape | and &, but means you we had to require arrays for the transition so we can detect deprecated usage. @jb55 WDYT?

@rustyrussell rustyrussell added this to the v22.10 milestone Aug 19, 2022
@rustyrussell rustyrussell changed the title Guilt/commando weird rune If you keep writing weird runes, I'll be back... Aug 19, 2022
@rustyrussell rustyrussell requested a review from jb55 August 19, 2022 04:47
@jb55
Copy link
Collaborator

jb55 commented Aug 19, 2022

interesting workaround... was a bit confusing at first but works for me!

@jb55
Copy link
Collaborator

jb55 commented Aug 20, 2022

I was worried about some of the videos I have out on commando being invalidated by this... but I'm putting together a web tool for constructing runes (https://jb55.com/runes) so perhaps it's not a big deal and people can just use that.

@cdecker
Copy link
Member

cdecker commented Aug 25, 2022

Rebased on top of master to get the macos CI fix again.

1. It depends on both request and reply schemas.
2. Wildcards aren't natively expanded in make, so use $(wildcard).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We didn't handle \ in fields properly, unless they were one-char long.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
JSON needs to escape \, since it can't be in front of anything unexpected,
so no \|.  So we need to return \\ to \, and in theory handle \n etc.

Changelog-Fixed: JSON-RPC: `commando-rune` now handles \\ escapes properly.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This avoids having to escape | or &, though we still allow that for
the deprecation period.

To detect deprecated usage, we insist that alternatives are *always*
an array (which could be loosened later), but that also means that
restrictions must *always* be an array for now.

Before:

```
# invoice, description either A or B
lightning-cli commando-rune '["method=invoice","pnamedescription=A|pnamedescription=B"]'
# invoice, description literally 'A|B'
lightning-cli commando-rune '["method=invoice","pnamedescription=A\\|B"]'
```

After:

```
# invoice, description either A or B
lightning-cli commando-rune '[["method=invoice"],["pnamedescription=A", "pnamedescription=B"]]'
# invoice, description literally 'A|B'
lightning-cli commando-rune '[["method=invoice"],["pnamedescription=A|B"]]'
```

Changelog-Deprecated: JSON-RPC: `commando-rune` restrictions is always an array, each element an array of alternatives.  Replaces a string with `|`-separators, so no escaping necessary except for `\\`.
@rustyrussell
Copy link
Contributor Author

I was worried about some of the videos I have out on commando being invalidated by this... but I'm putting together a web tool for constructing runes (https://jb55.com/runes) so perhaps it's not a big deal and people can just use that.

They also have 6 months, ideally...

@rustyrussell
Copy link
Contributor Author

Rebased again...

@cdecker
Copy link
Member

cdecker commented Sep 14, 2022

ACK 1d53827

@cdecker cdecker merged commit a6d4756 into ElementsProject:master Sep 14, 2022
@jb55
Copy link
Collaborator

jb55 commented Sep 14, 2022 via email

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.

commando-rune: jsonrpc escaped | in restrictions don't work
3 participants