Closed (fixed)
Project:
Contribute
Version:
8.x-1.x-dev
Component:
Code
Priority:
Minor
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
16 Jan 2018 at 05:39 UTC
Updated:
30 Jan 2018 at 16:34 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
markhalliwell<script>alert('test')</script>and an alert popped up... which it doesn't:Comment #3
sam152 commentedThe "Report a security vulnerability" link on the project page literally links to submitting a public issue with the "security" tag prefilled with the message:
For the the actual issue at hand, I've looked into this in some more detail.
If markup is not an instance of MarkupInterface (see
\Drupal\Core\Render\Renderer::ensureMarkupIsSafeit's run throughXss::filterAdminwith the following allowed tags. I don't know about you, but I'd rather someone not be able to stick images, divs and links into my status page remotely without even having an account on the site. I can imagine a few scenarios where such markup could encourage a site admin to expose some sensitive information...At the very least, shouldn't this be an open issue?
edit by @markcarver: removed the unnecessarily long list of allowed
Xss::filterAdmintags which can easily be referenced elsewhere.Comment #4
sam152 commentedI also think this technically still would have qualified for a security advisory had this module been stable, given the docblock of
\Drupal\Component\Utility\Xss::filterAdminreads:Having a profile on drupal.org !== an admin on my site.
Comment #5
sam152 commentedComment #6
markhalliwellBecause they can also remotely change which user's information is pulled from d.o? No...
Clearly, one has to put their own username/company in here manually. I'd hope you're smart enough not to "annoy" yourself with your own "images, divs and links" that were not designed to go in the "Work" section of d.o.... to begin with.
Again, your "reasoning" here makes no sense and appears to be fabricating imaginary scenarios to imply some "risk" when there really is none. This is a passive pull of remote content, which is intended to be a username/company that you (the administrator, who has access to the status page) must provide in the first place.
Perhaps, but it's not a "security" issue at all. The OP clearly insinuated it was without actually providing any real evidence. That's why I proved that it wasn't in #2 and closed the issue.
If @jrockowitz wants to "fix" this, then that is certainly his prerogative. I just don't like issues that cry wolf, especially when it's a new module (and a noble one at that!). Cheers!
Comment #7
sam152 commentedYeah, this title for the issue makes sense. Before digging into it in #3 I believed it was full blown XSS which is clearly isn't, hence the original title. Good clarification 👍
With regards to the security impact, it's more about attack surface than individuals trying to be annoying. Exploits are used in combination with each other to open holes.
Comment #8
jrockowitz commentedWow, a module that is less than 1 week old with 2 installations and already has a security ticket filed.
Thanks for catching that mistake and I would like to give Twig credit for preventing major issue on my part.
The attached patch renders the organization information as #plain_text instead of #markup.
Comment #9
sam152 commentedChecked if any other instances of user supplied data was being used with #markup, looks good to me 👌
Comment #10
jrockowitz commented