search

eigerco / lumina

Wasm friendly Celestia light node implementation in Rust
Apache License 2.0
118 stars 33 forks source link

Fix/empty bearer token #395

Closed msuiche closed 1 day ago

msuiche commented 1 week ago

If the token passed to the function is not None, it would still add it as a Bearer and there is no sanity check in case the token is invalid and it will try to pass empty tokens as a token.

zvolin commented 6 days ago

I'm not sure I like this behavior. If token was Some but didn't pass some validation then I think it should return an error rather than being silently skipped. On the other hand, I'm not sure if it's worth it, it would still allow stuff like Some("a"). Imo leaving validation of token for celestia-node, whatever it was, is okay

msuiche commented 5 days ago

Makes sense. It's now returning an error and I also added a unit test for it.

fl0rek commented 1 day ago

hey @msuiche, we have few concerns: The returned error variant (ProtocolNotSupported), doesn't exactly match what's happening. Additionally, this change filters one kind of invalid token (empty string), while still alowing other malformed ones (e.g. newlines in the string, there are probably other disallowed characters or obviously wrong values).

We've discussed this internally and think that covering all the possible edge cases will be cumbersome, in addition to while providing little value - server is going to validate the token anyway. I think we'd rather treat the token as a black box passed between the user and the server. Closing, but of course feel free to open a discussion.