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

Decoding can return negative values due to overflow #12

Closed
robhanlon22 opened this issue Nov 29, 2023 · 5 comments
Closed

Decoding can return negative values due to overflow #12

robhanlon22 opened this issue Nov 29, 2023 · 5 comments
Labels
enhancement New feature or request wontfix This will not be worked on

Comments

@robhanlon22
Copy link
Contributor

Minimum repro uses default alphabet, min length, and block list:

Sqids.builder().build().decode("001011100A010") // result is [-7900952963519449329]
@0x3333
Copy link
Collaborator

0x3333 commented Nov 29, 2023

I believe that this is expected behavior.

How to check if IDs are valid?

Decoding IDs will usually produce some kind of numeric output, but that doesn't necessarily mean that the ID is canonical. To check that the ID is valid, you can re-encode decoded numbers and check that the ID matches.

The reason this is not done automatically is that if the default blocklist changes in the future, we don't want to automatically invalidate the ID that has been generated in the past and might now be matching a new blocklist word.

Source: https://sqids.org/faq#valid-ids

@lukechen116
Copy link
Collaborator

The following operation likely resulted in an overflow, exceeding the range of a long data type.
number = number * charLength + alphabet.indexOf(c);
@0x3333 The current code does not use BigInteger, and an overflow should result in an ArithmeticException. Do you agree with modifying it as follows?
number = Math.addExact(Math.multiplyExact(number, (long)charLength), alphabet.indexOf(c));

@0x3333
Copy link
Collaborator

0x3333 commented Nov 29, 2023

I don't believe we should change this. The problem is that the input is malformed, so this is expected.

This is the same even for the reference implementation in javascript. See: https://codesandbox.io/s/gallant-sky-g7qnrc?file=/src/index.ts

@4kimov what is your opinion on that?

@4kimov
Copy link
Member

4kimov commented Nov 29, 2023

To me, it sounds like a spec issue, not a Java implementation issue ☹️

As of right now, decode() spec is designed to stop & return an empty array only when input is an empty string or there's a non-alphabet char in input.

All other scenarios are undefined -- which makes me lean towards leaving as-is, until spec is ironed out.

For now, the good news is that re-encoding would catch this.

@0x3333
Copy link
Collaborator

0x3333 commented Nov 29, 2023

@4kimov yep, sounds fair to me. Maybe add a more explicit entry in the FAQ?

@robhanlon22 will leave it as is until it is addressed in the reference implementation.

@0x3333 0x3333 closed this as completed Nov 29, 2023
@0x3333 0x3333 added enhancement New feature or request wontfix This will not be worked on labels Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants