[iOS] Add force relay connection on iOS#4928
Conversation
…ironment variables" This reverts commit 90ca011.
WalkthroughAdds an iOS-only EnvList type and plumbing to export its entries to OS environment variables; updates Client.Run to accept an EnvList and apply it before running. Several SDK files were restricted to the iOS build tag. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/ios/NetBirdSDK/env_list.go (2)
8-10: Consider thread safety if concurrent access is expected.The
EnvListstruct uses an unprotectedmap[string]string. If the iOS app might callPut,Get, orAllItemsconcurrently from multiple threads before passing the list toRun, data races could occur. Given the expected usage pattern (create, populate, pass to Run), this is likely safe, but consider adding async.RWMutexif concurrent access is a possibility.</review_comment_end>
27-29: Return a defensive copy of the map to prevent accidental external modification.
AllItems()is part of the gomobile-bound public API and currently returns the internal map directly. While the current internal usage inexportEnvListonly reads values, returning the underlying map could allow external callers to accidentally (or intentionally) modify the internal state. Consider making a copy to follow defensive programming patterns consistent with other similar methods in the codebase.func (el *EnvList) AllItems() map[string]string { - return el.data + result := make(map[string]string, len(el.data)) + for k, v := range el.data { + result[k] = v + } + return result }client/ios/NetBirdSDK/client.go (1)
439-453: Function logic is correct; consider simplifying logging.The
exportEnvListfunction properly handles nil input and iterates through environment variables with appropriate error handling. The verbose logging (current value, setting action, success confirmation) is helpful for debugging but could be simplified to reduce log volume.Optional simplification to reduce log verbosity:
func exportEnvList(list *EnvList) { if list == nil { return } for k, v := range list.AllItems() { - log.Debugf("Env variable %s's value is currently: %s", k, os.Getenv(k)) log.Debugf("Setting env variable %s: %s", k, v) - if err := os.Setenv(k, v); err != nil { log.Errorf("could not set env variable %s: %v", k, err) - } else { - log.Debugf("Env variable %s was set successfully", k) } } }</review_comment_end>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/ios/NetBirdSDK/client.go(3 hunks)client/ios/NetBirdSDK/env_list.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
🔇 Additional comments (2)
client/ios/NetBirdSDK/env_list.go (1)
31-34: LGTM! Clean re-export for iOS bindings.Exporting
peer.EnvKeyNBForceRelaythroughGetEnvKeyNBForceRelay()provides the iOS app with type-safe access to the environment variable key.</review_comment_end>
client/ios/NetBirdSDK/client.go (1)
94-95: LGTM! Clean integration of environment variable support.The
Runmethod now accepts anenvListparameter and exports it immediately before configuration loading, ensuring environment variables are available when needed. The nil handling is delegated toexportEnvList, which handles it gracefully.</review_comment_end>
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
client/ios/NetBirdSDK/client.go(3 hunks)client/ios/NetBirdSDK/gomobile.go(1 hunks)client/ios/NetBirdSDK/logger.go(1 hunks)client/ios/NetBirdSDK/login.go(1 hunks)client/ios/NetBirdSDK/peer_notifier.go(1 hunks)client/ios/NetBirdSDK/preferences.go(1 hunks)client/ios/NetBirdSDK/preferences_test.go(1 hunks)client/ios/NetBirdSDK/routes.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- client/ios/NetBirdSDK/login.go
- client/ios/NetBirdSDK/preferences.go
- client/ios/NetBirdSDK/preferences_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release
🔇 Additional comments (7)
client/ios/NetBirdSDK/logger.go (1)
1-2: LGTM! iOS build constraint added correctly.The build tag appropriately restricts this file to iOS compilation without changing any behavior.
client/ios/NetBirdSDK/gomobile.go (1)
1-2: LGTM! iOS build constraint added correctly.The build tag appropriately restricts this gomobile binding file to iOS compilation.
client/ios/NetBirdSDK/peer_notifier.go (1)
1-2: LGTM! iOS build constraint added correctly.The build tag appropriately restricts this file to iOS compilation without affecting the existing peer notification types and methods.
client/ios/NetBirdSDK/routes.go (1)
1-2: LGTM! iOS build constraint added correctly.The build tag appropriately restricts this file to iOS compilation without affecting the existing route selection types and methods.
client/ios/NetBirdSDK/client.go (3)
1-2: LGTM! iOS build constraint added correctly.The build tag appropriately restricts this file to iOS compilation, aligning with the PR's platform-specific changes.
9-9: LGTM! Import added for environment variable support.The
osimport is necessary for the newos.Setenvfunctionality inexportEnvList.
96-97: LGTM! Run signature updated to accept environment variables.The addition of the
envList *EnvListparameter enables iOS apps to pass environment variables before the Go client starts, which aligns with the PR objective. The call toexportEnvListat the start ofRunensures variables are set before configuration loading and authentication.
| func exportEnvList(list *EnvList) { | ||
| if list == nil { | ||
| return | ||
| } | ||
| for k, v := range list.AllItems() { | ||
| log.Debugf("Env variable %s's value is currently: %s", k, os.Getenv(k)) | ||
| log.Debugf("Setting env variable %s: %s", k, v) | ||
|
|
||
| if err := os.Setenv(k, v); err != nil { | ||
| log.Errorf("could not set env variable %s: %v", k, err) | ||
| } else { | ||
| log.Debugf("Env variable %s was set successfully", k) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid logging environment variable values to prevent credential leakage.
Lines 446-447 log both the current and new values of environment variables. Environment variables in this context (e.g., NB_FORCE_RELAY) may contain sensitive information such as authentication tokens, API keys, or relay credentials. Debug logs can be captured by logging frameworks, persisted to disk, or forwarded to external systems, creating a security risk.
Apply this diff to log only the key names without exposing values:
func exportEnvList(list *EnvList) {
if list == nil {
return
}
for k, v := range list.AllItems() {
- log.Debugf("Env variable %s's value is currently: %s", k, os.Getenv(k))
- log.Debugf("Setting env variable %s: %s", k, v)
+ log.Debugf("Setting env variable: %s", k)
if err := os.Setenv(k, v); err != nil {
log.Errorf("could not set env variable %s: %v", k, err)
} else {
log.Debugf("Env variable %s was set successfully", k)
}
}
}🤖 Prompt for AI Agents
In client/ios/NetBirdSDK/client.go around lines 441 to 455, the function
exportEnvList logs environment variable values (current and new), which can leak
credentials; change the logging to only mention the environment variable names
and whether the set succeeded or failed, removing any prints of os.Getenv(k) or
v; on error include the variable name and the error only, and on success log
that the variable was set without including its value.
2 participants
Describe your changes
iOS apps run in a sandbox context the same way android does, and setting environment variables from iOS's side won't work for the internal SDK peer configuration when it reads NB_FORCE_RELAY to decide whether to add an ICE listener to the handshaker.
This PR makes it so the iOS app can pass a list of environment variables in order to be exported by the time the go client starts running, the same way Android does.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.