microcosm-cc / bluemonday

bluemonday: a fast golang HTML sanitizer (inspired by the OWASP Java HTML Sanitizer) to scrub user generated content of XSS
https://github.com/microcosm-cc/bluemonday
BSD 3-Clause "New" or "Revised" License
3.12k stars 176 forks source link

Allow target="_blank" #129

Closed inliquid closed 2 years ago

inliquid commented 3 years ago

Is there a way to allow it other than:

policy.AllowAttrs("target").Matching(regexp.MustCompile(`_blank`)).OnElements("a")
Lolioy commented 3 years ago

AddTargetBlankToFullyQualifiedLinks

inliquid commented 3 years ago

@Lolioy it's set, and it's not the case.

buro9 commented 2 years ago

What are the links you are trying to trigger _blank on?

AddTargetBlankToFullyQualifiedLinks does work... but as the option states it's only for fully qualified links.

Relative links to local resources do not have _blank added.

Are you looking to add this to relative links too? This wasn't added originally as the example policies are for user generated content and whilst opening 3rd party links (fully qualified) in new tabs / windows is a typically good experience, opening every link in a new tab / window is a typically poor experience. But your use-case may be different, in which case if you can show what it is we can consider adding an additional option for AddTargetBlankToAllLinks though I'd want to avoid that if possible as for the vast majority of web sites that would be a worse user experience on the web.

inliquid commented 2 years ago

My question is pretty simple. I understand the reason behind not adding _blank to all links. My links are relative as you correctly suggested, so I have added this param before passing HTML to bluemonday. However this seems to be restricted by default policy, so in order to make them working I added

policy.AllowAttrs("target").Matching(regexp.MustCompile(`_blank`)).OnElements("a")

Is my understanding right and is this a correct way?

As for adding AddTargetBlankToAllLinks, I don't see any scenario, other than something very special. In my case all internal links are relative and most of external a fully qualified. So AddTargetBlankToFullyQualifiedLinks does work. Some of internal (relative) links however need to be opened in separate tabs. One thing I just thought of is whether allowing _blank param by default simplify this a little, or is there any sense in restricting it?

buro9 commented 2 years ago

Is my understanding right and is this a correct way?

Yes.

i.e.

func TestIssue129(t *testing.T) {
    tests := []test{
        {
            in:       `<a href="/hello" target="_blank"></a>`,
            expected: `<a href="/hello" target="_blank" rel="nofollow noopener"></a>`,
        },
        {
            in:       `<a href="/hello" target="foo"></a>`,
            expected: `<a href="/hello" rel="nofollow"></a>`,
        },
    }

    p := UGCPolicy()
    p.AllowAttrs("target").Matching(regexp.MustCompile(`^_blank$`)).OnElements("a")

    // These tests are run concurrently to enable the race detector to pick up
    // potential issues
    wg := sync.WaitGroup{}
    wg.Add(len(tests))
    for ii, tt := range tests {
        go func(ii int, tt test) {
            out := p.Sanitize(tt.in)
            if out != tt.expected {
                t.Errorf(
                    "test %d failed;\ninput   : %s\noutput  : %s\nexpected: %s",
                    ii,
                    tt.in,
                    out,
                    tt.expected,
                )
            }
            wg.Done()
        }(ii, tt)
    }
    wg.Wait()
}