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

Nameplates should always be numbers #45

Open
meejah opened this issue Aug 13, 2024 · 7 comments
Open

Nameplates should always be numbers #45

meejah opened this issue Aug 13, 2024 · 7 comments
Labels
enhancement New feature or request

Comments

@meejah
Copy link
Member

meejah commented Aug 13, 2024

It seems possible (see e.g. magic-wormhole/magic-wormhole.rs#193) for nameplates to be "whatever", which is clearly not the intent.

The server should enforce nameplates to be numbers, and not accept anything else. It looks like the database stores them as strings, and that can remain I suppose, but the intent of the protocol docs seems clearly that these are numbers.

I can't think of any reason for them not to be numbers, and the Python / reference client + server always chooses numbers.

@meejah meejah added the enhancement New feature or request label Aug 13, 2024
@felinira
Copy link

I don't think there is much of an issue in specifying this, and it makes a lot of sense IMO. We should just make sure to support non-number nameplates for a while on the server side, as we don't know if they are in widespread use anywhere. I'll add some error handling in the rust code, and we can then slowly migrate away from supporting this.

@meejah
Copy link
Member Author

meejah commented Aug 25, 2024

I don't directly control when new versions are deployed in public (as @warner runs the public Mailbox and Transit relay) but yeah it seems like it would be "least disruptive" to start issuing a "warning" kind of error before being rude (and closing sessions).

@warner
Copy link
Collaborator

warner commented Aug 29, 2024

Huh, yeah, it never even occurred to me that someone might use a non-number, but you're absolutely right that the protocol doesn't reject it. I'd be fine with it being rejected. I guess we'll need new warning code in one release, then wait a few months, then switch to rejecting them.

Although.. I'm not sure we've got a good pathway to warn without rejecting. The server can respond to the initial hello with a welcome message, and the CLI's handle_welcome() will print a motd message if it contains one (at least the python client will), but it normally sends that well before the client sends a CLAIM with the nameplate.

I'm pretty sure the server can respond with an error at any time, and the client will print it (or at least it gets embedded in the exception that gets raised).

But I can't find a more general-purpose "hey client, please show this string to you user" message that the server can send after the welcome step.

@meejah
Copy link
Member Author

meejah commented Aug 29, 2024

We could implement a non-invasive approach: the next release can start logging any client that sends a non-numeric nameplate, and you could publish the number of such connections after some period of time?

Perhaps in a similar vein, a statistical approach could fail 10% of "non-number CLAIM" messages with a hard error; if there are clients in the wild using non-number mailboxes, they wouldn't "break" completely (on average, a retry would succeed) but their users might notice?

Edit: this is kind of what "brownouts" look like for API changes, right?

@meejah
Copy link
Member Author

meejah commented Aug 29, 2024

(That said, I agree with @warner that there doesn't appear to be a great way to "warn now" for clients that are using not-integer nameplates).

Maaaaybe we could add a "mood" string? Technically, the current document says what are valid "mood" strings, and doesn't demand leniency for unknown ones. However, it might cause clients to notice (if they got a "non-numberic-mailbox-id" or something mood back, at the end of such a connection).

@felinira
Copy link

felinira commented Sep 3, 2024

I'm not sure adding a mood string is backwards compatible. I wouldn't be surprised if it would cause some clients to terminate the connection anyway.

As far as I know there aren't any clients that rely on this behaviour, at most there are some that allow it.

For dealing with fallout after the notice period I would like to quote Douglas Adams

There’s no point in acting surprised about it. All the planning charts and demolition orders have been on display at your local planning department in Alpha Centauri for 50 of your Earth years, so you’ve had plenty of time to lodge any formal complaint and it’s far too late to start making a fuss about it now. … What do you mean you’ve never been to Alpha Centauri? Oh, for heaven’s sake, mankind, it’s only four light years away, you know. I’m sorry, but if you can’t be bothered to take an interest in local affairs, that’s your own lookout. Energize the demolition beams.

@meejah
Copy link
Member Author

meejah commented Sep 3, 2024

I'm not sure adding a mood string is backwards compatible. I wouldn't be surprised if it would cause some clients to terminate the connection anyway.

Yeah, that would be "the" potential big downside.

These messages should always come at the end, so even clients that protocol-errored or panic'd would still be "okay" in the sense that they have already successfully concluded at that point (at least, concluded the file-transfer part).

....and yeah, "deprecation" always ends up at the "oh crap, it was actually changed" at some point 😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants