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

Special characters in folder name break the folder #3

Open
jonpavelich opened this issue Sep 10, 2018 · 7 comments
Open

Special characters in folder name break the folder #3

jonpavelich opened this issue Sep 10, 2018 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@jonpavelich
Copy link

Summary
Some URL special characters (&, /, etc.) are allowed in folder names, and render the folder inaccessible.

Environment
Version: Release v0.1
Python: Anaconda 3.6.5
Set up using Normal Method on Debian Stretch
Running gunicorn behind Nginx as described in README.md

Reproduction Steps

  1. Create a new folder with an alphanumeric name (e.g. Recipes)
  2. Add URLs to the folder
  3. From Home, rename the folder (Select -> Rename)
  4. Enter a name which contains a forward slash (/) or ampersand (&) (e.g. Recipes & Cooking)
  5. Observe the rename succeeding, and the new name appearing properly on the Home page
  6. Attempt to open the folder to view the URLs inside

Expected Behaviour
The folder should open as expected, and the special characters should be properly escaped in the page URL

Observed Behaviour
The page fails to load (HTTP 404), as the special characters present in the URL are not escaped and are invalid (e.g. Recipes & Cooking above tries loading as https://myserver.example/myusername/Recipes & Cooking)

Recovery
I was able to repair my installation by editing the database manually and replacing all instances of the invalid folder name with a valid one in the directory column of the page_library table.

Notes
It would be nice if these characters were allowed (and properly escaped so the page continues to load), but if this is infeasible then they should be disallowed in the folder name field to prevent breaking an installation.

I'd be happy to dig in to the project and contribute a pull request if you'd like. Thanks for all your work on this project so far, it's awesome!

@kanishka-linux
Copy link
Owner

kanishka-linux commented Sep 10, 2018

Forward slash ( '/' ) is certainly not allowed at the moment, since it will break url path. But it is possible to include '&', by changing url regex pattern. You are free to make changes and submit pull request with small test case.

By the way, about recovery process, I think, it is possible to rename invalid names from web-interface itself instead of manually editing database.

For a time being, I'll add note on invalid characters in the README.

this project so far, it's awesome!

It's good to hear that people are finding this project useful.

@jonpavelich
Copy link
Author

Thanks for the response. I just want to confirm that I tried renaming an invalid folder, and it doesn't work because the rename (.../myusername/Recipes & Cooking/rename) and remove (.../myusername/Recipes & Cooking/remove) URLs also 404. It would probably be worthwhile to check for special characters on submit and display an error message until they can be properly supported.

@kanishka-linux
Copy link
Owner

kanishka-linux commented Sep 11, 2018

This is strange.

As per urlpattern, 'Recipes & Cooking/rename' is completely allowed. I just checked for both rename and remove urls. Both are working completely fine.

Can you post version of django? Reminiscence works properly only with django v2.0+

@kanishka-linux kanishka-linux added the bug Something isn't working label Sep 14, 2018
@jonpavelich
Copy link
Author

The result of pip freeze in my virtual environment is:

aiohttp==3.3.2
amqp==2.3.2
async-timeout==3.0.0
attrs==18.1.0
beautifulsoup4==4.6.3
billiard==3.5.0.4
bs4==0.0.1
celery==4.2.1
chardet==3.0.4
cssselect==1.0.3
Django==2.1
django-widget-tweaks==1.4.2
gunicorn==19.9.0
idna==2.7
idna-ssl==1.1.0
kombu==4.2.1
lxml==4.2.4
multidict==4.3.1
nltk==3.3
psycopg2==2.7.5
pytz==2018.5
readability-lxml==0.7
redis==2.10.6
six==1.11.0
vine==1.1.4
yarl==1.2.6

@kanishka-linux
Copy link
Owner

kanishka-linux commented Sep 18, 2018

Django version is fine.
But I'm not able to reproduce the bug related to rename/remove!

@kanishka-linux
Copy link
Owner

By the way I've added support for '&' in devel branch.

Supporting every special character including '/' will require lot of changes in codebase including urlpatterns.

@jonpavelich
Copy link
Author

jonpavelich commented Sep 20, 2018

Let me know if there's any additional logs I can submit to help reproduce!

One suggestion for allowing all special characters is to use slugs, and store it along side the "pretty" title. Use the slug in the URL, but the pretty title in the list of folders shown to the user. You can automatically generate the slug from whatever title the user chooses. I'll work on this and put up a PR if you're interested.

Alternatively, I think just blocking invalid characters from being submitted as a valid name would help a lot. Perhaps the built in Regex Validator could be used.

Also glad to see '&' is now supported, that's a big one for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants