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

Percent-encoded data URLs support added #457

Conversation

moreau-afnic
Copy link
Contributor

Data URLs used for embedded images and fonts can be percent-encoded, so some HTML/CSS build tools (such as Parcel) generate such data URLs, which are currently not supported by Flying Saucer.

@asolntsev asolntsev self-assigned this Dec 12, 2024
@asolntsev
Copy link
Contributor

@moreau-afnic Are you sure URL starting with data:image/png;base64 can contain percent symbol?
I don't think so. "base64" format does NOT use %.

At least site https://base64.guru/tools/repair says that URL from your test is not valid:

image

Can you provide a real example of valid URL starting with data:image/png;base64 and containing % symbol?

@moreau-afnic
Copy link
Contributor Author

@asolntsev you seem to be confusing two different things.

There are two standards : base64 and data URLs (which can be base64 data URLs).

base64 is what comes after data:image/png;base64, in a base64 data URL.

Any data URL (with or without base64 data) can have percent-encoded data.

It's explained in the link posted when I opened the PR (https://developer.mozilla.org/en-US/docs/Web/URI/Schemes/data#syntax), which itself references the relevant RFC (RFC 3986).

The tool you're using checks base64 validity not base64 data URL validity, so it won't work with any base64 data URL whether it contains percent-encoded data or not (because of the base64 data URL prefix).

There is already an example of a base64 data URL with percent-encoded data in the unit test; if you copy/paste it into an HTML file, CSS file or even directly into the address bar of a browser, you will see that browsers accept this format as they should.

@asolntsev
Copy link
Contributor

@moreau-afnic No, I am not mixing. :)
I thought that URL needs to be encoded only if it contains some special characters like / or +, and I thought these chars are not used in Base64.

But now I found that + can be used in base64:

Certain characters, notably "+" and "/" in
the base 64 alphabet, are treated as word-breaks by legacy text
search/index tools.

So the fix is correct.

But I think we should even simplify it by removing if (b64encoded.contains("%")) part.
I think we should always call URLDecoder.decode. Right?

@asolntsev asolntsev added this to the 9.11.3 milestone Dec 16, 2024
@moreau-afnic
Copy link
Contributor Author

About removing the if, I did that first, but I had to add it because without it, the + of an unencoded data URL will be decoded as (space), which is something we don't want because the base64 data will no longer be valid anymore.

@asolntsev
Copy link
Contributor

About removing the if, I did that first, but I had to add it because without it, the + of an unencoded data URL will be decoded as (space), which is something we don't want because the base64 data will no longer be valid anymore.

Can you bring an example of valid unencoded data URL starting with data:image/png;base64,?
At least we should add such an example to unit-tests.

@moreau-afnic
Copy link
Contributor Author

In the unit test, the first embeddedBase64Image call already uses a valid unencoded base64 data URL (in fact, the same one that was already there before).
If you remove the if you will see that the base64 decoder will try to decode a space character and the test will fail.

The second embeddedBase64Image call is the new test I've added and uses a valid URL encoded base64 data URL (in fact it's the same image as in the first call, but where the data was URL encoded).

@asolntsev
Copy link
Contributor

asolntsev commented Dec 16, 2024

Yes, sure. My bad, I didn't see this sample.

@moreau-afnic Thank you for the patience. :)

@asolntsev asolntsev merged commit fec36b0 into flyingsaucerproject:main Dec 16, 2024
4 checks passed
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