[PR #6556] [MERGED] [client] fix nil pointer panic when applying SSH server setting to an existing config #28442

Closed
opened 2026-08-05 08:06:22 -04:00 by saavagebueno · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/6556
Author: @Adel-Ayoub
Created: 6/26/2026
Status: Merged
Merged: 6/28/2026
Merged by: @riccardomanfrin

Base: mainHead: fix/server-ssh-allowed-nil-panic


📝 Commits (1)

  • 3754e93 [client] fix nil pointer panic when applying SSH server setting to an existing config

📊 Changes

2 files changed (+30 additions, -1 deletions)

View changed files

📝 client/internal/profilemanager/config.go (+1 -1)
📝 client/internal/profilemanager/config_test.go (+29 -0)

📄 Description

Describe your changes

Config.apply dereferenced *config.ServerSSHAllowed when comparing the stored value
against the incoming ConfigInput.ServerSSHAllowed. New configs are safe because
createNewConfig seeds the field (util.False()), but configs written before
ServerSSHAllowed was introduced do not contain the field and unmarshal it to nil.
Updating such a config while also supplying the SSH server flag therefore panicked:

panic: runtime error: invalid memory address or nil pointer dereference
  (*Config).apply  client/internal/profilemanager/config.go:436
  update           client/internal/profilemanager/config.go:828
  UpdateConfig     client/internal/profilemanager/config.go:792

The nil case was already meant to be handled — the else if config.ServerSSHAllowed == nil
backwards-compatibility branch right below restores SSH for configs from older versions — but
the crash happens before that branch is reached. Every other *bool field in apply compares
pointers safely; only this one dereferenced *config.

The fix guards the dereference: when the stored value is nil, the user-supplied value is
applied directly. Added a table-driven test that reproduces the panic (it fails without the
change) for both enabling and disabling the SSH server.

N/A — found while reviewing client config handling.

Stack

Checklist

By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md).

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Internal crash fix with no user-facing change: the --allow-server-ssh flag and its behavior
are unchanged; this only stops a nil pointer panic when the value is applied to a config that
predates the field.

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

N/A

Summary by CodeRabbit

  • Bug Fixes

    • Fixed a crash that could occur when updating SSH access settings on older configurations that did not yet have that value set.
    • Updates now apply correctly whether the existing setting is missing or already defined.
  • Tests

    • Added coverage for updating SSH access settings from an older saved configuration to ensure the value is set as expected.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/netbirdio/netbird/pull/6556 **Author:** [@Adel-Ayoub](https://github.com/Adel-Ayoub) **Created:** 6/26/2026 **Status:** ✅ Merged **Merged:** 6/28/2026 **Merged by:** [@riccardomanfrin](https://github.com/riccardomanfrin) **Base:** `main` ← **Head:** `fix/server-ssh-allowed-nil-panic` --- ### 📝 Commits (1) - [`3754e93`](https://github.com/netbirdio/netbird/commit/3754e9356657014f5f486be484f3bf1b179dec67) [client] fix nil pointer panic when applying SSH server setting to an existing config ### 📊 Changes **2 files changed** (+30 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `client/internal/profilemanager/config.go` (+1 -1) 📝 `client/internal/profilemanager/config_test.go` (+29 -0) </details> ### 📄 Description ## Describe your changes `Config.apply` dereferenced `*config.ServerSSHAllowed` when comparing the stored value against the incoming `ConfigInput.ServerSSHAllowed`. New configs are safe because `createNewConfig` seeds the field (`util.False()`), but configs written before `ServerSSHAllowed` was introduced do not contain the field and unmarshal it to `nil`. Updating such a config while also supplying the SSH server flag therefore panicked: ``` panic: runtime error: invalid memory address or nil pointer dereference (*Config).apply client/internal/profilemanager/config.go:436 update client/internal/profilemanager/config.go:828 UpdateConfig client/internal/profilemanager/config.go:792 ``` The `nil` case was already meant to be handled — the `else if config.ServerSSHAllowed == nil` backwards-compatibility branch right below restores SSH for configs from older versions — but the crash happens before that branch is reached. Every other `*bool` field in `apply` compares pointers safely; only this one dereferenced `*config`. The fix guards the dereference: when the stored value is `nil`, the user-supplied value is applied directly. Added a table-driven test that reproduces the panic (it fails without the change) for both enabling and disabling the SSH server. ## Issue ticket number and link N/A — found while reviewing client config handling. ## Stack <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (if possible) - [x] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [[CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first)](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [[Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md)](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) Internal crash fix with no user-facing change: the `--allow-server-ssh` flag and its behavior are unchanged; this only stops a nil pointer panic when the value is applied to a config that predates the field. ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: N/A <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed a crash that could occur when updating SSH access settings on older configurations that did not yet have that value set. * Updates now apply correctly whether the existing setting is missing or already defined. * **Tests** * Added coverage for updating SSH access settings from an older saved configuration to ensure the value is set as expected. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
saavagebueno added the pull-request label 2026-08-05 08:06:22 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#28442