Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add webhome validator #2276

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Feb 24, 2025

What does this implement/fix?

Add specific validator for webserver.paths.webhome ensuring there are slashes both at the beginning and the end of the string


Related issue or feature (if applicable): Fixes #2274

Pull request in docs with documentation (if applicable): N/A


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

… slashes both at the beginning and the end of the string

Signed-off-by: DL6ER <[email protected]>
@DL6ER DL6ER requested a review from a team February 24, 2025 20:20
@DL6ER DL6ER mentioned this pull request Feb 24, 2025
@DL6ER DL6ER changed the title Ad webhome validator Add webhome validator Feb 24, 2025
@DL6ER DL6ER enabled auto-merge February 24, 2025 20:22
@PromoFaux
Copy link
Member

Computer says no:

image

@rdwebdesign
Copy link
Member

The check fails and the value is used.

I tried with:

  FTLCONF_webserver_paths_webroot: '/home/root/www'
  FTLCONF_webserver_paths_webhome: '/adm'   # (missing the last slash)

and the web interface is redirected to:
http://192.168.0.208/admlogin

@rdwebdesign
Copy link
Member

rdwebdesign commented Feb 25, 2025

The validator is not used when the env vars are used.

But if you try to add an invalid webhome via web interface or using pihole-FTL --config it will correctly fail:

image

2830143614c3:/# pihole-FTL --config webserver.paths.webhome '/admin'
Invalid value: webserver.paths.webhome: file path does not start and end with a slash ("/admin")

@DL6ER
Copy link
Member Author

DL6ER commented Feb 26, 2025

Yes, but that is because the validators cannot be used when using either the config file directly (or via env vars) because there is no way to communicate the warning/error to the user. What should we do in this case? Revert the config and overwrite what users have done? Ignore then env variable and use the default? It was unclear to me but rethinking this yesterday seems to make this a clear strategy.

Note: This is a problem of general scope and not limited to the webhome setting. The validators are never used for config file and env var values so far. If we want to change this, we should do it in a separate PR for cleanness. (#2294)

I think enforcing the validators on env vars (and falling back to ignoring the env var) is still somewhat doable as this will be in the container logs, I do find this more of an issue for direct config file changes. Your feelings may change, hence, I would like to talk about this.

@rdwebdesign
Copy link
Member

The validators are never used for config file and env var values so far.

Yeah... I noticed that when I was checking the code.

I agree with your PR suggestion, to validate env vars and send error messages to the log.

Note:

Not sure how to do it when the config file is directly edited.

We can enforce the validation even in this case. If invalid, we set the value to default and send the errors to FTL.log (not sure if this is very user friendly). Or we simply don't validate in this case and leave the "user input" there. At least, the wrong value will be on the debug log to help us understand the issue.

@DL6ER DL6ER mentioned this pull request Feb 26, 2025
5 tasks
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.

3 participants