fix: create default provider config on first run instead of fatal error (fixes #487) - #542
Conversation
Update README.md
…lack-username Slack Provider: Fixes slack_username and adds slack_icon_emoji
…or (fixes projectdiscovery#487) When notify is run for the first time without a provider config file (the default is ~/.config/notify/provider-config.yaml), the runner previously returned a fatal error: [FTL] Could not create runner: file doesn't exist This is confusing because the user hasn't been told where to create the file or what it should contain. Fix: if the default config path does not exist, create the config directory and write a commented template file. A helpful INFO message is printed pointing the user to the file they need to fill in. If the write fails (unlikely, but possible on read-only filesystems), a warning is logged and execution continues so the error message is clearer. This also fixes the Windows Server 2012 R2 regression where the .config/notify/ directory never existed and always caused a crash.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough
ChangesFirst-run provider config initialization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@internal/runner/runner.go`:
- Around line 44-67: The code has two issues: (1) It does not distinguish
between default and user-specified config paths when deciding whether to create
a template. Before the os.Stat check on options.ProviderConfig, track a boolean
flag indicating whether the default path is being used (set this flag when
options.ProviderConfig was empty and defaulted), then only perform the template
creation logic when this flag is true, ensuring user-specified paths via the -pc
flag return an error if missing. (2) The default template contains only comments
and no actual provider configuration, causing the YAML parse to produce an empty
providerOptions struct that silently succeeds when passed to providers.New with
no error or indication to the user. Add validation after parsing the YAML to
reject empty provider configurations with a clear error message explaining that
the file was created as a template and the user must add provider credentials,
or alternatively update the template to include a valid minimal example (such as
an uncommented but placeholder provider entry) to guide users toward correct
configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0eb5960-cdf8-4fae-bba8-e496467cd2bd
📒 Files selected for processing (1)
internal/runner/runner.go
| // If the default config file does not exist yet, create the directory and | ||
| // write an empty/commented template so the user knows what to fill in. | ||
| // This avoids a fatal error on first run (e.g. fresh Windows install). | ||
| if _, statErr := os.Stat(options.ProviderConfig); os.IsNotExist(statErr) { | ||
| if mkdirErr := os.MkdirAll(filepath.Dir(options.ProviderConfig), 0700); mkdirErr != nil { | ||
| return nil, errors.Wrap(mkdirErr, "could not create provider config directory") | ||
| } | ||
| defaultConfig := `# notify provider configuration | ||
| # Fill in your provider details below. Full documentation: | ||
| # https://docs.projectdiscovery.io/tools/notify/provider-config | ||
| # | ||
| # Example: | ||
| # slack: | ||
| # - id: "my-slack" | ||
| # slack_webhook_url: "https://hooks.slack.com/services/..." | ||
| # slack_username: "notify" | ||
| # slack_format: "{{data}}" | ||
| ` | ||
| if writeErr := os.WriteFile(options.ProviderConfig, []byte(defaultConfig), 0600); writeErr != nil { | ||
| gologger.Warning().Msgf("Could not create default provider config at %s: %s\n", options.ProviderConfig, writeErr) | ||
| } else { | ||
| gologger.Info().Msgf("Created default provider config at %s — please add your provider credentials.\n", options.ProviderConfig) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the providers.New implementation to understand empty-options behavior
ast-grep --pattern 'func New($$$) $$$'Repository: projectdiscovery/notify
Length of output: 12463
User-specified config paths incorrectly trigger template creation, violating PR objectives.
The PR objectives explicitly state: "The fix only applies when using the default config path (when -pc flag is not specified); explicitly specified config paths still return an error if missing." However, lines 44-67 do not distinguish between default and user-specified paths.
When a user runs notify -pc /custom/path.yaml and that file doesn't exist, this block will create the directory and write a template instead of returning an error. This directly contradicts the intended behavior. Lines 35-42 set options.ProviderConfig to the default only when empty, but once set (either by default or via -pc flag), line 47 treats both identically.
To fix, track whether the default path is being used before the existence check:
+ // Determine if we're using the default config path
+ defaultConfigPath := ""
+ if home, err := os.UserHomeDir(); err == nil {
+ defaultConfigPath = filepath.Join(home, types.DefaultProviderConfigLocation)
+ }
+ isDefaultPath := (options.ProviderConfig == defaultConfigPath)
+
// If the default config file does not exist yet, create the directory and
// write an empty/commented template so the user knows what to fill in.
// This avoids a fatal error on first run (e.g. fresh Windows install).
- if _, statErr := os.Stat(options.ProviderConfig); os.IsNotExist(statErr) {
+ if isDefaultPath && _, statErr := os.Stat(options.ProviderConfig); os.IsNotExist(statErr) {Empty provider config silently succeeds rather than failing with clear error.
The template at lines 51-61 contains only comments. After the file is created, line 74 parses this comment-only YAML, producing an empty providerOptions struct. When this empty struct is passed to providers.New at line 80, the function does not error—it silently returns a client with no providers configured. Users will then run notify with a freshly created config but receive no notification output and no error message, creating confusing UX where the tool appears to "work" but doesn't actually send anything.
Consider either (1) adding validation to reject empty provider configurations with a clear error message, or (2) including a valid minimal example in the template (e.g., an uncommented but placeholder provider entry) to guide users toward correct configuration.
🤖 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 `@internal/runner/runner.go` around lines 44 - 67, The code has two issues: (1)
It does not distinguish between default and user-specified config paths when
deciding whether to create a template. Before the os.Stat check on
options.ProviderConfig, track a boolean flag indicating whether the default path
is being used (set this flag when options.ProviderConfig was empty and
defaulted), then only perform the template creation logic when this flag is
true, ensuring user-specified paths via the -pc flag return an error if missing.
(2) The default template contains only comments and no actual provider
configuration, causing the YAML parse to produce an empty providerOptions struct
that silently succeeds when passed to providers.New with no error or indication
to the user. Add validation after parsing the YAML to reject empty provider
configurations with a clear error message explaining that the file was created
as a template and the user must add provider credentials, or alternatively
update the template to include a valid minimal example (such as an uncommented
but placeholder provider entry) to guide users toward correct configuration.
…mments YAML Per code review feedback: - Track usingDefaultPath flag before applying the default; only create the template when the path was defaulted. User-specified paths that don't exist now return an explicit error instead of silently creating a template. - Tolerate io.EOF from yaml.Decoder.Decode when the config file is empty or contains only comments (e.g. freshly-created default template), so first run no longer silently fails with a nil providerOptions.
|
Applied the code review suggestions:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@internal/runner/runner_test.go`:
- Around line 39-43: The test currently only validates file permissions (0600)
for the config file but does not verify the parent directory permissions (0700)
which are created in runner.go. Within the same conditional block checking
runtime.GOOS != "windows", add an additional permission check for the parent
directory by obtaining its file info (similar to how you get info for the file)
and verifying that the directory mode permissions equal 0o700, logging an error
if they do not match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0e2061d-bf10-402a-93a8-1a7938f2346e
📒 Files selected for processing (1)
internal/runner/runner_test.go
| if runtime.GOOS != "windows" { | ||
| if perm := info.Mode().Perm(); perm != 0o600 { | ||
| t.Errorf("config file perms = %o, want 0600", perm) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add directory permission check to validate 0700 on the parent directory.
The implementation creates the parent directory with 0700 permissions (as shown in context snippet runner.go:54), but the test only verifies file permissions. Directory permissions should also be validated since they're part of the security posture.
🔒 Proposed addition to verify directory permissions
if runtime.GOOS != "windows" {
if perm := info.Mode().Perm(); perm != 0o600 {
t.Errorf("config file perms = %o, want 0600", perm)
}
+ dirInfo, dirStatErr := os.Stat(filepath.Dir(options.ProviderConfig))
+ if dirStatErr != nil {
+ t.Fatalf("stat config directory: %v", dirStatErr)
+ }
+ if dirPerm := dirInfo.Mode().Perm(); dirPerm != 0o700 {
+ t.Errorf("config directory perms = %o, want 0700", dirPerm)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if runtime.GOOS != "windows" { | |
| if perm := info.Mode().Perm(); perm != 0o600 { | |
| t.Errorf("config file perms = %o, want 0600", perm) | |
| } | |
| } | |
| if runtime.GOOS != "windows" { | |
| if perm := info.Mode().Perm(); perm != 0o600 { | |
| t.Errorf("config file perms = %o, want 0600", perm) | |
| } | |
| dirInfo, dirStatErr := os.Stat(filepath.Dir(options.ProviderConfig)) | |
| if dirStatErr != nil { | |
| t.Fatalf("stat config directory: %v", dirStatErr) | |
| } | |
| if dirPerm := dirInfo.Mode().Perm(); dirPerm != 0o700 { | |
| t.Errorf("config directory perms = %o, want 0700", dirPerm) | |
| } | |
| } |
🤖 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 `@internal/runner/runner_test.go` around lines 39 - 43, The test currently only
validates file permissions (0600) for the config file but does not verify the
parent directory permissions (0700) which are created in runner.go. Within the
same conditional block checking runtime.GOOS != "windows", add an additional
permission check for the parent directory by obtaining its file info (similar to
how you get info for the file) and verifying that the directory mode permissions
equal 0o700, logging an error if they do not match.
# Conflicts: # cmd/integration-test/integration.go
5 participants
Summary
Fixes #487
Problem
On a fresh install (including Windows Server 2012 R2 which doesn't pre-create
~/.config/), runningnotifywithout a provider config produces:The error is technically correct but unhelpful — nobody tells the user where to create the file or what it should contain.
Fix
Before calling
SubstituteConfigFromEnvVars, check if the default config path exists. If not:mkdir -p)[INF]message directing them to the fileThe new first-run experience:
Notes
-pcis not specified); explicit-pcpaths still return an error if they don't exist[WRN]and execution continues to give a clearer error downstreamSummary by CodeRabbit