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

Update rootfs/api/urls.py #164

Merged
merged 1 commit into from
Mar 8, 2022
Merged

Conversation

eudalov
Copy link
Contributor

@eudalov eudalov commented Feb 14, 2022

Fix for the issue #162.

Copy link
Member

@Cryptophobia Cryptophobia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this will work. We can't use @ character in valid route DNS?

DNS names can contain only alphabetical characters (A-Z), numeric characters (0-9), the minus sign (-), and the period (.). Period characters are allowed only when they are used to delimit the components of domain style names

@eudalov
Copy link
Contributor Author

eudalov commented Feb 21, 2022

@Cryptophobia , the same regexp is used in the same file, for another URL processing:

url(r'^admin/perms/(?P<username>[\w.@+-]+)/?$',

It is used for parameter checking but not for the hostname.

@Cryptophobia
Copy link
Member

@eudalov , it looks like in that first case the regex actually works. But here it seems to still fail? Any reason why that would be?

https://regex101.com/r/t5pza9/1

@eudalov
Copy link
Contributor Author

eudalov commented Feb 22, 2022

@Cryptophobia , well, it's because the regular expression [-_\w.@+]+ applies only to the parameter username (the URL part coming after perms/ and untill next /) but not the whole URL:
Screenshot_488
Here it is a Django docs about the syntax used in case with urlpatterns and regexps: https://docs.djangoproject.com/en/4.0/topics/http/urls/#using-regular-expressions.

Copy link
Member

@Cryptophobia Cryptophobia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@Cryptophobia Cryptophobia merged commit 7b3c004 into teamhephy:master Mar 8, 2022
@eudalov eudalov deleted the patch-1 branch March 8, 2022 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants