Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Re-block running insecure content #3808

Merged
merged 4 commits into from
Sep 9, 2016
Merged

Re-block running insecure content #3808

merged 4 commits into from
Sep 9, 2016

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Sep 8, 2016

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

  1. Add an option to re-block running insecure content
  2. Limit the effect of running insecure content in regular/private frame scope

fix #3770, #3793

partially fix #3795 (only for runInsecureContent)

Auditors: @diracdeltas

Test Plan:
For re-block option:

  1. Go to https://mixed-script.badssl.com/
  2. Click urlIcon and click Load Unsafe Scripts
  3. Background should be red
  4. Click urlIcon and click Stop Loading Unsafe Scripts
  5. Background should be grey

For regular frame:

  1. Go to https://mixed-script.badssl.com
  2. Click urlIcon and click Load Unsafe Scripts
  3. Background should be red
  4. Go to https://mixed-script.badssl.com by regular new tab
  5. Background should be grey
  6. Go to https://mixed-script.badssl.com by private new tab
  7. Background should be grey
  8. Go to https://mixed-script.badssl.com by session new tab
  9. Background should be grey

~~For private frame:

  1. Go to https://mixed-script.badssl.com by private tab
  2. Click urlIcon and click Load Unsafe Scripts
  3. Background should be red
  4. Go to https://mixed-script.badssl.com by regular new tab
  5. Background should be grey
  6. Go to https://mixed-script.badssl.com by private new tab
  7. Background should be grey
  8. Go to https://mixed-script.badssl.com by session new tab
  9. Background should be grey~~

2. Re-block running insecure content when frame closed

fix brave#3770

Auditors: @diracdeltas

Test Plan:
For re-block option:
1. Go to `https://mixed-script.badssl.com/`
2. Click urlIcon and click `Load Unsafe Scripts`
3. Background should be red
4. Click urlIcon and click `Stop Loading Unsafe Scripts`
5. Background should be grey

For re-blocked on frame closed
1. Go to `https://mixed-script.badssl.com/`
2. Click urlIcon and click `Load Unsafe Scripts`
3. Background should be red
4. Close tab
5. Go to `https://mixed-script.badssl.com/` on new tab again
6. Background should be grey
allowRunInsecureContent=Load Unsafe Scripts
dismissDenyRunInsecureContent=Stay Unsecure
Copy link
Member

Choose a reason for hiding this comment

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

s/Unsecure/Insecure

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 138692c. Thanks

… scope

fix brave#3793

Auditors: @diracdeltas

Test Plan:
For regular frame:
1. Go to https://mixed-script.badssl.com
2. Click urlIcon and click Load Unsafe Scripts
3. Background should be red
4. Go to https://mixed-script.badssl.com by regular new tab
5. Background should be grey
6. Go to https://mixed-script.badssl.com by private new tab
7. Background should be grey

For private frame:
1. Go to https://mixed-script.badssl.com by private tab
2. Click urlIcon and click Load Unsafe Scripts
3. Background should be red
4. Go to https://mixed-script.badssl.com by regular new tab
5. Background should be grey
6. Go to https://mixed-script.badssl.com by private new tab
7. Background should be grey
ipc.emit(messages.SHORTCUT_ACTIVE_FRAME_LOAD_URL, {}, this.isBlockedRunInsecureContent)
this.props.onHide()
}
onDenyRunInsecureContent () {
appActions.removeSiteSetting(siteUtil.getOrigin(this.runInsecureContent), 'runInsecureContent')
Copy link
Member

Choose a reason for hiding this comment

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

should this have this.isPrivate as the last argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, you are right. I forgot to add it.

@bridiver
Copy link
Collaborator

bridiver commented Sep 8, 2016

the regular vs private profile code can be removed and is handled by brave/muon@b956fa5

@bridiver
Copy link
Collaborator

bridiver commented Sep 8, 2016

I guess my comment above isn't completely accurate because we are still writing them to the app state. We need to fully migrate site settings to content settings to fix the problem completely. In the meantime I suggest that we disallow the setting in private tabs for now just like the others

@diracdeltas
Copy link
Member

diracdeltas commented Sep 8, 2016

++, i think this feature should just be disabled in private tabs for now. AKA mixed content should always just be blocked in private tabs. sorry about that @darkdh

@darkdh
Copy link
Member Author

darkdh commented Sep 9, 2016

@diracdeltas , feedback addressed in 7aded75

}
onAllowRunInsecureContent () {
appActions.changeSiteSetting(siteUtil.getOrigin(this.isBlockedRunInsecureContent), 'runInsecureContent', true)
appActions.changeSiteSetting(siteUtil.getOrigin(this.isBlockedRunInsecureContent),
'runInsecureContent', true, this.isPrivate)
ipc.emit(messages.SHORTCUT_ACTIVE_FRAME_LOAD_URL, {}, this.isBlockedRunInsecureContent)
Copy link
Member

Choose a reason for hiding this comment

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

the code above can also use this.location instead of this.isBlockedRunInsecureContent

Copy link
Member

Choose a reason for hiding this comment

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

but that is more of a refactor issue so this looks good to merge now. :)

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