fix(generic info provider): urlencode urls#1242
fix(generic info provider): urlencode urls#1242hrueger wants to merge 2 commits intoPart-DB:masterfrom
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 😅 |
|
I do not know where your issue arises from. Do you have a reverse proxy or weirdly configured webserver, which somehow messes up pathes containing more than one colon? |
|
Hi @jbtronics, However, I'm unsure what settings to change ;-) services:
traefik:
image: traefik:latest
container_name: traefik
command:
- "--api.insecure=true"
- "--providers.docker=true"
- "--providers.docker.exposedbydefault=false"
- "--entrypoints.web.address=:8088"
ports:
- "8088:8088"
# - "8089:8080" # Traefik dashboard
volumes:
- /var/run/docker.sock:/var/run/docker.sock:ro
networks:
- traefik-net
restart: unless-stopped
partdb:
container_name: partdb
volumes:
- ./uploads:/var/www/html/uploads
- ./public_media:/var/www/html/public/media
- ./db:/var/www/html/var/db
restart: unless-stopped
image: jbtronics/part-db1:latest
networks:
- traefik-net
labels:
- "traefik.enable=true"
- "traefik.http.routers.partdb.rule=Host(`partdb.localhost`)"
- "traefik.http.routers.partdb.entrypoints=web"
- "traefik.http.services.partdb.loadbalancer.server.port=80"
environment:
# Put SQLite database in our mapped folder. You can configure some other kind of database here too.
- DATABASE_URL=sqlite:///%kernel.project_dir%/var/db/app.db
# In docker env logs will be redirected to stderr
- APP_ENV=docker
# Uncomment this, if you want to use the automatic database migration feature. With this you have you do not have to
# run the doctrine:migrations:migrate commands on installation or upgrade. A database backup is written to the uploads/
# folder (under .automigration-backup), so you can restore it, if the migration fails.
# This feature is currently experimental, so use it at your own risk!
# - DB_AUTOMIGRATE=true
# You can configure Part-DB using environment variables
# Below you can find the most essential ones predefined
# However you can add any other environment configuration you want here
# See .env file for all available options or https://docs.part-db.de/configuration.html
# !!! Do not use quotes around the values, as they will be interpreted as part of the value and this will lead to errors !!!
# The language to use serverwide as default (en, de, ru, etc.)
- DEFAULT_LANG=en
# The default timezone to use serverwide (e.g. Europe/Berlin)
- DEFAULT_TIMEZONE=Europe/Berlin
# The currency that is used inside the DB (and is assumed when no currency is set). This can not be changed later, so be sure to set it the currency used in your country
- BASE_CURRENCY=EUR
# The name of this installation. This will be shown as title in the browser and in the header of the website
- INSTANCE_NAME=Part-DB
# Allow users to download attachments to the server by providing an URL
# This could be a potential security issue, as the user can retrieve any file the server has access to (via internet)
- ALLOW_ATTACHMENT_DOWNLOADS=0
# Use gravatars for user avatars, when user has no own avatar defined
- USE_GRAVATAR=0
# Override value if you want to show a given text on homepage.
# When this is empty the content of config/banner.md is used as banner
#- BANNER=This is a test banner<br>with a line break
# If you use a reverse proxy in front of Part-DB, you must configure the trusted proxies IP addresses here (see reverse proxy documentation for more information):
- TRUSTED_PROXIES=127.0.0.0/8,::1,10.0.0.0/8,172.16.0.0/12,192.168.0.0/16
# If you need to install additional composer packages (e.g., for specific mailer transports), you can specify them here:
# The packages will be installed automatically when the container starts
# - COMPOSER_EXTRA_PACKAGES=symfony/mailgun-mailer symfony/sendgrid-mailer
networks:
traefik-net:
driver: bridge |
2 participants
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!