fix(NcReferenceList): check if text contains valid URL before resolving #5290
Conversation
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Antreesy
added
bug
|
@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.
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.
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.
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.
- 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.
Maybe we could replace URL_PATTERN with some combination RELATIVE_URL_PATTERN?
There was a problem hiding this comment.
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.
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.
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.
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.
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
nextrequested with a Vue 3 upgrade