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.

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. :-)

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 ?

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: &lt;a href="http://example.com">http://example.com</a>
0011: &lt;a href="http://example.com" target="_blank">http://example.com</a>
0101: &lt;a href="http://example.com" rel="noopener">http://example.com</a>
0111: &lt;a href="http://example.com" rel="noopener" target="_blank">http://example.com</a>
1001: &lt;a href="http://example.com" rel="noreferrer">http://example.com</a>
1011: &lt;a href="http://example.com" rel="noreferrer" target="_blank">http://example.com</a>
1101: &lt;a href="http://example.com" rel="noreferrer">http://example.com</a>
1111: &lt;a href="http://example.com" rel="noreferrer" target="_blank">http://example.com</a>

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

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.

• "http://mantis.companyname.local" expose the company name and the usage of mantis
• "https://issues01.srv25-east.companyname.com" discloses the company name, parts of the infrastructure and make it accessible to every[bot|one].

[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

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?

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 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.

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. Tracked in 0030918.

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.

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)   

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     |
|------------------|----------------------|
| `#1`          | 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.

@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.

@hotzeplotz I have updated PR https://github.com/mantisbt/mantisbt/pull/1834 so the noopener/noreferrer is added as appropriate, also when using Markdown.

The update fixes the issue of links being converted even when URL Processing is OFF that I mentioned in 0030791:0066845 (tracked in 0030918).

As for your cleanup suggestions in 0030791:0066865, I will open a separate issue to track and discuss them independently from this (-> 0030919)

very cool, damien!

just delete 0030791:0066850 and 0030791:0066865, they littering the context?

You implement a helper function to get the link attributes … sehr gut!

just delete 0030791:0066850 and 0030791:0066865, they littering the context?

It does not really matter, and it's better to leave them here now, since I have referenced these posts in 0030919