Report allowlist errors in path selector #4645
Conversation
Bubballoo3
changed the title
github-project-automation
bot
moved this from Awaiting Review
to Merged/Closed
in PR Review Pipeline
| const msg = err.message; | ||
| var prefix = ''; | ||
| if (msg.match('ALLOWLIST_PATH')) { | ||
| prefix = 'Permission denied: ' |
There was a problem hiding this comment.
Looks like this is missing a semi-colon here, though the file doesn't "use strict"; it should/will eventually.
There was a problem hiding this comment.
I have often seen that a single line conditional like this (already contained by brackets) doesn't use a semicolon, because there is no danger of ambiguity over where the line ends. Not sure what 'use strict' would have to say about that, but that was the justification in my mind when I did it. Happy to change if you want me to
There was a problem hiding this comment.
You're probably right, it's just at a glance it looks off. Though if you think about it it's fine, I'd just prefer not to have to think about it.
There was a problem hiding this comment.
That's a good enough reason for me
github-project-automation
bot
moved this from Awaiting Review
to Merged/Closed
in PR Review Pipeline
3 participants
Fixes #4264, the error reporting existed before, but attempted to match text in the error message that had since been removed. These changes remove that check and report any errors that are encountered. It also changes the error modal from warning to danger (color), to be consistent with the modal in the files app.
This PR does not address the second point brought up in the issue, that you are unable to navigate across the allowlist since this would always require visiting a directory not on the allowlist. The files app gives an out for this by allowing you to manually type a path, but given the limited size of the path selector modal I think that would add too much clutter (and is a poor solution for users anyway). I think the best solution for that would be to add the allowlist to the favorites, checking for duplicates in the supplied favorites. This could be restricted to the path selector or extend to the files app as well, but ought to be its own issue.
A system test was added that attempts to violate the allowlist and checks that errors are being reported properly. It tests two methods of leaving the allowlist, by clicking the up arrow in the modal, and by clicking the breadcrumb elements. As far as I can tell this is the only way to leave the allowlist, but I would be happy to add more if I am missing something. Either way, the checks for the allowlist happen outside of the path selector, so the errors should be the same regardless of the method of reaching them.
I did discover one latent bug due to the reporting, when you click the breadcrumb for the current directory (which lacks the 'clickable' class), it makes an incomplete request to the server and produces a generic "Not Acceptable" error. This was fine when errors were unreported, because you stay in the same directory and never notice that a request was made. We should either ignore these errors at the path selector or truly disable the breadcrumb, the former approach would likely fit well in this PR while the other could be its own.