Closed Bug 1517483 Opened 1 year ago Closed 1 year ago

Get rid of nsIScriptSecurityManager::IsSystemPrincipal

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: bzbarsky, Assigned: sakshaat.dc, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file, 1 obsolete file)

This state can be queried from the principal directly, in both JS and C++.
Priority: -- → P3

Hi, I am new to open source and looking for my first bug. can I start with this bug, looking forward to getting your guide in order to understand the problem.

Hello, Rohan.

The idea here is to find places where nsIScriptSecurityManager::IsSystemPrincipal is called and replace those with IsSystemPrincipal checks on the principal itself.

For C++ callers, you want to look at https://searchfox.org/mozilla-central/search?q=symbol:_ZN24nsIScriptSecurityManager17IsSystemPrincipalEP12nsIPrincipal&redirect=false and https://searchfox.org/mozilla-central/search?q=symbol:_ZN24nsIScriptSecurityManager17IsSystemPrincipalEP12nsIPrincipalPb&redirect=false

For JS callers you want to look at https://searchfox.org/mozilla-central/search?q=symbol:%23isSystemPrincipal%2C_ZN24nsIScriptSecurityManager17IsSystemPrincipalEP12nsIPrincipalPb&redirect=false (which may also include some C++ callers).

I would probably recommend creating three separate changesets here: one to remove C++ callers, one to remove JS callers, one to remove the API completely.

Please feel free to ask around in the #introduction channel on irc.mozilla.org for help getting a build environment set up if you haven't done already, or feel free to e-mail me directly for help with that.

Hi Boris, if Rohan isn't still working on it, is it ok if I give this bug a shot? I have built the codebase and found the affected files, I think I know what needs to be changed, and I'll try to split it between 3 change sets.

Flags: needinfo?(bzbarsky)

Sakshaat, feel free to work on this! Let me know if you need anything.

Flags: needinfo?(bzbarsky)

Hi Boris, I have fixed replaced a few of the uses, however, I am a little stuck with this particular case:

https://searchfox.org/mozilla-central/source/dom/security/FramingChecker.cpp#100

I am wondering how I should tackle this because it seems like it calls the function with a pointer and the function sets a truth value to the function, however, it also returns an nsresult type. How should I proceed with this? My intuition was to convert the following invocation

if (NS_SUCCEEDED(
        ssm->IsSystemPrincipal(topDoc->NodePrincipal(), &system)) &&
    system) {
    // Found a system-principled doc: last docshell was top.
    break;
}

into

if (NS_SUCCEEDED(
        topDoc->NodePrincipal())->GetIsSystemPrincipal(&system)) &&
    system) {
    // Found a system-principled doc: last docshell was top.
    break;
}

Am I on the right track? The code seemed to build just fine, however, that doesn't necessarily imply that it is correct. I would appreciate any help with this. I was also hoping you could help me figure out how I could test if I am breaking anything. I tried to just run ./mach gtest but I am unsure if that's the right set of tests. Thanks!

Flags: needinfo?(bzbarsky)

There are two relevant methods on nsIPrincipal.

There's nsIPrincipal::GetIsSystemPrincipal(), which returns an nsresult and takes a bool* argument, which it uses to set a boolean to a value. This method is basically there only for historical reasons and you shouldn't really use it. The nsresult it returns is always NS_OK, for what it's worth.

There's also nsIPrincipal::IsSystemPrincipal() which takes no arguments and returns a boolean. That's what you should use.

So the code in FramingChecker.cpp should become:

  if (topDoc->NodePrincipal()->IsSystemPrincipal()) {
    break;
  }

and the system local can go away entirely.

The code with your change would work (if the stray ')' after NodePrincipal() is removed); it's the right way to use nsIPrincipal::GetIsSystemPrincipal(). But it's not ideal. ;)

As far as tests, you have a few options. You could try running them locally, but the full set of test suites takes quite a while to run (and for a lot of them using the computer while they're running is not simple). The better options are to either attach the patches to this bug (or submit them to Phabricator) and have me push them to try for you or to follow the directions at https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server to get access to the try server yourself.

Flags: needinfo?(bzbarsky)

Replaced instances of callers in both C++ and JS files to query the state from the principal directly.

Hi Boris, I've created a patch for the issue. After I had committed I realized that you said you recommend two different change sets for the C++ and JS callers, so I am very sorry about that. I still have to remove the calls but I was just wondering if there any issues so far. Thanks for your help!

Flags: needinfo?(bzbarsky)

It's not a big deal to have the one changeset. I commented on the Phabricator revision; the main issue at the moment is that it's against a slightly old tree, so doesn't apply on tip.

Flags: needinfo?(bzbarsky)

Hi Boris, that is very odd. I have pulled from the mozilla-central repository by running hg pull and I also ran hg update, but for some reason it the file that you mentioned that shouldn't exist on TIP still exists. Am I missing something? I am a git user and don't know if I am using mercurial and arcanist right. Thanks!

Flags: needinfo?(bzbarsky)

Hmm. "hg update" run with no args will do something that depends on your exact branch setup.

What does "hg heads" show for you in that repository right now? What about "hg log -r default"? What about "hg parent"?

Assignee: nobody → sakshaat.dc
Flags: needinfo?(bzbarsky) → needinfo?(sakshaat.dc)

I was having really weird issues with mercurial where supposedly my entire repository was corrupted, I just set up a new environment by cloning a fresh version of mozilla-central and applying the diff file from Phabricator. It seems to have worked because the PeerConnection.js file does not exist in this version. I am currently working on fixing the issues you mentioned and then removing the API entirely, I'll submit a patch when it is done. Thank you so much for your help so far!

Flags: needinfo?(sakshaat.dc)

Getting rid of nsIScriptSecurityManager::IsSystemPrincipal by changing all usages to query from the principal itself, and removing the API

Component: DOM → DOM: Core & HTML

Hey, just to check, are you still working on this or planning to? If you're stuck on something, please let me know and I'll see what I can do to help!

Flags: needinfo?(sakshaat.dc)

Note: some of the changes being made in https://phabricator.services.mozilla.com/D23038 will conflict with these changes and need a merge.

Hi Boris, sorry I got really busy with some school work but I can work on it again now. So I looked at the tests, I have fixed the issues that were relatively easy to find such as removing unused vars, etc. I am still trying to figure out how I can find which files may have caused the error for some of the other failures, so I can begin trying to fix them. Right now, for some of the failures, it's not clear to me what may have caused it and where I should look to fix the issue. I would really appreciate any help with that.

For now, I am working on finding more instances of the command being used in places that I may have missed. Those files surprisingly did not show up when I used the links you provided in this thread. I tried to use searchfox to find instances that I may have missed, but I couldn't figure out how to really query it, so I am using grep to essentially find a replace them.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #15)

Note: some of the changes being made in https://phabricator.services.mozilla.com/D23038 will conflict with these changes and need a merge.

Should I wait for this to be merged to mozilla-central or is there something else that I need to do beforehand?

Thanks for all of your help!

Flags: needinfo?(sakshaat.dc) → needinfo?(bzbarsky)

Right now, for some of the failures, it's not clear to me what may have caused it and where I should look to fix the issue. I would really appreciate any help with that.

OK. Which failures are being troublesome in particular? Do you know how to run the relevant tests yourself?

Those files surprisingly did not show up when I used the links you provided in this thread

My apologies. Apparently the "search for IDL method" bit in searchfox searches .js and .xml files but not .html ones. :(

I don't think searchfox will do anything grep doesn't in this case, since it's not pre-indexing this stuff in a useful way.

Should I wait for this to be merged to mozilla-central or is there something else that I need to do beforehand?

It's just something to watch out for. If your changes are ready before that merges, it's fine to go ahead and land and bug 1508817 will get merged on top. If not, then you will need to merge on top of those changes.

Flags: needinfo?(bzbarsky)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #17)

OK. Which failures are being troublesome in particular? Do you know how to run the relevant tests yourself?

For example, the Gtest failure under Linux x64 asan, I see that it has failed but I'm not sure where to find out what test case caused the error. I'm not entirely sure how to run the tests, is it just ./mach test gtest?

It's just something to watch out for. If your changes are ready before that merges, it's fine to go ahead and land and bug 1508817 will get merged on top. If not, then you will need to merge on top of those changes.

Got it, thanks!

Flags: needinfo?(bzbarsky)

but I'm not sure where to find out what test case caused the error

The log line for that failure is:

TEST-UNEXPECTED-FAIL | WebRtcIceConnectTest.TestNetworkOnlineTriggersConsent | Value of: res

Searchfox says "TestNetworkOnlineTriggersConsent" is https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/media/mtransport/test/ice_unittest.cpp#3240-3253 which is of somewhat limited usefulness in terms of figuring out what's going wrong.

You can run that specific test using "mach gtest WebRtcIceConnectTest.TestNetworkOnlineTriggersConsent". Running all of gtest is possible with "mach gtest", but would take a long time and be annoying. ;)

What I expect is going on with that specific test is that it's calling into PeerConnection.jsm and running into https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/dom/media/PeerConnection.jsm#412 not having been updated.

Flags: needinfo?(bzbarsky)

That's very helpful, thank you.

Blocks: 1538262
Attachment #9049811 - Attachment is obsolete: true

So yeah, autolanding https://phabricator.services.mozilla.com/D22532 failed because bug 1508817 had already landed. Do you want to do the merging on top of that, or should I just deal with it?

Flags: needinfo?(sakshaat.dc)

I can do it. Does it just involve pulling, fixing a conflict, and re-uploading another differential?

Flags: needinfo?(sakshaat.dc) → needinfo?(bzbarsky)

Well, updating the existing differential revision. But yes, that's all that's needed.

Flags: needinfo?(bzbarsky)

Right, got it.

I've updated the differential, does it look okay? I had a bit of trouble trying to figure out how to use vimdiff, so I just wanted to make sure.

Flags: needinfo?(bzbarsky)

I commented on the differential revision.

Flags: needinfo?(bzbarsky)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/16bf818b02ac
Get rid of nsIScriptSecurityManager::IsSystemPrincipal r=bzbarsky
Flags: needinfo?(jorgk)

Good question. I'm not maintaining it. Let's ask FRG.

Flags: needinfo?(jorgk) → needinfo?(frgrahl)
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Jorg, there's one user in SeaMonkey at...
Good question. I'm not maintaining it. Let's ask FRG.
Thanks Jorg. will see that it gets fixed when we tackle a future release after 2.57.

Flags: needinfo?(frgrahl)

Sakshaat, thank you very much for fixing this! I thought I'd said that already, but I apparently failed to submit in that window.

I think I learned a lot from this bug fix. Thank you for all of your help!

You're very welcome.

You need to log in before you can comment on or make changes to this bug.