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

Broken INTERNAL link triggered whenever someone is removed from a meeting #582

Open
jonespm opened this issue Nov 7, 2024 · 4 comments · May be fixed by #585
Open

Broken INTERNAL link triggered whenever someone is removed from a meeting #582

jonespm opened this issue Nov 7, 2024 · 4 comments · May be fixed by #585
Assignees

Comments

@jonespm
Copy link
Member

jonespm commented Nov 7, 2024

Whenever a user is removed from a meeting by the host by clicking the trash can an email is triggered to us. This is because code in the api doesn't have a trailing slash for the removeMeeting and it's getting a 301 redirect. I'm not sure why a 301 redirect is causing us to get emails though.

So there's 2 problems here:

  1. Fix the removeMeeting fetch to have a trailing slash. This should avoid the 301 entirely

const resp = await fetch(`/api/meetings/${meeting_id}`, {

  1. Figure out why 301's (URL's without a trailing slash) are internally being treated as 404s.

https://github.com/zqian/remote-office-hours-queue/blob/064ae0cd2ce33f42c9d51efa8c961c9f2628cd09/src/officehours_api/middleware.py

I found this old issue that was fixed 9 years ago in Django that kind of relates to this, but I didn't see it in in Django 3 before we upgraded to Django 4.

To testing: Email's aren't sent for this locally, however there will be a log message from middleware.py stating "Super class BrokenLinkEmailsMiddleware handling 404 broken link". This URL pattern is allowed explicitly by this and could possibly also be denied, however when it checks
isinstance(response, HttpResponseNotFound): then response matches for this 301.

On the test server, emails are sent out.

@lsloan
Copy link
Member

lsloan commented Nov 7, 2024

One possibility for resolving the problem with trailing slash would be to change the pattern for the API endpoint(s).

websocket_urlpatterns = [
re_path(r'ws/queues/(?P<queue_id>\w+)/$', consumers.QueueConsumer.as_asgi()),
re_path(r'ws/users/(?P<user_id>\w+)/$', consumers.UserConsumer.as_asgi()),
]

It requires a slash, but if that could be made optional, then maybe the redirect wouldn't be required. If the style of regular expression used here is similar in syntax to common REs, then maybe replace /$ with /+$, indicating one optional slash.

@lsloan lsloan self-assigned this Nov 7, 2024
@lsloan
Copy link
Member

lsloan commented Nov 8, 2024

Note that we have been receiving emails from "root@localhost" since at least April 2024. Some days there are near 200 messages and other days there are few or none. Yesterday there were well over 200, which is probably what brought this to our attention, in conjunction with us actively looking for problems after the deployment.

There haven't been any problem reports from users about this since April.

@jonespm
Copy link
Member Author

jonespm commented Nov 8, 2024

There's definitely something different about these than the ones before. I've received over 200 emails since this update was rolled out.

Prior to this the last email I see is back from June.

The pattern for these now is

Referrer: https://officehours.it.umich.edu/manage/###
Requested URL: /api/meetings/######

The ones in the past didn't have this referrer. It would be nice to figure out what changed and why these are becoming 404's with this but the fix is likely straightforward.

I don't think those websocket url's are the ones that are causing this issue. I believe these are from DRF. Here's some discussion about how that might be achieved but I don't think it's necessary since we can just fix the single call in the frontend without changing how the backend handles trailing slashes.

@jonespm
Copy link
Member Author

jonespm commented Nov 8, 2024

I added some more information and test plan steps to the initial description. The filtering for which emails are sent to us was changed a bit in #477 but we didn't see this increase in the last 1.10.0 release.

lsloan added a commit to lsloan/rohq that referenced this issue Nov 12, 2024
After this, requesting endpoints with a trailing slash results in a HTTP 404 (not found) error.  Following this change, it's important to check other parts of the application to ensure they do not use a trailing slash when calling API endpoints.
@lsloan lsloan linked a pull request Nov 12, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants