vikunja/frontend
Archived
6
39
Fork 48
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

Allow unknown protocols in project's description links #3758

Closed
jackymancs4 wants to merge 2 commits from jackymancs4/vikunja-frontend:feature/allow-unknown-protocols into main
pull from: jackymancs4/vikunja-frontend:feature/allow-unknown-protocols
merge into: vikunja:main
Conversation 9 Commits 2 Files Changed 1 +1 -1
jackymancs4 commented 2023-10-04 08:15:17 +00:00
Contributor
Copy Link

Hello,
sometimes I include inside a project's description a link to the related Joplin note, in the format:

[note](joplin://x-callback-url/openNote?id=4e8cf76f426c47f39b3f9893a9a5883b)

Unfortunately, it get rendered as an empty link.

I think this PR is an acceptable security compromise since the project description can be modified only by an authenticated user,
which should have full responsibility of what he put in the description.

Docs

https://github.com/cure53/DOMPurify#control-permitted-attribute-values
https://joplinapp.org/external_links/

Hello, sometimes I include inside a project's description a link to the related Joplin note, in the format: ``` [note](joplin://x-callback-url/openNote?id=4e8cf76f426c47f39b3f9893a9a5883b) ``` Unfortunately, it get rendered as an empty link. I think this PR is an acceptable security compromise since the project description can be modified only by an authenticated user, which should have full responsibility of what he put in the description. ## Docs https://github.com/cure53/DOMPurify#control-permitted-attribute-values https://joplinapp.org/external_links/
jackymancs4 added 1 commit 2023-10-04 08:15:40 +00:00
Initial work
All checks were successful
continuous-integration/drone/pr Build is passing
Details
41cbf8b43f
frederick commented 2023-10-04 08:20:16 +00:00
Member
Copy Link

Hi jackymancs4!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3758-feature-allow-unknown-protocols--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi jackymancs4! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3758-feature-allow-unknown-protocols--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
konrad requested changes 2023-10-04 08:34:30 +00:00
konrad left a comment
Owner

Thanks for the PR!

Can you extend this to task descriptions and comments?

We should filter out schemes like javascript:, vbscript: and data: as they can directly execute code.
Even if the people editing text are authenticated, they are not always users (link shares can send comments or edit a task description) and everyone can see the description. A malicious attacker could create a project/task on an instance their users trust, then send them the link, and they would click on it.

Gitea got bitten by this in one of their more recent releases: https://blog.gitea.com/release-of-1.20.1/#%EF%B8%8F-any-url-scheme-may-be-used-for-links-24805

Thanks for the PR! Can you extend this to task descriptions and comments? We should filter out schemes like `javascript:`, `vbscript:` and `data:` as they can directly execute code. Even if the people editing text are authenticated, they are not always users (link shares can send comments or edit a task description) and everyone can _see_ the description. A malicious attacker could create a project/task on an instance their users trust, then send them the link, and they would click on it. Gitea got bitten by this in one of their more recent releases: https://blog.gitea.com/release-of-1.20.1/#%EF%B8%8F-any-url-scheme-may-be-used-for-links-24805
jackymancs4 commented 2023-10-04 18:39:03 +00:00
Author
Contributor
Copy Link

Can you extend this to task descriptions and comments?

Will do.

Gitea got bitten by this in one of their more recent releases: https://blog.gitea.com/release-of-1.20.1/#%EF%B8%8F-any-url-scheme-may-be-used-for-links-24805

Most interesting.

We should filter out schemes like javascript:, vbscript: and data: as they can directly execute code.

Do you happen do be a RegEx wizard?
I'm having difficulties adapting the default regex /^(?:(?:(?:f|ht)tps?|mailto|tel|callto|sms|cid|xmpp|xxx):|[^a-z]|[a-z+.\-]+(?:[^a-z+.\-:]|$))/i to be used in ALLOWED_URI_REGEXP in a negative (exclusive) way

>Can you extend this to task descriptions and comments? Will do. > Gitea got bitten by this in one of their more recent releases: https://blog.gitea.com/release-of-1.20.1/#%EF%B8%8F-any-url-scheme-may-be-used-for-links-24805 Most interesting. > We should filter out schemes like javascript:, vbscript: and data: as they can directly execute code. Do you happen do be a RegEx wizard? I'm having difficulties adapting the default regex `/^(?:(?:(?:f|ht)tps?|mailto|tel|callto|sms|cid|xmpp|xxx):|[^a-z]|[a-z+.\-]+(?:[^a-z+.\-:]|$))/i` to be used in ALLOWED_URI_REGEXP in a negative (exclusive) way
konrad commented 2023-10-05 07:44:38 +00:00
Owner
Copy Link

Do you happen do be a RegEx wizard?
I'm having difficulties adapting the default regex /^(?:(?:(?:f|ht)tps?|mailto|tel|callto|sms|cid|xmpp|xxx):|[^a-z]|[a-z+.-]+(?:[^a-z+.-:]|$))/i to be used in ALLOWED_URI_REGEXP in a negative (exclusive) way

Not sure if that's the way to go about this. AFAIK you can do pretty much everything with regex but I think Dompurify should have an option to prevent this. Do you want to open an issue over at their repo?

> Do you happen do be a RegEx wizard? > I'm having difficulties adapting the default regex /^(?:(?:(?:f|ht)tps?|mailto|tel|callto|sms|cid|xmpp|xxx):|[^a-z]|[a-z+.\-]+(?:[^a-z+.\-:]|$))/i to be used in ALLOWED_URI_REGEXP in a negative (exclusive) way Not sure if that's the way to go about this. AFAIK you can do pretty much everything with regex but I think Dompurify should have an option to prevent this. Do you want to open an issue over at their repo?
jackymancs4 commented 2023-10-05 09:14:46 +00:00
Author
Contributor
Copy Link

Do you want to open an issue over at their repo?

I'm not sure what the issue on their side would be. Are you suggesting a DENIED_URI_REGEXP config that matches unwanted URI (like javascript:)?

> Do you want to open an issue over at their repo? I'm not sure what the issue on their side would be. Are you suggesting a DENIED_URI_REGEXP config that matches unwanted URI (like `javascript:`)?
konrad commented 2023-10-05 10:36:36 +00:00
Owner
Copy Link

Do you want to open an issue over at their repo?

I'm not sure what the issue on their side would be. Are you suggesting a DENIED_URI_REGEXP config that matches unwanted URI (like javascript:)?

Yes pretty much. Either they have that option and it's not obvious or they don't have it and in that case probably a good reason for it.

> > Do you want to open an issue over at their repo? > > I'm not sure what the issue on their side would be. Are you suggesting a DENIED_URI_REGEXP config that matches unwanted URI (like `javascript:`)? Yes pretty much. Either they have that option and it's not obvious or they don't have it and in that case probably a good reason for it.
jackymancs4 added 1 commit 2023-10-05 14:16:25 +00:00
Merge branch 'main' into feature/allow-unknown-protocols
All checks were successful
continuous-integration/drone/pr Build is passing
Details
c83a07a4db
jackymancs4 commented 2023-10-05 14:37:54 +00:00
Author
Contributor
Copy Link

Since they straight up allow to disable the check with ALLOW_UNKNOWN_PROTOCOLS, I guess there isn't a real good reason. Just that nobody asked for it before. I'll open the issue.

In the mean time, would you settle for it if get the ALLOWED_URI_REGEXP working as described?

What if I extended the default one with to include joplin in the whitelist, just for my use case?
E.g
/^(?:(?:(?:f|ht)tps?|joplin|mailto|tel|callto|sms|cid|xmpp|xxx):|[^a-z]|[a-z+.-]+(?:[^a-z+.-:]|$))/i

Since they straight up allow to disable the check with ALLOW_UNKNOWN_PROTOCOLS, I guess there isn't a real good reason. Just that nobody asked for it before. I'll open the issue. In the mean time, would you settle for it if get the ALLOWED_URI_REGEXP working as described? What if I extended the default one with to include `joplin `in the whitelist, just for my use case? E.g `/^(?:(?:(?:f|ht)tps?|joplin|mailto|tel|callto|sms|cid|xmpp|xxx):|[^a-z]|[a-z+.-]+(?:[^a-z+.-:]|$))/i `
konrad commented 2023-10-10 13:41:44 +00:00
Owner
Copy Link

In the mean time, would you settle for it if get the ALLOWED_URI_REGEXP working as described?

yeah that could work.

What if I extended the default one with to include joplin in the whitelist, just for my use case?

Others have been requested already (obsidian for example), better to fix this with one general solution.

> In the mean time, would you settle for it if get the ALLOWED_URI_REGEXP working as described? yeah that could work. > What if I extended the default one with to include joplin in the whitelist, just for my use case? Others have been requested already (obsidian for example), better to fix this with one general solution.
konrad commented 2023-12-06 13:08:23 +00:00
Owner
Copy Link

Do you want to rework this PR with the new editor we've switched to?

Do you want to rework this PR with the new editor we've switched to?
konrad commented 2024-01-16 12:10:24 +00:00
Owner
Copy Link

Closing due to inactivity. Please ping if you want to pick it up again.

Closing due to inactivity. Please ping if you want to pick it up again.
konrad closed this pull request 2024-01-16 12:10:24 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
konrad
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#3758
No description provided.
Delete Branch "jackymancs4/vikunja-frontend:feature/allow-unknown-protocols"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?