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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement dialog (alert/confirm) blocking as a user switch after the first dialog #11613

Merged
merged 10 commits into from Mar 6, 2018

Conversation

6 participants
@MarshallOfSound
Member

MarshallOfSound commented Jan 10, 2018

  • This is to enable more browser-like behavior so that users who run third-party code
    will not be DOS'ed with alerts and confirms. This is already handled like this
    in most major browsers so this will greatly help these developers.
  • This also removes our JS implementation of alert and confirm which minimizes risk and works because we have a working JavascriptDialogManager now 馃憤

@MarshallOfSound MarshallOfSound requested review from electron/docs as code owners Jan 10, 2018

@ckerr

Most of the comments have clear fixes suggested.

Really not sure what to do about the i18n question though. I haven't actually done any user-facing strings yet and am not sure how we handle translations in the c++ files...?

if (checkbox_checked) {
origin_counts_->erase(origin);
origin_counts_->insert({ origin, -1 });
}

This comment has been minimized.

@ckerr

ckerr Jan 25, 2018

Member

simpler:

if (checkbox_checked) {
  (*origin_counts_)[origin] = -1;
}
@@ -30,6 +31,15 @@ void AtomJavaScriptDialogManager::RunJavaScriptDialog(
const base::string16& default_prompt_text,
const DialogClosedCallback& callback,
bool* did_suppress_message) {
std::string origin = origin_url.GetOrigin().spec();

This comment has been minimized.

@ckerr

ckerr Jan 25, 2018

Member

const std::string origin

@@ -37,9 +38,12 @@ class AtomJavaScriptDialogManager : public content::JavaScriptDialogManager {
private:
static void OnMessageBoxCallback(const DialogClosedCallback& callback,
const std::string& origin,
std::map<std::string, int>* origins_,

This comment has been minimized.

@ckerr

ckerr Jan 25, 2018

Member

Why is this passed as a pointer instead of a reference?

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jan 26, 2018

Member

Something weird about the way reference was being bound to the callback, it just kinda didn't work. Pointer worked first try and didn't want to waste time really figuring out why, they're computationally equivalent I believe.

@@ -41,12 +51,20 @@ void AtomJavaScriptDialogManager::RunJavaScriptDialog(
buttons.push_back("Cancel");
}
origin_counts_[origin]++;
std::string checkbox_string = "";

This comment has been minimized.

@ckerr

ckerr Jan 25, 2018

Member

= "" is unnecessary, checkbox_string is initialized to an empty string anyway

std::string checkbox_string = "";
if (origin_counts_[origin] > 1 &&
WebContentsPreferences::UsesSafeDialogs(web_contents)) {
checkbox_string = "Prevent this app from creating additional dialogs";

This comment has been minimized.

@ckerr

ckerr Jan 25, 2018

Member

Hmmm how do we handle i18n for this?

I'm not aware of any other user-facing strings like this in Atom. There are thrown errors, but those are intended more for developers than end-users and so don't have translations.

This comment has been minimized.

@nitsakh

nitsakh Jan 26, 2018

Contributor

I saw some other cases like in the web_dialog_helper.cc for example. chrome/ui has l10n utlis namespace which provides localization support, but that might be a whole different thing.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jan 26, 2018

Member

Maybe we can make the string user defined in webPreferences? Then it's up to implementers to both choose the string and translate it?

This comment has been minimized.

@zcbenz

zcbenz Feb 13, 2018

Contributor

Since this string has already been translated in Chromium, we can build the components_strings target in libcc and then ship the components_strings_$locale.pak files, which can be then loaded in Electron to provide localized strings.

And I think it is fine not to do localization inside Electron for now, we already have some English-only strings (for example "All Files") and this does not make things worse.

origin_counts_[origin] = 0;
}
if (origin_counts_[origin] == -1) {

This comment has been minimized.

@ckerr

ckerr Jan 25, 2018

Member

Not in love with the use of -1 as a magic number here.

It's awkward to mix the int to be both a count and a flag, but I see the reasoning because once the flag is set, we never care about the count again. But let's at least give the the flag a symbolic name, e.g. at the top of the file have

namespace {
  constexpr int USER_WANTS_NO_MORE_DIALOGS = -1;
}

and then use USER_WANTS_NO_MORE_DIALOGS here and in OnMessageBoxCallback

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Feb 15, 2018

@ckerr PR updated, uses a constant now and the string can be localized by users if they want but defaults to an english string 馃憤

* `safeDialogs` Boolean (optional) - Whether to enable browser style
consecutive dialog protection.
* `safeDialogsMessage` String (optional) - The message to display when consecutive
dialog protection is triggered.

This comment has been minimized.

@YurySolovyov

YurySolovyov Feb 15, 2018

Contributor

should mention defaults?

@ckerr ckerr added this to Candidates in 3.0.x Feb 16, 2018

int code,
bool checkbox_checked);
api::WebContents* api_web_contents_;
std::map<std::string, int> origin_counts_;

This comment has been minimized.

@poiru

poiru Feb 20, 2018

Member

Maybe unordered_map instead?

MarshallOfSound and others added some commits Jan 10, 2018

Implement dialog (alert/confirm) blocking as a user switch after the 鈥
鈥irst dialog

* This is to enable more browser-like behavior so that users who run third-party code
  will not be DOS'ed with alerts and confirms.  This is already handled like this
  in most major browsers so this will greatly help these developers
fix cpplint error
atom/browser/atom_javascript_dialog_manager.h:9:  Include "map" not in alphabetical order  [build/include_alpha] [4]
fix cpplint errors
atom/browser/atom_javascript_dialog_manager.cc:39:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
atom/browser/atom_javascript_dialog_manager.cc:39:  If/else bodies with multiple statements require braces  [readability/braces] [4]
atom/browser/atom_javascript_dialog_manager.cc:62:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
atom/browser/atom_javascript_dialog_manager.cc:89:  If/else bodies with multiple statements require braces  [readability/braces] [4]
@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Mar 6, 2018

I'm rebasing this PR on master.

@zcbenz

zcbenz approved these changes Mar 6, 2018

@zcbenz zcbenz merged commit e73326a into master Mar 6, 2018

9 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins: macOS/pr-head This commit looks good
Details

3.0.x automation moved this from Candidates to Fixed Mar 6, 2018

@zcbenz zcbenz deleted the safe-dialogs branch Mar 6, 2018

@ckerr ckerr moved this from Fixed to Fixed (3.0.0-beta.1) in 3.0.x Jul 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment