Fix: application startup with error "Cannot read properties of null (reading 'id')"

[faryon93]

Summary

On every startup the application crashes with the following error message:

Application: Mattermost 5.11.2 [commit: 5.11.2]
Platform: Linux 5.10.236-1-MANJARO x64
TypeError: Cannot read properties of null (reading 'id')
    at App.<anonymous> (/usr/lib/mattermost-desktop/app.asar/index.js:2:109191)
    at App.emit (node:events:524:28)

This PR checks for the null value, and aborts operation.

Device Information

This PR was tested on: Arch Linux

Release Note

Fix: application startup with error "Cannot read properties of null (reading 'id')"

[mattermost-build]

Hello @faryon93,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project.

Please help complete the Mattermost contribution license agreement?
Once you have signed the CLA, please comment with /check-cla and confirm that the CLA check is green.

This is a standard procedure for many open source projects.

Please let us know if you have any questions.

We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team.

[faryon93]

/check-cla

[devinbinnie]

@faryon93 Thanks for the PR! Looks like a good error catch, my question is in what scenario does this occur? Is it anytime when you have a server with basic auth configured?

[faryon93]

my question is in what scenario does this occur?

The Messagebox pops up when the application is started. When I don't interact with the message box, I can use mattermost-desktop like normal. When klicken one of the 3 buttons mattermost is closed.

Is it anytime when you have a server with basic auth configured?

The configured server uses Gitlab Authentication. But the message pops up even when there is no server configured at all. The "Welcome -> Get Started" screen is displayed together with the error message.

EDIT
Regarding basic auth: The mattermost desktop instance is sitting behind a http proxy which does http basic auth. It's definately the proxy server; When setting http_proxy and https_proxy evironment variable to empty string, I don't see the error message on startup.

The electron documentation marked the webContents variable as optional, so in my opinion having a null check is good practice.

[devinbinnie]

EDIT Regarding basic auth: The mattermost desktop instance is sitting behind a http proxy which does http basic auth. It's definately the proxy server; When setting http_proxy and https_proxy evironment variable to empty string, I don't see the error message on startup.

The electron documentation marked the webContents variable as optional, so in my opinion having a null check is good practice.

Definitely cool with having the extra null check, I just want to understand what the case is for this. So it looks like it's being called from a Utility process, which probably means that it's for the initial server ping where we retrieve the version and other information. We still want that to succeed though, so we shouldn't just escape from the auth prompt here.

What we likely need to do then is get the server URL and information another way in this handler and still pop the login dialog so that the request doesn't fail. Is this something you'd be willing to take on? If not I can try and have a look at some point.

[faryon93]

it looks like it's being called from a Utility process, which probably means that it's for the initial server ping where we retrieve the version and other information. We still want that to succeed though, so we shouldn't just escape from the auth prompt here.

I don't think the callback is coming from initial server ping, as no server is configured.

Is this something you'd be willing to take on?

You are absolutely right, just returning is not a good idea. I will have a look on monday.

[faryon93]

Further investigation: Electron/Chromium requests the spell checking dictionary on startup. That is done (in my case) by fetching the following url:

https://redirector.gvt1.com/edgedl/chrome/dict/en-us-10-1.bdic

My theory: webContents is null because fetching the dictionary happens before even rendering the first page, but I'm not sure about that.

When I change the spell checking url in the mattermost app settings, the configured URL is requested, so this is definitely the spell checker at work. Disabling spell cheking in the settings, does not stop the dictionary from beeing loaded at startup.

As this happens even without a server configured, checking against the trusted url list (currently implemented) is not possible at all. Would it be a "mattermost-philosophy" approved option to always prompt for login (without permission prompt) when requesting the dictionary url?

EDIT: might be hard to get chromes default url for spell checking, to exactly match it...

[devinbinnie]

My theory: webContents is null because fetching the dictionary happens before even rendering the first page, but I'm not sure about that.

This sounds about right. The dictionary does download before the main window is shown or any webContents are created: https://github.com/mattermost/desktop/blob/release-5.12/src/main/app/initialize.ts#L346

When I change the spell checking url in the mattermost app settings, the configured URL is requested, so this is definitely the spell checker at work. Disabling spell cheking in the settings, does not stop the dictionary from beeing loaded at startup.

What happens when you simply remove the custom spellcheck URL? Does the app behave normally without an error?

As this happens even without a server configured, checking against the trusted url list (currently implemented) is not possible at all. Would it be a "mattermost-philosophy" approved option to always prompt for login (without permission prompt) when requesting the dictionary url?

Once you login once I believe your credentials would be saved until you reset your session or the cookie expired. Right now we just rely on users logging in with basic auth every time (it's not a common case to use basic auth). I wouldn't make use of the trusted origins list for this purpose.

What I'm thinking is that both the spellchecker case and the server check case should be handled, ie. anytime webContents is null we should grab the URL from the request and run basic auth against that. Unfortunately for the spellchecker, that means the window needs to exist first, so we may need to wait for the window to exist before running the check as well.

I'd test out both cases and see if the null check fails in both instances, then account for both.

[faryon93]

What happens when you simply remove the custom spellcheck URL? Does the app behave normally without an error?

When not having a custom spell checking URL configured, chrome requests the default google dictionary (I posted the URL some comments ago).

Once you login once I believe your credentials would be saved until you reset your session or the cookie expired. Right now we just rely on users logging in with basic auth every time (it's not a common case to use basic auth). I wouldn't make use of the trusted origins list for this purpose.

I added coded to display the login form when no webContents is present, like this:

if (!webContents)
{
    this.loginCallbackMap.set(request.url, callback);
    this.popLoginModal(request, authInfo);
    return;
}

This kinda works. Login form is displayed and I can enter my proxy credentials; the dictionary is downloaded without error. Only propblem is that the page beeing displayed stays empty until application restart. The username/password for http basic auth is saved somewhere, as I'm not asked for it on subsequent starting of the application.

[devinbinnie]

I added coded to display the login form when no webContents is present, like this:

if (!webContents)
{
    this.loginCallbackMap.set(request.url, callback);
    this.popLoginModal(request, authInfo);
    return;
}

This kinda works. Login form is displayed and I can enter my proxy credentials; the dictionary is downloaded without error. Only propblem is that the page beeing displayed stays empty until application restart. The username/password for http basic auth is saved somewhere, as I'm not asked for it on subsequent starting of the application.

So this approach looks right, I'm guessing the application won't load correctly without the dictionary, so maybe we might have to wait for the dictionary to load before the app fully loads. One thing that might help here is turning on debug logging and seeing what's not loading normally.

BTW thanks for the work on this, I know it kinda grew in scope :)

[faryon93]

So this approach looks right, I'm guessing the application won't load correctly without the dictionary, so maybe we might have to wait for the dictionary to load before the app fully loads. One thing that might help here is turning on debug logging and seeing what's not loading normally.

I will have a look at this later this day.

BTW thanks for the work on this, I know it kinda grew in scope :)

Yeah, I was a bit naive submitting a PR without considering context or implications ... But a good opportunity to get used to mattermost internals ;)

[faryon93]

The welcome screen is not displayed, because the login prompt is a modal and loading of the welcome screen is prohibited when a modal is open:

desktop/src/main/app/intercom.ts

Lines 40 to 43 in 831f0c7

	if (showWelcomeScreen) {
	if (ModalManager.isModalDisplayed()) {
	return;
	}

As v5.12.0 was release a day after I started investigating, my work was based on the previous release v5.11.2. Nevertheless in the current release, handling of the welcome screen was improved and an open modal doesn't prevent the welcome screen from loading. So the empty screen problem does not exist anymore :)

I will update my PR and add the code proposed in my previous comment. Hope this works out for you.

[faryon93]

@devinbinnie is there more to do? I would be happy to help if there is anything stopping this from getting approved.

[devinbinnie]

@devinbinnie is there more to do? I would be happy to help if there is anything stopping this from getting approved.

Sorry I didn't get a notification until now - will review.

[devinbinnie]

I think we should add an additional check here to ensure that the URL requesting this is either:
a) a trusted URL (see below) - meaning that it's coming from a configured server
b) it's the spellcheck URL

[faryon93]

I don't think there is a way to request the spellchecking url which is used by chrome for downloading the dictionary. So the only feasable way is to hardcode the url, which I don't think is a good solution.

As the mattermost desktop app supports adding multiple servers, should the url be tested against all servers trusted urls?

[devinbinnie]

The default one would not trigger this flow - it's only if you use a custom one, which we can get from the Config.

And yes we should test against all of them, basically just need to make sure one of the configured servers sent this request.

[faryon93]

The default one would not trigger this flow - it's only if you use a custom one, which we can get from the Config.

Nope, the default url (no custom spellchecker url configured in mattermost settings) triggers the flow as well. Thats how I discovered the error in the first place. When browsing the electron documentation, I can't find a way to get the url used to fetch the dictionary. Would it be an option to display the permission modal (like when the url is not trusted).

And yes we should test against all of them, basically just need to make sure one of the configured servers sent this request

Okay, I will try to match against at least one Servers trusted URLs. What should happen when there is no server configured, for example when mattermost is freshly installed?

[devinbinnie]

Nope, the default url (no custom spellchecker url configured in mattermost settings) triggers the flow as well. Thats how I discovered the error in the first place. When browsing the electron documentation, I can't find a way to get the url used to fetch the dictionary. Would it be an option to display the permission modal (like when the url is not trusted).

Hmm, is this because you're connected through a proxy that uses basic auth? The default dictionary shouldn't be protected by basic auth.

Okay, I will try to match against at least one Servers trusted URLs. What should happen when there is no server configured, for example when mattermost is freshly installed?

Then we should default to reject if it doesn't match a trusted URL. However in the above case where it's a proxy, we might need some special logic here...

[faryon93]

Yep, the default dictionary itself is not protected by http basic auth, as the dictionary ist hosted by google for all chromium based browsers.

The problem arises when the mattermost desktop app is sitting behind a http proxy with basic authentication enabled, as sometimes seen in corporate or school environments.

Maybe we can intercept all 407 Proxy Authentication Required requests and show the login modal, without checking against trusted URLs at all, as the Proxy-Authenticate will only be evaluated by the proxy server and is stripped before forwarding the request to the target http server.
Do you think this would be a way to go and it is worth investigating?

[devinbinnie]

So the emitted event for login contains an isProxy field, see: https://www.electronjs.org/docs/latest/api/app#event-login. We could use this to detect if it's a proxy requesting auth, but I'm not sure what the implications are around that from a security perspective, because that means we are allowing any untrusted URL to trigger this flow.

cc @enzowritescode Any concerns here?

[enzowritescode]

I've added this to my backlog. I know it's a small change but I need to go back and read through all the conversations and everything.

[faryon93]

@devinbinnie is it worth updating the pull-request to use isProxy to decide wether to show the login modal or not, before @enzowritescode had a look at the discussion or should I wait?

[devinbinnie]

I'd be okay with that

[mattermost-build]

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

[devinbinnie]

@enzowritescode Are you able to have a look at this any time soon?

[enzowritescode]

@devinbinnie it's in my backlog, but I don't have any idea on when I can get to this.

[mattermost-build]

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

[enzowritescode]

I still haven't had time to dive deeper into this, but I quickly ran it through Claude Code's /security-review to at least provide some sort of feedback and this was the result:

Based on my analysis, I have identified one high-confidence security vulnerability. The filtering task confirmed this is a legitimate security issue
  with a confidence score of 8/10, which meets the threshold for inclusion.

  Security Review Report

  Vuln 1: Authentication Bypass: src/main/authManager.ts:46

  - Severity: High
  - Category: Authentication Bypass / Authorization Logic Flaw
  - Description: The PR introduces a critical authentication bypass vulnerability when webContents is null. The new code path skips all URL validation
  and permission checks, directly presenting login modals for arbitrary HTTP basic authentication requests without security validation.
  - Exploit Scenario: An attacker could trigger HTTP basic auth challenges for malicious domains through utility processes or background requests where
  webContents is null. The application would present authentic-looking login prompts for attacker-controlled URLs (e.g., evil-server.com), potentially
  harvesting user credentials. Users might enter their Mattermost credentials thinking the prompt is legitimate, as no URL validation occurs.
  - Recommendation: Implement URL validation even when webContents is null. Add either: (1) TrustedOriginsStore.checkPermission(parsedURL, 
  BASIC_AUTH_PERMISSION) validation before presenting the modal, (2) whitelist specific utility domains like spell checker services, or (3) always show
  permission modal instead of direct login modal for null webContents scenarios. Also add logging for monitoring authentication requests without
  webContents.