Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

-- is not allowed in HTML comments #6

Open
rugk opened this issue Feb 4, 2018 · 7 comments
Open

-- is not allowed in HTML comments #6

rugk opened this issue Feb 4, 2018 · 7 comments

Comments

@rugk
Copy link

rugk commented Feb 4, 2018

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 rugk changed the title -- is not allowed in HTML comments -- is not allowed in HTML comments Feb 4, 2018
@tasn
Copy link
Owner

tasn commented Feb 5, 2018

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.

@rugk
Copy link
Author

rugk commented Apr 11, 2018

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.

@tasn
Copy link
Owner

tasn commented Apr 11, 2018

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

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

@rugk
Copy link
Author

rugk commented Apr 11, 2018

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!

@tasn
Copy link
Owner

tasn commented Aug 12, 2018

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

@rugk
Copy link
Author

rugk commented Aug 13, 2018

"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?

@tasn
Copy link
Owner

tasn commented Aug 13, 2018

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!

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

No branches or pull requests

2 participants