action #95842
closedURLs containing "#" should be parsed as complete URLs size:S
0%
Description
Observation¶
Putting an URL like https://build.suse.de/request/show/245055#comment-3760740 into an openQA comment field is converted into HTML code
also commented on <a href="https://build.suse.de/request/show/245055">https://build.suse.de/request/show/245055</a>#comment-3760740
also commented on https://build.suse.de/request/show/245055#comment-3760740
Expected result¶
should be
also commented on <a href="https://build.suse.de/request/show/245055#comment-3760740">https://build.suse.de/request/show/245055#comment-3760740</a>
also commented on https://build.suse.de/request/show/245055#comment-3760740
Acceptance criteria¶
- AC1: The complete URL including the part after
#is converted into an<a href>
Suggestions¶
- Add a test, e.g. in t/ui/15-comments.t
- Try to fix https://rt.cpan.org/Public/Bug/Display.html?id=55545 upstream . As alternative solve it downstream on our side in https://github.com/os-autoinst/openQA/blob/master/lib/OpenQA/Utils.pm#L433
Updated by okurz over 3 years ago
- Subject changed from URLs containing "#" should be parsed as complete URLs to URLs containing "#" should be parsed as complete URLs size:S
- Status changed from New to Workable
estimated asynchronously by vanastasiadis and okurz
Updated by kraih over 3 years ago
Took a quick look at this since i wrote the Markdown conversion code. And i'm afraid it is not as easy as it looks at first. We are currently using a regex from Regexp::Common to detect URLs in OpenQA::Markdown. That regex will need to be replaced with one that includes fragments. This is a serious XSS/HTML injection attack target, so the regex will need to be selected very carefully. And new tests should cover as many edge cases as possible.
Updated by VANASTASIADIS over 3 years ago
This line produces the same error as well. Is there a reason to leave it out?
Updated by VANASTASIADIS over 3 years ago
btw, with a quick look, the current implementations both in Markdown.pm and Utils.pm both seem to be already open to injection attacks. So I think it makes sense to change both.
Updated by okurz over 3 years ago
- Due date set to 2021-08-12
- Status changed from Workable to In Progress
Updated by VANASTASIADIS over 3 years ago
- Status changed from In Progress to Resolved
Resolved with https://github.com/os-autoinst/openQA/pull/4091/files
Updated by VANASTASIADIS over 3 years ago
- Status changed from Resolved to Feedback
Updated by VANASTASIADIS over 3 years ago
- Status changed from Feedback to Resolved
Deployed. Resolving the ticket
https://openqa.suse.de/changelog