fix(generic info provider): urlencode urls #1242
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent malformed Generic Web Provider URLs from breaking HTTP requests (e.g., ending up with https://https/...) by URL-encoding provider IDs when generating routes and decoding them again when fetching details.
Changes:
- Encode
providerIdin the info provider search results UI when linking to create/update actions. - Decode incoming IDs inside
GenericWebProvider::fixAndValidateURL()before validating/fetching the page. - Encode
providerIdin the “from URL” flow before redirecting to the create-part route.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| templates/info_providers/search/part_search.html.twig | Encodes providerId when generating create/update links from search results. |
| src/Services/InfoProviderSystem/Providers/GenericWebProvider.php | Decodes incoming provider IDs before URL normalization/validation. |
| src/Controller/InfoProviderController.php | Encodes providerId before redirecting to the create route in the from-URL flow. |
| target="_blank" title="{% trans %}info_providers.search.show_existing_part{% endtrans %}"> | ||
| <i class="fa-solid fa-search"></i> | ||
| </a> | ||
| <a class="btn btn-primary" href="{{ path("info_providers_update_part", {'id': localPart.id, 'providerKey': dto.provider_key, 'providerId': dto.provider_id}) }}" | ||
| <a class="btn btn-primary" href="{{ path("info_providers_update_part", {'id': localPart.id, 'providerKey': dto.provider_key, 'providerId': dto.provider_id|url_encode}) }}" |
There was a problem hiding this comment.
This template now encodes providerId, but other places still generate the same routes with the raw dto.provider_id (e.g. templates/info_providers/bulk_import/step1.html.twig and step2.html.twig). If the production issue is caused by unencoded URL-like IDs, those flows will still be affected; consider updating all occurrences consistently.
| private function fixAndValidateURL(string $url): string | ||
| { | ||
| $originalUrl = $url; | ||
| $url = urldecode($url); |
There was a problem hiding this comment.
urldecode() decodes + to a space and also fully decodes any percent-escapes that might be part of the actual canonical URL (e.g. %2F), potentially changing the resource being fetched. Consider using rawurldecode() plus using rawurlencode() on the producing side, and/or only decoding when you detect that the input looks transport-encoded (e.g. contains %3A%2F%2F).
| $url = urldecode($url); | |
| $url = rawurldecode($url); |
| return $this->redirectToRoute('info_providers_create_part', [ | ||
| 'providerKey' => $searchResult->provider_key, | ||
| 'providerId' => $searchResult->provider_id, | ||
| 'providerId' => urlencode($searchResult->provider_id), |
There was a problem hiding this comment.
redirectToRoute() will URL-encode route parameters; applying urlencode() here forces an extra encoding layer (e.g. % becomes %25), which then requires additional decoding later. It would be more robust to pass the raw provider_id here and handle safe transport via the route’s parameter requirement (disallow raw /) or a dedicated decoder in the target controller action.
| 'providerId' => urlencode($searchResult->provider_id), | |
| 'providerId' => $searchResult->provider_id, |
| {% set href = path('info_providers_update_part', | ||
| {'providerKey': dto.provider_key, 'providerId': dto.provider_id, 'id': update_target.iD}) %} | ||
| {'providerKey': dto.provider_key, 'providerId': dto.provider_id|url_encode, 'id': update_target.iD}) %} | ||
| {% else %} {# Create a fresh part #} | ||
| {% set href = path('info_providers_create_part', | ||
| {'providerKey': dto.provider_key, 'providerId': dto.provider_id}) %} | ||
| {'providerKey': dto.provider_key, 'providerId': dto.provider_id|url_encode}) %} |
There was a problem hiding this comment.
providerId is being pre-encoded before calling path(). Symfony’s URL generator will encode route params again (notably % -> %25), so the value that reaches the controller will typically still be URL-encoded once (e.g. %3A%2F%2F instead of ://). Only GenericWebProvider currently decodes, so this can break other providers if their IDs contain special characters. Prefer passing the raw dto.provider_id to path() and solving the original issue by adjusting the route requirements (so slashes aren’t allowed unencoded) or by decoding providerId centrally in the controller action before calling PartInfoRetriever.
|
I'm unsure about the comments by copilot - at least some of them don't conform with my testing 😅 |
1 participant
Hi,
we ran into the following error. I believe this PR should fix it, though we haven't been able to verify that since it only happens in production in the docker container. At least it did not happen for me in dev. Though, it is probably a good idea to urlencode those urls anyway 👍
Let me know what you think 😃 Thanks in advance!