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

Fix validation error in ScExplorerCli class #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yuki-js
Copy link
Contributor

@yuki-js yuki-js commented Sep 12, 2024

This pull request fixes a validation error in the ScExplorerCli class. Previously, the key argument was only allowed to be a string, but now it can also be an integer. This permits digit PIN.

Note: argument validation or runtime type enforcing (not to be confused with static type checking) should be done with smarter way in the future.

@soltia48
Copy link
Contributor

soltia48 commented Sep 12, 2024

I think this change is not necessary.
If a key contains numeric only, this software can interpretation as a str by representation like \"0123\". It's because this software depends to Python Fire. Also, if key is allowed to be interpreted as an int, then keys with leading zeros will have those zeros removed, causing confusion. What do you think about it?

@yuki-js
Copy link
Contributor Author

yuki-js commented Sep 12, 2024

@soltia48 Thank you for addressing the problem!
But actually I got an error while unlocking JPKI Auth Key with PIN 1234.

Let's try introducing the following function:

class ScExplorerCli:
    def typecast(self, key=None) -> Self:
        print(f"{key} = {type(key)}")
        return self

Then run:

PS C:\Users\user\codes\sc-tools> python .\sc_explorer_cli\sc_explorer_cli.py typecast "1234" typecast 0o021 typecast 1 typecast 0o021 typecast "0xc" typecast "0123"
[2024-09-13 00:52:19,928] INFO [__main__.__init__:105] Connected to card.
1234 = <class 'int'>
17 = <class 'int'>
1 = <class 'int'>
17 = <class 'int'>
12 = <class 'int'>
0123 = <class 'str'>
No last response

Any valid int literals become int type. On the other hands, decimals leading zeros are not permitted, so they become str type.

@yuki-js
Copy link
Contributor Author

yuki-js commented Sep 12, 2024

Related issue: google/python-fire#453

@soltia48
Copy link
Contributor

I think that Python Fire has arguments parsing related issue (not GitHub Issue). However, when this PR is applied, the difference between “1234”, “0123”, and “12 34” confuses users. If there is a better solution, it would be better.

@soltia48
Copy link
Contributor

Removing Python Fire is another option, although not one that I would actively want to take.

@yuki-js
Copy link
Contributor Author

yuki-js commented Sep 18, 2024

How about prefixing like pin:1234 or pin:ABC123 ?
Also, the VERIFY operation is expected a function for checking remaining count. How about add count operation and pin: operation?

Or, JIS X 6320 says that P2 can be used for selecting object. So how about doing like 0a:1234? This will be encoded as P2=0x0a, Data=31 32 33 34. It enables specifying P2 and will resolve this issue.
image

image

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