-
Notifications
You must be signed in to change notification settings - Fork 43
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
fix[ws]: handle unsupported paths and message types #592
base: main
Are you sure you want to change the base?
Conversation
aa3470f
to
4d071b8
Compare
|
||
async def connect(self): | ||
await self.accept() | ||
await self.send('{"error": "invalid path"}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alex-Izquierdo Should we reveal this, it might be better to just close the connection
wsapi_router = URLRouter( | ||
[path("ansible-rulebook", consumers.AnsibleRulebookConsumer.as_asgi())] | ||
[ | ||
path("ansible-rulebook", consumers.AnsibleRulebookConsumer.as_asgi()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alex-Izquierdo @bzwei has a PR which will put authentication on this path #540
@Alex-Izquierdo |
Hi @bzwei, I agree. Middleware seems like a good approach too, right now I don't fully understand how that middleware works, and even if we copy it, it probably should have its own tests. Since you are more familiar with this, if you really trust on that code, I have no problem using it, otherwise I would think the current approach is also simpler and effortless. |
If this is not urgently needed, I can try out the middleware solution. |
@bzwei Yeah, I don't think it is critical since ansible-rulebook is the only real client of the websockets. Just I noticed that and I thought I could address it easily. |
We are not handling WS requests for unsupported paths having a 500 unhandled exception
Also I have found a bug where we are not handling unsupported message types.
Jira: https://issues.redhat.com/browse/AAP-19294