[ANCHOR-328] Add homeDomain and webAuthDomain length validation #942
Conversation
| try { | ||
| new ManageDataOperation.Builder(String.format("%s %s", homeDomain, "auth"), nonce).build(); | ||
| } catch (IllegalArgumentException iaex) { | ||
| errors.rejectValue( |
There was a problem hiding this comment.
Can we have a special error for when domains are too long? Current error message is hard to debug for outside developer.
There was a problem hiding this comment.
Hey Gleb, thanks for commenting. Just a reminder that this is still a draft.
There was a problem hiding this comment.
I will modify the error messages.
There was a problem hiding this comment.
Ah sorry about that, I thought it wasn't a draft. Sounds good, thank you
|
Reference Server Preview is available here: |
|
Reference Server Preview is available here: |
1 similar comment
|
Reference Server Preview is available here: |
|
Reference Server Preview is available here: |
lijamie98
force-pushed
the
feature/anchor-328-fix-sep-10-domain-validation
branch
from
700de57 to
73989f1
Compare
|
Reference Server Preview is available here: |
| @@ -87,6 +87,18 @@ void validateConfig(Errors errors) { | |||
| errors.rejectValue( | |||
| "homeDomain", "home-domain-empty", "The sep10.home_domain is not defined."); | |||
| } else { | |||
| try { | |||
| new ManageDataOperation.Builder(String.format("%s %s", homeDomain, "auth"), new byte[64]) | |||
There was a problem hiding this comment.
Can we make the length check more explicit? Something like:
byte[] bytes = homeDomain.getBytes(StandardCharsets.US_ASCII);
if (bytes.length > 64) {
errors.reject( ... )
}There was a problem hiding this comment.
That's a good alternative. However, the size check (<=64) happens in the ManageDataOperation constructor. It would be more robust if we call the ManageDataOperation.
There was a problem hiding this comment.
I'm not sure if I'm convinced that is more robust. For example, if there are new validations added to the ManageDataOperation constructor, we will fail with a message complaining about the size. Or if the allowed since increases from 64, we will need to update the message here.
There was a problem hiding this comment.
This is actually intended if there is a new validation added to ManageDataOperation, the validation should fail because AP uses ManageDataOperation to create SEP-10 challenge.
If the size is changed in ManageDataOperation is changed, we should change the message as well. One option is to remove the size 64. But this seems to cause more harm than benefits because the ManageDataOperation change requires XDR change which is not very likely.
If there is no strong reason, I think we can stay with the current approach and change it in the future PR if necessary.
PR Checklist
PR Structure
otherwise).
paymentservice.stellar, orallordocif the changes are broad or impact manypackages.
Thoroughness
What
Add validation that calls
ManageDataOperation.Builderto validate thehomeDomainandwebAuthDomain.Why
Cath the errors early.