webext-signed-pages icon indicating copy to clipboard operation
webext-signed-pages copied to clipboard

-- is not allowed in HTML comments

Open rugk opened this issue 7 years ago • 7 comments

When following the HTML comment syntax correctly -- is not allowed in the comments. At least Firefox complains about this when viewing the site in the source code mode.

So maybe you can omit it? (and just internally add it, or so)

rugk avatar Feb 04 '18 16:02 rugk

Thanks for reporting!

I also noticed Firefox complaining about it. According to the w3c validator, it's a warning because it means the page is not XML 1.0 complaint. I guess it's an issue because some people use XML parsers to parse HTML, maybe? Not sure.

Anyhow, yes, it should be fixed. I'll change it to underscores instead, or better yet, upgrade to a format that is also versioned and is more versatile while at it.

tasn avatar Feb 05 '18 10:02 tasn

Maybe you can just use a HTML meta tag? You would then of course need to strip that tag before verifying the HTML, but IMHO this would be a nice syntax (and good semantics) and not too bad to do.

rugk avatar Apr 11 '18 11:04 rugk

That's a very interesting idea. I like the good semantics too!

I'll try to take a look into it over the weekend.

tasn avatar Apr 11 '18 11:04 tasn

BTW when stripping the tag I strongly suggest to only strip the actual key (so just the value of the meta tag). So leave an empty tag behind. Because otherwise one may circumvent the whole add-on by maybe adding some attribute with JavaScript to the meta tag. Don't know how exactly, but maybe something like onclick or onload or so if something like this exists. Or so something else which may supported in browsers in meta tags, ot whatever. You never know how browsers behave.😉 (or what weird exploit one could find for this) The reason is just that everything your strip is of course not verified by the extension, so strip as few as possible!

rugk avatar Apr 11 '18 11:04 rugk

I forgot to reply, but yeah, I'm fully aware of such browser oddities, and I'm therefore being extra defensive about it.

I have two concerns with putting it in the meta tag:

  1. It means the mechanism becomes HTML only which sucks a bit. I like being able to also verify TXT.
  2. It means I'd need to know how to parse HTML (and actually parse it) when validating. Not the end of the world in browser, but annoying for external services that would validate urls (think a distributed network of validators).

I solution to both would be just looking for a certain text, e.g.: "SIGNED-PAGES: <SIGNATURE HERE>" which can be anywhere in the document, not just the meta tag, and then it's up to the user (and shows in our examples) to put it inside the "value" in the meta tag. This will also support #15.

One disadvantage with that though is that if a page doesn't have a signature having this configuration be anywhere could be a problem with user generated content and #1. A malicious user on a forum with no such configuration could trigger a request to be added to the user's signed pages (we are potentially already vulnerable to that, luckily #1 is not yet implemented).

In summary: I'd need to think about it, though maybe after all it is best to just have it in the meta tag.

tasn avatar Aug 12 '18 15:08 tasn

"SIGNED-PAGES: " which can be anywhere in the document, not just the meta tag

What happens if it is found more than once? That could e.g. be the result of an XSS attack. When an XSS attack can inject a signature, I guess that's not so nice. 😄 (see you also covered this problem with the forum user example)

I would rather prefer a fallback - use meta tag by default and fall back to current behaviour?

rugk avatar Aug 13 '18 06:08 rugk

Even now it only looks for the first. So only the page that doesn't have one case is a concern.

Anyhow, plenty of ideas here. Just need to find the time to implement them. :)

Been awfully busy unfortunately, so never go around to implementing all of the things I want to. Though patches are welcome!

tasn avatar Aug 13 '18 17:08 tasn