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.

Sign up for GitHub

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

Jump to bottom

fix(NcReferenceList): check if text contains valid URL before resolving #5290

Closed
wants to merge 1 commit into from
Closed

fix(NcReferenceList): check if text contains valid URL before resolving #5290

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/components/NcRichText/NcReferenceList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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?

Copy link
Contributor Author

@Antreesy Antreesy Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Plain text
    test message - it tries to resolve a link: https://nextcloud.local/index.php/call/test%20message
  2. 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
  3. Absolute URL
    https://nextcloud.local/index.php/call/w38mo5vy - resolves correctly
  4. Relative URL
    ../apps/spreed - creates a reference to an app root

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@Antreesy Antreesy Feb 22, 2024 •
edited
Loading

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

Copy link
Contributor Author

@Antreesy Antreesy Feb 22, 2024 •
edited
Loading

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)

Copy link
Contributor

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.

if (matchArray) {
return new URL(matchArray[0], window.location).href
}
return null
},
},
watch: {
Expand All @@ -100,7 +104,7 @@ export default {
return
}

if (!(new RegExp(URL_PATTERN).exec(this.fullUrl.href))) {
if (!(new RegExp(URL_PATTERN).exec(this.fullUrl))) {
this.loading = false
return
}
Expand All @@ -116,7 +120,7 @@ export default {
})
},
resolve() {
const match = (new RegExp(URL_PATTERN).exec(this.fullUrl.href))
const match = (new RegExp(URL_PATTERN).exec(this.fullUrl))
if (this.limit === 1 && match) {
return axios.get(generateOcsUrl('references/resolve', 2) + `?reference=${encodeURIComponent(match[0])}`)
}
Expand Down
Loading