urlDecode() - throws "Invalid URL encoding" exception

dev
1

The function urlDecode() throws an exception when the string parameter has invalid characters. The error message is:

Invalid URL encoding: not a valid digit (radix 16): 71

The input string that throws the exception is:

busbar+100a+100%25G

I can confirm that this does not throw an exception in Lucee 5.4.7.3, but it does in Lucee 6.1.1.118 as well as 6.2.1.122.

Should I create a JIRA ticket for this? Seems important since it would be easy to generate a ton of exceptions on any site that calls urlDecode().

2

Despite Lucee’s Jira asking you to check in the forums before creating a ticket - it is actually their request that you log that the ticket anyway.

Just make sure that you write up as much information as you can.

And if you can - if you were to attach a unit test for the failure - that is especially helpful and oftentimes gets you a faster solution as it means the Lucee team don’t have to write the test and get straight to the testing / troubleshooting stage.

It is quite often the case that you will see a post - much like yours - with a statement along the lines of - "I have logged ticket “XXXX”…

1 Like
3

please keep posting here first, otherwise, we get a lot of invalid tickets which makes searching jira (even more) frustrating.

@Redtopia please raise a ticket, once you have created a ticket, if you want to go the full mile, filing a PR with a testcase is the gold standard

nice detailed bug report btw :slight_smile:

4

Ticket was created here LDEV-5560

1 Like
5

I’ve triaged the ticket, %25 is a % and lucee already automatically decodes the url scope, so you are trying to decode what is strictly a badly encoded url.

https://luceeserver.atlassian.net/browse/LDEV-5560?focusedCommentId=60674

Lucee used to be less strict and ACF likewise, but we switched to a standard library with 6, for better support for various other encoding edge cases

https://luceeserver.atlassian.net/browse/LDEV-4648

1 Like
6

Just an FYI, I added a comment to this ticket, which has been closed/fixed/deployed (unfortunately):

https://luceeserver.atlassian.net/browse/LDEV-5560

The problem is that the fix still throws an exception, which is a problem because that means that any URL param that needs to be decoded has to have it’s own try/catch around it in order to catch this new exception, which previously didn’t exist in Lucee 5 and doesn’t exist in any version of ACF.

In my opinion, urlDecode() should never throw an exception.

7

But Lucee already automatically decodes any validly encoded url param in the url scope?

As far as I understand, the only problem is when you are manually decoding an invalid string with urlDecode?

With the 6.2.2, any url scope decoding which fails, it just passes back the raw value

that’s just going to potentially cause other problems isn’t it

sure the stricter behaviour might require some code changes, but you are now working with something that is more robust

you want the old behaviour?

    function laxUrlDecode( s ){
        try {
            return urlDecode( s );
        } catch ( e ){
            return s
        }
    }
8

Whether or not other problems exist isn’t the issue. The issue is that Lucee 6 introduces a new exception when calling a function that previously never threw an exception (and doesn’t on ACF). Therefore whatever code was used to deal with those problems will never get executed. Even though you could argue that Lucee is doing it more correctly, I’m arguing that it’s a regression in light of all known previous (and current behavior).

9

I’ve added a third argument strict, which when set to false, falls back on the old lax decoder, which while incomplete, simply strips out and ignores invalid encodings, i.e the old behaviour

LDEV-5773: Add urlDecode strict argument which when false, strips out invalid encodingsNew

Added to 6.2.3.22 / 7.0.0.348

1 Like
10

I want the improved behavior, but I also don’t want the function to throw an exception. As an alternative to your suggestion, a strict=true could mean to throw an exception as is currently implemented, and strict=false (default) means that if an exception is thrown using the new logic, revert to the old behavior (no exception).

11

Yeah I think that fallback approach makes sense for false

12

Added the fallback approach for 6.2.3.24-SNAPSHOT and 7.0.0.351-SNAPSHOT

13

Great! I noticed that the docs might still show the new strict param as defaulting to true. Is that still the case?

14

The function definitions are updated, they’ll show up next time I bump the snapshot version used to render out the docs

Done! Thanks for the useful feedback on this, I think we’ve nailed a good solution

2 Likes