Closed
Bug 998007
Opened 10 years ago
Closed 10 years ago
use safebrowsing app id for application reputation remote query
Categories
(Core :: DOM: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: mmc, Assigned: mmc)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
2.89 KB,
patch
|
mmc
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Updated•10 years ago
|
Blocks: downloadprotection
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8408552 -
Attachment is obsolete: true
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8408553 [details] [diff] [review] Use Safebrowsing app id for application reputation checks ( Review of attachment 8408553 [details] [diff] [review]: ----------------------------------------------------------------- I can't include anything that relies on ipc/chromium in the toolkit/components/downloads directory, because it interacts badly with existing Windows code. Refactor LoadContext.h not to depend on SerializedLoadContext which depends on ipc/chromium.
Attachment #8408553 -
Flags: review?(mozilla)
Attachment #8408553 -
Flags: review?(gpascutto)
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8408553 [details] [diff] [review] Use Safebrowsing app id for application reputation checks ( Review of attachment 8408553 [details] [diff] [review]: ----------------------------------------------------------------- Hi Olli, I notice you previously did some reviews on LoadContext.h. If you have time, please review that part of it. Thanks, Monica
Attachment #8408553 -
Flags: review?(bugs)
Comment 5•10 years ago
|
||
Comment on attachment 8408553 [details] [diff] [review] Use Safebrowsing app id for application reputation checks ( Review of attachment 8408553 [details] [diff] [review]: ----------------------------------------------------------------- I don't really know this code but the mIsNotNull thing looks really strange. ::: docshell/base/SimpleLoadContext.h @@ +46,5 @@ > + , mUsePrivateBrowsing(aUsePrivateBrowsing) > + , mUseRemoteTabs(aUseRemoteTabs) > + , mIsInBrowserElement(aIsInBrowserElement) > +#ifdef DEBUG > + , mIsNotNull(true) This makes no sense. If you're going to fix it to true, then you should remove all the debugging checks in SimpleLoadContext that assert if it is null. But if they're there for a reason, why not pass it down?
Attachment #8408553 -
Flags: review?(gpascutto) → review-
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8408553 [details] [diff] [review] Use Safebrowsing app id for application reputation checks ( Review of attachment 8408553 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/SimpleLoadContext.h @@ +46,5 @@ > + , mUsePrivateBrowsing(aUsePrivateBrowsing) > + , mUseRemoteTabs(aUseRemoteTabs) > + , mIsInBrowserElement(aIsInBrowserElement) > +#ifdef DEBUG > + , mIsNotNull(true) It's only ever not null in LoadContext.h. I don't think I can only have mIsNotNull in LoadContext.h, though because it's just a constructor for SimpleLoadContext that knows about SerializedLoadContext, and SimpleLoadContext.cpp is the only implementation of them both. This is the original: http://mxr.mozilla.org/mozilla-central/source/docshell/base/LoadContext.h#58
Comment 7•10 years ago
|
||
Comment on attachment 8408553 [details] [diff] [review] Use Safebrowsing app id for application reputation checks ( Review of attachment 8408553 [details] [diff] [review]: ----------------------------------------------------------------- WriteUp from IRC chat: It seems you are only changing LoadContext to SimpleLoadContext, right? Can you summarize real quick, why using the regular LoadContext does not do the trick? You said, it required some IPC/chromium changes that undefed some windows apis? What for example? Still don't fully understand why we can't use the regular LoadContext. Other than that, this looks good! I still don't think i should be reviewing this, but of course am available for feedback. ::: toolkit/components/downloads/ApplicationReputation.cpp @@ +769,5 @@ > NS_LITERAL_CSTRING("POST"), false); > NS_ENSURE_SUCCESS(rv, rv); > > + // Set the Safebrowsing cookie jar. > + nsCOMPtr<nsIInterfaceRequestor> loadContext = Nit: Please extend that comment; why are we setting the safebrowsing id here.
Attachment #8408553 -
Flags: review?(mozilla)
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7) > Comment on attachment 8408553 [details] [diff] [review] > Use Safebrowsing app id for application reputation checks ( > > Review of attachment 8408553 [details] [diff] [review]: > ----------------------------------------------------------------- > > WriteUp from IRC chat: > It seems you are only changing LoadContext to SimpleLoadContext, right? Can > you summarize real quick, why using the regular LoadContext does not do the > trick? You said, it required some IPC/chromium changes that undefed some > windows apis? What for example? Still don't fully understand why we can't > use the regular LoadContext. > Other than that, this looks good! I still don't think i should be reviewing > this, but of course am available for feedback. > > ::: toolkit/components/downloads/ApplicationReputation.cpp > @@ +769,5 @@ > > NS_LITERAL_CSTRING("POST"), false); > > NS_ENSURE_SUCCESS(rv, rv); > > > > + // Set the Safebrowsing cookie jar. > > + nsCOMPtr<nsIInterfaceRequestor> loadContext = > > Nit: Please extend that comment; why are we setting the safebrowsing id here. Thanks, Chris -- I mostly asked you since you were the original author of the SAFEBROWSING_APP_ID stuff :) Including LoadContext forces inclusion of ipc/chromium which contains #undef CreateEvent (https://mxr.mozilla.org/mozilla-central/search?string=CreateEvent&find=.h%24&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central) which breaks nsDownloadScanner in this directory with "unknown identifier: CreateEvent".
Comment 9•10 years ago
|
||
Isn't that something that you could work around by reordering the order of includes and potentially disabling unified building for that file? If the #undef happens after the place where CreateEvent is used, it should work?
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Gian-Carlo Pascutto [:gcp] from comment #9) > Isn't that something that you could work around by reordering the order of > includes and potentially disabling unified building for that file? If the > #undef happens after the place where CreateEvent is used, it should work? I tried including a bunch of these headers at the top of DownloadScanner but it still failed. http://msdn.microsoft.com/en-us/library/windows/desktop/ms682396(v=vs.85).aspx
Comment 11•10 years ago
|
||
Comment on attachment 8408553 [details] [diff] [review] Use Safebrowsing app id for application reputation checks ( This feels complicated. To have LoadContext and SimpleLoadContext. Could you not just un-inline LoadContext's ctor and forward declare IPC::SerializedLoadContext in the .h file?
Attachment #8408553 -
Flags: review?(bugs) → review-
Comment 12•10 years ago
|
||
Here's the approach from my comment, i.e. disable the unified build for that file so the #undef doesn't spread: https://tbpl.mozilla.org/?tree=Try&rev=460d7fde7ed4
Assignee | ||
Comment 13•10 years ago
|
||
Not sure if gcp can review this since he modified the patch.
Attachment #8408553 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8410503 -
Flags: review?(sstamm)
Comment 14•10 years ago
|
||
Comment on attachment 8410503 [details] [diff] [review] Use safebrowsing app-id for application reputation check Review of attachment 8410503 [details] [diff] [review]: ----------------------------------------------------------------- This looks somewhat reasonable. Can you test it easily? I'd like ckerschb to review this as well, since he's familiar with the code.
Attachment #8410503 -
Flags: review?(mozilla)
Comment 15•10 years ago
|
||
Comment on attachment 8410503 [details] [diff] [review] Use safebrowsing app-id for application reputation check Review of attachment 8410503 [details] [diff] [review]: ----------------------------------------------------------------- gps: can you sanity-check the changes to moz.build?
Attachment #8410503 -
Flags: feedback?(gps)
Assignee | ||
Comment 16•10 years ago
|
||
I think the cookie separation is tested pretty well in the cookie tests that ckerschb already wrote. Is that sufficient? https://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_cookiejars_safebrowsing.js
Comment 17•10 years ago
|
||
Comment on attachment 8410503 [details] [diff] [review] Use safebrowsing app-id for application reputation check Review of attachment 8410503 [details] [diff] [review]: ----------------------------------------------------------------- If I am understanding everything correctly, we use the SAFEBROWSING_APP_ID since Google maintains an extension to Safebrowsing that tracks binary file reputation and we want those cookies to be separated into the same jar as safebrowsing cookies, correct? As you mentioned, we already have tests that verify correct cookie-jar separation for safebrowsing cookies, so in my opinion that should be sufficient in terms of testing the cookie-jar separation. I assume you have a testcase the verifies correct behavior of the application reputation service in a different bug, right? As I mentioned in a previous comment, maybe you want to extend the comment in ApplicationReputation.cpp a little, before you set the SAFEBROWSING_APP_ID. Other than that, this looks good to me and ready to go.
Attachment #8410503 -
Flags: review?(mozilla) → review+
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8410503 [details] [diff] [review] Use safebrowsing app-id for application reputation check Thanks, ckerschb! From froydnj, this counts as a trivial build change that does not require build peer review. Clearing flags. <mmc> glandium, does a build change to disable unified builds in toolkit/components/url-classifier require build peer review? 10:47 AM <froydnj> mmc: if you're just shuffling things between SOURCES and UNIFIED_SOURCES, I don't think it does 10:48 AM <froydnj> since that falls under "adding and removing from existing moz.build lists"
Attachment #8410503 -
Flags: review?(sstamm)
Attachment #8410503 -
Flags: feedback?(gps)
Comment 19•10 years ago
|
||
Great, land it.
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8410503 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Comment on attachment 8411200 [details] [diff] [review] Use SafeBrowsing app id for application reputation checks. Review of attachment 8411200 [details] [diff] [review]: ----------------------------------------------------------------- Updated comment to be: + // Set the Safebrowsing cookie jar, so that the regular Google cookie is not + // sent with this request. See bug 897516. (In reply to Christoph Kerschbaumer [:ckerschb] from comment #17) > If I am understanding everything correctly, we use the SAFEBROWSING_APP_ID > since Google maintains an extension to Safebrowsing that tracks binary file > reputation and we want those cookies to be separated into the same jar as > safebrowsing cookies, correct? Yes, same as your previous bug for safebrowsing cookie isolation. > As you mentioned, we already have tests that > verify correct cookie-jar separation for safebrowsing cookies, so in my > opinion that should be sufficient in terms of testing the cookie-jar > separation. I assume you have a testcase the verifies correct behavior of > the application reputation service in a different bug, right? Yes, existing tests are in the same directory under test_app_rep.js and test_app_rep_windows.js. Marking checkin-needed since the tree is closed. Try was green at https://tbpl.mozilla.org/?tree=Try&rev=460d7fde7ed4
Attachment #8411200 -
Flags: review+
Attachment #8411200 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 22•10 years ago
|
||
Comment on attachment 8411200 [details] [diff] [review] Use SafeBrowsing app id for application reputation checks. Review of attachment 8411200 [details] [diff] [review]: ----------------------------------------------------------------- https://hg.mozilla.org/integration/mozilla-inbound/rev/c3954977ec62
Attachment #8411200 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 23•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c3954977ec62
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•