Skip to content

Support URL-encoded data-URL favicons - #6191

Open
evnchn wants to merge 4 commits into
zauberzeug:mainfrom
evnchn:midnight/favicon-107
Open

Support URL-encoded data-URL favicons#6191
evnchn wants to merge 4 commits into
zauberzeug:mainfrom
evnchn:midnight/favicon-107

Conversation

@evnchn

@evnchn evnchn commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Motivation

Developer passes favicon='data:image/svg+xml,%3Csvg%20.../%3E' (a valid, URL-encoded data URL, the same form NiceGUI generates internally) to ui.run(). is_file() returns False (data: guard), so startup wires get_favicon_response; a request to /favicon.ico calls _data_url_to_bytes -> b64decode drops the '%','<' etc. and returns corrupt bytes served as image/svg+xml. A value like 'data:foo' (no comma) instead raises ValueError -> HTTP 500. Fix: detect the ';base64' marker and branch to urllib.parse.unquote for non-base64 data URLs; guard the missing-comma case.

Minimal reproduction (nicegui/favicon.py:107):

from nicegui import core, favicon

svg = '<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16"><circle cx="8" cy="8" r="7"/></svg>'
data_url = favicon._svg_to_data_url(svg)
assert data_url.startswith('data:') and ';base64' not in data_url

core.app.config.favicon = data_url

try:
    favicon.get_favicon_response()
except Exception as e:
    print(f'BUG REPRODUCED: serving a NiceGUI-generated data-URL favicon raised '
          f'{type(e).__name__}: {e}')
else:
    raise SystemExit('BUG FIXED: data-URL favicon served without error')

BUG REPRODUCED: serving a NiceGUI-generated data-URL favicon raised Error: Incorrect padding

Implementation

Fixed _data_url_to_bytes to branch on ;base64 marker (base64 path byte-identical; percent-encoded uses urllib.parse.unquote) and partition() to drop the comma-less ValueError.

Verification
  • fail-first test tests/test_favicon.
  • Local gates: pre-commit · mypy ./nicegui · pylint 10.00/10 · pytest (touched) — all green.

Progress

  • The PR title is a short phrase starting with a verb.
  • The implementation is complete.
  • This PR does not address a security issue.
  • Pytests have been added/updated.
  • Documentation is not necessary.
  • No breaking changes to the public API.

evnchn and others added 2 commits July 25, 2026 00:14
DateInput applied .props('no-parent-event') to the inner QDate, but
Quasar 2.18.5's QDate has no such prop (noParentEvent belongs to the
anchor-props mixin behind QMenu/QTooltip). It was an inert no-op
attribute; removing it changes no behavior (open/close is driven by the
parent QMenu).

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
`_data_url_to_bytes` assumed every `data:` favicon was base64, so a
percent-encoded data URL (the form `_svg_to_data_url` itself generates)
was fed to `b64decode`, silently corrupting the bytes or raising
`binascii.Error` on `/favicon.ico`. Branch on the `;base64` marker,
mirroring `_svg_to_data_url`'s `urllib.parse.quote`, and use `partition`
so a comma-less `data:` value no longer raises `ValueError`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@evnchn
evnchn marked this pull request as ready for review July 25, 2026 06:17
@evnchn evnchn added bug Type/scope: Incorrect behavior in existing functionality review Status: PR is open and needs review labels Jul 26, 2026
@falkoschindler falkoschindler self-assigned this Jul 26, 2026
@falkoschindler
falkoschindler self-requested a review July 26, 2026 20:37
falkoschindler and others added 2 commits July 28, 2026 22:23
The test now sets a URL-encoded data URL via `ui.run` and checks what
`/favicon.ico` actually serves, mirroring `test_data_url` right above it.
Reverting the fix makes it fail on the user-visible symptom (the route
returns 500) rather than on a helper's return value.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Type/scope: Incorrect behavior in existing functionality review Status: PR is open and needs review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants