View Issue Details

IDProjectCategoryView StatusLast Update
0030791mantisbtsecuritypublic2022-08-08 11:03
Reporterhotzeplotz Assigned Todregad  
PrioritynormalSeverityminorReproducibilityalways
Status assignedResolutionopen 
Product Version2.25.5 
Target Version2.25.6 
Summary0030791: Allow adding relation type noopener/noreferrer to outgoing links
Description

There are a lot of security related topics around the rel="noopener noreferrer" attribute of outgoing links. Due its security related, it would be nice haveing this in mantisbt.

Steps To Reproduce

Activate process_urls in plugin MantisCoreFormatting, insert a URL in bug description or bug note. The resulting a Element has no attribute rel, but it should, especially on outgoing links.

Additional Information

See: https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types

scrolldown to noopener and noreferrer

noopener

Instructs the browser to open the link without granting the new browsing context access to the document that opened it — by not setting the Window.opener property on the opened window (it returns null).

This is especially useful when opening untrusted links, in order to ensure they cannot tamper with the originating document via the Window.opener property (see About rel=noopener for more details), while still providing the Referer HTTP header (unless noreferrer is used as well).

Note that when noopener is used, nonempty target names other than _top, _self, and _parent are all treated like _blank in terms of deciding whether to open a new window/tab.

noreferrer

Prevents the browser, when navigating to another page, to send this page address, or any other value, as referrer via the Referer: HTTP header.
(In Firefox, before Firefox 37, this worked only in links found in pages. Links clicked in the UI, like "Open in a new tab" via the contextual menu, ignored this).

TagsNo tags attached.

Activities

dregad

dregad

2022-07-23 05:56

developer   ~0066823

Sounds like good practice indeed, but why specify both ? noreferrer implies noopener so it's redundant; I do not see in what way preventing the referer header is increasing security. I would think noopener is sufficient to cover the vulnerability.

hotzeplotz

hotzeplotz

2022-07-23 18:02

reporter   ~0066825

Yea, your right. Read about it, but can't remember where and can not find it right now, but the point was, that the different browser implement it differently, and so the recommendation was "use both".

I am not really sure about it, but will try.

[–] in what way preventing the referer header is increasing security.

The referrer expose the super-secret-internal URL of the mantis. :-)

dregad

dregad

2022-07-24 04:13

developer   ~0066827

different browser implement it differently

I did some research and it seems indeed to be a browser compatibility issue. noreferrer wider support than noopener, e.g. Chrome 16 vs 49, Firefox 33 vs 52, and IE 11 partial support vs none, etc.

Considering we are no longer supporting IE, and require a "recent" browser, I think it's safe to only use noopener.

The referrer expose the super-secret-internal URL of the mantis. :-)

I guess your use case is MantisBT on an company network, and don't want the URL to be sent to external sites, right ?

dregad

dregad

2022-07-24 07:00

developer   ~0066828

Last edited: 2022-07-24 07:10

PR https://github.com/mantisbt/mantisbt/pull/1834

Test script

<?php
include 'core.php';
for( $i = 0; $i < 16; $i++ ) {
    if( $i && ! ( $i & 1 ) ) continue; # Skip meaningless combinations with OFF
    config_flush_cache( 'html_make_links' );
    $GLOBALS['g_html_make_links'] = $i;
    printf( "%04b: %s\n", $i, string_insert_hrefs( 'http://example.com' )
    );
}

Output

0000: http://example.com
0001: <a href="http://example.com">http://example.com</a>
0011: <a href="http://example.com" target="_blank">http://example.com</a>
0101: <a href="http://example.com" rel="noopener">http://example.com</a>
0111: <a href="http://example.com" rel="noopener" target="_blank">http://example.com</a>
1001: <a href="http://example.com" rel="noreferrer">http://example.com</a>
1011: <a href="http://example.com" rel="noreferrer" target="_blank">http://example.com</a>
1101: <a href="http://example.com" rel="noreferrer">http://example.com</a>
1111: <a href="http://example.com" rel="noreferrer" target="_blank">http://example.com</a>
atrol

atrol

2022-07-24 08:13

developer   ~0066829

@dregad not sure you are aware (I was not) that the "URL Processing" setting does not apply if "Markdown Processing" is activated.

hotzeplotz

hotzeplotz

2022-07-24 08:15

reporter   ~0066830

Last edited: 2022-07-24 08:16

Cool, is this also covering the process_markdown:true output?

Is there the need to populate the new attribute[1] to parsdowns inlineLink-function?

// MantisMarkdown.php#L262
if( isset( $block['element']['attributes']['href'] )) {
    $this->processAmpersand( $block['element']['attributes']['href'] );
   /*
    * needs something like:
    * $block['element']['attributes']['rel'] = ??
    */
}

because the attribute is not in defaults at parsedowns site [2]

// Parsedown.php#L1388
protected function inlineLink($Excerpt)
    {
        $Element = array(
            'name' => 'a',
            'handler' => array(
                'function' => 'lineElements',
                'argument' => null,
                'destination' => 'elements',
            ),
            'nonNestables' => array('Url', 'Link'),
            'attributes' => array(
                'href' => null,
                'title' => null,
                // "rel" missing here?
                // ^^^^^^^^^^^^^^^^ 
            ),
        );
 // […]

I guess your use case is MantisBT on an company network, and don't want the URL to be sent to external sites, right ?

Yes. Not mine but well known installations.

[1] https://github.com/mantisbt/mantisbt/blob/master/plugins/MantisCoreFormatting/core/MantisMarkdown.php#L262
[2] https://github.com/erusev/parsedown/blob/master/Parsedown.php#L1388

hotzeplotz

hotzeplotz

2022-07-24 08:20

reporter   ~0066831

The escaping of the two links in my previous looks broken?

input:

- "http://mantis.companyname.local" expose …
- "https://issues01.srv25-east.companyname.com" …

does not match the intented output?

hotzeplotz

hotzeplotz

2022-07-24 09:02

reporter   ~0066832

Last edited: 2022-07-24 09:17

Is there the need to populate the new attribute[1] to parsdowns inlineLink-function?

No, not "populate to parsedown", the attribute is added after the block is created by Parsdown.

No idea when inlineLink is called … so far i can confirm that overwriting inlineUrl works for bug description and bug notes, internal links ("#1233") are not affected

// MantisMarkdown.php
protected function inlineUrl( $block ) {
    $block = parent::inlineUrl( $block );

    if( isset( $block['element']['attributes']['href'] )) {
        $block['element']['attributes']['rel'] = 'noopener noreferrer';
    }

    return $block;
}
dregad

dregad

2022-07-25 03:57

developer   ~0066834

@dregad not sure you are aware (I was not) that the "URL Processing" setting does not apply if "Markdown Processing" is activated.

I must have known it at some point, i.e. when I was fixing some bugs on custom MantisMarkdown class, like 5 years ago... But I forgot ;-)

is this also covering the process_markdown:true output?

Nope, based on the above and as you found out, it is not. On my dev box yesterday, I was testing with Markdown disabled.
An adjustment to MantisMarkdown is probably needed to handle this use case.

The escaping of the two links in my previous looks broken

Known, long-standing issue, see 0024628

Is there the need to populate the new attribute[1] to parsdowns inlineLink-function?

I am not sure off-hand where the change should be made. It's been a long time since I've looked at Parsedown and it's a complex beast.
I will check and update the PR.

dregad

dregad

2022-07-30 18:10

developer   ~0066845

Last edited: 2022-07-30 18:12

So, after analysis:

  • inlineLink() is processing Markdown-style links such as

    [text](url)
    --
    [text][label] 
    \[label]: url
    --
    [label]
    \[label]: url
  • inlineUrl() deals with plain, unformatted links such as

    url
    <url>

It's worth mentioning that when Markdown is enabled, such URLs are always converted to links, regardless of MantisCoreFormatting URL Processing setting's value. IMO this is a bug, they should only be processed if it is ON.

There is also inlineUrlTag(), which should be called when processing <url> in single-line context, but this does not actually work because the text goes through htmlspecialchars() first, so the angle brackets are converted to html entities, which messes up Parsedown's processing. This is probably the same root cause as 0024628.

I have a work-in-progress branch on my dev box, that looks promising but needs some more testing before I can push it to the PR. It's getting late so I'll do that maybe tomorrow night.

hotzeplotz

hotzeplotz

2022-08-03 17:12

reporter   ~0066850

Last edited: 2022-08-05 11:24

Yes, i think your right, the processing is mixing up things, what is causing the most of all the topics around Markdown.

can you take a look here: https://github.com/grummbeer/mantisbt/tree/markdown-fix

mantis-markdown.png (203,018 bytes)   
mantis-markdown.png (203,018 bytes)   
hotzeplotz

hotzeplotz

2022-08-05 11:25

reporter   ~0066865

Last edited: 2022-08-08 11:03

I'm new to the source code site of Mantis and just figuring out how the things work. So there is a high probability that i may be completely wrong, but I am quite confused and the most is not totally clear to me.

Sorry, for this is coming off topic, but let me share my thoughts. See it as as proposal, its just what i have learned from a naive point of view.

The well known root of all evil its that the strings are processed by Parsedown and processText(). The solution must be fixing this.

Before try to solve this, there are several cleanup tasks to solve before, to make the path clean and straightforward to follow.

I think merging your PR (https://github.com/mantisbt/mantisbt/pull/1834) would be a good start, because having this function already present makes the feeling of success nicer.

1. cleanup quotings

  • add an external css file, only if process_markdown is enabled.
  • remove the "__qoute()"-part from from MantisMarkdown.php
  • fix the test to match the new output without inline styles.

see https://github.com/mantisbt/mantisbt/compare/master...grummbeer:mantisbt:markdown-cleanup-01-quotings

This is related to 0022190 and https://github.com/mantisbt/mantisbt/pull/1004 (especially the last comment about emails and inline-styles).

For decades the opening arrow > has been the generally accepted character to mark a quote, mail clients take care of this and apply their own formatting, so there is no need to add inline styles to a quote.

2. cleanup tables

  • add note to Test, because the [link](/) thing makes this test highly dependent on "process_urls", which is not recognized at all.
  • remove __doTable()- stuff from MantisMarkdown.php

Parsedown provide a helpfull method where all "marked" elements accessible. Use it.

see https://github.com/mantisbt/mantisbt/compare/master...grummbeer:mantisbt:markdown-cleanup-02-tables

3. cleanup table tests

[link] (/) makes this test highly dependent on "process_urls", but this is not recognized here, "MantisMarkdown::convert_text" does not know anything about it. Testing links should have a separate test, because its not part of the "tableClassDefinition".

  • reduce the sample to match more the intended purpose of this test

see https://github.com/mantisbt/mantisbt/compare/master...grummbeer:mantisbt:markdown-cleanup-03-table-test

4. cleanup the blockHeader part

The Tests testHashLetters, testHashNumberAny and testHashLettersAny failing.

  • invert the logic, do not search for a valid markdown heading, search for a valid "buglink" instead.
  • Change regex to be more generous with the use of colons.
  • Add test for the regular expression.

The Tests testHashLetters, testHashNumberAny and testHashLettersAny are successfully again afterward.

todo: Clarify what a valid issue mention is.

| input            | is issue mention     |
|------------------|----------------------|
| `0000001`          | true                 |
| `#123 `          | true                 |
| ` #123 `         | true                 |
| `#123 `          | true                 |
| `#123 summary`   | true                 |
| `#123: summary`  | true                 |
| `#123:summary`   | true                 |
| `#123:: summary` | true but should not? |
| `#123summary`    | false                |
| `# 123:summary`  | false                |
| `# summary `     | false                |
| `# 123`          | false                |

Modify the regex to meet the mantis requirements.

see https://github.com/mantisbt/mantisbt/compare/master...grummbeer:mantisbt:markdown-cleanup-04-blockheader

5. cleanup formatting process

  • Rearrange the processing. Moving the Markdown part before ON == $s_text, return the result and do not doing further processing.
  • Make MantisMarkdown know about the plugin config and process the links and mentions within the parser context
  • mark all the methods that make use of $this->processAmpersand() as deprecated, because the output is no loger messed up.
  • remove all methods which marked as "deprecated"

Parsedown provides a usefull method called unmarkedText, which make all "unmarked" strings accessible. Seems the right place to process the links.

see https://github.com/mantisbt/mantisbt/compare/master...grummbeer:mantisbt:markdown-cleanup-05-formatting

After that and merging all parts together the whole part looks very nice and its a huge impact for user experience.

The branches are all independent but could make it a cascading series. What do you think about? I am not experienced in contributing, so this may look strange to you, but I have no idea what the right approach is.

todo

  • the tests are now failing because MantisMarkdown.php is getting the plugin config using plugin_config_get, which is not available inside the unit tests. Make it possible to pass a configuration to the MantisMarkdown?
  • emails still not proceed with markdown. hidden deep inside string_insert_hrefs make a new api function to catch them?
  • add prism to raise up the user experience.

Fixing this todos would be a really nice finishing point.

dregad

dregad

2022-08-05 11:53

developer   ~0066866

@hotzeplotz many thanks for your in-depth analysis.

At first glance the approach seems good, unfortunately I do not have the time to have a detailed look at your proposal right now, and won't be able to, until end of August.

Please bear with me, thanks for your patience and understanding.