Skip to content
This repository has been archived by the owner on Feb 2, 2021. It is now read-only.

Why "target" attributes are not allowed for anchor "A" elements? #1991

Open
dvoytenko opened this issue Jan 26, 2016 · 10 comments
Open

Why "target" attributes are not allowed for anchor "A" elements? #1991

dvoytenko opened this issue Jan 26, 2016 · 10 comments

Comments

@dvoytenko
Copy link
Contributor

I can't seem to find any reason documented for this. Is there some reason "a::target" is blacklisted?

@kpreid
Copy link
Contributor

kpreid commented Jan 26, 2016

Because specific values of target= other than "_blank" (which we do allow) are a namespace in the host page. If you open two links with the same target= value then they overwrite each other, so if the guest can set arbitrary target= then it can interfere with the functionality of the host or other guests in the same page.

I don't immediately see a reason why we couldn't rewrite them to have a guest-specific prefix/suffix in the same way we do ids and classes, but currently such methods have to be implemented ad-hoc and supporting this has never been requested before.

@dvoytenko
Copy link
Contributor Author

@kpreid thanks, this makes sense. The value of _blank is actually what my current problem is with. Basically what happens right now is that guest specifies _blank, but it's removed by the sanitizer and HTML defaults it to _self. So, in a way, it creates a security issue. I think some other values might be more controversial and require a deeper look, but _blank should be a good idea to whitelist. WDYT?

@kpreid
Copy link
Contributor

kpreid commented Jan 26, 2016

It should be already whitelisted — there's a configuration option which defaults to allowing it. Sounds like there's a bug. If you want to dig into the code, look for identifiers optTargetAttributePresets and targetAttributePresets.

@dvoytenko
Copy link
Contributor Author

@kpreid I've been using src/com/google/caja/plugin/html-sanitizer.js and I don't think those options are exposed there. Could you please advise if this is not something I should be using?

@kpreid
Copy link
Contributor

kpreid commented Jan 26, 2016

The sanitizer should simply have target whitelisted. The relevant part of the default (only) whitelist is in src/com/google/caja/lang/html/html4-attributes-whitelist.json, in which I see "A::TARGET" in the allowed list.

@dvoytenko
Copy link
Contributor Author

Do you mean that it should be already whitelisted by default? I'm looking into the compiled html-sanitizer-bundle.js, where it's inlined as:

html4.ATTRIBS = {
  'a::target': 10,
}

From what I can tell, the value of 10 (FRAME_TARGET) is filtered out in the sanitizeAttribs method in the html-sanitizer.js.

@kpreid
Copy link
Contributor

kpreid commented Jan 26, 2016

Sorry, your analysis is correct. We've got a little too many hardcoded policies...

@dvoytenko
Copy link
Contributor Author

@kpreid Do you think this could be fixed?

@dvoytenko
Copy link
Contributor Author

@kpreid Friendly ping. Have you given this issue a thought?

@GaloisGirl
Copy link

I'd love to see this fixed too.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants
@dvoytenko @kpreid @GaloisGirl and others