StevenWeathers / thunderdome-planning-poker

⚡ Thunderdome is an open source agile planning poker, sprint retro, and story mapping tool
https://thunderdome.dev
Apache License 2.0
396 stars 103 forks source link

extend WebSocket origin check to allow APP_DOMAIN #550

Closed gwimmel closed 3 months ago

gwimmel commented 3 months ago

Description

This PR extends the WebSocket origin check by also allowing APP_DOMAIN as origin

What type of PR is this? (check all applicable)

Related Tickets & Documents

Fixes #522

Steps to QA

Can be tested using a reverse proxy setup (I could provide one if necessary).

gwimmel commented 3 months ago

Here's a first attempt, only for the poker client, which works for my use case. Doesn't allow for subdomains (yet). Can you provide some feedback before I possibly change the other clients? Is there a reason why the value for APP_DOMAIN in the Dockerfile/Makefile starts with "." (i.e., .127.0.0.1), whereas the default doesn't ("thunderdome.dev")?

StevenWeathers commented 3 months ago

Here's a first attempt, only for the poker client, which works for my use case. Doesn't allow for subdomains (yet). Can you provide some feedback before I possibly change the other clients? Is there a reason why the value for APP_DOMAIN in the Dockerfile/Makefile starts with "." (i.e., .127.0.0.1), whereas the default doesn't ("thunderdome.dev")?

So the Makefile commands are for local development, I've actually got a PR in motion to do auth and I've changed the .127.0.0.1 to localhost and things seem to continue to function, I had issues at one point with localhost and cookies or something however it seems to be working just fine now. As far as defaults go they're just that, defaults for convenience and expected to be changed per installation in real environments.

StevenWeathers commented 3 months ago

I tested this locally and it seems to work just fine, I would suggest just copy pasting the logic to each respective features websocket code for now as there is another ticket for abstraction which will introduce an internal/websocket package for all the common parts.

gwimmel commented 3 months ago

I tested this locally and it seems to work just fine, I would suggest just copy pasting the logic to each respective features websocket code for now as there is another ticket for abstraction which will introduce an internal/websocket package for all the common parts.

Great. I've copied the logic to the other features, please take another look.

gwimmel commented 3 months ago

Here's a first attempt, only for the poker client, which works for my use case. Doesn't allow for subdomains (yet). Can you provide some feedback before I possibly change the other clients? Is there a reason why the value for APP_DOMAIN in the Dockerfile/Makefile starts with "." (i.e., .127.0.0.1), whereas the default doesn't ("thunderdome.dev")?

So the Makefile commands are for local development, I've actually got a PR in motion to do auth and I've changed the .127.0.0.1 to localhost and things seem to continue to function, I had issues at one point with localhost and cookies or something however it seems to be working just fine now. As far as defaults go they're just that, defaults for convenience and expected to be changed per installation in real environments.

Thanks for the explanation. I mainly wondered about the leading "." (i.e., ".127.0.0.1", not "127.0.0.1", which would have been what I'd have expected).

StevenWeathers commented 3 months ago

Run goimports -w .