这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@dogancanbakir
Copy link
Member

@dogancanbakir dogancanbakir commented Sep 24, 2025

closes #1648

Summary by CodeRabbit

  • New Features

    • Support for overriding default configuration file locations via environment variables SUBFINDER_CONFIG and SUBFINDER_PROVIDER_CONFIG.
  • Documentation

    • Added an Environment Variables section to the README explaining these variables.
    • Clarified installation instructions with a go install command example.
  • Tests

    • Updated test suite to ignore an additional external data source during passive tests.

@coderabbitai
Copy link

coderabbitai bot commented Sep 24, 2025

Walkthrough

Adds support for two environment variables read at init to override the default config file locations and documents them in the README: SUBFINDER_CONFIG (config.yaml) and SUBFINDER_PROVIDER_CONFIG (provider-config.yaml). Also updates tests to ignore one additional passive source.

Changes

Cohort / File(s) Summary
Documentation: Env vars
README.md
Added an "Environment Variables" section documenting SUBFINDER_CONFIG and SUBFINDER_PROVIDER_CONFIG to override default config file paths; installation snippet left unchanged.
Runner options: Env overrides
pkg/runner/options.go
Read SUBFINDER_CONFIG and SUBFINDER_PROVIDER_CONFIG at initialization using the env util and set defaultConfigLocation and defaultProviderConfigLocation via envutil.GetEnvOrDefault before flag parsing. No exported signatures changed.
Tests: passive sources
pkg/passive/sources_wo_auth_test.go
Added "threatcrowd" to the ignored sources list in TestSourcesWithoutKeys, excluding it from that test run.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant User
  participant Env as Environment
  participant CLI as subfinder CLI
  participant Runner as ParseOptions

  User->>CLI: Run command
  CLI->>Runner: ParseOptions()
  Runner->>Env: Get SUBFINDER_CONFIG
  Env-->>Runner: Value or empty
  Runner->>Env: Get SUBFINDER_PROVIDER_CONFIG
  Env-->>Runner: Value or empty
  alt Env vars present
    Note over Runner: Override default config paths
  else Not present
    Note over Runner: Use built-in default locations
  end
  Runner-->>CLI: Options with resolved config paths
  CLI-->>User: Proceed with execution
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • ehsandeep

Poem

I hop through code and read the signs,
Two env paths set like dotted lines.
Configs found where I decide,
In Docker burrows I now glide.
A rabbit's cheer — quick, neat, and wide. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning The addition of "threatcrowd" to the list of ignored sources in the passive sources test is unrelated to the feature request for environment variable support and does not address any objectives outlined in issue #1648. Remove or relocate the test change for ignoring "threatcrowd" into a separate pull request to keep this PR focused solely on the environment variable support feature.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly and concisely indicates the addition of environment variable support specifically for configuration file paths, matching the primary change introduced by the pull request and avoiding unnecessary detail or generic phrasing.
Linked Issues Check ✅ Passed The implementation meets the objectives by using environment variables SUBFINDER_CONFIG and SUBFINDER_PROVIDER_CONFIG to override default configuration paths, updating the documentation accordingly and maintaining fallback behavior to preserve backward compatibility with existing functionality.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 1648_add_env_var_support

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
README.md (1)

112-118: Clarify precedence and include a quick usage example.

Explicitly stating precedence reduces ambiguity and helps Docker/CI users copy-paste.

 ## Environment Variables

 Subfinder supports environment variables to specify custom paths for configuration files:

 - `SUBFINDER_CONFIG` - Path to config.yaml file (overrides default `$CONFIG/subfinder/config.yaml`)
 - `SUBFINDER_PROVIDER_CONFIG` - Path to provider-config.yaml file (overrides default `$CONFIG/subfinder/provider-config.yaml`)
+
+Precedence: CLI flags override environment variables, which override defaults.
+
+Example:
+
+```sh
+export SUBFINDER_CONFIG=/mnt/config/config.yaml
+export SUBFINDER_PROVIDER_CONFIG=/mnt/config/provider-config.yaml
+subfinder -d example.com
+```
pkg/runner/options.go (1)

121-128: Normalize env-provided paths to avoid whitespace/relative-path gotchas.

Trim and clean the paths; use LookupEnv to distinguish “unset” vs “set to empty”.

-	if envConfig := os.Getenv("SUBFINDER_CONFIG"); envConfig != "" {
-		defaultConfigLocation = envConfig
-	}
+	if v, ok := os.LookupEnv("SUBFINDER_CONFIG"); ok {
+		v = filepath.Clean(strings.TrimSpace(v))
+		if v != "" {
+			defaultConfigLocation = v
+		}
+	}
 
-	if envProviderConfig := os.Getenv("SUBFINDER_PROVIDER_CONFIG"); envProviderConfig != "" {
-		defaultProviderConfigLocation = envProviderConfig
-	}
+	if v, ok := os.LookupEnv("SUBFINDER_PROVIDER_CONFIG"); ok {
+		v = filepath.Clean(strings.TrimSpace(v))
+		if v != "" {
+			defaultProviderConfigLocation = v
+		}
+	}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 45fac83 and 20682b5.

📒 Files selected for processing (2)
  • README.md (1 hunks)
  • pkg/runner/options.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). (4)
  • GitHub Check: Test Builds (windows-latest)
  • GitHub Check: Test Builds (ubuntu-latest)
  • GitHub Check: Test Builds (macOS-13)
  • GitHub Check: Analyze (go)
🔇 Additional comments (2)
pkg/runner/options.go (2)

121-128: LGTM — env defaults applied before flag registration; CLI flags still win.
Repo search shows SUBFINDER_CONFIG / SUBFINDER_PROVIDER_CONFIG and the default* variables only in pkg/runner/options.go (definition/env-read/flag registration), pkg/runner/runner.go (usage) and README — no other resets found.


121-128: Confirm intended behavior for config.yaml loading with env defaults. Currently SUBFINDER_CONFIG only changes defaultConfigLocation (pkg/runner/options.go:121-128) and the flag default is set from that value (pkg/runner/options.go:130); MergeConfigFile is only called when options.Config != defaultConfigLocation (pkg/runner/options.go:166-172). With SUBFINDER_CONFIG set but no -config flag, MergeConfigFile will not run — change the condition if you want the env var to trigger loading.

@dogancanbakir
Copy link
Member Author

we can ignore test fails

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pkg/runner/options.go (1)

154-158: Create provider-config at the user-selected path (not always the default)

Post-parse, honor -pc/ProviderConfig (which may come from env) when creating the file. Using the default path here ignores user choice.

Apply:

- if exists := fileutil.FileExists(defaultProviderConfigLocation); !exists {
-     if err := createProviderConfigYAML(defaultProviderConfigLocation); err != nil {
+ if exists := fileutil.FileExists(options.ProviderConfig); !exists {
+     if err := createProviderConfigYAML(options.ProviderConfig); err != nil {
          gologger.Error().Msgf("Could not create provider config file: %s\n", err)
      }
  }
🧹 Nitpick comments (2)
pkg/runner/options.go (2)

160-165: Optionally auto-load config.yaml when SUBFINDER_CONFIG is set

Current logic only merges when --config differs from default. If users set SUBFINDER_CONFIG (without --config), they may expect it to load. Consider merging when the env var is present.

Apply:

- if options.Config != defaultConfigLocation {
+ if options.Config != defaultConfigLocation || os.Getenv("SUBFINDER_CONFIG") != "" {
     // An empty source file is not a fatal error
     if err := flagSet.MergeConfigFile(options.Config); err != nil && !errors.Is(err, io.EOF) {
         gologger.Fatal().Msgf("Could not read config: %s\n", err)
     }
 }

If you prefer to keep current semantics, please explicitly document that SUBFINDER_CONFIG only sets the default path and does not trigger auto-loading.


123-125: Nice-to-have: surface env vars in flag help

Makes discoverability better from -h alone.

- flagSet.StringVar(&options.Config, "config", defaultConfigLocation, "flag config file"),
- flagSet.StringVarP(&options.ProviderConfig, "provider-config", "pc", defaultProviderConfigLocation, "provider config file"),
+ flagSet.StringVar(&options.Config, "config", defaultConfigLocation, "flag config file (env: SUBFINDER_CONFIG)"),
+ flagSet.StringVarP(&options.ProviderConfig, "provider-config", "pc", defaultProviderConfigLocation, "provider config file (env: SUBFINDER_PROVIDER_CONFIG)"),
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 20682b5 and e9338c2.

📒 Files selected for processing (1)
  • pkg/runner/options.go (2 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). (4)
  • GitHub Check: Test Builds (windows-latest)
  • GitHub Check: Test Builds (ubuntu-latest)
  • GitHub Check: Test Builds (macOS-13)
  • GitHub Check: Analyze (go)
🔇 Additional comments (2)
pkg/runner/options.go (2)

18-18: Import of env util looks good

Alias and package path are consistent with ProjectDiscovery utils.


27-28: Approve env var documentation
README.md includes SUBFINDER_CONFIG and SUBFINDER_PROVIDER_CONFIG under the configuration section.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
pkg/passive/sources_wo_auth_test.go (2)

47-52: Reduce bursty rate-limits to avoid 3rd‑party bans/timeouts

Using math.MaxInt32 with a 1ms window is hostile to remote services and likely contributes to flaky failures (incl. unmarshal errors on stressed endpoints).

Consider a gentler default for tests:

-		multiRateLimiter, _ = addRateLimiter(ctxParent, multiRateLimiter, source.Name(), math.MaxInt32, time.Millisecond)
+		// Reduce burst to lower risk of bans/timeouts in CI
+		multiRateLimiter, _ = addRateLimiter(ctxParent, multiRateLimiter, source.Name(), 10, 200*time.Millisecond)

If the project has per-source recommended limits, prefer wiring those instead.

Please confirm this still yields at least one result per source consistently in CI.


26-37: Gate “threatcrowd” ignore to CI-only

Apply in pkg/passive/sources_wo_auth_test.go:

-   "threatcrowd",    // failing with "randomly failing with unmarshal error when hit multiple times"
-}
+}
+// CI-only quarantine for flaky source; keep local coverage
+if os.Getenv("CI") != "" {
+   ignoredSources = append(ignoredSources, "threatcrowd") // intermittently fails with unmarshal errors when invoked multiple times
+}

Also add to imports:

import (
    // …
    "os"
)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e9338c2 and c292a9c.

⛔ Files ignored due to path filters (1)
  • .github/workflows/build-test.yml is excluded by !**/*.yml
📒 Files selected for processing (1)
  • pkg/passive/sources_wo_auth_test.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). (4)
  • GitHub Check: Test Builds (macOS-latest)
  • GitHub Check: Test Builds (windows-latest)
  • GitHub Check: Test Builds (ubuntu-latest)
  • GitHub Check: Analyze (go)

@Mzack9999 Mzack9999 merged commit 1464ef0 into dev Sep 24, 2025
10 checks passed
@Mzack9999 Mzack9999 deleted the 1648_add_env_var_support branch September 24, 2025 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: Environment Variables for Config File Paths

3 participants