MBS-13886: Don't crash on wrong link_type for paged rels #3434
+9
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mwiencek
reviewed
Jan 7, 2025
| my $link_type = $c->model('LinkType')->get_by_id($link_type_id); | ||
| unless ($link_type) { | ||
| $c->stash( | ||
| message => l('The provided relationship type does not exist'), |
There was a problem hiding this comment.
We have the following string in root/components/RelationshipsTable.js which makes it sound like this case should be handled already: The provided relationship type ID is not valid.. Erroring seems fine, but perhaps we can use the existing string and remove that unused fallback.
There was a problem hiding this comment.
Oh yeah, I guess that fallback doesn't actually work because the linkPhrase calculation crashes before we can check this. Wonder if it originally worked? I actually even tried to find an existing string to reuse and failed, sigh. Thanks!
This was hit and logged on Sentry (leads to a TypeError on JS). I assume someone was just playing around with the URL params, but it seems to make sense to block this at the controller level and since I cannot think of any other way of getting here than the user fiddling 400 seems like a good fit. We were trying to do this at the JS level earlier, but it crashes when trying to calculate the linkPhrase we were hoping to check (possibly a new issue after some changes, or possibly it never worked) so it never gets to return the error. It seems sensible in any case to check on the Perl layer and just not bother with any of the loading if there's a clear broken situation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix MBS-13886
Problem
Trying to access a paged relationships list with a non-existing link type, such as
/work/8dcffe3f-cb5c-3459-b6d7-0558a2174808?direction=2&link_type_id=277&page=1, causes a JSTypeError. Found in Sentry.Solution
I assume someone was just playing around with the URL params and I cannot think of any other way of getting here than the user fiddling like that. As such, it seems to make sense to block this at the controller level and a 400 BAD REQUEST seems like a good fit.
Testing
Manually, on the URL above.