Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion desktop/angular/src/app/pages/app-view/app-view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,28 +218,28 @@
return;
}

if (!this.appProfile!.Config) {

Check warning on line 221 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Forbidden non-null assertion
this.appProfile.Config = {}
}

// If the value has been "reset to global value" we need to
// set the value to "undefined".
if (event.isDefault) {
setAppSetting(this.appProfile!.Config, event.key, undefined);

Check warning on line 228 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Forbidden non-null assertion
} else {
setAppSetting(this.appProfile!.Config, event.key, event.value);

Check warning on line 230 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Forbidden non-null assertion
}

// Actually safe the profile
this.profileService.saveProfile(this.appProfile!).subscribe({

Check warning on line 234 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Forbidden non-null assertion
next: () => {
if (!!event.accepted) {

Check failure on line 236 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Redundant double negation
event.accepted();
}
},
error: (err) => {
// if there's a callback function for errors call it.
if (!!event.rejected) {

Check failure on line 242 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Redundant double negation
event.rejected(err);
}

Expand Down Expand Up @@ -275,15 +275,33 @@
return;
}

const source = this.appProfile.Source;
const id = this.appProfile.ID;

this.dialog
.create(EditProfileDialog, {
backdrop: true,
autoclose: false,
data: `${this.appProfile.Source}/${this.appProfile.ID}`,
data: `${source}/${id}`,
})
.onAction('deleted', () => {
// navigate to the app overview if it has been deleted.
this.router.navigate(['/app/']);
})
.onAction('saved', () => {
// After save, the backend may have migrated the profile to a new ID
// (when fingerprints changed). Verify it still exists at the same ID;
// if not, navigate to the overview so the user can re-open the app
// and the component reinitializes with the correct new profile.
this.profileService.getAppProfile(`${source}/${id}`).subscribe({
error: () => {
this.actionIndicator.info(
'Profile ID Changed',
'The fingerprint change caused the profile to be re-keyed. You can find the app in the app list to continue editing.'
);
this.router.navigate(['/app/']);
},
Comment on lines +291 to +303

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Differentiate “profile migrated” from generic fetch failures

At Line 297, every getAppProfile() error is treated as fingerprint migration. This will mislabel transient/backend errors as “Profile ID Changed” and force navigation away from the page.

Use status-aware handling (e.g., only treat not-found as migration), and surface other errors normally.

Suggested fix
       .onAction('saved', () => {
         // After save, the backend may have migrated the profile to a new ID
         // (when fingerprints changed). Verify it still exists at the same ID;
         // if not, navigate to the overview so the user can re-open the app
         // and the component reinitializes with the correct new profile.
         this.profileService.getAppProfile(`${source}/${id}`).subscribe({
-          error: () => {
-            this.actionIndicator.info(
-              'Profile ID Changed',
-              'The fingerprint change caused the profile to be re-keyed. You can find the app in the app list to continue editing.'
-            );
-            this.router.navigate(['/app/']);
-          },
+          error: (err) => {
+            const status = (err as { status?: number })?.status;
+            if (status === 404) {
+              this.actionIndicator.info(
+                'Profile ID Changed',
+                'The fingerprint change caused the profile to be re-keyed. You can find the app in the app list to continue editing.'
+              );
+              this.router.navigate(['/app/']);
+              return;
+            }
+
+            this.actionIndicator.error(
+              'Failed to verify saved profile',
+              this.actionIndicator.getErrorMessgae(err)
+            );
+          },
         });
       });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@desktop/angular/src/app/pages/app-view/app-view.ts` around lines 291 - 303,
The error handler in the onAction('saved') flow currently treats any error from
profileService.getAppProfile(`${source}/${id}`) as a profile migration; update
the handler to check the HTTP status (or error type) from getAppProfile and only
show the "Profile ID Changed" message and call router.navigate(['/app/']) when
the server returns a 404/not-found; for other errors, surface them via
actionIndicator.error (or rethrow/log) so transient/backend errors are not
misclassified. Locate the getAppProfile call in the onAction('saved') block and
adjust the subscribe error callback to branch on error.status (or the API's
error shape) before calling actionIndicator.info and router.navigate.

});
});
}

Expand All @@ -301,13 +319,13 @@
.cleanProfileHistory(this.appProfile.Source + '/' + this.appProfile.ID)
.subscribe({
next: (res) => {
observer.next!(res);

Check warning on line 322 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Forbidden non-null assertion
this.historyAvailableSince = null;
this.connectionsInHistory = 0;
this.cdr.markForCheck();
},
error: (err) => {
observer.error!(err);

Check warning on line 328 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Forbidden non-null assertion
},
});
}
Expand Down Expand Up @@ -397,11 +415,11 @@
distinctUntilChanged()
),
]).subscribe(
async ([profile, queryMap, global, allSettings, viewSetting]) => {

Check warning on line 418 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

'viewSetting' is defined but never used
const previousProfile = this.appProfile;

if (!!profile) {

Check failure on line 421 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Redundant double negation
const key = profile![0].Source + '/' + profile![0].ID;

Check warning on line 422 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Forbidden non-null assertion

Check warning on line 422 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Forbidden non-null assertion

const query: Condition = {
profile: key,
Expand Down Expand Up @@ -442,7 +460,7 @@
.subscribe((result) => {
if (result.length > 0) {
this.historyAvailableSince = new Date(
result[0].first_connection!

Check warning on line 463 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Forbidden non-null assertion
);
this.connectionsInHistory = result[0].totalCount;
} else {
Expand Down Expand Up @@ -477,7 +495,7 @@

this._loading = false;

if (!!this.appProfile?.PresentationPath) {

Check failure on line 498 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Redundant double negation
let parts: string[] = [];
let sep = '/';
if (this.appProfile.PresentationPath[0] === '/') {
Expand All @@ -501,7 +519,7 @@

// if we have a profile flatten it's configuration map to something
// more useful.
if (!!this.appProfile) {

Check failure on line 522 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Redundant double negation
profileConfig = flattenProfileConfig(this.appProfile.Config);
}

Expand All @@ -519,7 +537,7 @@
// the action button would fail and the user would not notice something
// changing.
//
if (!!this.highlightSettingKey) {

Check failure on line 540 in desktop/angular/src/app/pages/app-view/app-view.ts

View workflow job for this annotation

GitHub Actions / Lint

Redundant double negation
if (profileConfig[this.highlightSettingKey] === undefined) {
this.viewSettingChange.next('all');
}
Expand Down
30 changes: 28 additions & 2 deletions service/profile/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package profile

import (
"errors"
"fmt"
"strings"

"github.com/safing/portmaster/base/config"
Expand Down Expand Up @@ -61,8 +62,26 @@ func startProfileUpdateChecker() error {
return errors.New("subscription canceled")
}

// Get active profile.
scopedID := strings.TrimPrefix(r.Key(), ProfilesDBPath)

// Check if fingerprints changed such that the profile ID needs to be
// re-derived. This must run before the activeProfile lookup so that it
// also applies when the app is not currently running.
if !r.Meta().IsDeleted() {
if receivedProfile, parseErr := EnsureProfile(r); parseErr == nil && !receivedProfile.savedInternally {
if len(receivedProfile.Fingerprints) > 0 &&
DeriveProfileID(receivedProfile.Fingerprints) != receivedProfile.ID {
if renameErr := migrateProfileOnFingerprintChange(receivedProfile); renameErr != nil {
log.Errorf("profile: failed to rename profile %s after fingerprint change: %s", scopedID, renameErr)
}
// The rename saves a new profile and deletes the old one, each of
// which produces its own feed event. Skip further processing here.
continue profileFeed
}
}
}

// Get active profile.
activeProfile := getActiveProfile(scopedID)
if activeProfile == nil {
// Check if profile is being deleted.
Expand Down Expand Up @@ -109,7 +128,7 @@ type databaseHook struct {
database.HookBase
}

// UsesPrePut implements the Hook interface and returns false.
// UsesPrePut implements the Hook interface and returns true.
func (h *databaseHook) UsesPrePut() bool {
return true
}
Expand All @@ -133,6 +152,13 @@ func (h *databaseHook) PrePut(r record.Record) (record.Record, error) {
// prepare profile
profile.prepProfile()

// Validate fingerprints: reject saves with unparseable fingerprints (e.g. bad regex)
// to prevent a profile from being saved in a state where it can never match.
_, fpErr := ParseFingerprints(profile.Fingerprints, profile.LinkedPath)
if fpErr != nil {
return nil, fmt.Errorf("invalid fingerprint: %w", fpErr)
}

// parse config
err = profile.parseConfig()
if err != nil {
Expand Down
60 changes: 60 additions & 0 deletions service/profile/merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import (
"sync"
"time"

"github.com/safing/portmaster/base/database"
"github.com/safing/portmaster/base/database/record"
"github.com/safing/portmaster/base/log"
"github.com/safing/portmaster/service/profile/binmeta"
)

Expand Down Expand Up @@ -102,3 +104,61 @@ func addFingerprints(existing, add []Fingerprint, from string) []Fingerprint {

return existing
}

// migrateProfileOnFingerprintChange creates a new profile whose ID is derived
// from the updated fingerprints, copies all settings from the old profile,
// deletes the old profile, and emits EventMigrated. This mirrors the pattern
// used by MergeProfiles and ensures that:
// - history DB entries are migrated (via EventMigrated → netquery handler)
// - active connections are re-attributed (via EventDelete → reAttributeConnections)
// - future connections find the profile via normal fingerprint matching
func migrateProfileOnFingerprintChange(old *Profile) error {
newDerivedID := DeriveProfileID(old.Fingerprints)

// Abort if a profile with the target ID already exists — the user may have
// set fingerprints that conflict with another existing profile.
_, existsErr := profileDB.Get(ProfilesDBPath + MakeScopedID(old.Source, newDerivedID))
if existsErr == nil {
log.Debugf("profile: skipping rename of %s: target ID %s already exists", old.ScopedID(), newDerivedID)
return nil
}
if !errors.Is(existsErr, database.ErrNotFound) {
return fmt.Errorf("failed to check for existing profile %s: %w", newDerivedID, existsErr)
}

// Build the new profile. ID is left empty so New() derives it from Fingerprints.
newProfile := New(&Profile{
Source: old.Source,
Name: old.Name,
Description: old.Description,
Warning: old.Warning,
WarningLastUpdated: old.WarningLastUpdated,
Homepage: old.Homepage,
Icon: old.Icon,
IconType: old.IconType,
Icons: old.Icons,
LinkedPath: old.LinkedPath,
PresentationPath: old.PresentationPath,
UsePresentationPath: old.UsePresentationPath,
Fingerprints: old.Fingerprints,
Config: old.Config,
LastEdited: time.Now().Unix(),
Internal: old.Internal,
})
// Preserve the original creation timestamp (New() always overwrites it).
newProfile.Created = old.Created

if err := newProfile.Save(); err != nil {
return fmt.Errorf("failed to save renamed profile: %w", err)
}

if err := old.delete(); err != nil {
return fmt.Errorf("failed to delete old profile %s: %w", old.ScopedID(), err)
}

module.EventMigrated.Submit([]string{old.ScopedID(), newProfile.ScopedID()})
log.Infof("profile: renamed profile %q from %s to %s due to fingerprint change",
old.Name, old.ScopedID(), newProfile.ScopedID())

return nil
}
Loading