tl-its-umich-edu / remote-office-hours-queue

Virtual queuing tool supporting Zoom video conferencing and/or in person meetings.
https://officehours.it.umich.edu/
Apache License 2.0
12 stars 28 forks source link

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

Open jonespm opened 2 weeks ago

jonespm commented 2 weeks ago

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 removeMeetingand 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 removeMeetingfetch to have a trailing slash. This should avoid the 301 entirely

https://github.com/tl-its-umich-edu/remote-office-hours-queue/blob/777a0addf41806380d24694ea2d059dafb58ffec/src/assets/src/services/api.ts#L164

2) 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 commented 2 weeks ago

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

https://github.com/tl-its-umich-edu/remote-office-hours-queue/blob/777a0addf41806380d24694ea2d059dafb58ffec/src/officehours_api/routing.py#L5-L8

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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.