-
Notifications
You must be signed in to change notification settings - Fork 85
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
fix(NcReferenceList): check if text contains valid URL before resolving #5290
Conversation
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@mejo- please, give it a test with Text and Collectives |
@@ -83,7 +83,11 @@ export default { | |||
} | |||
}, | |||
fullUrl() { | |||
return new URL(this.text.trim(), window.location) | |||
const matchArray = this.text.trim().match(new RegExp(URL_PATTERN)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this will not work as URL_PATTERN
matches only full URLs, not relative or absolute URLs without a location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you lead me to the place in Text, and give a way to reproduce it with relative URLs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact the whole idea of #5272 was to turn relative links (e.g. ../Target
) and absolute links (e.g. /index.php/apps/collectives/Garden/Watering
) into full URLs with a location before they're matched against URL_PATTERN
😬
Could you provide an example text of what is passed to NcReferenceList
as property text
which breaks currently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Plain text
test message
- it tries to resolve a link:https://nextcloud.local/index.php/call/test%20message
- Plain text with absolute URL
test message with a link https://nextcloud.local/index.php/call/w38mo5vy
- same:https://nextcloud.local/index.php/call/test%20message%20with%20a%20link%20https://nextcloud.local/index.php/call/w38mo5vy
- Absolute URL
https://nextcloud.local/index.php/call/w38mo5vy
- resolves correctly - Relative URL
../apps/spreed
- creates a reference to an app root
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could replace URL_PATTERN with some combination RELATIVE_URL_PATTERN?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be an option to sanitize the links inside Talk and make sure to only pass valid URLs to NcReferenceList
?
Given that we cannot distinguish between valid relative links and mere text I only see two paths forward:
- Either make sure only links (be it full URLs, relative or absolute paths) are passed to NcReferenceList
- Or move the logic to transform relative/absolute paths into full URLs to Text. That would mean that relative/absolute paths from other apps no longer get regarded by NcReferenceList.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could replace URL_PATTERN with some combination RELATIVE_URL_PATTERN?
That could work if use something like ^(\.){1,2}\/.*
as regex for RELATIVE_URL_PATTERN
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to only pass valid URLs to
NcReferenceList
?
It's library issue, not Talk. NcRichText passes complete text
Or move the logic to transform relative/absolute paths into full URLs to Text.
That would be preferred, I guess. It was not expected before #5272 to render references for relative links, I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean one of these places?
https://github.com/nextcloud/text/blob/35be559ca27f384cd80e9e5fe76ee739c7c754e6/src/nodes/ParagraphView.vue#L105-L112
https://github.com/nextcloud/text/blob/35be559ca27f384cd80e9e5fe76ee739c7c754e6/src/components/Link/LinkBubbleView.vue#L148-L155
If yes, then I believe, it's totally doable on Text app side. NcReferenceWidget itself is capable to handle links within app by itself (see getRoute()
method in autolink.js)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, both probably. I'm fine with trying to handle this in Text. So let's revert my earlier PR and I'll look into it in Text.
See discussions here for further information: * nextcloud-libraries/nextcloud-vue#5290 * nextcloud-libraries/nextcloud-vue#5272 Signed-off-by: Jonas <jonas@freesources.org>
See discussions here for further information: * nextcloud-libraries/nextcloud-vue#5290 * nextcloud-libraries/nextcloud-vue#5272 Signed-off-by: Jonas <jonas@freesources.org>
See discussions here for further information: * nextcloud-libraries/nextcloud-vue#5290 * nextcloud-libraries/nextcloud-vue#5272 Signed-off-by: Jonas <jonas@freesources.org>
See discussions here for further information: * nextcloud-libraries/nextcloud-vue#5290 * nextcloud-libraries/nextcloud-vue#5272 Signed-off-by: Jonas <jonas@freesources.org>
See discussions here for further information: * nextcloud-libraries/nextcloud-vue#5290 * nextcloud-libraries/nextcloud-vue#5272 Signed-off-by: Jonas <jonas@freesources.org>
☑️ Resolves
🖼️ Screenshots
🚧 Tasks
🏁 Checklist
next
requested with a Vue 3 upgrade