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

Console log message to prevent self xss #18088

Merged
merged 2 commits into from
Oct 30, 2017

Conversation

kloon
Copy link
Contributor

@kloon kloon commented Sep 15, 2017

This adds a developer console message informing the user about the possibility of a self XSS attack if they were asked to enter a value in the console.

Image of message

This closes #15, it could do with additions like translating the string based on locale and checking if the console support colors and styles, the latter only possible via user agent checking, unfortunately.

Could also be useful to setup a page on wordpress.com explaining self XSS and then linking to that page.

@kloon kloon added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Enhancement labels Sep 15, 2017
@kloon kloon self-assigned this Sep 15, 2017
@matticbot
Copy link
Contributor

@kloon kloon added this to Needs Review in GM 2017 - Good First Change Sep 15, 2017
@gziolo
Copy link
Member

gziolo commented Sep 20, 2017

There is also #11625 opened with a similar change. We should land one of the solutions. @dmsnell you reviewed the other PR, so it would be nice to finally one of the solutions.

@gziolo gziolo mentioned this pull request Sep 20, 2017
@spen
Copy link
Contributor

spen commented Sep 20, 2017

What do you think about a "We're hiring" nudge in this?
Something with the sentiment of: "Don't do continue if you don't know what you're doing, but if you do come and join in"

It could just confuse people since it gives mixed messages, so it might not be a great idea 🤷‍♂️

@kloon
Copy link
Contributor Author

kloon commented Sep 21, 2017

@spen I thought about adding the hiring message but figured it would not go well with the message about self XSS. There's also a x-hacker header with details about hiring on all wp.com pages x-hacker:If you're reading this, you should visit automattic.com/jobs and apply to join the fun, mention this header.

@gziolo gziolo requested a review from ehg September 21, 2017 13:10
Copy link
Contributor

@belcherj belcherj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, might be good to wait for another review tho

@gziolo
Copy link
Member

gziolo commented Sep 21, 2017

Agree, it would be nice to get thumbs up from @nb or @ehg before merging this change.

@spen
Copy link
Contributor

spen commented Sep 21, 2017

Thanks @kloon, I think that make sense so as not to confuse folks :)

Another consideration might be to suppress the message in non-production environments - I'm not sure the best path for doing so in our jade templates, and not sure if this would cause more problems than it solves.

@gziolo
Copy link
Member

gziolo commented Oct 5, 2017

@hoverduck, does it solve #15 as you expected?

@hoverduck
Copy link
Contributor

@gziolo - Yes, this generally solves #15, although @allendav was the original author...I only ported the issue over from the pre-oss repo 😄

A couple issues, though -
It works fine in Chrome, but I checked IE11 and it doesn't show up. Those were the only browsers I looked at.

And just a nit-pick, but I'd update it to say "This browser feature is intended for developers" (singular) instead of "This browser features is intended for developers" (plural)

@gziolo gziolo requested a review from allendav October 6, 2017 06:06
if ( window.console ) {
console.log( "%cSTOP!", "color:#f00;font-size:xx-large" );
console.log(
"%cThis browser features is intended for developers. " +
Copy link
Contributor

@allendav allendav Oct 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar nit: features should probably be feature

Edit: lol - sorry - I see @hoverduck caught that already too

Copy link
Contributor

@allendav allendav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change requested. Works well. Pre-approving.

@dmsnell dmsnell added the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Oct 6, 2017
@dmsnell
Copy link
Contributor

dmsnell commented Oct 6, 2017

@folletto or @melchoyce or @adambbecker or someone could we get some design review of the text here as this will be public-facing communication from Automattic?

a part of me feels like it's too dread-serious for most of our other communication.

@folletto folletto added [Status] Needs Copy Review Add this when you'd like to get a review / feedback from the Editorial team on your PR and removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Oct 9, 2017
@folletto
Copy link
Contributor

folletto commented Oct 9, 2017

Let's just tag Editorial ;)

@michelleweber
Copy link

It's okay to be serious when there are potentially bad consequences! That said, I would tweak a bit to give people a little more context about why this is potentially bad:

Wait! This browser feature runs code that can alter your website or its security, and is intended for developers. If you've been told to copy and paste something here to enable a feature, someone may be trying to compromise your account. Please make sure you understand the code and trust the source before adding anything here.

@dmsnell
Copy link
Contributor

dmsnell commented Oct 9, 2017

Please make sure you understand the code and trust the source before adding anything here.

Thanks @michelleweber! I like how your wording acknowledges that the console can still be useful (we're not trying to forbid people from using it) and that some people will have the right/skill/know-how to get in there and dig around (while still warning off people who may be mislead by something they read online).

@dmsnell dmsnell force-pushed the add/console-log-self-xss-warning branch from 36051c5 to 6e28fe7 Compare October 30, 2017 21:25
Copy link
Contributor

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kloon and @gziolo I updated this PR to match the text which @michelleweber suggested. If we are okay with it we should probably just merge it :shipit:

@gziolo
Copy link
Member

gziolo commented Oct 30, 2017

Yes, feel free to merge 👍

@dmsnell dmsnell merged commit e9b86f3 into master Oct 30, 2017
@dmsnell dmsnell deleted the add/console-log-self-xss-warning branch October 30, 2017 22:51
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 30, 2017
@stephanethomas
Copy link
Contributor

Could we disable this message in development? We have already enough warnings popping up in the console in this environment. And I suspect that if you use the latter, you already know what you're doing.

@kwight
Copy link
Contributor

kwight commented Oct 31, 2017

This does feel very distracting and unnecessary in the development environment; #19355 to remove.

rclations pushed a commit to rclations/wp-calypso that referenced this pull request Nov 15, 2017
Add console log message notifying user about possible self xss

This is to guard against people accidentally pasting in code which they might read online which was written with malicious intent. By pasting code into the developer console it's possible to do things with the user's authentication; we want to make people aware that they shouldn't be randomly pasting in stuff unless they know what they are doing.
@gziolo gziolo moved this from Needs Review to Done 🎉 in GM 2017 - Good First Change Mar 21, 2018
@sirreal sirreal removed the [Status] Needs Copy Review Add this when you'd like to get a review / feedback from the Editorial team on your PR label Mar 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Security: Add Self XSS Warning to Dev Console