Skip to content

Fix/lapi empty response #6

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Nydauron
Copy link
Contributor

  • Fixes the default config to work with the default LAPI config.
  • Adds extra error handling to properly distinguish when the LAPI request 404s due to double backslashes (e.g. http://localhost:8080//v1/decisions?ip=). Before this patch, crowdsec_proxy() would return OK with the response being the empty string.

This is more or less a bandage fix and doesn't fix the underlying
problem within the module.
@blotus
Copy link
Member

blotus commented Jan 30, 2025

Hey,

Thanks for the PR !

What do you think about also removing automatically extra / at the end of the provided URL ?

That way, the bouncer will work in both case, and it limits the risk of user error.

@Nydauron
Copy link
Contributor Author

Nydauron commented Jan 31, 2025

What do you think about also removing automatically extra / at the end of the provided URL ?

That way, the bouncer will work in both case, and it limits the risk of user error.

Sure, I think that would be a good idea. It would make sense if we simply stored the schema and authority prefix without any trailing slashes inside sconf->url when the module sets it with CrowdSecURL (e.g. http://127.0.0.1).

Some other things to think about:

  • Would we want to add a parsing check for the URL to prevent apache from starting up with a bad URL?
    • An error message can then be provided that gets printed to logs
  • Is CrowdSecURL strictly expecting a <scheme>://<authority>/ format, or would we allow a say values without the scheme (e.g. <hostname>:<port>) to also be valid?

@Nydauron Nydauron force-pushed the fix/lapi-empty-response branch from b8c8870 to e4e0cdb Compare January 31, 2025 22:19
@Nydauron Nydauron force-pushed the fix/lapi-empty-response branch from e4e0cdb to 6fbf8ee Compare February 6, 2025 05:14
@Nydauron
Copy link
Contributor Author

Nydauron commented Feb 6, 2025

Went ahead and implemented a URL validation check when CrowdsecURL gets set. Since most of the other bouncers expect at least a schema and authority in the URL, it makes sense to check if the URL prefix follows <scheme>://<authority> and ignores the rest if there is any.

This simply parses the URL string to ensure the prefix contains a scheme
and an authority, following the RFC 3986 standard. If the scheme and
authority cannot be parsed due to the passed string not following the
RFC standard, Apache will fail to start and print an error to logs.

The path, query, and fragment are subsequently ignored and a warning
gets printed if anything besides the string "/" is found after the
authority.
@Nydauron Nydauron force-pushed the fix/lapi-empty-response branch from 6fbf8ee to 247d0bc Compare February 6, 2025 05:35
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.

2 participants