Add support for missing events (fix indexedDB, etc) #629
Conversation
| namespace { | ||
|
|
||
| const uint32_t kRenderFilteredMessageClasses[] = { | ||
| ChromeMsgStart, NetworkHintsMsgStart, |
There was a problem hiding this comment.
I think we only need ChromeMsgStart because we are not handling any NetworkHintsMsg_* messages
| const base::string16& name, | ||
| const base::string16& display_name, | ||
| bool* allowed) { | ||
| *allowed = true; |
There was a problem hiding this comment.
these need to check cookie content settings or they will bypass blocking
There was a problem hiding this comment.
also OnAllowDOMStorage , OnRequestFileSystemAccess and OnAllowIndexedDB
| DCHECK_CURRENTLY_ON(BrowserThread::IO); | ||
|
|
||
| Send(new ChromeViewMsg_RequestFileSystemAccessAsyncResponse( | ||
| render_frame_id, request_id, true)); |
There was a problem hiding this comment.
cc @diracdeltas - this doesn't seem like the right thing to do here. There is a permission in Chrome for this
| DCHECK_CURRENTLY_ON(BrowserThread::IO); | ||
|
|
||
|
|
||
| ChromeViewHostMsg_RequestFileSystemAccessSync::WriteReplyParams(reply_msg, |
There was a problem hiding this comment.
also cc @diracdeltas same as below sync response
There was a problem hiding this comment.
I removed this as I don't believe we need it
| const GURL& top_origin_url, | ||
| const base::string16& name, | ||
| bool* allowed) { | ||
| *allowed = true; |
There was a problem hiding this comment.
this should also check cookie content settings
|
i don't understand this code very well but agree it generally needs to respect cookie blocking content settings. the setting for cookie blocking should be applied to all types of persistent local storage (including FileSystem API, indexdb, and DOM storage). you can find a test for this in browser-laptop test/fixtures/cookies.html |
|
Thanks for the review feedback- looking at this now... |
|
Ready for re-review! I added a test plan and also removed the |
bsclifton
changed the title
| const base::string16& display_name, | ||
| bool* allowed) { | ||
| *allowed = | ||
| cookie_settings_->IsCookieAccessAllowed(origin_url, top_origin_url); |
There was a problem hiding this comment.
this should be using the HostContentSettingsMap. We don't use Chrome's CookieSettingsFactory or IsCookieAccessAllowed. HostContentSettingsMapFactory::GetForProfile(profile_)
There was a problem hiding this comment.
I can update the code - but it does work as-is. Once I fetch the HostContentSettingsMap, what would I check?
There was a problem hiding this comment.
ContentSetting GetContentSetting(
const GURL& primary_url,
const GURL& secondary_url,
ContentSettingsType content_type,
const std::string& resource_identifier) const;
|
ready for re-re-review edit: SQUASHED! |
- AllowDatabase - AllowDOMStorage - AllowIndexedDB *Includes fixes from review feedback* Fixes brave/browser-laptop#12463 Fixes brave/browser-laptop#14475 Possibly addresses brave/browser-laptop#10685
| bool BraveRenderMessageFilter::OnMessageReceived(const IPC::Message& message) { | ||
| bool handled = true; | ||
| IPC_BEGIN_MESSAGE_MAP(BraveRenderMessageFilter, message) | ||
| IPC_MESSAGE_HANDLER(ChromeViewHostMsg_AllowDatabase, OnAllowDatabase) |
There was a problem hiding this comment.
there's also an async version of these messages
Add support for missing events (fix indexedDB, etc)
Test plan
IndexedDB / AllowDatabase testing
Cookie Controlshields are set toBlock 3rd Party CookiesRun @ WebWorkerbuttonCookie Controlshields setting toBlock all cookiesRun @ WebWorkerbuttonlocal storage testing
Cookie Controlshields are set toBlock 3rd Party CookiesChoose background colordrop downCookie Controlshields setting toBlock all cookiesChoose background colortwitch.tv testing
Details
Add support for missing events:
Fixes brave/browser-laptop#12463
Fixes brave/browser-laptop#14475
Possibly addresses brave/browser-laptop#10685
Auditors: @darkdh, @bridiver