search

mercure-imaging / mercure

mercure DICOM Orchestrator
https://mercure-imaging.org
MIT License
65 stars 31 forks source link

Unreachable endpoint when module name contains a slash #64

Closed LennyN95 closed 9 months ago

LennyN95 commented 10 months ago

Describe the bug I noticed the following behaviour: the UI currently allows the usage of / characters e.g. module names. However, these module names are unable to be edited or deleted from the web GUI as the slash is not encoded and thus interpreted as part of the URL, consequently showing a Not Found error.

Huge thanks to the developers for building promising open-source tools like Mercure!

To Reproduce Steps to reproduce the behavior:

  1. Navigate to http://localhost:8000/modules/
  2. Create new module, e.g. use name Test/MyModel
  3. Click Edit
  4. URL resolves to http://localhost:8000/modules/edit/Test/MyModel which will be "Not Found"

Expected behavior Either prohibit the use of unencoded characters or better use URL encoding for model names.

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

Smartphone (please complete the following information):

Additional context Add any other context about the problem here.

RoyWiggins commented 9 months ago

The UI uses a pattern attribute on the input control to validate the module name, though there's no server-side validation. I'm not sure how Firefox let you submit the form in the first place, honestly- I can't replicate the issue.

image

LennyN95 commented 9 months ago

Hey Roy, thx for picking this up. I'll look into this again and see if I can provide any valuable insights into why the front-end restriction didn't apply in my case.

I suggest re-consider implementing backend checks on all user inputs. UI restrictions are more of a UIX feature than reliable (and, of course, not secure).

tblock79 commented 9 months ago

Seems that there was a bug in the regex expression used for restricting the input pattern (the - char was not escaped). This is fixed now, and also added a check on the backend.

Fixed with commit 3ed0a3d4932d77cef5976831809b968d1848f17c